On Wed, Nov 01, 2017 at 03:44:51PM +1100, Dave Chinner wrote:
> On Wed, Nov 01, 2017 at 11:46:39AM +0800, Eryu Guan wrote:
> > On Wed, Nov 01, 2017 at 09:58:04AM +1100, Dave Chinner wrote:
> > > On Fri, Oct 27, 2017 at 08:53:28PM +0800, Eryu Guan wrote:
> > > > On truncate down, if new size is not block size aligned, we zero the
> > > > rest of block via iomap_truncate_page() to avoid exposing stale data
> > > > to user, and iomap_truncate_page() skips zeroing if the range is
> > > > already in unwritten status or a hole.
> > >
> > > Unless the page is in the page cache already, and then it gets
> > > zeroed in memory as part of truncate_setsize() call.
> > >
> > > > But it's possible that a buffer write overwrites the unwritten
> > > > extent, which won't be converted to a normal extent until I/O
> > > > completion, and iomap_truncate_page() skips zeroing wrongly because
> > > > of the not-converted unwritten extent. This would cause a subsequent
> > > > mmap read sees non-zeros beyond EOF.
> > >
> > > Yes, it should skip the zeroing on disk. The page in the page cache
> > > over the unwritten extent will be zeroed on read.
> > >
> > > The real question is this: where are the zeros in the page that fsx
> > > is complaining about?
> >
> > The partial block that iomap_truncate_page() skipped zeroing was latter
> > written back to disk, and the punch_hole before mmap read invalidated
> > the page cache so mmap read from disk and saw non-zeros. This is a
> > hard-to-hit sequence, it took me almost 2000 iterations of generic/112
> > runs to hit one failure. I'll provide more details below.
>
> Oh, ok, so they weren't close together operations but far apart in
> the trace. I usually indicate that by showing [....] lines between
> the operations if there's stuff that occurred between them.
They are not strictly one-by-one operations in the original fsxops log,
but are close enough. Then I tailored the ops into a minimal
step-by-step reproducer.
>
> > > > simplified fsx operation sequence is like (assuming 4k block size
> > > > xfs):
> > >
> > > What should have is:
> > >
> > > > fallocate 0x0 0x1000 0x0 keep_size
> > >
> > > Unwritten, no data.
> >
> > Yes, assuming 4k block size and 4k page size, unwritten extent with 1
> > block allocated, i_size stays 0.
> >
> > >
> > > > write 0x0 0x1000 0x0
> > >
> > > Unwritten, contains data in page cache.
> >
> > Exactly, and in-core i_size is 4k now, but on-disk di_size is still 0.
> >
> > >
> > > > truncate 0x0 0x800 0x1000
> > >
> > > Unwritten, page contains data 0-0x800, zeros 0x800-0x1000
> >
> > Yes, the page cache after truncate is correct. But before we zero the
> > page cache (in truncate_setsize()), we skipped zeroing the partial block
> > range 0x800-0x1000 and then triggered a writeback on range
> > [di_size, newsize], which was 0-0x800, and 0x800-0x1000 was written back
> > to disk too, which contained non-zeros.
> >
> > (newsize(2k) > di_size(0) && oldsize(4k) != di_size(0)) was true.
> >
> > if (did_zeroing ||
> > (newsize > ip->i_d.di_size && oldsize != ip->i_d.di_size)) {
> > error = filemap_write_and_wait_range(mapping, ip->i_d.di_size,
> > newsize - 1);
> > if (error)
> > return error;
> > }
>
> Ok, so we're writing data between di_size and newsize before
> removing the page cache beyond newsize. As such, the page of data
> that newsize lies in has not been zeroed by page cache invalidation
> before it is written.
>
> Ok, that explains why the EOF page zeroing in xfs_do_writepage()
> isn't catching this - we haven't updated the inode size yet.
>
> IOWs, the /three places/ where we normally catch this and zero the
> partial tail page beyond EOF are not doing it because:
>
> 1. iomap_truncate_page() sees unwritten and skips.
> 2. truncate_setsize() has not yet been called so can't
> zero the tail of the page.
> 3. we haven't changed where EOF is yet, so
> xfs_do_writepage() hasn't triggered it's "zero data
> beyond EOF" case before it sends the page to disk.
>
> So, we have three options here:
>
> 1. iomap_truncate_page() always zeros
> 2. update inode size before writeback after zeroing so the
> xfs_do_writepage() zeros the tail page, or
> 3. move truncate_setsize() to before writeback so the page
> cache invalidation zeros the part page at the new EOF.
This really helps summarize the problem and solution, thanks!
Yeah, I started to realize moving the order of writeback vs setsize
around might be a fix when I was writing my last reply - explaining the
problem to someone else really helps understand the problem itself :)
>
> I think 1) is a no-go for performance reasons. 2) is better, but
> I don't like the idea of separating the page cache invalidation
> from the size truncation. That leaves 3) - moving
> truncate_setsize().
>
> I think I prefer 3) because it triggers multiple layers of defense
> against writing stale data past EOF, and from an crash behaviour
> point of view it makes no difference whether we truncate the page
> cache before or after triggering writeback because it will just make
> the result the same as if we were zeroing a written extent....
I'm testing an updated patch based on option 3 now, the finished tests
look good. I'll send the new version out for review soon. Thanks a lot
for the suggestion and review!
Eryu
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
