| Message ID | 5a406a607fcccec01684056ab011ff0742f06439.1727432566.git.fdmanana@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: send: fix invalid clone operation for file that got its size decreased | expand |
在 2024/9/27 19:55, 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 the correct size before we send a clone > operation for an unaligned extent that ends at the final i_size of the file. > > 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> Just a small nitpick inlined below. > --- > fs/btrfs/send.c | 19 ++++++++++++++++++- > 1 file changed, 18 insertions(+), 1 deletion(-) > > diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c > index 5871ca845b0e..3ee27840a95a 100644 > --- a/fs/btrfs/send.c > +++ b/fs/btrfs/send.c > @@ -6189,8 +6189,25 @@ 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 that size (or to the start offset of the extent) otherwise > + * the clone operation is invalid because it's unaligned and it > + * ends before the current EOF. > + * We do this truncate when we finish processing the inode, but > + * it's too late by then, so we must do it now. > + */ > + if (sctx->parent_root != NULL) { > + ret = send_truncate(sctx, sctx->cur_ino, > + sctx->cur_inode_gen, > + sctx->cur_inode_size); I know this is a little overkilled, but can we just truncate the inode size to round_down(cur_inode_size, secotrsize)? As the truncate will still dirty the last sector, and later clone() will writeback the range covering the last sector, causing unnecessary IO for the sector we are going to drop immediately. Thanks, Qu > + if (ret < 0) > + return ret; > + } > goto clone_data; > + } > > write_data: > ret = send_extent_data(sctx, path, offset, num_bytes);
On Fri, Sep 27, 2024 at 11:34 AM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote: > > > > 在 2024/9/27 19:55, 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 the correct size before we send a clone > > operation for an unaligned extent that ends at the final i_size of the file. > > > > 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> > > Just a small nitpick inlined below. > > --- > > fs/btrfs/send.c | 19 ++++++++++++++++++- > > 1 file changed, 18 insertions(+), 1 deletion(-) > > > > diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c > > index 5871ca845b0e..3ee27840a95a 100644 > > --- a/fs/btrfs/send.c > > +++ b/fs/btrfs/send.c > > @@ -6189,8 +6189,25 @@ 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 that size (or to the start offset of the extent) otherwise > > + * the clone operation is invalid because it's unaligned and it > > + * ends before the current EOF. > > + * We do this truncate when we finish processing the inode, but > > + * it's too late by then, so we must do it now. > > + */ > > + if (sctx->parent_root != NULL) { > > + ret = send_truncate(sctx, sctx->cur_ino, > > + sctx->cur_inode_gen, > > + sctx->cur_inode_size); > > I know this is a little overkilled, but can we just truncate the inode > size to round_down(cur_inode_size, secotrsize)? > > As the truncate will still dirty the last sector, and later clone() will > writeback the range covering the last sector, causing unnecessary IO for > the sector we are going to drop immediately. For that it's more logical to truncate to the start offset which is always aligned. I'll do that. Thanks. > > Thanks, > Qu > > > + if (ret < 0) > > + return ret; > > + } > > goto clone_data; > > + } > > > > write_data: > > ret = send_extent_data(sctx, path, offset, num_bytes); >
在 2024/9/27 20:06, Filipe Manana 写道: > On Fri, Sep 27, 2024 at 11:34 AM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote: >> >> >> >> 在 2024/9/27 19:55, 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 the correct size before we send a clone >>> operation for an unaligned extent that ends at the final i_size of the file. >>> >>> 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> >> >> Just a small nitpick inlined below. >>> --- >>> fs/btrfs/send.c | 19 ++++++++++++++++++- >>> 1 file changed, 18 insertions(+), 1 deletion(-) >>> >>> diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c >>> index 5871ca845b0e..3ee27840a95a 100644 >>> --- a/fs/btrfs/send.c >>> +++ b/fs/btrfs/send.c >>> @@ -6189,8 +6189,25 @@ 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 that size (or to the start offset of the extent) otherwise >>> + * the clone operation is invalid because it's unaligned and it >>> + * ends before the current EOF. >>> + * We do this truncate when we finish processing the inode, but >>> + * it's too late by then, so we must do it now. >>> + */ >>> + if (sctx->parent_root != NULL) { >>> + ret = send_truncate(sctx, sctx->cur_ino, >>> + sctx->cur_inode_gen, >>> + sctx->cur_inode_size); >> >> I know this is a little overkilled, but can we just truncate the inode >> size to round_down(cur_inode_size, secotrsize)? >> >> As the truncate will still dirty the last sector, and later clone() will >> writeback the range covering the last sector, causing unnecessary IO for >> the sector we are going to drop immediately. > > For that it's more logical to truncate to the start offset which is > always aligned. Right, that's more reasonable. Thanks for the fix. Qu > I'll do that. > > Thanks. > >> >> Thanks, >> Qu >> >>> + if (ret < 0) >>> + return ret; >>> + } >>> goto clone_data; >>> + } >>> >>> write_data: >>> ret = send_extent_data(sctx, path, offset, num_bytes); >> >
diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c index 5871ca845b0e..3ee27840a95a 100644 --- a/fs/btrfs/send.c +++ b/fs/btrfs/send.c @@ -6189,8 +6189,25 @@ 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 that size (or to the start offset of the extent) otherwise + * the clone operation is invalid because it's unaligned and it + * ends before the current EOF. + * We do this truncate when we finish processing the inode, but + * it's too late by then, so we must do it now. + */ + if (sctx->parent_root != NULL) { + ret = send_truncate(sctx, sctx->cur_ino, + sctx->cur_inode_gen, + sctx->cur_inode_size); + if (ret < 0) + return ret; + } goto clone_data; + } write_data: ret = send_extent_data(sctx, path, offset, num_bytes);