| Message ID | 20181004222733.GM19324@magnolia (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xfsprogs-4.19: transaction cleanups | expand |
On 10/4/18 5:27 PM, Darrick J. Wong wrote: > From: Darrick J. Wong <darrick.wong@oracle.com> > > The dedupe command should only complain about non-matching extents if > the kernel hasn't managed to dedupe /any/ of the input range. > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> Should it be "no extents matched" then perhaps? I mean xfs_io is not exactly-quite a purpose-built dedupe tool, but should we be a bit more specific if we're issuing a message at all? if (info->status == XFS_EXTENT_DATA_DIFFERS) { if (deduped == 0) fprintf(stderr, "XFS_IOC_FILE_EXTENT_SAME: %s\n", _("No extents matched.")); else fprintf(stderr, "XFS_IOC_FILE_EXTENT_SAME: %s\n", _("Some extents did not match.")); } And the manpage implies that any difference will make it fail: > map length bytes at offset dst_offset in the open file to the same physical > blocks that are mapped at offset src_offset in the file src_file, but only > if the contents of both ranges are identical. now you're telling me it'll make a best effort? ;) I think the manpage needs clarification too ... > --- > io/reflink.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/io/reflink.c b/io/reflink.c > index 26eb2e32..72dfe32d 100644 > --- a/io/reflink.c > +++ b/io/reflink.c > @@ -70,7 +70,8 @@ dedupe_ioctl( > _(strerror(-info->status))); > goto done; > } > - if (info->status == XFS_EXTENT_DATA_DIFFERS) { > + if (deduped == 0 && > + info->status == XFS_EXTENT_DATA_DIFFERS) { > fprintf(stderr, "XFS_IOC_FILE_EXTENT_SAME: %s\n", > _("Extents did not match.")); > goto done; >
On Fri, Oct 05, 2018 at 04:37:14PM -0500, Eric Sandeen wrote: > On 10/4/18 5:27 PM, Darrick J. Wong wrote: > > From: Darrick J. Wong <darrick.wong@oracle.com> > > > > The dedupe command should only complain about non-matching extents if > > the kernel hasn't managed to dedupe /any/ of the input range. > > > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > > Should it be "no extents matched" then perhaps? > > I mean xfs_io is not exactly-quite a purpose-built dedupe tool, but should > we be a bit more specific if we're issuing a message at all? > > if (info->status == XFS_EXTENT_DATA_DIFFERS) { > if (deduped == 0) > fprintf(stderr, "XFS_IOC_FILE_EXTENT_SAME: %s\n", > _("No extents matched.")); > else > fprintf(stderr, "XFS_IOC_FILE_EXTENT_SAME: %s\n", > _("Some extents did not match.")); We don't need to tell the user that /some/ extents did not match. If we have two files containing "moo cow" and "moo bow", you'd agree that the first four bytes match, right? So the output should be: $ xfs_io -c 'dedupe /a 0 0 7' /b linked 4/7 bytes at offset 0 4 bytes, 1 ops; 0.0000 sec The "4/7" tells us that some extents didn't match, because we didn't fulfill the entire range requested. Pretend that this fs can dedupe at byte alignment. Now if the files contained "boo cow" and "aaaaaaa", you'd expect: $ xfs_io -c 'dedupe /a 0 0 7' /b XFS_IOC_EXTENT_SAME: No extents matched. And if the files contained "moo cow" and "moo cow": $ xfs_io -c 'dedupe /a 0 0 7' /b linked 7/7 bytes at offset 0 7 bytes, 1 ops; 0.0000 sec > } > > And the manpage implies that any difference will make it fail: > > > map length bytes at offset dst_offset in the open file to the same physical > > blocks that are mapped at offset src_offset in the file src_file, but only > > if the contents of both ranges are identical. It also says: "Upon successful completion of this ioctl, the number of bytes successfully deduplicated is returned in bytes_deduped...." Which contradicts: "If even a single byte in the range does not match, the deduplication request will be ignored and status set to FILE_DEDUPE_RANGE_DIFFERS. But that makes no sense because if our only choices were really "all the bytes" or "none of the bytes" then there wouldn't be a bytes_deduped. > now you're telling me it'll make a best effort? ;) I think the manpage > needs clarification too ... Yep. --D > > > --- > > > > io/reflink.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/io/reflink.c b/io/reflink.c > > index 26eb2e32..72dfe32d 100644 > > --- a/io/reflink.c > > +++ b/io/reflink.c > > @@ -70,7 +70,8 @@ dedupe_ioctl( > > _(strerror(-info->status))); > > goto done; > > } > > - if (info->status == XFS_EXTENT_DATA_DIFFERS) { > > + if (deduped == 0 && > > + info->status == XFS_EXTENT_DATA_DIFFERS) { > > fprintf(stderr, "XFS_IOC_FILE_EXTENT_SAME: %s\n", > > _("Extents did not match.")); > > goto done; > >
diff --git a/io/reflink.c b/io/reflink.c index 26eb2e32..72dfe32d 100644 --- a/io/reflink.c +++ b/io/reflink.c @@ -70,7 +70,8 @@ dedupe_ioctl( _(strerror(-info->status))); goto done; } - if (info->status == XFS_EXTENT_DATA_DIFFERS) { + if (deduped == 0 && + info->status == XFS_EXTENT_DATA_DIFFERS) { fprintf(stderr, "XFS_IOC_FILE_EXTENT_SAME: %s\n", _("Extents did not match.")); goto done;