| Message ID | 20181212180559.15249-5-fdmanana@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Btrfs: a few more cleanups and fixes for clone/deduplication | expand |
On 12.12.18 г. 20:05 ч., fdmanana@kernel.org wrote: > From: Filipe Manana <fdmanana@suse.com> > > Comparing the content of the pages in the range to deduplicate is now done > by the generic helper generic_remap_file_range_prep(), which takes care of very minor nit: the checks are performed in generic_remap_checks which is called by generic_remap_file_range_prep. But even without this change LGTM. > ensuring we do not compare/deduplicate undefined data beyond a file's eof > (range from eof to the next block boundary). So remove these checks which > are now redundant. > > Signed-off-by: Filipe Manana <fdmanana@suse.com> > --- > fs/btrfs/ioctl.c | 19 ++----------------- > 1 file changed, 2 insertions(+), 17 deletions(-) > > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c > index 4e9efc93340e..3a27efa2b955 100644 > --- a/fs/btrfs/ioctl.c > +++ b/fs/btrfs/ioctl.c > @@ -3206,31 +3206,16 @@ static void btrfs_double_inode_lock(struct inode *inode1, struct inode *inode2) > inode_lock_nested(inode2, I_MUTEX_CHILD); > } > > -static int btrfs_extent_same_range(struct inode *src, u64 loff, u64 olen, > +static int btrfs_extent_same_range(struct inode *src, u64 loff, u64 len, > struct inode *dst, u64 dst_loff) > { > - u64 bs = BTRFS_I(src)->root->fs_info->sb->s_blocksize; > int ret; > - u64 len = olen; > - > - if (loff + len == src->i_size) > - len = ALIGN(src->i_size, bs) - loff; > - /* > - * For same inode case we don't want our length pushed out past i_size > - * as comparing that data range makes no sense. > - * > - * This effectively means we require aligned extents for the single > - * inode case, whereas the other cases allow an unaligned length so long > - * as it ends at i_size. > - */ > - if (dst == src && len != olen) > - return -EINVAL; > > /* > * Lock destination range to serialize with concurrent readpages(). > */ > lock_extent(&BTRFS_I(dst)->io_tree, dst_loff, dst_loff + len - 1); > - ret = btrfs_clone(src, dst, loff, olen, len, dst_loff, 1); > + ret = btrfs_clone(src, dst, loff, len, len, dst_loff, 1); > unlock_extent(&BTRFS_I(dst)->io_tree, dst_loff, dst_loff + len - 1); > > return ret; >
On Wed, Dec 12, 2018 at 6:07 PM <fdmanana@kernel.org> wrote: > > From: Filipe Manana <fdmanana@suse.com> > > Comparing the content of the pages in the range to deduplicate is now done > by the generic helper generic_remap_file_range_prep(), which takes care of > ensuring we do not compare/deduplicate undefined data beyond a file's eof > (range from eof to the next block boundary). So remove these checks which > are now redundant. > > Signed-off-by: Filipe Manana <fdmanana@suse.com> Any reason why this was not yet picked? Thanks. > --- > fs/btrfs/ioctl.c | 19 ++----------------- > 1 file changed, 2 insertions(+), 17 deletions(-) > > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c > index 4e9efc93340e..3a27efa2b955 100644 > --- a/fs/btrfs/ioctl.c > +++ b/fs/btrfs/ioctl.c > @@ -3206,31 +3206,16 @@ static void btrfs_double_inode_lock(struct inode *inode1, struct inode *inode2) > inode_lock_nested(inode2, I_MUTEX_CHILD); > } > > -static int btrfs_extent_same_range(struct inode *src, u64 loff, u64 olen, > +static int btrfs_extent_same_range(struct inode *src, u64 loff, u64 len, > struct inode *dst, u64 dst_loff) > { > - u64 bs = BTRFS_I(src)->root->fs_info->sb->s_blocksize; > int ret; > - u64 len = olen; > - > - if (loff + len == src->i_size) > - len = ALIGN(src->i_size, bs) - loff; > - /* > - * For same inode case we don't want our length pushed out past i_size > - * as comparing that data range makes no sense. > - * > - * This effectively means we require aligned extents for the single > - * inode case, whereas the other cases allow an unaligned length so long > - * as it ends at i_size. > - */ > - if (dst == src && len != olen) > - return -EINVAL; > > /* > * Lock destination range to serialize with concurrent readpages(). > */ > lock_extent(&BTRFS_I(dst)->io_tree, dst_loff, dst_loff + len - 1); > - ret = btrfs_clone(src, dst, loff, olen, len, dst_loff, 1); > + ret = btrfs_clone(src, dst, loff, len, len, dst_loff, 1); > unlock_extent(&BTRFS_I(dst)->io_tree, dst_loff, dst_loff + len - 1); > > return ret; > -- > 2.11.0 >
On Thu, Jan 31, 2019 at 4:31 PM Filipe Manana <fdmanana@kernel.org> wrote: > > On Wed, Dec 12, 2018 at 6:07 PM <fdmanana@kernel.org> wrote: > > > > From: Filipe Manana <fdmanana@suse.com> > > > > Comparing the content of the pages in the range to deduplicate is now done > > by the generic helper generic_remap_file_range_prep(), which takes care of > > ensuring we do not compare/deduplicate undefined data beyond a file's eof > > (range from eof to the next block boundary). So remove these checks which > > are now redundant. > > > > Signed-off-by: Filipe Manana <fdmanana@suse.com> > > Any reason why this was not yet picked? > Thanks. Another ping. Thanks. > > > --- > > fs/btrfs/ioctl.c | 19 ++----------------- > > 1 file changed, 2 insertions(+), 17 deletions(-) > > > > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c > > index 4e9efc93340e..3a27efa2b955 100644 > > --- a/fs/btrfs/ioctl.c > > +++ b/fs/btrfs/ioctl.c > > @@ -3206,31 +3206,16 @@ static void btrfs_double_inode_lock(struct inode *inode1, struct inode *inode2) > > inode_lock_nested(inode2, I_MUTEX_CHILD); > > } > > > > -static int btrfs_extent_same_range(struct inode *src, u64 loff, u64 olen, > > +static int btrfs_extent_same_range(struct inode *src, u64 loff, u64 len, > > struct inode *dst, u64 dst_loff) > > { > > - u64 bs = BTRFS_I(src)->root->fs_info->sb->s_blocksize; > > int ret; > > - u64 len = olen; > > - > > - if (loff + len == src->i_size) > > - len = ALIGN(src->i_size, bs) - loff; > > - /* > > - * For same inode case we don't want our length pushed out past i_size > > - * as comparing that data range makes no sense. > > - * > > - * This effectively means we require aligned extents for the single > > - * inode case, whereas the other cases allow an unaligned length so long > > - * as it ends at i_size. > > - */ > > - if (dst == src && len != olen) > > - return -EINVAL; > > > > /* > > * Lock destination range to serialize with concurrent readpages(). > > */ > > lock_extent(&BTRFS_I(dst)->io_tree, dst_loff, dst_loff + len - 1); > > - ret = btrfs_clone(src, dst, loff, olen, len, dst_loff, 1); > > + ret = btrfs_clone(src, dst, loff, len, len, dst_loff, 1); > > unlock_extent(&BTRFS_I(dst)->io_tree, dst_loff, dst_loff + len - 1); > > > > return ret; > > -- > > 2.11.0 > >
On Thu, Jan 31, 2019 at 04:31:49PM +0000, Filipe Manana wrote: > On Wed, Dec 12, 2018 at 6:07 PM <fdmanana@kernel.org> wrote: > > > > From: Filipe Manana <fdmanana@suse.com> > > > > Comparing the content of the pages in the range to deduplicate is now done > > by the generic helper generic_remap_file_range_prep(), which takes care of > > ensuring we do not compare/deduplicate undefined data beyond a file's eof > > (range from eof to the next block boundary). So remove these checks which > > are now redundant. > > > > Signed-off-by: Filipe Manana <fdmanana@suse.com> > > Any reason why this was not yet picked? Added to misc-next now.
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c index 4e9efc93340e..3a27efa2b955 100644 --- a/fs/btrfs/ioctl.c +++ b/fs/btrfs/ioctl.c @@ -3206,31 +3206,16 @@ static void btrfs_double_inode_lock(struct inode *inode1, struct inode *inode2) inode_lock_nested(inode2, I_MUTEX_CHILD); } -static int btrfs_extent_same_range(struct inode *src, u64 loff, u64 olen, +static int btrfs_extent_same_range(struct inode *src, u64 loff, u64 len, struct inode *dst, u64 dst_loff) { - u64 bs = BTRFS_I(src)->root->fs_info->sb->s_blocksize; int ret; - u64 len = olen; - - if (loff + len == src->i_size) - len = ALIGN(src->i_size, bs) - loff; - /* - * For same inode case we don't want our length pushed out past i_size - * as comparing that data range makes no sense. - * - * This effectively means we require aligned extents for the single - * inode case, whereas the other cases allow an unaligned length so long - * as it ends at i_size. - */ - if (dst == src && len != olen) - return -EINVAL; /* * Lock destination range to serialize with concurrent readpages(). */ lock_extent(&BTRFS_I(dst)->io_tree, dst_loff, dst_loff + len - 1); - ret = btrfs_clone(src, dst, loff, olen, len, dst_loff, 1); + ret = btrfs_clone(src, dst, loff, len, len, dst_loff, 1); unlock_extent(&BTRFS_I(dst)->io_tree, dst_loff, dst_loff + len - 1); return ret;