Re: [RFC] [PATCH] Fix race when checking i_size on direct i/o read

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Jan 16, 2014 at 4:35 PM, Steven Whitehouse <swhiteho@xxxxxxxxxx> wrote:
>
> Following on from the "Re: [PATCH v3] vfs: fix a bug when we do some dio
> reads with append dio writes" thread on linux-fsdevel, this patch is my
> current version of the fix proposed as option (b) in that thread.
>
> Removing the i_size test from the direct i/o read path at VFS level
> means that filesystems now have to deal with requests which are beyond
> i_size themselves. These I've divided into three sets:
>
>  a) Those with "no op" ->direct_IO (9p, cifs, ceph)
> These are obviously not going to be an issue
>
>  b) Those with "home brew" ->direct_IO (nfs, fuse)
> I've been told that NFS should not have any problem with the larger
> i_size, however I've added an extra test to FUSE to duplicate the
> original behaviour just to be on the safe side. Someone who knows fuse
> better maybe able to confirm whether this is actually required or not.
>
>  c) Those using __blockdev_direct_IO()
> These call through to ->get_block() which should deal with the EOF
> condition correctly. I've verified that with GFS2 and I believe that
> Zheng has verified it for ext4. I've also run the test on XFS and it
> passes both before and after this change.
>
> The part of the patch in filemap.c looks a lot larger than it really is
> - there are only two lines of real change. The rest is just indentation
> of the contained code.
>
> There remains a test of i_size though, which was added for btrfs. It
> doesn't cause the other filesystems a problem as the test is performed
> after ->direct_IO has been called. It is possible that there is a race
> that does matter to btrfs, however this patch doesn't change that, so
> its still an overall improvement.
>
> So please have a look at this and let me know what you think. I guess
> that when time comes to submit it, it should probably be via the vfs
> tree.
>
> Signed-off-by: Steven Whitehouse <swhiteho@xxxxxxxxxx>
> Reported-by: Zheng Liu <gnehzuil.liu@xxxxxxxxx>
> Cc: Jan Kara <jack@xxxxxxx>
> Cc: Dave Chinner <david@xxxxxxxxxxxxx>
> Cc: Miklos Szeredi <miklos@xxxxxxxxxx>
> Cc: Chris Mason <clm@xxxxxx>
> Cc: Josef Bacik <jbacik@xxxxxx>
> Cc: Christoph Hellwig <hch@xxxxxxxxxxxxx>
> Cc: Alexander Viro <viro@xxxxxxxxxxxxxxxxxx>
>
> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> index 7e70506..89fdfd1 100644
> --- a/fs/fuse/file.c
> +++ b/fs/fuse/file.c
> @@ -2710,6 +2710,9 @@ fuse_direct_IO(int rw, struct kiocb *iocb, const struct iovec *iov,
>         inode = file->f_mapping->host;
>         i_size = i_size_read(inode);
>
> +       if ((rw == READ) && (offset > i_size))
> +               return 0;
> +

Hmm, OK.   It's not strictly needed, but a valid optimization.  So ACK.

Thanks,
Miklos
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [Samba]     [Device Mapper]     [CEPH Development]
  Powered by Linux