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

[Zhang Yi]

On 2024/4/26 2:29, Darrick J. Wong wrote:
> On Thu, Apr 25, 2024 at 09:13:30PM +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 preallocations. If you write some data into a file:
>>
>> 	[A...B)
>>
>> and XFS decides to preallocate some post-eof blocks, then it can create
>> a delayed allocation reservation:
>>
>> 	[A.........D)
>>
>> The writeback path tries to convert delayed extents to real ones by
>> allocating blocks. If there aren't enough contiguous free space, we can
>> end up with two extents, the first real and the second still delalloc:
>>
>> 	[A....C)[C.D)
>>
>> After that, both the in-memory and the on-disk file sizes are still B.
>> If we clone into the range [E...F) from another file:
>>
>> 	[A....C)[C.D)      [E...F)
>>
>> then xfs_reflink_zero_posteof() calls iomap_zero_range() to zero out the
>> range [B, E) beyond EOF and flush it. Since [C, D) is still a delalloc
>> extent, its pagecache will be zeroed and both the in-memory and on-disk
>> size will be updated to D after flushing but before cloning. This is
>> wrong, because the user can see the size change and read the zeroes
>> while the clone operation is ongoing.
>>
>> 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
>> invalidate any cached data over that range beyond EOF.
>>
>> Suggested-by: Dave Chinner <david@xxxxxxxxxxxxx>
>> Signed-off-by: Zhang Yi <yi.zhang@xxxxxxxxxx>
>> ---
>> Changes since v4:
>>
>> Move the delalloc converting hunk before searching the COW fork. Because
>> if the file has been reflinked and copied on write,
>> xfs_bmap_extsize_align() aligned the range of COW delalloc extent, after
>> the writeback, there might be some unwritten extents left over in the
>> COW fork that overlaps the delalloc extent we found in data fork.
>>
>>   data fork  ...wwww|dddddddddd...
>>   cow fork          |uuuuuuuuuu...
>>                     ^
>>                   i_size
>>
>> In my v4, we search the COW fork before checking the delalloc extent,
>> goto found_cow tag and return unconverted delalloc srcmap in the above
>> case, so the delayed extent in the data fork will have no chance to
>> convert to unwritten, it will lead to delalloc extent residue and break
>> generic/522 after merging patch 6.
> 
> Hmmm.  I suppose that works, but it feels a little funny to convert the
> delalloc mapping in the data fork to unwritten /while/ there's unwritten
> extents in the cow fork too.  Would it make more sense to remap the cow
> fork extents here?
> 

Yeah, it looks more reasonable. But from the original scene, the
xfs_bmap_extsize_align() aligned the new extent that added to the cow fork
could overlaps the unreflinked range, IIUC, I guess that spare range is
useless exactly, is there any situation that would use it?

> OTOH unwritten extents in the cow fork get changed to written ones by
> all the cow remapping functions.  Soooo maybe we don't want to go
> digging /that/ deep into the system.
> 

Yeah, I think it's okay now unless there's some strong claims.

> Reviewed-by: Darrick J. Wong <djwong@xxxxxxxxxx>
> 
> --D
> 
>>
>>  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 236ee78aa75b..2857ef1b0272 100644
>> --- a/fs/xfs/xfs_iomap.c
>> +++ b/fs/xfs/xfs_iomap.c
>> @@ -1022,6 +1022,24 @@ xfs_buffered_write_iomap_begin(
>>  		goto out_unlock;
>>  	}
>>  
>> +	/*
>> +	 * For zeroing, trim a delalloc extent that extends beyond the EOF
>> +	 * block.  If it starts beyond the EOF block, convert it to an
>> +	 * unwritten extent.
>> +	 */
>> +	if ((flags & IOMAP_ZERO) && imap.br_startoff <= offset_fsb &&
>> +	    isnullstartblock(imap.br_startblock)) {
>> +		xfs_fileoff_t eof_fsb = XFS_B_TO_FSB(mp, XFS_ISIZE(ip));
>> +
>> +		if (offset_fsb >= eof_fsb)
>> +			goto convert_delay;
>> +		if (end_fsb > eof_fsb) {
>> +			end_fsb = eof_fsb;
>> +			xfs_trim_extent(&imap, offset_fsb,
>> +					end_fsb - offset_fsb);
>> +		}
>> +	}
>> +
>>  	/*
>>  	 * Search the COW fork extent list even if we did not find a data fork
>>  	 * extent.  This serves two purposes: first this implements the
>> @@ -1167,6 +1185,17 @@ xfs_buffered_write_iomap_begin(
>>  	xfs_iunlock(ip, lockmode);
>>  	return xfs_bmbt_to_iomap(ip, iomap, &imap, flags, 0, seq);
>>  
>> +convert_delay:
>> +	xfs_iunlock(ip, lockmode);
>> +	truncate_pagecache(inode, offset);
>> +	error = xfs_bmapi_convert_delalloc(ip, XFS_DATA_FORK, offset,
>> +					   iomap, NULL);
>> +	if (error)
>> +		return error;
>> +
>> +	trace_xfs_iomap_alloc(ip, offset, count, XFS_DATA_FORK, &imap);
>> +	return 0;
>> +
>>  found_cow:
>>  	seq = xfs_iomap_inode_sequence(ip, 0);
>>  	if (imap.br_startoff <= offset_fsb) {
>> -- 
>> 2.39.2
>>
>>