Re: [PATCH 2/4] xfs: convert delayed extents to unwritten when zeroing post eof blocks

[Zhang Yi]

On 2024/3/13 0:21, Darrick J. Wong wrote:
> On Tue, Mar 12, 2024 at 08:31:58PM +0800, Zhang Yi wrote:
>> 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.
> 
> I thought xfs_bmapi_convert_delalloc passes out (via @iomap) the extent
> that xfs_bmapi_allocate (or anyone else) allocated (bma.got).  If that
> mapping is shorter, won't xfs_buffered_write_iomap_begin pass the
> shortened mapping out to the iomap machinery?  In which case that
> iomap_iter loop will call ->iomap_begin on the unfinished delalloc
> conversion work?

Yeah, make sense, it works, I forgot this loop in iomap_iter().

Thanks,
Yi.