| Message ID | 1430614560-4680-1-git-send-email-fdmanana@suse.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
On Sun, May 03, 2015 at 01:56:00AM +0100, Filipe Manana wrote: > Marc reported a problem where the receiving end of an incremental send > was performing clone operations that failed with -EINVAL. This happened > because, unlike for uncompressed extents, we were not checking if the > source clone offset and length, after summing the data offset, falls > within the source file's boundaries. > > So make sure we do such checks when attempting to issue clone operations > for compressed extents. > > Problem reproducible with the following steps: > > $ mkfs.btrfs -f /dev/sdb > $ mount -o compress /dev/sdb /mnt > $ mkfs.btrfs -f /dev/sdc > $ mount -o compress /dev/sdc /mnt2 > > # Create the file with a single extent of 128K. This creates a metadata file > # extent item with a data start offset of 0 and a logical length of 128K. > $ xfs_io -f -c "pwrite -S 0xaa 64K 128K" -c "fsync" /mnt/foo > > # Now rewrite the range 64K to 112K of our file. This will make the inode's > # metadata continue to point to the 128K extent we created before, but now > # with an extent item that points to the extent with a data start offset of > # 112K and a logical length of 16K. > # That metadata file extent item is associated with the logical file offset > # at 176K and covers the logical file range 176K to 192K. > $ xfs_io -c "pwrite -S 0xbb 64K 112K" -c "fsync" /mnt/foo > > # Now rewrite the range 180K to 12K. This will make the inode's metadata > # continue to point the the 128K extent we created earlier, with a single > # extent item that points to it with a start offset of 112K and a logical > # length of 4K. > # That metadata file extent item is associated with the logical file offset > # at 176K and covers the logical file range 176K to 180K. > $ xfs_io -c "pwrite -S 0xcc 180K 12K" -c "fsync" /mnt/foo > > $ btrfs subvolume snapshot -r /mnt /mnt/snap1 > > $ touch /mnt/bar > # Calls the btrfs clone ioctl. > $ ~/xfstests/src/cloner -s $((176 * 1024)) -d $((176 * 1024)) \ > -l $((4 * 1024)) /mnt/foo /mnt/bar > > $ btrfs subvolume snapshot -r /mnt /mnt/snap2 > > $ btrfs send /mnt/snap1 | btrfs receive /mnt2 > At subvol /mnt/snap1 > At subvol snap1 > > $ btrfs send -p /mnt/snap1 /mnt/snap2 | btrfs receive /mnt2 > At subvol /mnt/snap2 > At snapshot snap2 > ERROR: failed to clone extents to bar > Invalid argument > > A test case for fstests follows soon. > > Reported-by: Marc MERLIN <marc@merlins.org> > Tested-by: Marc MERLIN <marc@merlins.org> > Signed-off-by: Filipe Manana <fdmanana@suse.com> Tested-by: David Sterba <dsterba@suse.cz> -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, May 3, 2015 at 2:56 AM, Filipe Manana <fdmanana@suse.com> wrote: > Marc reported a problem where the receiving end of an incremental send > was performing clone operations that failed with -EINVAL. This happened > because, unlike for uncompressed extents, we were not checking if the > source clone offset and length, after summing the data offset, falls > within the source file's boundaries. > > So make sure we do such checks when attempting to issue clone operations > for compressed extents. > > Problem reproducible with the following steps: > > $ mkfs.btrfs -f /dev/sdb > $ mount -o compress /dev/sdb /mnt > $ mkfs.btrfs -f /dev/sdc > $ mount -o compress /dev/sdc /mnt2 > > # Create the file with a single extent of 128K. This creates a metadata file > # extent item with a data start offset of 0 and a logical length of 128K. > $ xfs_io -f -c "pwrite -S 0xaa 64K 128K" -c "fsync" /mnt/foo > > # Now rewrite the range 64K to 112K of our file. This will make the inode's > # metadata continue to point to the 128K extent we created before, but now > # with an extent item that points to the extent with a data start offset of > # 112K and a logical length of 16K. > # That metadata file extent item is associated with the logical file offset > # at 176K and covers the logical file range 176K to 192K. > $ xfs_io -c "pwrite -S 0xbb 64K 112K" -c "fsync" /mnt/foo > > # Now rewrite the range 180K to 12K. This will make the inode's metadata > # continue to point the the 128K extent we created earlier, with a single > # extent item that points to it with a start offset of 112K and a logical > # length of 4K. > # That metadata file extent item is associated with the logical file offset > # at 176K and covers the logical file range 176K to 180K. > $ xfs_io -c "pwrite -S 0xcc 180K 12K" -c "fsync" /mnt/foo > > $ btrfs subvolume snapshot -r /mnt /mnt/snap1 > > $ touch /mnt/bar > # Calls the btrfs clone ioctl. > $ ~/xfstests/src/cloner -s $((176 * 1024)) -d $((176 * 1024)) \ > -l $((4 * 1024)) /mnt/foo /mnt/bar > > $ btrfs subvolume snapshot -r /mnt /mnt/snap2 > > $ btrfs send /mnt/snap1 | btrfs receive /mnt2 > At subvol /mnt/snap1 > At subvol snap1 > > $ btrfs send -p /mnt/snap1 /mnt/snap2 | btrfs receive /mnt2 > At subvol /mnt/snap2 > At snapshot snap2 > ERROR: failed to clone extents to bar > Invalid argument > > A test case for fstests follows soon. > > Reported-by: Marc MERLIN <marc@merlins.org> > Tested-by: Marc MERLIN <marc@merlins.org> > Signed-off-by: Filipe Manana <fdmanana@suse.com> Tested-by: Jan Alexander Steffens (heftig) <jan.steffens@gmail.com> -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, May 3, 2015 at 2:56 AM, Filipe Manana <fdmanana@suse.com> wrote: > Marc reported a problem where the receiving end of an incremental send > was performing clone operations that failed with -EINVAL. This happened > because, unlike for uncompressed extents, we were not checking if the > source clone offset and length, after summing the data offset, falls > within the source file's boundaries. > > So make sure we do such checks when attempting to issue clone operations > for compressed extents. > > Problem reproducible with the following steps: > > $ mkfs.btrfs -f /dev/sdb > $ mount -o compress /dev/sdb /mnt > $ mkfs.btrfs -f /dev/sdc > $ mount -o compress /dev/sdc /mnt2 > > # Create the file with a single extent of 128K. This creates a metadata file > # extent item with a data start offset of 0 and a logical length of 128K. > $ xfs_io -f -c "pwrite -S 0xaa 64K 128K" -c "fsync" /mnt/foo > > # Now rewrite the range 64K to 112K of our file. This will make the inode's > # metadata continue to point to the 128K extent we created before, but now > # with an extent item that points to the extent with a data start offset of > # 112K and a logical length of 16K. > # That metadata file extent item is associated with the logical file offset > # at 176K and covers the logical file range 176K to 192K. > $ xfs_io -c "pwrite -S 0xbb 64K 112K" -c "fsync" /mnt/foo > > # Now rewrite the range 180K to 12K. This will make the inode's metadata > # continue to point the the 128K extent we created earlier, with a single > # extent item that points to it with a start offset of 112K and a logical > # length of 4K. > # That metadata file extent item is associated with the logical file offset > # at 176K and covers the logical file range 176K to 180K. > $ xfs_io -c "pwrite -S 0xcc 180K 12K" -c "fsync" /mnt/foo > > $ btrfs subvolume snapshot -r /mnt /mnt/snap1 > > $ touch /mnt/bar > # Calls the btrfs clone ioctl. > $ ~/xfstests/src/cloner -s $((176 * 1024)) -d $((176 * 1024)) \ > -l $((4 * 1024)) /mnt/foo /mnt/bar > > $ btrfs subvolume snapshot -r /mnt /mnt/snap2 > > $ btrfs send /mnt/snap1 | btrfs receive /mnt2 > At subvol /mnt/snap1 > At subvol snap1 > > $ btrfs send -p /mnt/snap1 /mnt/snap2 | btrfs receive /mnt2 > At subvol /mnt/snap2 > At snapshot snap2 > ERROR: failed to clone extents to bar > Invalid argument > > A test case for fstests follows soon. > > Reported-by: Marc MERLIN <marc@merlins.org> > Tested-by: Marc MERLIN <marc@merlins.org> > Signed-off-by: Filipe Manana <fdmanana@suse.com> > --- > fs/btrfs/send.c | 18 +++++++++++++++++- > 1 file changed, 17 insertions(+), 1 deletion(-) > > diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c > index 895f1b1..50ebc62 100644 > --- a/fs/btrfs/send.c > +++ b/fs/btrfs/send.c > @@ -1159,6 +1159,9 @@ struct backref_ctx { > /* may be truncated in case it's the last extent in a file */ > u64 extent_len; > > + /* data offset in the file extent item */ > + u64 data_offset; > + > /* Just to check for bugs in backref resolving */ > int found_itself; > }; > @@ -1222,7 +1225,7 @@ static int __iterate_backrefs(u64 ino, u64 offset, u64 root, void *ctx_) > if (ret < 0) > return ret; > > - if (offset + bctx->extent_len > i_size) > + if (offset + bctx->data_offset + bctx->extent_len > i_size) > return 0; > > /* > @@ -1364,6 +1367,19 @@ static int find_extent_clone(struct send_ctx *sctx, > backref_ctx->cur_offset = data_offset; > backref_ctx->found_itself = 0; > backref_ctx->extent_len = num_bytes; > + /* > + * For non-compressed extents iterate_extent_inodes() gives us extent > + * offsets that already take into account the data offset, but not for > + * compressed extents, since the offset is logical and not relative to > + * the physical extent locations. We must take this into account to > + * avoid sending clone offsets that go beyond the source file's size, > + * which would result in the clone ioctl failing with -EINVAL on the > + * receiving end. > + */ > + if (compressed == BTRFS_COMPRESS_NONE) > + backref_ctx->data_offset = 0; > + else > + backref_ctx->data_offset = btrfs_file_extent_offset(eb, fi); > > /* > * The last extent of a file may be too large due to page alignment. > -- > 2.1.3 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html Is this patch still useful? It applies to 4.1. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jul 17, 2015 at 4:49 AM, Jan Alexander Steffens <jan.steffens@gmail.com> wrote: > On Sun, May 3, 2015 at 2:56 AM, Filipe Manana <fdmanana@suse.com> wrote: >> Marc reported a problem where the receiving end of an incremental send >> was performing clone operations that failed with -EINVAL. This happened >> because, unlike for uncompressed extents, we were not checking if the >> source clone offset and length, after summing the data offset, falls >> within the source file's boundaries. >> >> So make sure we do such checks when attempting to issue clone operations >> for compressed extents. >> >> Problem reproducible with the following steps: >> >> $ mkfs.btrfs -f /dev/sdb >> $ mount -o compress /dev/sdb /mnt >> $ mkfs.btrfs -f /dev/sdc >> $ mount -o compress /dev/sdc /mnt2 >> >> # Create the file with a single extent of 128K. This creates a metadata file >> # extent item with a data start offset of 0 and a logical length of 128K. >> $ xfs_io -f -c "pwrite -S 0xaa 64K 128K" -c "fsync" /mnt/foo >> >> # Now rewrite the range 64K to 112K of our file. This will make the inode's >> # metadata continue to point to the 128K extent we created before, but now >> # with an extent item that points to the extent with a data start offset of >> # 112K and a logical length of 16K. >> # That metadata file extent item is associated with the logical file offset >> # at 176K and covers the logical file range 176K to 192K. >> $ xfs_io -c "pwrite -S 0xbb 64K 112K" -c "fsync" /mnt/foo >> >> # Now rewrite the range 180K to 12K. This will make the inode's metadata >> # continue to point the the 128K extent we created earlier, with a single >> # extent item that points to it with a start offset of 112K and a logical >> # length of 4K. >> # That metadata file extent item is associated with the logical file offset >> # at 176K and covers the logical file range 176K to 180K. >> $ xfs_io -c "pwrite -S 0xcc 180K 12K" -c "fsync" /mnt/foo >> >> $ btrfs subvolume snapshot -r /mnt /mnt/snap1 >> >> $ touch /mnt/bar >> # Calls the btrfs clone ioctl. >> $ ~/xfstests/src/cloner -s $((176 * 1024)) -d $((176 * 1024)) \ >> -l $((4 * 1024)) /mnt/foo /mnt/bar >> >> $ btrfs subvolume snapshot -r /mnt /mnt/snap2 >> >> $ btrfs send /mnt/snap1 | btrfs receive /mnt2 >> At subvol /mnt/snap1 >> At subvol snap1 >> >> $ btrfs send -p /mnt/snap1 /mnt/snap2 | btrfs receive /mnt2 >> At subvol /mnt/snap2 >> At snapshot snap2 >> ERROR: failed to clone extents to bar >> Invalid argument >> >> A test case for fstests follows soon. >> >> Reported-by: Marc MERLIN <marc@merlins.org> >> Tested-by: Marc MERLIN <marc@merlins.org> >> Signed-off-by: Filipe Manana <fdmanana@suse.com> >> --- >> fs/btrfs/send.c | 18 +++++++++++++++++- >> 1 file changed, 17 insertions(+), 1 deletion(-) >> >> diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c >> index 895f1b1..50ebc62 100644 >> --- a/fs/btrfs/send.c >> +++ b/fs/btrfs/send.c >> @@ -1159,6 +1159,9 @@ struct backref_ctx { >> /* may be truncated in case it's the last extent in a file */ >> u64 extent_len; >> >> + /* data offset in the file extent item */ >> + u64 data_offset; >> + >> /* Just to check for bugs in backref resolving */ >> int found_itself; >> }; >> @@ -1222,7 +1225,7 @@ static int __iterate_backrefs(u64 ino, u64 offset, u64 root, void *ctx_) >> if (ret < 0) >> return ret; >> >> - if (offset + bctx->extent_len > i_size) >> + if (offset + bctx->data_offset + bctx->extent_len > i_size) >> return 0; >> >> /* >> @@ -1364,6 +1367,19 @@ static int find_extent_clone(struct send_ctx *sctx, >> backref_ctx->cur_offset = data_offset; >> backref_ctx->found_itself = 0; >> backref_ctx->extent_len = num_bytes; >> + /* >> + * For non-compressed extents iterate_extent_inodes() gives us extent >> + * offsets that already take into account the data offset, but not for >> + * compressed extents, since the offset is logical and not relative to >> + * the physical extent locations. We must take this into account to >> + * avoid sending clone offsets that go beyond the source file's size, >> + * which would result in the clone ioctl failing with -EINVAL on the >> + * receiving end. >> + */ >> + if (compressed == BTRFS_COMPRESS_NONE) >> + backref_ctx->data_offset = 0; >> + else >> + backref_ctx->data_offset = btrfs_file_extent_offset(eb, fi); >> >> /* >> * The last extent of a file may be too large due to page alignment. >> -- >> 2.1.3 >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html > > Is this patch still useful? It applies to 4.1. Yes, Chris sent it in the pull request to Linus for the 4.2-rc1. > -- > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c index 895f1b1..50ebc62 100644 --- a/fs/btrfs/send.c +++ b/fs/btrfs/send.c @@ -1159,6 +1159,9 @@ struct backref_ctx { /* may be truncated in case it's the last extent in a file */ u64 extent_len; + /* data offset in the file extent item */ + u64 data_offset; + /* Just to check for bugs in backref resolving */ int found_itself; }; @@ -1222,7 +1225,7 @@ static int __iterate_backrefs(u64 ino, u64 offset, u64 root, void *ctx_) if (ret < 0) return ret; - if (offset + bctx->extent_len > i_size) + if (offset + bctx->data_offset + bctx->extent_len > i_size) return 0; /* @@ -1364,6 +1367,19 @@ static int find_extent_clone(struct send_ctx *sctx, backref_ctx->cur_offset = data_offset; backref_ctx->found_itself = 0; backref_ctx->extent_len = num_bytes; + /* + * For non-compressed extents iterate_extent_inodes() gives us extent + * offsets that already take into account the data offset, but not for + * compressed extents, since the offset is logical and not relative to + * the physical extent locations. We must take this into account to + * avoid sending clone offsets that go beyond the source file's size, + * which would result in the clone ioctl failing with -EINVAL on the + * receiving end. + */ + if (compressed == BTRFS_COMPRESS_NONE) + backref_ctx->data_offset = 0; + else + backref_ctx->data_offset = btrfs_file_extent_offset(eb, fi); /* * The last extent of a file may be too large due to page alignment.
Marc reported a problem where the receiving end of an incremental send was performing clone operations that failed with -EINVAL. This happened because, unlike for uncompressed extents, we were not checking if the source clone offset and length, after summing the data offset, falls within the source file's boundaries. So make sure we do such checks when attempting to issue clone operations for compressed extents. Problem reproducible with the following steps: $ mkfs.btrfs -f /dev/sdb $ mount -o compress /dev/sdb /mnt $ mkfs.btrfs -f /dev/sdc $ mount -o compress /dev/sdc /mnt2 # Create the file with a single extent of 128K. This creates a metadata file # extent item with a data start offset of 0 and a logical length of 128K. $ xfs_io -f -c "pwrite -S 0xaa 64K 128K" -c "fsync" /mnt/foo # Now rewrite the range 64K to 112K of our file. This will make the inode's # metadata continue to point to the 128K extent we created before, but now # with an extent item that points to the extent with a data start offset of # 112K and a logical length of 16K. # That metadata file extent item is associated with the logical file offset # at 176K and covers the logical file range 176K to 192K. $ xfs_io -c "pwrite -S 0xbb 64K 112K" -c "fsync" /mnt/foo # Now rewrite the range 180K to 12K. This will make the inode's metadata # continue to point the the 128K extent we created earlier, with a single # extent item that points to it with a start offset of 112K and a logical # length of 4K. # That metadata file extent item is associated with the logical file offset # at 176K and covers the logical file range 176K to 180K. $ xfs_io -c "pwrite -S 0xcc 180K 12K" -c "fsync" /mnt/foo $ btrfs subvolume snapshot -r /mnt /mnt/snap1 $ touch /mnt/bar # Calls the btrfs clone ioctl. $ ~/xfstests/src/cloner -s $((176 * 1024)) -d $((176 * 1024)) \ -l $((4 * 1024)) /mnt/foo /mnt/bar $ btrfs subvolume snapshot -r /mnt /mnt/snap2 $ btrfs send /mnt/snap1 | btrfs receive /mnt2 At subvol /mnt/snap1 At subvol snap1 $ btrfs send -p /mnt/snap1 /mnt/snap2 | btrfs receive /mnt2 At subvol /mnt/snap2 At snapshot snap2 ERROR: failed to clone extents to bar Invalid argument A test case for fstests follows soon. Reported-by: Marc MERLIN <marc@merlins.org> Tested-by: Marc MERLIN <marc@merlins.org> Signed-off-by: Filipe Manana <fdmanana@suse.com> --- fs/btrfs/send.c | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-)