Re: [Ocfs2-devel] [RFC PATCH 0/5] Remove rw parameter from direct_IO()
On Mon, Mar 16, 2015 at 04:33:48AM -0700, Omar Sandoval wrote: Hi, Al, here's some cleanup that you mentioned back in December that I got around to (https://lkml.org/lkml/2014/12/15/28). Applied. See #for-next ___ Ocfs2-devel mailing list Ocfs2-devel@oss.oracle.com https://oss.oracle.com/mailman/listinfo/ocfs2-devel
Re: [Ocfs2-devel] [RFC PATCH 0/5] Remove rw parameter from direct_IO()
On Mon, Mar 16, 2015 at 04:33:48AM -0700, Omar Sandoval wrote: Hi, Al, here's some cleanup that you mentioned back in December that I got around to (https://lkml.org/lkml/2014/12/15/28). In summary, the rw parameter to a_ops-direct_IO() is redundant with .type in struct iov_iter. Additionally, rw is inconsistently checked for being a WRITE; some filesystems do rw == WRITE, others do rw WRITE, and others do both within the same function :) The distinction is that swapout may OR in the ITER_BVEC flag in the rw passed to -direct_IO(), so the two are not equivalent (although this really only happens for swap-over-NFS, but it's scary nonetheless). After looking through all of these, it definitely looks like every check means for ANY write, not just non-kernel writes. So, the solution presented here is: - Add a helper, iov_iter_rw(), which always returns either READ or WRITE, no ITER_.* or REQ_.* nonsense mixed in. For consistency, the return value is always checked for equality TBH, I'm not sure I like such calling conventions, but I guess we can live with that. I decided to squish all of the filesystems together in patch 4 to avoid inundating the mailing lists with 20+ mostly two-line patches, but I can split those out if that's any better. Additionally, patch 1 pulls fs.h into uio.h, which seems undesirable. ... and easily avoided if you use a macro instead of inline, without losing type safety or getting double evaluation, etc. Look: 0 ? (struct T *)0 : (x) always evaluates to x. Now look at 6.5.15p3 in C99: the second and the third arguments are both pointers, so we are left with p3.4 (both arguments are pointers to qualified or unqualified versions of compatible types), p3.5 (one operand is a pointer and another null pointer constant) and p3.6 (one operand is a pointer to an object or incomplete type, and the other is a pointer to qualified or unqualied version of void. The first variant means that x is a pointer to qualified or unqualified struct T; the type of result is, per 6.5.15p6, the same as that of x. The second variant means that x is a null pointer constant ((struct T *)0 isn't one) and result is a null pointer to T. The third one means that x is a pointer to qualified or unqualified void. The type of result is the same as that of x. Now note that your variant is no better wrt type safety; worse, actually, since it does accept any pointer to void. (0 ? (struct iov_iter *)0 : (x))-type will reject those. And we obviously don't have double evaluation here either. ___ Ocfs2-devel mailing list Ocfs2-devel@oss.oracle.com https://oss.oracle.com/mailman/listinfo/ocfs2-devel
[Ocfs2-devel] [RFC PATCH 0/5] Remove rw parameter from direct_IO()
Hi,
Al, here's some cleanup that you mentioned back in December that I got
around to (https://lkml.org/lkml/2014/12/15/28).
In summary, the rw parameter to a_ops-direct_IO() is redundant with
.type in struct iov_iter. Additionally, rw is inconsistently checked for
being a WRITE; some filesystems do rw == WRITE, others do rw WRITE,
and others do both within the same function :) The distinction is that
swapout may OR in the ITER_BVEC flag in the rw passed to -direct_IO(),
so the two are not equivalent (although this really only happens for
swap-over-NFS, but it's scary nonetheless). After looking through all of
these, it definitely looks like every check means for ANY write, not
just non-kernel writes.
So, the solution presented here is:
- Add a helper, iov_iter_rw(), which always returns either READ or
WRITE, no ITER_.* or REQ_.* nonsense mixed in. For consistency, the
return value is always checked for equality
- Get rid of all uses of rw in any implementations of direct_IO,
starting with the generic code
- Nuke the actual parameter and update the documentation
I decided to squish all of the filesystems together in patch 4 to avoid
inundating the mailing lists with 20+ mostly two-line patches, but I can
split those out if that's any better. Additionally, patch 1 pulls fs.h
into uio.h, which seems undesirable.
These were mostly just compile tested, with a couple of direct I/O
xfstests run on btrfs as quick sanity check, so getting some more eyes
on is probably a good thing. They should apply on top of v4.0-rc4.
Please comment away.
Thank you,
Omar Sandoval (5):
new helper: iov_iter_rw()
Remove rw from {,__,do_}blockdev_direct_IO()
Remove rw from dax_{do_,}io()
direct_IO: use iov_iter_rw() instead of rw everywhere
direct_IO: remove rw from a_ops-direct_IO()
Documentation/filesystems/Locking | 2 +-
Documentation/filesystems/vfs.txt | 2 +-
drivers/staging/lustre/lustre/llite/rw26.c | 22 -
fs/9p/vfs_addr.c | 2 +-
fs/affs/file.c | 9 +++
fs/block_dev.c | 8 +++---
fs/btrfs/inode.c | 24 +-
fs/ceph/addr.c | 3 +--
fs/cifs/file.c | 3 +--
fs/dax.c | 27 ++---
fs/direct-io.c | 39 ++
fs/exofs/inode.c | 4 +--
fs/ext2/inode.c| 11 -
fs/ext3/inode.c| 14 +--
fs/ext4/ext4.h | 4 +--
fs/ext4/indirect.c | 25 ++-
fs/ext4/inode.c| 28 ++---
fs/f2fs/data.c | 22 -
fs/fat/inode.c | 9 +++
fs/fuse/file.c | 16 ++--
fs/gfs2/aops.c | 16 ++--
fs/hfs/inode.c | 8 +++---
fs/hfsplus/inode.c | 9 +++
fs/jfs/inode.c | 8 +++---
fs/nfs/direct.c| 4 +--
fs/nilfs2/inode.c | 10 +++-
fs/ocfs2/aops.c| 22 +++--
fs/reiserfs/inode.c| 8 +++---
fs/udf/file.c | 3 +--
fs/udf/inode.c | 7 +++---
fs/xfs/xfs_aops.c | 12 -
include/linux/fs.h | 24 +-
include/linux/nfs_fs.h | 2 +-
include/linux/uio.h| 10
mm/filemap.c | 4 +--
mm/page_io.c | 4 +--
36 files changed, 206 insertions(+), 219 deletions(-)
--
2.3.3
___
Ocfs2-devel mailing list
Ocfs2-devel@oss.oracle.com
https://oss.oracle.com/mailman/listinfo/ocfs2-devel
