[PATCH] bcachefs: support REMAP_FILE_DEDUP in bch2_remap_file_range

[PATCH] bcachefs: support REMAP_FILE_DEDUP in bch2_remap_file_range

[Reed Riley]

By removing the early-exit when REMAP_FILE_DEDUP is set, we should be
able to support the fideduperange ioctl, albeit less efficiently than if
we handled some of the extent locking and comparison logic inside
bcachefs.  Extent comparison logic already exists inside of
`__generic_remap_file_range_prep`.

Signed-off-by: Reed Riley <reed@riley.engineer>
---
 fs/bcachefs/fs-io.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/fs/bcachefs/fs-io.c b/fs/bcachefs/fs-io.c
index 20b40477425f..4f513f22a66a 100644
--- a/fs/bcachefs/fs-io.c
+++ b/fs/bcachefs/fs-io.c
@@ -857,9 +857,6 @@ loff_t bch2_remap_file_range(struct file *file_src, loff_t pos_src,
        if (remap_flags & ~(REMAP_FILE_DEDUP|REMAP_FILE_ADVISORY))
                return -EINVAL;
 
-       if (remap_flags & REMAP_FILE_DEDUP)
-               return -EOPNOTSUPP;
-
        if ((pos_src & (block_bytes(c) - 1)) ||
            (pos_dst & (block_bytes(c) - 1)))
                return -EINVAL;
-- 
2.44.0

Re: [PATCH] bcachefs: support REMAP_FILE_DEDUP in bch2_remap_file_range

[Brian Foster]

On Sat, May 11, 2024 at 12:20:12AM +0000, Reed Riley wrote:
> By removing the early-exit when REMAP_FILE_DEDUP is set, we should be
> able to support the fideduperange ioctl, albeit less efficiently than if
> we handled some of the extent locking and comparison logic inside
> bcachefs.  Extent comparison logic already exists inside of
> `__generic_remap_file_range_prep`.
> 
> Signed-off-by: Reed Riley <reed@riley.engineer>
> ---

Seems reasonable:

Reviewed-by: Brian Foster <bfoster@redhat.com>

Have you run any tests just to make sure there are no surprises? If not,
it looks like xfs_io has a 'dedupe' command that would make it easy to
run a quick test or two from the command line. fstests has a bunch of
tests in the dedupe group (which I presume this patch should now allow
to run on bcachefs) as well.

Brian

>  fs/bcachefs/fs-io.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/fs/bcachefs/fs-io.c b/fs/bcachefs/fs-io.c
> index 20b40477425f..4f513f22a66a 100644
> --- a/fs/bcachefs/fs-io.c
> +++ b/fs/bcachefs/fs-io.c
> @@ -857,9 +857,6 @@ loff_t bch2_remap_file_range(struct file *file_src, loff_t pos_src,
>         if (remap_flags & ~(REMAP_FILE_DEDUP|REMAP_FILE_ADVISORY))
>                 return -EINVAL;
>  
> -       if (remap_flags & REMAP_FILE_DEDUP)
> -               return -EOPNOTSUPP;
> -
>         if ((pos_src & (block_bytes(c) - 1)) ||
>             (pos_dst & (block_bytes(c) - 1)))
>                 return -EINVAL;
> -- 
> 2.44.0
> 
> 

Re: [PATCH] bcachefs: support REMAP_FILE_DEDUP in bch2_remap_file_range

[Reed Riley]

On Monday, May 13th, 2024 at 5:34 AM, Brian Foster <bfoster@redhat.com> wrote:

> On Sat, May 11, 2024 at 12:20:12AM +0000, Reed Riley wrote:
> 
> > By removing the early-exit when REMAP_FILE_DEDUP is set, we should be
> > able to support the fideduperange ioctl, albeit less efficiently than if
> > we handled some of the extent locking and comparison logic inside
> > bcachefs. Extent comparison logic already exists inside of
> > `__generic_remap_file_range_prep`.
> > 
> > Signed-off-by: Reed Riley reed@riley.engineer
> > ---
> 
> 
> Seems reasonable:
> 
> Reviewed-by: Brian Foster bfoster@redhat.com
> 
> 
> Have you run any tests just to make sure there are no surprises? If not,
> it looks like xfs_io has a 'dedupe' command that would make it easy to
> run a quick test or two from the command line. fstests has a bunch of
> tests in the dedupe group (which I presume this patch should now allow
> to run on bcachefs) as well.

I worked with Kent to run his CI tests against the patch (https://evilpiepirate.org/~testdashboard/ci?branch=bcachefs-fideduperange&commit=1945149c8d7549b924cd88f57f0cd938b3bb7125) and also used xfs_io to do some basic sanity checks.

Specifically, I sanity checked that:
 1. fideduperange doesn’t dedupe if file content doesn’t match,
 2. fideduperange does dedupe stuff when they do (according to filefrag -v reporting shared extents), and
 3. That neither of the above operations changed file checksums.

I’d be happy to run more tests if anyone can suggest them?

> 
> Brian
> 
> > fs/bcachefs/fs-io.c | 3 ---
> > 1 file changed, 3 deletions(-)
> > 
> > diff --git a/fs/bcachefs/fs-io.c b/fs/bcachefs/fs-io.c
> > index 20b40477425f..4f513f22a66a 100644
> > --- a/fs/bcachefs/fs-io.c
> > +++ b/fs/bcachefs/fs-io.c
> > @@ -857,9 +857,6 @@ loff_t bch2_remap_file_range(struct file *file_src, loff_t pos_src,
> > if (remap_flags & ~(REMAP_FILE_DEDUP|REMAP_FILE_ADVISORY))
> > return -EINVAL;
> > 
> > - if (remap_flags & REMAP_FILE_DEDUP)
> > - return -EOPNOTSUPP;
> > -
> > if ((pos_src & (block_bytes(c) - 1)) ||
> > (pos_dst & (block_bytes(c) - 1)))
> > return -EINVAL;
> > --
> > 2.44.0

Re: [PATCH] bcachefs: support REMAP_FILE_DEDUP in bch2_remap_file_range

[Brian Foster]

On Mon, May 13, 2024 at 11:42:42PM +0000, Reed Riley wrote:
> On Monday, May 13th, 2024 at 5:34 AM, Brian Foster <bfoster@redhat.com> wrote:
> 
> > On Sat, May 11, 2024 at 12:20:12AM +0000, Reed Riley wrote:
> > 
> > > By removing the early-exit when REMAP_FILE_DEDUP is set, we should be
> > > able to support the fideduperange ioctl, albeit less efficiently than if
> > > we handled some of the extent locking and comparison logic inside
> > > bcachefs. Extent comparison logic already exists inside of
> > > `__generic_remap_file_range_prep`.
> > > 
> > > Signed-off-by: Reed Riley reed@riley.engineer
> > > ---
> > 
> > 
> > Seems reasonable:
> > 
> > Reviewed-by: Brian Foster bfoster@redhat.com
> > 
> > 
> > Have you run any tests just to make sure there are no surprises? If not,
> > it looks like xfs_io has a 'dedupe' command that would make it easy to
> > run a quick test or two from the command line. fstests has a bunch of
> > tests in the dedupe group (which I presume this patch should now allow
> > to run on bcachefs) as well.
> 
> I worked with Kent to run his CI tests against the patch (https://evilpiepirate.org/~testdashboard/ci?branch=bcachefs-fideduperange&commit=1945149c8d7549b924cd88f57f0cd938b3bb7125) and also used xfs_io to do some basic sanity checks.
> 

Ah, great. I threw this up on my test branch yesterday as well just to
see what happens. It looks like it enabled more tests (likely the
fstests that _require_dedupe), which is good to see.

> Specifically, I sanity checked that:
>  1. fideduperange doesn’t dedupe if file content doesn’t match,
>  2. fideduperange does dedupe stuff when they do (according to filefrag -v reporting shared extents), and
>  3. That neither of the above operations changed file checksums.
> 

Makes sense. Thanks!

Brian

> I’d be happy to run more tests if anyone can suggest them?
> 
> > 
> > Brian
> > 
> > > fs/bcachefs/fs-io.c | 3 ---
> > > 1 file changed, 3 deletions(-)
> > > 
> > > diff --git a/fs/bcachefs/fs-io.c b/fs/bcachefs/fs-io.c
> > > index 20b40477425f..4f513f22a66a 100644
> > > --- a/fs/bcachefs/fs-io.c
> > > +++ b/fs/bcachefs/fs-io.c
> > > @@ -857,9 +857,6 @@ loff_t bch2_remap_file_range(struct file *file_src, loff_t pos_src,
> > > if (remap_flags & ~(REMAP_FILE_DEDUP|REMAP_FILE_ADVISORY))
> > > return -EINVAL;
> > > 
> > > - if (remap_flags & REMAP_FILE_DEDUP)
> > > - return -EOPNOTSUPP;
> > > -
> > > if ((pos_src & (block_bytes(c) - 1)) ||
> > > (pos_dst & (block_bytes(c) - 1)))
> > > return -EINVAL;
> > > --
> > > 2.44.0
> 

Re: [PATCH] bcachefs: support REMAP_FILE_DEDUP in bch2_remap_file_range

[Reed Riley]

Any chance of getting this patch merged upstream?

I've been running with it on my system (and using the fideduperange ioctl fairly regularly) for a couple of weeks now with no issues.

Thanks,
Reed


On Tue, May 14, 2024 at 3:58 AM, Brian Foster <bfoster@redhat.com> wrote:

> On Mon, May 13, 2024 at 11:42:42PM +0000, Reed Riley wrote:
> > On Monday, May 13th, 2024 at 5:34 AM, Brian Foster <bfoster@redhat.com> wrote:
> >
> > > On Sat, May 11, 2024 at 12:20:12AM +0000, Reed Riley wrote:
> > >
> > > > By removing the early-exit when REMAP_FILE_DEDUP is set, we should be
> > > > able to support the fideduperange ioctl, albeit less efficiently than if
> > > > we handled some of the extent locking and comparison logic inside
> > > > bcachefs. Extent comparison logic already exists inside of
> > > > `__generic_remap_file_range_prep`.
> > > >
> > > > Signed-off-by: Reed Riley reed@riley.engineer
> > > > ---
> > >
> > >
> > > Seems reasonable:
> > >
> > > Reviewed-by: Brian Foster bfoster@redhat.com
> > >
> > >
> > > Have you run any tests just to make sure there are no surprises? If not,
> > > it looks like xfs_io has a 'dedupe' command that would make it easy to
> > > run a quick test or two from the command line. fstests has a bunch of
> > > tests in the dedupe group (which I presume this patch should now allow
> > > to run on bcachefs) as well.
> >
> > I worked with Kent to run his CI tests against the patch (https://evilpiepirate.org/~testdashboard/ci?branch=bcachefs-fideduperange&commit=1945149c8d7549b924cd88f57f0cd938b3bb7125) and also used xfs_io to do some basic sanity checks.
> >
> 
> Ah, great. I threw this up on my test branch yesterday as well just to
> see what happens. It looks like it enabled more tests (likely the
> fstests that _require_dedupe), which is good to see.
> 
> > Specifically, I sanity checked that:
> > 1. fideduperange doesn’t dedupe if file content doesn’t match,
> > 2. fideduperange does dedupe stuff when they do (according to filefrag -v reporting shared extents), and
> > 3. That neither of the above operations changed file checksums.
> >
> 
> Makes sense. Thanks!
> 
> Brian
> 
> > I’d be happy to run more tests if anyone can suggest them?
> >
> > >
> > > Brian
> > >
> > > > fs/bcachefs/fs-io.c | 3 ---
> > > > 1 file changed, 3 deletions(-)
> > > >
> > > > diff --git a/fs/bcachefs/fs-io.c b/fs/bcachefs/fs-io.c
> > > > index 20b40477425f..4f513f22a66a 100644
> > > > --- a/fs/bcachefs/fs-io.c
> > > > +++ b/fs/bcachefs/fs-io.c
> > > > @@ -857,9 +857,6 @@ loff_t bch2_remap_file_range(struct file *file_src, loff_t pos_src,
> > > > if (remap_flags & ~(REMAP_FILE_DEDUP|REMAP_FILE_ADVISORY))
> > > > return -EINVAL;
> > > >
> > > > - if (remap_flags & REMAP_FILE_DEDUP)
> > > > - return -EOPNOTSUPP;
> > > > -
> > > > if ((pos_src & (block_bytes(c) - 1)) ||
> > > > (pos_dst & (block_bytes(c) - 1)))
> > > > return -EINVAL;
> > > > --
> > > > 2.44.0
> >

Re: [PATCH] bcachefs: support REMAP_FILE_DEDUP in bch2_remap_file_range

[Kent Overstreet]

On Mon, Jun 03, 2024 at 12:05:36AM +0000, Reed Riley wrote:
> Any chance of getting this patch merged upstream?
> 
> I've been running with it on my system (and using the fideduperange
> ioctl fairly regularly) for a couple of weeks now with no issues.

Yep, applied