Re: [PATCH v3] vfs: fix a bug when we do some dio reads with append dio writes

[Zheng Liu <gnehzuil.liu@gmail.com>]

Hi all,

Sorry for the delay because of holiday and other issues.

On Tue, Jan 14, 2014 at 08:19:01PM +0100, Jan Kara wrote:
[...]
> > Sorry for the delay - I have been working on this and I've now figured
> > out whats really going on here. I'll describe the GFS2 case first, but
> > it seems that it is something that will affect other fs too, including
> > local fs most likely.
> > 
> > So we have one thread doing writes to a file, 1M at a time, using
> > O_DIRECT. Now with GFS2 that results in more or less a fallback to a
> > buffered write - there is one important difference however, and that is
> > that the direct i/o page invalidations still occur, just as if it were a
> > direct i/o write.
> > 
> > Thread two is doing direct i/o reads. When this results in calling
> > ->direct_IO, all is well. GFS2 does all the correct locking to ensure
> > that this is coherent with the buffered writes. There is a problem
> > though - in generic_file_aio_read() we have:
> > 
> >        /* coalesce the iovecs and go direct-to-BIO for O_DIRECT */
> >         if (filp->f_flags & O_DIRECT) {
> >                 loff_t size;
> >                 struct address_space *mapping;
> >                 struct inode *inode;
> > 
> >                 mapping = filp->f_mapping;
> >                 inode = mapping->host;
> >                 if (!count)
> >                         goto out; /* skip atime */
> >                 size = i_size_read(inode);
> >                 if (pos < size) {
> >                         retval = filemap_write_and_wait_range(mapping, pos,
> >                                         pos + iov_length(iov, nr_segs) - 1);
> >                         if (!retval) {
> >                                 retval = mapping->a_ops->direct_IO(READ, iocb,
> >                                                         iov, pos, nr_segs);
> >                         }
> > 
> > The important thing here is that it is checking the i_size with no
> > locking! This means that it can check i_size, find its the same as the
> > request read position (if the read gets in first before the next block
> > is written to the file) and then fall back to buffered read.
> > 
> > That buffered read then gets a page which is invalidated by the page
> > invalidation after the (DIO, but fallen back to buffered) write has
> > occurred. I added some trace_printk() calls to the read path and it is
> > fairly clear that is the path thats being taken when the failure occurs.
> > Also, it explains why, despite the disk having been used and reused many
> > times, the page that is returned from the failure is always 0, so its
> > not a page thats been read from disk. It also explains why I've only
> > ever seen this for the first page of the read, and the rest of the read
> > is ok.
>   Yes, race like this is something Zheng and I have been talking about from
> the beginning. Thanks for tracking down the details.
> 
> > Changing the test "if (pos < size) {" to "if (1) {" results in zero
> > failures from the test program, since it takes the ->direct_IO path each
> > time which then checks the i_size under the glock, in a race free
> > manner.
> > 
> > So far as I can tell, this can be just as much a problem for local
> > filesystems, since they may update i_size at any time too. I note that
> > XFS appears to wrap this bit of code under its iolock, so that probably
> > explains why XFS doesn't suffer from this issue.
> > 
> > So what is the correct fix... we have several options I think:
> > 
> >  a) Ignore it on the basis that its not important for normal workloads
>   Zheng has a real world usecase which hits this race and it does look
> rather normal. So don't like a) very much.

Yes, as far as I know, at least there are two applications that has met
this problem in our product system at Taobao.  So that is why I dig into
this bug and hope it can be fixed.

> 
> >  b) Simply remove the "if (pos < size) {" test and leave the fs to get
> > on with it (I know that leaves the btrfs test which also uses i_size
> > later on, but that doesn't appear to be an issue since its after we've
> > called ->direct_IO). Is it an issue that this may call an extra
> > filemap_write_and_wait_range() in some cases where it didn't before? I
> > suspect not since the extra calls would be for cases where the pages
> > were beyond the end of file anyway (according to i_size), so a no-op in
> > most cases.

I try this approach and I can confirm that it is ok for ext4.

Dave, I have send out a patch for xfstests to add a new testcase
according to this bug but I don't get any response [1].  Could you
please review it?  Thanks.

1. http://comments.gmane.org/gmane.comp.file-systems.xfs.general/58428

Regards,
                                                - Zheng