| Message ID | 794af660cbd6c6fc417a683bfc914bbf9fb34ab0.1727434488.git.fdmanana@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] btrfs: send: fix invalid clone operation for file that got its size decreased | expand |
On Fri, Sep 27, 2024 at 12:03:55PM +0100, fdmanana@kernel.org wrote: > From: Filipe Manana <fdmanana@suse.com> > > During an incremental send we may end up sending an invalid clone > operation, for the last extent of a file which ends at an unaligned offset > that matches the final i_size of the file in the send snapshot, in case > the file had its initial size (the size in the parent snapshot) decreased > in the send snapshot. In this case the destination will fail to apply the > clone operation because its end offset is not sector size aligned and it > ends before the current size of the file. > > Sending the truncate operation always happens when we finish processing an > inode, after we process all its extents (and xattrs, names, etc). So fix > this by ensuring the file has a valid size before we send a clone > operation for an unaligned extent that ends at the final i_size of the > file. The size we truncate to matches the start offset of the clone range > but it could be any value between that start offset and the final size of > the file since the clone operation will expand the i_size if the current > size is smaller than the end offset. The start offset of the range was > chosen because it's always sector size aligned and avoids a truncation > into the middle of a page, which results in dirtying the page due to > filling part of it with zeroes and then making the clone operation at the > receiver trigger IO. > > The following test reproduces the issue: > > $ cat test.sh > #!/bin/bash > > DEV=/dev/sdi > MNT=/mnt/sdi > > mkfs.btrfs -f $DEV > mount $DEV $MNT > > # Create a file with a size of 256K + 5 bytes, having two extents, one > # with a size of 128K and another one with a size of 128K + 5 bytes. > last_ext_size=$((128 * 1024 + 5)) > xfs_io -f -d -c "pwrite -S 0xab -b 128K 0 128K" \ > -c "pwrite -S 0xcd -b $last_ext_size 128K $last_ext_size" \ > $MNT/foo > > # Another file which we will later clone foo into, but initially with > # a larger size than foo. > xfs_io -f -c "pwrite -S 0xef 0 1M" $MNT/bar > > btrfs subvolume snapshot -r $MNT/ $MNT/snap1 > > # Now resize bar and clone foo into it. > xfs_io -c "truncate 0" \ > -c "reflink $MNT/foo" $MNT/bar > > btrfs subvolume snapshot -r $MNT/ $MNT/snap2 > > rm -f /tmp/send-full /tmp/send-inc > btrfs send -f /tmp/send-full $MNT/snap1 > btrfs send -p $MNT/snap1 -f /tmp/send-inc $MNT/snap2 > > umount $MNT > mkfs.btrfs -f $DEV > mount $DEV $MNT > > btrfs receive -f /tmp/send-full $MNT > btrfs receive -f /tmp/send-inc $MNT > > umount $MNT > > Running it before this patch: > > $ ./test.sh > (...) > At subvol snap1 > At snapshot snap2 > ERROR: failed to clone extents to bar: Invalid argument > > A test case for fstests will be sent soon. > > Reported-by: Ben Millwood <thebenmachine@gmail.com> > Link: https://lore.kernel.org/linux-btrfs/CAJhrHS2z+WViO2h=ojYvBPDLsATwLbg+7JaNCyYomv0fUxEpQQ@mail.gmail.com/ > Fixes: 46a6e10a1ab1 ("btrfs: send: allow cloning non-aligned extent if it ends at i_size") > Signed-off-by: Filipe Manana <fdmanana@suse.com> Reviewed-by: Josef Bacik <josef@toxicpanda.com> Thanks, Josef
在 2024/9/27 20:33, fdmanana@kernel.org 写道: > From: Filipe Manana <fdmanana@suse.com> > > During an incremental send we may end up sending an invalid clone > operation, for the last extent of a file which ends at an unaligned offset > that matches the final i_size of the file in the send snapshot, in case > the file had its initial size (the size in the parent snapshot) decreased > in the send snapshot. In this case the destination will fail to apply the > clone operation because its end offset is not sector size aligned and it > ends before the current size of the file. > > Sending the truncate operation always happens when we finish processing an > inode, after we process all its extents (and xattrs, names, etc). So fix > this by ensuring the file has a valid size before we send a clone > operation for an unaligned extent that ends at the final i_size of the > file. The size we truncate to matches the start offset of the clone range > but it could be any value between that start offset and the final size of > the file since the clone operation will expand the i_size if the current > size is smaller than the end offset. The start offset of the range was > chosen because it's always sector size aligned and avoids a truncation > into the middle of a page, which results in dirtying the page due to > filling part of it with zeroes and then making the clone operation at the > receiver trigger IO. > > The following test reproduces the issue: > > $ cat test.sh > #!/bin/bash > > DEV=/dev/sdi > MNT=/mnt/sdi > > mkfs.btrfs -f $DEV > mount $DEV $MNT > > # Create a file with a size of 256K + 5 bytes, having two extents, one > # with a size of 128K and another one with a size of 128K + 5 bytes. > last_ext_size=$((128 * 1024 + 5)) > xfs_io -f -d -c "pwrite -S 0xab -b 128K 0 128K" \ > -c "pwrite -S 0xcd -b $last_ext_size 128K $last_ext_size" \ > $MNT/foo > > # Another file which we will later clone foo into, but initially with > # a larger size than foo. > xfs_io -f -c "pwrite -S 0xef 0 1M" $MNT/bar > > btrfs subvolume snapshot -r $MNT/ $MNT/snap1 > > # Now resize bar and clone foo into it. > xfs_io -c "truncate 0" \ > -c "reflink $MNT/foo" $MNT/bar > > btrfs subvolume snapshot -r $MNT/ $MNT/snap2 > > rm -f /tmp/send-full /tmp/send-inc > btrfs send -f /tmp/send-full $MNT/snap1 > btrfs send -p $MNT/snap1 -f /tmp/send-inc $MNT/snap2 > > umount $MNT > mkfs.btrfs -f $DEV > mount $DEV $MNT > > btrfs receive -f /tmp/send-full $MNT > btrfs receive -f /tmp/send-inc $MNT > > umount $MNT > > Running it before this patch: > > $ ./test.sh > (...) > At subvol snap1 > At snapshot snap2 > ERROR: failed to clone extents to bar: Invalid argument > > A test case for fstests will be sent soon. > > Reported-by: Ben Millwood <thebenmachine@gmail.com> > Link: https://lore.kernel.org/linux-btrfs/CAJhrHS2z+WViO2h=ojYvBPDLsATwLbg+7JaNCyYomv0fUxEpQQ@mail.gmail.com/ > Fixes: 46a6e10a1ab1 ("btrfs: send: allow cloning non-aligned extent if it ends at i_size") > Signed-off-by: Filipe Manana <fdmanana@suse.com> Reviewed-by: Qu Wenruo <wqu@suse.com> Thanks, Qu > --- > fs/btrfs/send.c | 23 ++++++++++++++++++++++- > 1 file changed, 22 insertions(+), 1 deletion(-) > > diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c > index 5871ca845b0e..27306d98ec43 100644 > --- a/fs/btrfs/send.c > +++ b/fs/btrfs/send.c > @@ -6189,8 +6189,29 @@ static int send_write_or_clone(struct send_ctx *sctx, > if (ret < 0) > return ret; > > - if (clone_root->offset + num_bytes == info.size) > + if (clone_root->offset + num_bytes == info.size) { > + /* > + * The final size of our file matches the end offset, but it may > + * be that its current size is larger, so we have to truncate it > + * to any value between the start offset of the range and the > + * final i_size, otherwise the clone operation is invalid > + * because it's unaligned and it ends before the current EOF. > + * We do this truncate to the final i_size when we finish > + * processing the inode, but it's too late by then. And here we > + * truncate to the start offset of the range because it's always > + * sector size aligned while if it were the final i_size it > + * would result in dirtying part of a page, filling part of a > + * page with zeroes and then having the clone operation at the > + * receiver trigger IO and wait for it due to the dirty page. > + */ > + if (sctx->parent_root != NULL) { > + ret = send_truncate(sctx, sctx->cur_ino, > + sctx->cur_inode_gen, offset); > + if (ret < 0) > + return ret; > + } > goto clone_data; > + } > > write_data: > ret = send_extent_data(sctx, path, offset, num_bytes);
Am 27.09.24 um 13:03 schrieb fdmanana@kernel.org: > From: Filipe Manana <fdmanana@suse.com> > > During an incremental send we may end up sending an invalid clone > operation, for the last extent of a file which ends at an unaligned offset > that matches the final i_size of the file in the send snapshot, in case > the file had its initial size (the size in the parent snapshot) decreased > in the send snapshot. In this case the destination will fail to apply the > clone operation because its end offset is not sector size aligned and it > ends before the current size of the file. > > Sending the truncate operation always happens when we finish processing an > inode, after we process all its extents (and xattrs, names, etc). So fix > this by ensuring the file has a valid size before we send a clone > operation for an unaligned extent that ends at the final i_size of the > file. The size we truncate to matches the start offset of the clone range > but it could be any value between that start offset and the final size of > the file since the clone operation will expand the i_size if the current > size is smaller than the end offset. The start offset of the range was > chosen because it's always sector size aligned and avoids a truncation > into the middle of a page, which results in dirtying the page due to > filling part of it with zeroes and then making the clone operation at the > receiver trigger IO. I came across this patch/message after I had the "failed to clone extents" problem 3-4x on Debian 12, 6.1.0-27-amd64. For us, it only occurs since we periodically run a Btrfs balance via Cronjob. That's why I'm wondering: Is it possible that Btrfs Balance increases the likelihood of the problem occurring? Best, Markus
On Wed, Nov 13, 2024 at 1:07 PM Markus <markus@opsone.ch> wrote: > > Am 27.09.24 um 13:03 schrieb fdmanana@kernel.org: > > From: Filipe Manana <fdmanana@suse.com> > > > > During an incremental send we may end up sending an invalid clone > > operation, for the last extent of a file which ends at an unaligned offset > > that matches the final i_size of the file in the send snapshot, in case > > the file had its initial size (the size in the parent snapshot) decreased > > in the send snapshot. In this case the destination will fail to apply the > > clone operation because its end offset is not sector size aligned and it > > ends before the current size of the file. > > > > Sending the truncate operation always happens when we finish processing an > > inode, after we process all its extents (and xattrs, names, etc). So fix > > this by ensuring the file has a valid size before we send a clone > > operation for an unaligned extent that ends at the final i_size of the > > file. The size we truncate to matches the start offset of the clone range > > but it could be any value between that start offset and the final size of > > the file since the clone operation will expand the i_size if the current > > size is smaller than the end offset. The start offset of the range was > > chosen because it's always sector size aligned and avoids a truncation > > into the middle of a page, which results in dirtying the page due to > > filling part of it with zeroes and then making the clone operation at the > > receiver trigger IO. > > I came across this patch/message after I had the "failed to clone > extents" problem 3-4x on Debian 12, 6.1.0-27-amd64. For us, it only > occurs since we periodically run a Btrfs balance via Cronjob. I don't know what Debian's 6.1.0-27 matches, but upstream the fix went into 6.1.113, and the bug first appeared in 6.1.107. So for 6.1 kernels, it only affected releases between 6.1.107 and 6.1.112. So check if that kernel corresponds to 6.1.113+, and if the issue still happens, run 'btrfs receive' with -vv and provide the output to help figure out if it's the same issue or something else. > > That's why I'm wondering: Is it possible that Btrfs Balance increases > the likelihood of the problem occurring? Balance doesn't make aligned extents become aligned and vice-versa (if so it would change file sizes and cause corruption), doesn't make extents that were not shared become shared and vice-versa, and doesn't do any changes to the extent layout of a file. So, no, balance is totally unrelated. > > Best, > Markus
Am 13.11.24 um 16:01 schrieb Filipe Manana: > On Wed, Nov 13, 2024 at 1:07 PM Markus <markus@opsone.ch> wrote: > > I don't know what Debian's 6.1.0-27 matches, but upstream the fix went > into 6.1.113, and the bug first appeared in 6.1.107. > So for 6.1 kernels, it only affected releases between 6.1.107 and 6.1.112. > > So check if that kernel corresponds to 6.1.113+, and if the issue > still happens, run 'btrfs receive' with -vv and provide the output to > help figure out if it's the same issue or something else. Debian Linux Kernel 6.1.0-26-amd64 is 6.1.112. However, my problem has just been solved with Debian 12.8 [1] released on 9 November 2024. After the installation, I am on 6.1.115, now I can no longer reproduce the problem with the instructions from the patch. [1] https://lists.debian.org/debian-announce/2024/msg00008.html > Balance doesn't make aligned extents become aligned and vice-versa (if > so it would change file sizes and cause corruption), doesn't make > extents that were not shared become shared and vice-versa, and doesn't > do any changes to the extent layout of a file. So, no, balance is > totally unrelated. Many thanks for your explanations! Thanks, Markus
diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c index 5871ca845b0e..27306d98ec43 100644 --- a/fs/btrfs/send.c +++ b/fs/btrfs/send.c @@ -6189,8 +6189,29 @@ static int send_write_or_clone(struct send_ctx *sctx, if (ret < 0) return ret; - if (clone_root->offset + num_bytes == info.size) + if (clone_root->offset + num_bytes == info.size) { + /* + * The final size of our file matches the end offset, but it may + * be that its current size is larger, so we have to truncate it + * to any value between the start offset of the range and the + * final i_size, otherwise the clone operation is invalid + * because it's unaligned and it ends before the current EOF. + * We do this truncate to the final i_size when we finish + * processing the inode, but it's too late by then. And here we + * truncate to the start offset of the range because it's always + * sector size aligned while if it were the final i_size it + * would result in dirtying part of a page, filling part of a + * page with zeroes and then having the clone operation at the + * receiver trigger IO and wait for it due to the dirty page. + */ + if (sctx->parent_root != NULL) { + ret = send_truncate(sctx, sctx->cur_ino, + sctx->cur_inode_gen, offset); + if (ret < 0) + return ret; + } goto clone_data; + } write_data: ret = send_extent_data(sctx, path, offset, num_bytes);