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@xxxxxxxxxxxxxxx/ > > Reported-by: Ansgar Lößer (ansgar.loesser@xxxxxxxxxxxxxxxxxxx) > Reported-by: Max Schlecht (max.schlecht@xxxxxxxxxxxxxxxxxxxxxxx) > Reported-by: Björn Scheuermann (scheuermann@xxxxxxxxxxxxxxxxxxx) > Signed-off-by: Ansgar Lößer <ansgar.loesser@xxxxxxxxxxxxxxxxxxx> > --- > > > 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@xxxxxxxxxxxxx
