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

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

On Tue, Dec 17, 2013 at 12:17:44PM +0000, Steven Whitehouse wrote:
> Hi,
> 
> On Tue, 2013-12-17 at 19:16 +0800, Zheng Liu wrote:
> > Hi Steven,
> > 
> > On Tue, Dec 17, 2013 at 09:43:42AM +0000, Steven Whitehouse wrote:
> > > Hi,
> > > 
> > > On Mon, 2013-12-16 at 11:01 +0100, Jan Kara wrote:
> > > > On Mon 16-12-13 09:37:34, Steven Whitehouse wrote:
> > > > > Hi,
> > > > > 
> > > > > On Sat, 2013-12-07 at 18:55 +0800, Zheng Liu wrote:
> > > > > > From: Zheng Liu <wenqing.lz@taobao.com>
> > > > > > 
> > > > > > Currently we check i_size in buffered read path after we know the page
> > > > > > is update.  But it could return some zero-filled pages to the userspace
> > > > > > when we do some append dio writes.  We could use the following code
> > > > > > snippet to reproduce this problem in a ext2/3/4 filesystem.
> > > > > > 
> > > > > If the page is not uptodate, then neither (potentially) is i_size, since
> > > > > the underlying fs has not had a chance to get its own locks and update
> > > > > the inode size.
> > > >   Hum, this isn't really about page being uptodate or not, is it? It is
> > > > more about the fact that gfs2 updates i_size only from gfs2_readpage()
> > > > function when obtaining inode lock. Or do you know about other filesystem
> > > > having the same problem as gfs2? E.g. ocfs2 updates i_size from
> > > > ocfs2_file_aio_read() before calling generic_file_aio_read(), cannot gfs2
> > > > do something similar?
> > > > 
> > > Well we could do that, but I don't think it makes sense really. The
> > > point of having the page cache is to put it "in front" of the expensive
> > > operations where possible (such as getting cluster locks). I don't think
> > > it makes sense to grab and drop a cluster lock on every call to
> > > generic_file_aio_read() when, for cached pages, we would not even need
> > > to call ->readpage.
> > > 
> > > The problem appears to be that somehow we are getting a page returned
> > > when it should not be. So I suspect that the correct place to catch that
> > > is in ->readpage(s)
> > > 
> > > > > I suspect that the correct fix would be to implement ->launder_page to
> > > > > avoid the race that you've identified here, if I've understood it
> > > > > correctly,
> > > >   I don't understand how that would work. Can you elaborate a bit? Here the
> > > > problem is i_size gets extended by DIO write during buffered read (which is
> > > > a fallback from DIO read) so we return zero-filled page instead of either
> > > > data filled page or short read. I don't see where launder_page() would come
> > > > into the picture...
> > > > 
> > > > 								Honza
> > > 
> > > Ah, sorry. I was thinking the other way around wrt to
> > > read/write :( However, I still don't think that generic_file_aio_read()
> > > is the right place to fix this. I note that XFS seems to pass the test
> > > and it also uses mpage_readpage and mpage_readpages as well as
> > > generic_file_aio_read() so maybe that is a clue as to where the answer
> > > lies. GFS2 seems to fail the test in the same way as ext3 at the moment.
> > 
> > Yes, xfs can pass the test under direct IO.  I suspect that the reason
> > is that xfs uses ilock/iolock.  But it doesn't means that all file
> > systems need to fix this issue by themselves because the root cause is
> > in vfs layer.
> >
> I'm not convinced that this is the case though. The VFS is asking the fs
> to provide a page via ->readpage(s) and therefore the fs should not be
> providing a page (presumably marked uptodate in this case) where one
> does not already exist, and if it does exist, then the content of the
> page should be correct.

As far as I understand, ->readpage(s) should return zero-filled pages if
this page doesn't have a block mapping because the content should be '0'.

> 
> I don't see how this test can be done at a point before the fs has a
> chance to get its own locks, and thus ensuring that the inode size is
> actually correct.
> 
> However, I'd like to understand what is going on at ->readpage level,
> and thus why we get this page generated incorrectly, and what actual i/o
> gets generated (or not) in the problematic case.
> 
> > > 
> > > The other question that we've not really answered is why it is useful to
> > > mix buffered and direct i/o to the same file at the same time anyway.
> > > While I agree that it ought to work, I'm not convinced that its
> > > enormously useful, which is maybe why it has not been spotted before,
> > 
> > Yes, mixed buffered read and direct write seems to be useless.  But in
> > our product system, the issue will appear under direct read/write.  The
> > application has a reader and a writer.  The reader should read nothing
> > or the content that has been written by writer.  But now the reader gets
> > the zero-filled page.  Hence the application needs to prevent this issue
> > using a mutex or other flag.  It doesn't make sense to the userspace
> > programmer.
> > 
> I'm not sure I fully understand what you are saying here... if you see
> the same issue using only O_DIRECT reads and writes, how are you getting
> that?

Sorry, maybe I don't clarify the test program.  In commit log the test
program does direct read/write.  I found this problem because the app
developer asks me why his application doesn't work well undert direct
read/write.

> 
> > TBH, we have found this issue two year ago but we don't take care of it.
> > So currently the application just solves it as I said above.  But it will
> > issue a flush calling fsync(2) to flush the dirty page in order to make
> > sure the data has been flushed into the disk.  Obviously, the performance
> > of this application is impacted by this issue.  Now we have known that we
> > don't need to flush the dirty page but I think we'd better fix it.
> > 
> > Regards,
> >                                                 - Zheng
> 
> Calling fsync is a separate issue though, since that is required anyway
> if you want to be sure that the data has actually reached the disk,

Yes, calling fsync is another issue.  I just want to say that this issue
has impacted the application.  At least in our product system.

Thanks,
                                                - Zheng