Re: [PATCH] vf/remap: return the amount of bytes actually deduplicated

[Dave Chinner <david@fromorbit.com>]

On Wed, Jul 13, 2022 at 08:51:44PM +0200, Ansgar Lößer wrote:
> When using the FIDEDUPRANGE ioctl, in case of success the requested size
> is returned. In some cases this might not be the actual amount of bytes
> deduplicated.
> 
> This change modifies vfs_dedupe_file_range() to report the actual amount
> of bytes deduplicated, instead of the requested amount.
> 
> Link: https://lore.kernel.org/linux-fsdevel/5548ef63-62f9-4f46-5793-03165ceccacc@tu-darmstadt.de/
> 
> Reported-by: Ansgar Lößer (ansgar.loesser@kom.tu-darmstadt.de)
> Reported-by: Max Schlecht (max.schlecht@informatik.hu-berlin.de)
> Reported-by: Björn Scheuermann (scheuermann@kom.tu-darmstadt.de)
> Signed-off-by: Ansgar Lößer <ansgar.loesser@kom.tu-darmstadt.de>
> ---
> 
> > Mind sending it with a sign-off and a short commit message?
> > 
> >               Linus
> Sure, thank you!
> 
> This is my first commit, so I hope it is ok like this.
> 
> 
>  fs/remap_range.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/remap_range.c b/fs/remap_range.c
> index e112b5424cdb..072c2c48aeed 100644
> --- a/fs/remap_range.c
> +++ b/fs/remap_range.c
> @@ -546,7 +546,7 @@ int vfs_dedupe_file_range(struct file *file, struct
> file_dedupe_range *same)
>                 else if (deduped < 0)
>                         info->status = deduped;
>                 else
> -                       info->bytes_deduped = len;
> +                       info->bytes_deduped = deduped;
> 
>  next_fdput:
>                 fdput(dst_fd);
> --
> 2.35.1

As I suspected would occur, this change causes test failures. e.g
generic/517 in fstests fails with:

generic/517 1s ... - output mismatch (see /home/dave/src/xfstests-dev/results//xfs_quota/generic/517.out.bad)
--- tests/generic/517.out   2019-10-29 11:47:07.464539451 +1100
+++ /home/dave/src/xfstests-dev/results//xfs_quota/generic/517.out.bad      2022-07-14 18:00:04.833536434 +1000
@@ -13,7 +13,7 @@
*
0786528 ae ae ae ae
0786532
-deduped 131172/131172 bytes at offset 65536
+deduped 131072/131172 bytes at offset 65536
XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
File content after first deduplication and before unmounting:
...
(Run 'diff -u /home/dave/src/xfstests-dev/tests/generic/517.out /home/dave/src/xfstests-dev/results//xfs_quota/generic/517.out.bad'  to see the entire diff)

The whole diff is:

-- /home/dave/src/xfstests-dev/tests/generic/517.out   2019-10-29 11:47:07.464539451 +1100
+++ /home/dave/src/xfstests-dev/results//xfs_quota/generic/517.out.bad  2022-07-14 18:00:04.833536434 +1000
@@ -13,7 +13,7 @@
 *
 0786528 ae ae ae ae
 0786532
-deduped 131172/131172 bytes at offset 65536
+deduped 131072/131172 bytes at offset 65536
 XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 File content after first deduplication and before unmounting:
 0000000 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b
@@ -33,8 +33,6 @@
 0786532
 wrote 100/100 bytes at offset 0
 XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-deduped 100/100 bytes at offset 655360
-XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 File content after second deduplication:
 0000000 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b
 *

This shows user visible API behaviours (i.e. "short dedupe"
behaviour) that may be unexpected by userspace.  So while the change
might be technically correct, it definitely changes the behaviour at
least one test expects to occur.

This is why I asked if this had been tested - it tells us if
userspace is likely to see issues with the API change. "Correct but
breaks tests" is basically a warning that we should expect
users/applications to notice the API change and that there's a good
likelyhood of downstream problems arising from it...

Linus, can you please revert this commit for the 5.19 series (before
the stable autosel bot sends it back to stable kernels, please!) to
give us more time to investigate and consider the impact of the the
API change on userspace applications before we commit to changing
the API.

-Dave.
-- 
Dave Chinner
david@fromorbit.com