Received-SPF: pass (google.com: domain of linux-kernel+bounces-102468-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Wed, 13 Mar 2024 13:05:43 -0700
From: "Darrick J. Wong" <djwong@kernel.org>
To: Zhang Yi <yi.zhang@huaweicloud.com>
Cc: linux-xfs@vger.kernel.org, linux-fsdevel@vger.kernel.org,
	linux-kernel@vger.kernel.org, hch@infradead.org, brauner@kernel.org,
	david@fromorbit.com, tytso@mit.edu, jack@suse.cz,
	chengzhihao1@huawei.com, yukuai3@huawei.com, yi.zhang@huawei.com
Subject: Re: [PATCH 2/4] xfs: convert delayed extents to unwritten when
 zeroing post eof blocks
Message-ID: <20240313200543.GO1927156@frogsfrogsfrogs>
References: <20240311122255.2637311-1-yi.zhang@huaweicloud.com>
 <20240311122255.2637311-3-yi.zhang@huaweicloud.com>
 <20240311153737.GT1927156@frogsfrogsfrogs>
 <aab454d0-d8f3-61c8-0d14-a5ae4c35746e@huaweicloud.com>
 <20240312162150.GB1927156@frogsfrogsfrogs>
 <e29aa6df-5307-5c95-6471-fbaf3452d76f@huaweicloud.com>
 <cde25a6b-b468-33be-d82f-0172b840b064@huaweicloud.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <cde25a6b-b468-33be-d82f-0172b840b064@huaweicloud.com>

On Wed, Mar 13, 2024 at 09:25:49PM +0800, Zhang Yi wrote:
> On 2024/3/13 15:07, Zhang Yi wrote:
> > 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@huawei.com>
> >>>>>
> >>>>> 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@fromorbit.com>
> >>>>> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
> >>>>> ---
> >>>>>  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().
> 
> Sorry, I've found that it doesn't always work. Think about a special case,
> If we have a file below:
> 
> 	A          B           C                    D
> 	+wwwwwwwwww+DDDDDDDDDDD+dddddddddddddddddddd+
> 	          EOF         EOF
>                (on disk)  (in memory)
> 
> where 'd' is a delalloc block with no data and 'D' is a delalloc
> block with dirty folios over it.
> 
> xfs_bmapi_convert_delalloc() might only convert some blocks from B to B',
> 
> 	A          B   B'       C                    D
> 	+wwwwwwwwww+UUU+DDDDDDD+dddddddddddddddddddd+
> 	          EOF         EOF
>                (on disk)  (in memory)
> 
> After that, it will trigger below warning in iomap_iter_done():
> 
>  WARN_ON_ONCE(iter->iomap.offset + iter->iomap.length <= iter->pos);
> 
> So I guess the loop is still needed, I plane to revise and use
> xfs_convert_blocks() here.

Ah, sounds good to me.  Though, I wouldn't work too hard to hammer that
writeback helper into a write helper.

--D

> Yi.
> 
>