xfs: flush the range before zero partial block range on truncate down

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.

Quoted 5 lines · Eryu Guan

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.

I suspect the right fix is to look at the in-core state im the iomap truncate helpers instead of doing a duplicate flush.

Quoted 9 lines · Christoph Hellwig

Quoted 5 lines · Eryu Guan

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.

I suspect the right fix is to look at the in-core state im the iomap truncate helpers instead of doing a duplicate flush.

I may (and very likely) miss something, but my understanding is that iomap_truncate_page() already looks at the in-core extent state provided by xfs_file_iomap_begin()

xfs_setattr_size()
iomap_truncate_page()
iomap_zero_range()
iomap_apply()
xfs_file_iomap_begin() # finds extent according to the range
iomap_zero_range_actor() # sees iomap->type == IOMAP_UNWRITTEN

And this in-core extent state won't be converted to XFS_EXT_NORM until writeback & I/O completion, so I think a flush is required.

Thanks,
Eryu
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 22 lines · Eryu Guan

Quoted 9 lines · Christoph Hellwig

Quoted 5 lines · Eryu Guan

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.

I suspect the right fix is to look at the in-core state im the iomap truncate helpers instead of doing a duplicate flush.

I may (and very likely) miss something, but my understanding is that iomap_truncate_page() already looks at the in-core extent state provided by xfs_file_iomap_begin()

xfs_setattr_size()
iomap_truncate_page()
iomap_zero_range()
iomap_apply()
xfs_file_iomap_begin() # finds extent according to the range
iomap_zero_range_actor() # sees iomap->type == IOMAP_UNWRITTEN

Quoted 2 lines

And this in-core extent state won't be converted to XFS_EXT_NORM until writeback & I/O completion, so I think a flush is required.

If we get to iomap_zero_range_actor & type == UNWRITTEN, can we ask the pagecache if there's a page backing the (unwritten) range and if so zero the appropriate parts of that page?

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.

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.

I occasionally see this in fsx runs in fstests generic/112, a simplified fsx operation sequence is like (assuming 4k block size xfs):

fallocate 0x0 0x1000 0x0 keep_size

write 0x0 0x1000 0x0

truncate 0x0 0x800 0x1000

punch_hole 0x0 0x800 0x800

mapread 0x0 0x800 0x800

Quoted 4 lines · Eryu Guan

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.

Quoted 5 lines

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?

Quoted 4 lines

I occasionally see this in fsx runs in fstests generic/112, a simplified fsx operation sequence is like (assuming 4k block size xfs):

What should have is:

Quoted 1 line

fallocate 0x0 0x1000 0x0 keep_size

Unwritten, no data.

Quoted 2 lines

write 0x0 0x1000 0x0

Unwritten, contains data in page cache.

Quoted 2 lines

truncate 0x0 0x800 0x1000

Unwritten, page contains data 0-0x800, zeros 0x800-0x1000

Quoted 2 lines

punch_hole 0x0 0x800 0x800

Unwritten, page contains zeros 0x0-0x1000

Quoted 2 lines

mapread 0x0 0x800 0x800

Should map a page full of zeros as it is either a read over an unwritten extent or a hole, or it finds a page cache page that is fully zeroed.

The only wrinkle in this is if the write is direct IO, but then the truncate would see a written extent and this whole problem doesn't occur.

So, more info required. :P

Cheers,

Dave.

Signature · 2 lines

Dave Chinner
[email protected]

Quoted 18 lines · Dave Chinner

Quoted 4 lines · Eryu Guan

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.

Quoted 5 lines

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.

Quoted 2 lines

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.

Quoted 8 lines

Quoted 2 lines

simplified fsx operation sequence is like (assuming 4k block size xfs):

What should have is:

Quoted 1 line

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.

Quoted 4 lines

Quoted 2 lines

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.

Quoted 4 lines

Quoted 2 lines

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;                                                                                                                                                                                            
        }

Quoted 19 lines · Eryu Guan

Quoted 18 lines · Dave Chinner

Quoted 4 lines · Eryu Guan

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.

Quoted 5 lines

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.

Quoted 8 lines

Quoted 2 lines

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.

Quoted 33 lines

Quoted 9 lines

Quoted 3 lines

simplified fsx operation sequence is like (assuming 4k block size xfs):

What should have is:

Quoted 1 line

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.

Quoted 4 lines

Quoted 2 lines

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.

Quoted 4 lines

Quoted 2 lines

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.

Quoted 7 lines

        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.

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....