* FIDEDUPERANGE with src_length == 0 @ 2016-07-12 0:35 Omar Sandoval 2016-07-13 5:26 ` Darrick J. Wong 0 siblings, 1 reply; 9+ messages in thread From: Omar Sandoval @ 2016-07-12 0:35 UTC (permalink / raw) To: Darrick J. Wong, linux-btrfs; +Cc: linux-fsdevel Hey, Darrick, generic/182 is failing on Btrfs for me with the following output: --- tests/generic/182.out 2016-07-07 19:51:54.000000000 -0700 +++ /tmp/fixxfstests/xfstests/results//generic/182.out.bad 2016-07-11 17:28:28.230039216 -0700 @@ -1,12 +1,10 @@ QA output created by 182 Create the original files -dedupe: Extents did not match. f4820540fc0ac02750739896fe028d56 TEST_DIR/test-182/file1 69ad53078a16243d98e21d9f8704a071 TEST_DIR/test-182/file2 69ad53078a16243d98e21d9f8704a071 TEST_DIR/test-182/file2.chk Compare against check files Make the original file almost dedup-able -dedupe: Extents did not match. f4820540fc0ac02750739896fe028d56 TEST_DIR/test-182/file1 158d4e3578b94b89cbb44493a2110fb9 TEST_DIR/test-182/file2 158d4e3578b94b89cbb44493a2110fb9 TEST_DIR/test-182/file2.chk It looks like that test is checking that a dedupe with length == 0 is treated as a dedupe to EOF, but Btrfs doesn't do that [1]. As far as I can tell, it never did, but maybe I'm just confused. What was the behavior when you introduced that test? That seems like a reasonable thing to do, but I wanted to clear this up before changing/fixing Btrfs. Thanks. 1: http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/fs/btrfs/ioctl.c?h=v4.7-rc7#n3122 -- Omar ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: FIDEDUPERANGE with src_length == 0 2016-07-12 0:35 FIDEDUPERANGE with src_length == 0 Omar Sandoval @ 2016-07-13 5:26 ` Darrick J. Wong 2016-07-13 13:19 ` David Sterba 2016-07-14 2:47 ` Christoph Hellwig 0 siblings, 2 replies; 9+ messages in thread From: Darrick J. Wong @ 2016-07-13 5:26 UTC (permalink / raw) To: Omar Sandoval; +Cc: linux-btrfs, linux-fsdevel On Mon, Jul 11, 2016 at 05:35:37PM -0700, Omar Sandoval wrote: > Hey, Darrick, > > generic/182 is failing on Btrfs for me with the following output: > > --- tests/generic/182.out 2016-07-07 19:51:54.000000000 -0700 > +++ /tmp/fixxfstests/xfstests/results//generic/182.out.bad 2016-07-11 17:28:28.230039216 -0700 > @@ -1,12 +1,10 @@ > QA output created by 182 > Create the original files > -dedupe: Extents did not match. > f4820540fc0ac02750739896fe028d56 TEST_DIR/test-182/file1 > 69ad53078a16243d98e21d9f8704a071 TEST_DIR/test-182/file2 > 69ad53078a16243d98e21d9f8704a071 TEST_DIR/test-182/file2.chk > Compare against check files > Make the original file almost dedup-able > -dedupe: Extents did not match. > f4820540fc0ac02750739896fe028d56 TEST_DIR/test-182/file1 > 158d4e3578b94b89cbb44493a2110fb9 TEST_DIR/test-182/file2 > 158d4e3578b94b89cbb44493a2110fb9 TEST_DIR/test-182/file2.chk > > It looks like that test is checking that a dedupe with length == 0 is > treated as a dedupe to EOF, but Btrfs doesn't do that [1]. As far as I > can tell, it never did, but maybe I'm just confused. What was the > behavior when you introduced that test? That seems like a reasonable > thing to do, but I wanted to clear this up before changing/fixing Btrfs. It's a shortcut that we're introducing in the upcoming XFS implementation, since it shares the same back end as clone/clonerange, which both have this behavior. --D > > Thanks. > > 1: http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/fs/btrfs/ioctl.c?h=v4.7-rc7#n3122 > -- > Omar ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: FIDEDUPERANGE with src_length == 0 2016-07-13 5:26 ` Darrick J. Wong @ 2016-07-13 13:19 ` David Sterba 2016-07-14 18:06 ` Darrick J. Wong 2016-07-14 2:47 ` Christoph Hellwig 1 sibling, 1 reply; 9+ messages in thread From: David Sterba @ 2016-07-13 13:19 UTC (permalink / raw) To: Darrick J. Wong; +Cc: Omar Sandoval, linux-btrfs, linux-fsdevel On Tue, Jul 12, 2016 at 10:26:43PM -0700, Darrick J. Wong wrote: > On Mon, Jul 11, 2016 at 05:35:37PM -0700, Omar Sandoval wrote: > > Hey, Darrick, > > > > generic/182 is failing on Btrfs for me with the following output: > > > > --- tests/generic/182.out 2016-07-07 19:51:54.000000000 -0700 > > +++ /tmp/fixxfstests/xfstests/results//generic/182.out.bad 2016-07-11 17:28:28.230039216 -0700 > > @@ -1,12 +1,10 @@ > > QA output created by 182 > > Create the original files > > -dedupe: Extents did not match. > > f4820540fc0ac02750739896fe028d56 TEST_DIR/test-182/file1 > > 69ad53078a16243d98e21d9f8704a071 TEST_DIR/test-182/file2 > > 69ad53078a16243d98e21d9f8704a071 TEST_DIR/test-182/file2.chk > > Compare against check files > > Make the original file almost dedup-able > > -dedupe: Extents did not match. > > f4820540fc0ac02750739896fe028d56 TEST_DIR/test-182/file1 > > 158d4e3578b94b89cbb44493a2110fb9 TEST_DIR/test-182/file2 > > 158d4e3578b94b89cbb44493a2110fb9 TEST_DIR/test-182/file2.chk > > > > It looks like that test is checking that a dedupe with length == 0 is > > treated as a dedupe to EOF, but Btrfs doesn't do that [1]. As far as I > > can tell, it never did, but maybe I'm just confused. What was the > > behavior when you introduced that test? That seems like a reasonable > > thing to do, but I wanted to clear this up before changing/fixing Btrfs. > > It's a shortcut that we're introducing in the upcoming XFS implementation, > since it shares the same back end as clone/clonerange, which both have > this behavior. The support for zero length does not seem to be mentioned anywhere with the dedupe range ioctl [1], so the current implemetnation is "up to spec". That it should be valid is hidden in clone_verify_area where a zero length is substituted with OFFSET_MAX http://lxr.free-electrons.com/source/fs/read_write.c#L1607 So it looks like it's up to the implementation in the filesystem to handle that. As the btrfs ioctl was extent-based, a zero length extent does not make sense, so this case was not handled. But in your patch 2b3909f8a7fe94e0234850aa9d120cca15b6e1f7 btrfs: use new dedupe data function pointer it was suddenly expected to work. So the missing bits are either 'not supported' for zero length or actually implement iteration over the whole file. [1] https://www.mankier.com/2/ioctl_fideduperange ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: FIDEDUPERANGE with src_length == 0 2016-07-13 13:19 ` David Sterba @ 2016-07-14 18:06 ` Darrick J. Wong 2016-07-14 18:12 ` Chris Mason 0 siblings, 1 reply; 9+ messages in thread From: Darrick J. Wong @ 2016-07-14 18:06 UTC (permalink / raw) To: dsterba, Omar Sandoval, linux-btrfs, linux-fsdevel On Wed, Jul 13, 2016 at 03:19:38PM +0200, David Sterba wrote: > On Tue, Jul 12, 2016 at 10:26:43PM -0700, Darrick J. Wong wrote: > > On Mon, Jul 11, 2016 at 05:35:37PM -0700, Omar Sandoval wrote: > > > Hey, Darrick, > > > > > > generic/182 is failing on Btrfs for me with the following output: > > > > > > --- tests/generic/182.out 2016-07-07 19:51:54.000000000 -0700 > > > +++ /tmp/fixxfstests/xfstests/results//generic/182.out.bad 2016-07-11 17:28:28.230039216 -0700 > > > @@ -1,12 +1,10 @@ > > > QA output created by 182 > > > Create the original files > > > -dedupe: Extents did not match. > > > f4820540fc0ac02750739896fe028d56 TEST_DIR/test-182/file1 > > > 69ad53078a16243d98e21d9f8704a071 TEST_DIR/test-182/file2 > > > 69ad53078a16243d98e21d9f8704a071 TEST_DIR/test-182/file2.chk > > > Compare against check files > > > Make the original file almost dedup-able > > > -dedupe: Extents did not match. > > > f4820540fc0ac02750739896fe028d56 TEST_DIR/test-182/file1 > > > 158d4e3578b94b89cbb44493a2110fb9 TEST_DIR/test-182/file2 > > > 158d4e3578b94b89cbb44493a2110fb9 TEST_DIR/test-182/file2.chk > > > > > > It looks like that test is checking that a dedupe with length == 0 is > > > treated as a dedupe to EOF, but Btrfs doesn't do that [1]. As far as I > > > can tell, it never did, but maybe I'm just confused. What was the > > > behavior when you introduced that test? That seems like a reasonable > > > thing to do, but I wanted to clear this up before changing/fixing Btrfs. > > > > It's a shortcut that we're introducing in the upcoming XFS implementation, > > since it shares the same back end as clone/clonerange, which both have > > this behavior. > > The support for zero length does not seem to be mentioned anywhere with > the dedupe range ioctl [1], so the current implemetnation is "up to > spec". That it should be valid is hidden in clone_verify_area where a > zero length is substituted with OFFSET_MAX > > http://lxr.free-electrons.com/source/fs/read_write.c#L1607 > > So it looks like it's up to the implementation in the filesystem to > handle that. As the btrfs ioctl was extent-based, a zero length extent > does not make sense, so this case was not handled. But in your patch > > 2b3909f8a7fe94e0234850aa9d120cca15b6e1f7 > btrfs: use new dedupe data function pointer > > it was suddenly expected to work. So the missing bits are either 'not > supported' for zero length or actually implement iteration over the > whole file. > > [1] https://www.mankier.com/2/ioctl_fideduperange Well, we can't change the semantics now because there could be programs that aren't expecting a nonzero return from a length == 0 dedupe, so like Christoph said, I'll just change generic/182 and make the VFS wrapper emulate the btrfs behavior so that any subsequent implementation won't hit this. I'll update the clone/clonerange manpages to mention the 0 -> EOF behavior. Would've been awesome if anyone had bothered to document the damn ioctls and write tests to call out these subtleties rather than leave the next implementer to clean up all the technical debt. I dunno, maybe there /is/ a secret cache of btrfs regression tests somewhere that I haven't found. <grumble> --D ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: FIDEDUPERANGE with src_length == 0 2016-07-14 18:06 ` Darrick J. Wong @ 2016-07-14 18:12 ` Chris Mason 2016-07-14 18:16 ` Omar Sandoval 0 siblings, 1 reply; 9+ messages in thread From: Chris Mason @ 2016-07-14 18:12 UTC (permalink / raw) To: Darrick J. Wong, dsterba, Omar Sandoval, linux-btrfs, linux-fsdevel On 07/14/2016 02:06 PM, Darrick J. Wong wrote: > On Wed, Jul 13, 2016 at 03:19:38PM +0200, David Sterba wrote: >> On Tue, Jul 12, 2016 at 10:26:43PM -0700, Darrick J. Wong wrote: >>> On Mon, Jul 11, 2016 at 05:35:37PM -0700, Omar Sandoval wrote: >>>> Hey, Darrick, >>>> >>>> generic/182 is failing on Btrfs for me with the following output: >>>> >>>> --- tests/generic/182.out 2016-07-07 19:51:54.000000000 -0700 >>>> +++ /tmp/fixxfstests/xfstests/results//generic/182.out.bad 2016-07-11 17:28:28.230039216 -0700 >>>> @@ -1,12 +1,10 @@ >>>> QA output created by 182 >>>> Create the original files >>>> -dedupe: Extents did not match. >>>> f4820540fc0ac02750739896fe028d56 TEST_DIR/test-182/file1 >>>> 69ad53078a16243d98e21d9f8704a071 TEST_DIR/test-182/file2 >>>> 69ad53078a16243d98e21d9f8704a071 TEST_DIR/test-182/file2.chk >>>> Compare against check files >>>> Make the original file almost dedup-able >>>> -dedupe: Extents did not match. >>>> f4820540fc0ac02750739896fe028d56 TEST_DIR/test-182/file1 >>>> 158d4e3578b94b89cbb44493a2110fb9 TEST_DIR/test-182/file2 >>>> 158d4e3578b94b89cbb44493a2110fb9 TEST_DIR/test-182/file2.chk >>>> >>>> It looks like that test is checking that a dedupe with length == 0 is >>>> treated as a dedupe to EOF, but Btrfs doesn't do that [1]. As far as I >>>> can tell, it never did, but maybe I'm just confused. What was the >>>> behavior when you introduced that test? That seems like a reasonable >>>> thing to do, but I wanted to clear this up before changing/fixing Btrfs. >>> >>> It's a shortcut that we're introducing in the upcoming XFS implementation, >>> since it shares the same back end as clone/clonerange, which both have >>> this behavior. >> >> The support for zero length does not seem to be mentioned anywhere with >> the dedupe range ioctl [1], so the current implemetnation is "up to >> spec". That it should be valid is hidden in clone_verify_area where a >> zero length is substituted with OFFSET_MAX >> >> https://urldefense.proofpoint.com/v2/url?u=http-3A__lxr.free-2Delectrons.com_source_fs_read-5Fwrite.c-23L1607&d=CwIBAg&c=5VD0RTtNlTh3ycd41b3MUw&r=9QPtTAxcitoznaWRKKHoEQ&m=CKo3CgE8Up_NBDdC9t7fCuwHwsdf6nZG2nKcl5-NqnI&s=ZymMvbZ2mZOYBKya3guibggSaaqOHZUqedhz0pT5PPc&e= >> >> So it looks like it's up to the implementation in the filesystem to >> handle that. As the btrfs ioctl was extent-based, a zero length extent >> does not make sense, so this case was not handled. But in your patch >> >> 2b3909f8a7fe94e0234850aa9d120cca15b6e1f7 >> btrfs: use new dedupe data function pointer >> >> it was suddenly expected to work. So the missing bits are either 'not >> supported' for zero length or actually implement iteration over the >> whole file. >> >> [1] https://urldefense.proofpoint.com/v2/url?u=https-3A__www.mankier.com_2_ioctl-5Ffideduperange&d=CwIBAg&c=5VD0RTtNlTh3ycd41b3MUw&r=9QPtTAxcitoznaWRKKHoEQ&m=CKo3CgE8Up_NBDdC9t7fCuwHwsdf6nZG2nKcl5-NqnI&s=NYdHr9JyZZNKPLsOf_VmtZ-3X2B1azTYfyE4Lf1Fa5w&e= > > Well, we can't change the semantics now because there could be programs that > aren't expecting a nonzero return from a length == 0 dedupe, so like Christoph > said, I'll just change generic/182 and make the VFS wrapper emulate the btrfs > behavior so that any subsequent implementation won't hit this. > > I'll update the clone/clonerange manpages to mention the 0 -> EOF behavior. Its fine with me if we change btrfs to do the 0->EOF. It's a corner case I'm happy to include. -chris ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: FIDEDUPERANGE with src_length == 0 2016-07-14 18:12 ` Chris Mason @ 2016-07-14 18:16 ` Omar Sandoval 2016-07-15 16:54 ` Darrick J. Wong 2016-08-01 19:32 ` Darrick J. Wong 0 siblings, 2 replies; 9+ messages in thread From: Omar Sandoval @ 2016-07-14 18:16 UTC (permalink / raw) To: Chris Mason; +Cc: Darrick J. Wong, dsterba, linux-btrfs, linux-fsdevel On Thu, Jul 14, 2016 at 02:12:58PM -0400, Chris Mason wrote: > > > On 07/14/2016 02:06 PM, Darrick J. Wong wrote: > > On Wed, Jul 13, 2016 at 03:19:38PM +0200, David Sterba wrote: > > > On Tue, Jul 12, 2016 at 10:26:43PM -0700, Darrick J. Wong wrote: > > > > On Mon, Jul 11, 2016 at 05:35:37PM -0700, Omar Sandoval wrote: > > > > > Hey, Darrick, > > > > > > > > > > generic/182 is failing on Btrfs for me with the following output: > > > > > > > > > > --- tests/generic/182.out 2016-07-07 19:51:54.000000000 -0700 > > > > > +++ /tmp/fixxfstests/xfstests/results//generic/182.out.bad 2016-07-11 17:28:28.230039216 -0700 > > > > > @@ -1,12 +1,10 @@ > > > > > QA output created by 182 > > > > > Create the original files > > > > > -dedupe: Extents did not match. > > > > > f4820540fc0ac02750739896fe028d56 TEST_DIR/test-182/file1 > > > > > 69ad53078a16243d98e21d9f8704a071 TEST_DIR/test-182/file2 > > > > > 69ad53078a16243d98e21d9f8704a071 TEST_DIR/test-182/file2.chk > > > > > Compare against check files > > > > > Make the original file almost dedup-able > > > > > -dedupe: Extents did not match. > > > > > f4820540fc0ac02750739896fe028d56 TEST_DIR/test-182/file1 > > > > > 158d4e3578b94b89cbb44493a2110fb9 TEST_DIR/test-182/file2 > > > > > 158d4e3578b94b89cbb44493a2110fb9 TEST_DIR/test-182/file2.chk > > > > > > > > > > It looks like that test is checking that a dedupe with length == 0 is > > > > > treated as a dedupe to EOF, but Btrfs doesn't do that [1]. As far as I > > > > > can tell, it never did, but maybe I'm just confused. What was the > > > > > behavior when you introduced that test? That seems like a reasonable > > > > > thing to do, but I wanted to clear this up before changing/fixing Btrfs. > > > > > > > > It's a shortcut that we're introducing in the upcoming XFS implementation, > > > > since it shares the same back end as clone/clonerange, which both have > > > > this behavior. > > > > > > The support for zero length does not seem to be mentioned anywhere with > > > the dedupe range ioctl [1], so the current implemetnation is "up to > > > spec". That it should be valid is hidden in clone_verify_area where a > > > zero length is substituted with OFFSET_MAX > > > > > > https://urldefense.proofpoint.com/v2/url?u=http-3A__lxr.free-2Delectrons.com_source_fs_read-5Fwrite.c-23L1607&d=CwIBAg&c=5VD0RTtNlTh3ycd41b3MUw&r=9QPtTAxcitoznaWRKKHoEQ&m=CKo3CgE8Up_NBDdC9t7fCuwHwsdf6nZG2nKcl5-NqnI&s=ZymMvbZ2mZOYBKya3guibggSaaqOHZUqedhz0pT5PPc&e= > > > > > > So it looks like it's up to the implementation in the filesystem to > > > handle that. As the btrfs ioctl was extent-based, a zero length extent > > > does not make sense, so this case was not handled. But in your patch > > > > > > 2b3909f8a7fe94e0234850aa9d120cca15b6e1f7 > > > btrfs: use new dedupe data function pointer > > > > > > it was suddenly expected to work. So the missing bits are either 'not > > > supported' for zero length or actually implement iteration over the > > > whole file. > > > > > > [1] https://urldefense.proofpoint.com/v2/url?u=https-3A__www.mankier.com_2_ioctl-5Ffideduperange&d=CwIBAg&c=5VD0RTtNlTh3ycd41b3MUw&r=9QPtTAxcitoznaWRKKHoEQ&m=CKo3CgE8Up_NBDdC9t7fCuwHwsdf6nZG2nKcl5-NqnI&s=NYdHr9JyZZNKPLsOf_VmtZ-3X2B1azTYfyE4Lf1Fa5w&e= > > > > Well, we can't change the semantics now because there could be programs that > > aren't expecting a nonzero return from a length == 0 dedupe, so like Christoph > > said, I'll just change generic/182 and make the VFS wrapper emulate the btrfs > > behavior so that any subsequent implementation won't hit this. > > > > I'll update the clone/clonerange manpages to mention the 0 -> EOF behavior. > > Its fine with me if we change btrfs to do the 0->EOF. It's a corner case > I'm happy to include. > > -chris Yeah, I think it's a nice shortcut. Are there any programs which wouldn't want this, though? It's a milder sort of correctness problem since dedupe is "safe", but maybe there's some tool which is being dumb and trying to dedupe nothing. -- Omar ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: FIDEDUPERANGE with src_length == 0 2016-07-14 18:16 ` Omar Sandoval @ 2016-07-15 16:54 ` Darrick J. Wong 2016-08-01 19:32 ` Darrick J. Wong 1 sibling, 0 replies; 9+ messages in thread From: Darrick J. Wong @ 2016-07-15 16:54 UTC (permalink / raw) To: Omar Sandoval; +Cc: Chris Mason, dsterba, linux-btrfs, linux-fsdevel On Thu, Jul 14, 2016 at 11:16:47AM -0700, Omar Sandoval wrote: > On Thu, Jul 14, 2016 at 02:12:58PM -0400, Chris Mason wrote: > > > > > > On 07/14/2016 02:06 PM, Darrick J. Wong wrote: > > > On Wed, Jul 13, 2016 at 03:19:38PM +0200, David Sterba wrote: > > > > On Tue, Jul 12, 2016 at 10:26:43PM -0700, Darrick J. Wong wrote: > > > > > On Mon, Jul 11, 2016 at 05:35:37PM -0700, Omar Sandoval wrote: > > > > > > Hey, Darrick, > > > > > > > > > > > > generic/182 is failing on Btrfs for me with the following output: > > > > > > > > > > > > --- tests/generic/182.out 2016-07-07 19:51:54.000000000 -0700 > > > > > > +++ /tmp/fixxfstests/xfstests/results//generic/182.out.bad 2016-07-11 17:28:28.230039216 -0700 > > > > > > @@ -1,12 +1,10 @@ > > > > > > QA output created by 182 > > > > > > Create the original files > > > > > > -dedupe: Extents did not match. > > > > > > f4820540fc0ac02750739896fe028d56 TEST_DIR/test-182/file1 > > > > > > 69ad53078a16243d98e21d9f8704a071 TEST_DIR/test-182/file2 > > > > > > 69ad53078a16243d98e21d9f8704a071 TEST_DIR/test-182/file2.chk > > > > > > Compare against check files > > > > > > Make the original file almost dedup-able > > > > > > -dedupe: Extents did not match. > > > > > > f4820540fc0ac02750739896fe028d56 TEST_DIR/test-182/file1 > > > > > > 158d4e3578b94b89cbb44493a2110fb9 TEST_DIR/test-182/file2 > > > > > > 158d4e3578b94b89cbb44493a2110fb9 TEST_DIR/test-182/file2.chk > > > > > > > > > > > > It looks like that test is checking that a dedupe with length == 0 is > > > > > > treated as a dedupe to EOF, but Btrfs doesn't do that [1]. As far as I > > > > > > can tell, it never did, but maybe I'm just confused. What was the > > > > > > behavior when you introduced that test? That seems like a reasonable > > > > > > thing to do, but I wanted to clear this up before changing/fixing Btrfs. > > > > > > > > > > It's a shortcut that we're introducing in the upcoming XFS implementation, > > > > > since it shares the same back end as clone/clonerange, which both have > > > > > this behavior. > > > > > > > > The support for zero length does not seem to be mentioned anywhere with > > > > the dedupe range ioctl [1], so the current implemetnation is "up to > > > > spec". That it should be valid is hidden in clone_verify_area where a > > > > zero length is substituted with OFFSET_MAX > > > > > > > > https://urldefense.proofpoint.com/v2/url?u=http-3A__lxr.free-2Delectrons.com_source_fs_read-5Fwrite.c-23L1607&d=CwIBAg&c=5VD0RTtNlTh3ycd41b3MUw&r=9QPtTAxcitoznaWRKKHoEQ&m=CKo3CgE8Up_NBDdC9t7fCuwHwsdf6nZG2nKcl5-NqnI&s=ZymMvbZ2mZOYBKya3guibggSaaqOHZUqedhz0pT5PPc&e= > > > > > > > > So it looks like it's up to the implementation in the filesystem to > > > > handle that. As the btrfs ioctl was extent-based, a zero length extent > > > > does not make sense, so this case was not handled. But in your patch > > > > > > > > 2b3909f8a7fe94e0234850aa9d120cca15b6e1f7 > > > > btrfs: use new dedupe data function pointer > > > > > > > > it was suddenly expected to work. So the missing bits are either 'not > > > > supported' for zero length or actually implement iteration over the > > > > whole file. > > > > > > > > [1] https://urldefense.proofpoint.com/v2/url?u=https-3A__www.mankier.com_2_ioctl-5Ffideduperange&d=CwIBAg&c=5VD0RTtNlTh3ycd41b3MUw&r=9QPtTAxcitoznaWRKKHoEQ&m=CKo3CgE8Up_NBDdC9t7fCuwHwsdf6nZG2nKcl5-NqnI&s=NYdHr9JyZZNKPLsOf_VmtZ-3X2B1azTYfyE4Lf1Fa5w&e= > > > > > > Well, we can't change the semantics now because there could be programs that > > > aren't expecting a nonzero return from a length == 0 dedupe, so like Christoph > > > said, I'll just change generic/182 and make the VFS wrapper emulate the btrfs > > > behavior so that any subsequent implementation won't hit this. > > > > > > I'll update the clone/clonerange manpages to mention the 0 -> EOF behavior. > > > > Its fine with me if we change btrfs to do the 0->EOF. It's a corner case > > I'm happy to include. > > > > -chris > > Yeah, I think it's a nice shortcut. Are there any programs which > wouldn't want this, though? It's a milder sort of correctness problem > since dedupe is "safe", but maybe there's some tool which is being dumb > and trying to dedupe nothing. <shrug> The only problems I can see here is some program that calls dedupe with a length == 0 /and/ doesn't expect a non-zero return value... or gets confused that bytes_deduped > 0. I don't think duperemove has either of those problems. Is that the only client? --D > > -- > Omar ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: FIDEDUPERANGE with src_length == 0 2016-07-14 18:16 ` Omar Sandoval 2016-07-15 16:54 ` Darrick J. Wong @ 2016-08-01 19:32 ` Darrick J. Wong 1 sibling, 0 replies; 9+ messages in thread From: Darrick J. Wong @ 2016-08-01 19:32 UTC (permalink / raw) To: Omar Sandoval; +Cc: Chris Mason, dsterba, linux-btrfs, linux-fsdevel On Thu, Jul 14, 2016 at 11:16:47AM -0700, Omar Sandoval wrote: > On Thu, Jul 14, 2016 at 02:12:58PM -0400, Chris Mason wrote: > > > > > > On 07/14/2016 02:06 PM, Darrick J. Wong wrote: > > > On Wed, Jul 13, 2016 at 03:19:38PM +0200, David Sterba wrote: > > > > On Tue, Jul 12, 2016 at 10:26:43PM -0700, Darrick J. Wong wrote: > > > > > On Mon, Jul 11, 2016 at 05:35:37PM -0700, Omar Sandoval wrote: > > > > > > Hey, Darrick, > > > > > > > > > > > > generic/182 is failing on Btrfs for me with the following output: > > > > > > > > > > > > --- tests/generic/182.out 2016-07-07 19:51:54.000000000 -0700 > > > > > > +++ /tmp/fixxfstests/xfstests/results//generic/182.out.bad 2016-07-11 17:28:28.230039216 -0700 > > > > > > @@ -1,12 +1,10 @@ > > > > > > QA output created by 182 > > > > > > Create the original files > > > > > > -dedupe: Extents did not match. > > > > > > f4820540fc0ac02750739896fe028d56 TEST_DIR/test-182/file1 > > > > > > 69ad53078a16243d98e21d9f8704a071 TEST_DIR/test-182/file2 > > > > > > 69ad53078a16243d98e21d9f8704a071 TEST_DIR/test-182/file2.chk > > > > > > Compare against check files > > > > > > Make the original file almost dedup-able > > > > > > -dedupe: Extents did not match. > > > > > > f4820540fc0ac02750739896fe028d56 TEST_DIR/test-182/file1 > > > > > > 158d4e3578b94b89cbb44493a2110fb9 TEST_DIR/test-182/file2 > > > > > > 158d4e3578b94b89cbb44493a2110fb9 TEST_DIR/test-182/file2.chk > > > > > > > > > > > > It looks like that test is checking that a dedupe with length == 0 is > > > > > > treated as a dedupe to EOF, but Btrfs doesn't do that [1]. As far as I > > > > > > can tell, it never did, but maybe I'm just confused. What was the > > > > > > behavior when you introduced that test? That seems like a reasonable > > > > > > thing to do, but I wanted to clear this up before changing/fixing Btrfs. > > > > > > > > > > It's a shortcut that we're introducing in the upcoming XFS implementation, > > > > > since it shares the same back end as clone/clonerange, which both have > > > > > this behavior. > > > > > > > > The support for zero length does not seem to be mentioned anywhere with > > > > the dedupe range ioctl [1], so the current implemetnation is "up to > > > > spec". That it should be valid is hidden in clone_verify_area where a > > > > zero length is substituted with OFFSET_MAX > > > > > > > > https://urldefense.proofpoint.com/v2/url?u=http-3A__lxr.free-2Delectrons.com_source_fs_read-5Fwrite.c-23L1607&d=CwIBAg&c=5VD0RTtNlTh3ycd41b3MUw&r=9QPtTAxcitoznaWRKKHoEQ&m=CKo3CgE8Up_NBDdC9t7fCuwHwsdf6nZG2nKcl5-NqnI&s=ZymMvbZ2mZOYBKya3guibggSaaqOHZUqedhz0pT5PPc&e= > > > > > > > > So it looks like it's up to the implementation in the filesystem to > > > > handle that. As the btrfs ioctl was extent-based, a zero length extent > > > > does not make sense, so this case was not handled. But in your patch > > > > > > > > 2b3909f8a7fe94e0234850aa9d120cca15b6e1f7 > > > > btrfs: use new dedupe data function pointer > > > > > > > > it was suddenly expected to work. So the missing bits are either 'not > > > > supported' for zero length or actually implement iteration over the > > > > whole file. > > > > > > > > [1] https://urldefense.proofpoint.com/v2/url?u=https-3A__www.mankier.com_2_ioctl-5Ffideduperange&d=CwIBAg&c=5VD0RTtNlTh3ycd41b3MUw&r=9QPtTAxcitoznaWRKKHoEQ&m=CKo3CgE8Up_NBDdC9t7fCuwHwsdf6nZG2nKcl5-NqnI&s=NYdHr9JyZZNKPLsOf_VmtZ-3X2B1azTYfyE4Lf1Fa5w&e= > > > > > > Well, we can't change the semantics now because there could be programs that > > > aren't expecting a nonzero return from a length == 0 dedupe, so like Christoph > > > said, I'll just change generic/182 and make the VFS wrapper emulate the btrfs > > > behavior so that any subsequent implementation won't hit this. > > > > > > I'll update the clone/clonerange manpages to mention the 0 -> EOF behavior. > > > > Its fine with me if we change btrfs to do the 0->EOF. It's a corner case > > I'm happy to include. > > > > -chris > > Yeah, I think it's a nice shortcut. Are there any programs which > wouldn't want this, though? It's a milder sort of correctness problem > since dedupe is "safe", but maybe there's some tool which is being dumb > and trying to dedupe nothing. The only users of extent-same that I know of are duperemove and xfs_io. duperemove doesn't seem to send zero-length dedupe requests. xfs_io will if you tell it to, but the xfs_io feature is there primarily to run xfstests. Christoph has a valid point that we don't know the full set of users, so we could be breaking them by changing this aspect. OTOH this was an undocumented ioctl for quite a while... --D > > -- > Omar ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: FIDEDUPERANGE with src_length == 0 2016-07-13 5:26 ` Darrick J. Wong 2016-07-13 13:19 ` David Sterba @ 2016-07-14 2:47 ` Christoph Hellwig 1 sibling, 0 replies; 9+ messages in thread From: Christoph Hellwig @ 2016-07-14 2:47 UTC (permalink / raw) To: Darrick J. Wong; +Cc: Omar Sandoval, linux-btrfs, linux-fsdevel On Tue, Jul 12, 2016 at 10:26:43PM -0700, Darrick J. Wong wrote: > It's a shortcut that we're introducing in the upcoming XFS implementation, > since it shares the same back end as clone/clonerange, which both have > this behavior. We will have to preserve the existing btrfs semantics when reusing the ioctl number. ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2016-08-01 19:34 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-07-12 0:35 FIDEDUPERANGE with src_length == 0 Omar Sandoval 2016-07-13 5:26 ` Darrick J. Wong 2016-07-13 13:19 ` David Sterba 2016-07-14 18:06 ` Darrick J. Wong 2016-07-14 18:12 ` Chris Mason 2016-07-14 18:16 ` Omar Sandoval 2016-07-15 16:54 ` Darrick J. Wong 2016-08-01 19:32 ` Darrick J. Wong 2016-07-14 2:47 ` Christoph Hellwig
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).