On 2024/3/11 23:37, Darrick J. Wong wrote:
> On Mon, Mar 11, 2024 at 08:22:53PM +0800, Zhang Yi wrote:
>> From: Zhang Yi <yi.zhang@xxxxxxxxxx>
>>
>> Current clone operation could be non-atomic if the destination of a file
>> is beyond EOF, user could get a file with corrupted (zeroed) data on
>> crash.
>>
>> The problem is about to pre-alloctions. If you write some data into a
>> file [A, B) (the position letters are increased one by one), and xfs
>> could pre-allocate some blocks, then we get a delayed extent [A, D).
>> Then the writeback path allocate blocks and convert this delayed extent
>> [A, C) since lack of enough contiguous physical blocks, so the extent
>> [C, D) is still delayed. After that, both the in-memory and the on-disk
>> file size are B. If we clone file range into [E, F) from another file,
>> xfs_reflink_zero_posteof() would call iomap_zero_range() to zero out the
>> range [B, E) beyond EOF and flush range. Since [C, D) is still a delayed
>> extent, it will be zeroed and the file's in-memory && on-disk size will
>> be updated to D after flushing and before doing the clone operation.
>> This is wrong, because user can user can see the size change and read
>> zeros in the middle of the clone operation.
>>
>> We need to keep the in-memory and on-disk size before the clone
>> operation starts, so instead of writing zeroes through the page cache
>> for delayed ranges beyond EOF, we convert these ranges to unwritten and
>> invalidating any cached data over that range beyond EOF.
>>
>> Suggested-by: Dave Chinner <david@xxxxxxxxxxxxx>
>> Signed-off-by: Zhang Yi <yi.zhang@xxxxxxxxxx>
>> ---
>> fs/xfs/xfs_iomap.c | 29 +++++++++++++++++++++++++++++
>> 1 file changed, 29 insertions(+)
>>
>> diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
>> index ccf83e72d8ca..2b2aace25355 100644
>> --- a/fs/xfs/xfs_iomap.c
>> +++ b/fs/xfs/xfs_iomap.c
>> @@ -957,6 +957,7 @@ xfs_buffered_write_iomap_begin(
>> struct xfs_mount *mp = ip->i_mount;
>> xfs_fileoff_t offset_fsb = XFS_B_TO_FSBT(mp, offset);
>> xfs_fileoff_t end_fsb = xfs_iomap_end_fsb(mp, offset, count);
>> + xfs_fileoff_t eof_fsb = XFS_B_TO_FSBT(mp, XFS_ISIZE(ip));
>> struct xfs_bmbt_irec imap, cmap;
>> struct xfs_iext_cursor icur, ccur;
>> xfs_fsblock_t prealloc_blocks = 0;
>> @@ -1035,6 +1036,22 @@ xfs_buffered_write_iomap_begin(
>> }
>>
>> if (imap.br_startoff <= offset_fsb) {
>> + /*
>> + * For zeroing out delayed allocation extent, we trim it if
>> + * it's partial beyonds EOF block, or convert it to unwritten
>> + * extent if it's all beyonds EOF block.
>> + */
>> + if ((flags & IOMAP_ZERO) &&
>> + isnullstartblock(imap.br_startblock)) {
>> + if (offset_fsb > eof_fsb)
>> + goto convert_delay;
>> + if (end_fsb > eof_fsb) {
>> + end_fsb = eof_fsb + 1;
>> + xfs_trim_extent(&imap, offset_fsb,
>> + end_fsb - offset_fsb);
>> + }
>> + }
>> +
>> /*
>> * For reflink files we may need a delalloc reservation when
>> * overwriting shared extents. This includes zeroing of
>> @@ -1158,6 +1175,18 @@ xfs_buffered_write_iomap_begin(
>> xfs_iunlock(ip, lockmode);
>> return xfs_bmbt_to_iomap(ip, iomap, &imap, flags, 0, seq);
>>
>> +convert_delay:
>> + end_fsb = min(end_fsb, imap.br_startoff + imap.br_blockcount);
>> + xfs_iunlock(ip, lockmode);
>> + truncate_pagecache_range(inode, offset, XFS_FSB_TO_B(mp, end_fsb));
>> + error = xfs_iomap_write_direct(ip, offset_fsb, end_fsb - offset_fsb,
>> + flags, &imap, &seq);
>
> I expected this to be a direct call to xfs_bmapi_convert_delalloc.
> What was the reason not for using that?
>
It's because xfs_bmapi_convert_delalloc() isn't guarantee to convert
enough blocks once a time, it may convert insufficient blocks since lack
of enough contiguous free physical blocks. If we are going to use it, I
suppose we need to introduce a new helper something like
xfs_convert_blocks(), add a loop to do the conversion.
xfs_iomap_write_direct() has done all the work of converting, but the
name of this function is non-obviousness than xfs_bmapi_convert_delalloc(),
I can change to use it if you think xfs_bmapi_convert_delalloc() is
better. :)
Thanks,
Yi.
> --D
>
>> + if (error)
>> + return error;
>> +
>> + trace_xfs_iomap_alloc(ip, offset, count, XFS_DATA_FORK, &imap);
>> + return xfs_bmbt_to_iomap(ip, iomap, &imap, flags, IOMAP_F_NEW, seq);
>> +
>> found_cow:
>> seq = xfs_iomap_inode_sequence(ip, 0);
>> if (imap.br_startoff <= offset_fsb) {
>> --
>> 2.39.2
>>
>>
