From: Zhang Yi <yi.zhang@huaweicloud.com>
To: "Darrick J. Wong" <djwong@kernel.org>
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, chandanbabu@kernel.org,
tytso@mit.edu, jack@suse.cz, yi.zhang@huawei.com,
chengzhihao1@huawei.com, yukuai3@huawei.com
Subject: Re: [PATCH v5 4/9] xfs: convert delayed extents to unwritten when zeroing post eof blocks
Date: Fri, 26 Apr 2024 14:24:19 +0800 [thread overview]
Message-ID: <3be86418-e629-c7e6-fd73-f59f97a73a89@huaweicloud.com> (raw)
In-Reply-To: <20240425182904.GA360919@frogsfrogsfrogs>
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@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 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@fromorbit.com>
>> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
>> ---
>> 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@kernel.org>
>
> --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
>>
>>
next prev parent reply other threads:[~2024-04-26 6:24 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-25 13:13 [PATCH v5 0/9] xfs/iomap: fix non-atomic clone operation and don't update size when zeroing range post eof Zhang Yi
2024-04-25 13:13 ` [PATCH v5 1/9] xfs: match lock mode in xfs_buffered_write_iomap_begin() Zhang Yi
2024-04-25 13:13 ` [PATCH v5 2/9] xfs: make the seq argument to xfs_bmapi_convert_delalloc() optional Zhang Yi
2024-04-25 13:13 ` [PATCH v5 3/9] xfs: make xfs_bmapi_convert_delalloc() to allocate the target offset Zhang Yi
2024-04-25 13:13 ` [PATCH v5 4/9] xfs: convert delayed extents to unwritten when zeroing post eof blocks Zhang Yi
2024-04-25 18:29 ` Darrick J. Wong
2024-04-26 6:24 ` Zhang Yi [this message]
2024-04-26 6:33 ` Christoph Hellwig
2024-04-26 7:18 ` Zhang Yi
2024-04-27 6:59 ` Christoph Hellwig
2024-04-28 3:26 ` Zhang Yi
2024-04-29 4:41 ` Christoph Hellwig
2024-04-29 7:11 ` Zhang Yi
2024-04-25 13:13 ` [PATCH v5 5/9] iomap: drop the write failure handles when unsharing and zeroing Zhang Yi
2024-04-25 13:13 ` [PATCH v5 6/9] iomap: don't increase i_size if it's not a write operation Zhang Yi
2024-04-25 13:13 ` [PATCH v5 7/9] iomap: use a new variable to handle the written bytes in iomap_write_iter() Zhang Yi
2024-04-25 13:13 ` [PATCH v5 8/9] iomap: make iomap_write_end() return a boolean Zhang Yi
2024-04-25 13:13 ` [PATCH v5 9/9] iomap: do some small logical cleanup in buffered write Zhang Yi
-- strict thread matches above, loose matches on Subject: below --
2024-03-20 11:05 [PATCH v4 4/9] xfs: convert delayed extents to unwritten when zeroing post eof blocks Zhang Yi
2024-04-23 11:17 ` [PATCH v5 " Zhang Yi
2024-04-25 12:22 ` Christoph Hellwig
2024-04-25 12:32 ` Zhang Yi
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=3be86418-e629-c7e6-fd73-f59f97a73a89@huaweicloud.com \
--to=yi.zhang@huaweicloud.com \
--cc=brauner@kernel.org \
--cc=chandanbabu@kernel.org \
--cc=chengzhihao1@huawei.com \
--cc=david@fromorbit.com \
--cc=djwong@kernel.org \
--cc=hch@infradead.org \
--cc=jack@suse.cz \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-xfs@vger.kernel.org \
--cc=tytso@mit.edu \
--cc=yi.zhang@huawei.com \
--cc=yukuai3@huawei.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).