Re: [PATCH] generic/517: notrun on NFS due to unaligned dedupe in test

[Trond Myklebust <trondmy@hammerspace.com>]

On Thu, 2019-05-30 at 09:03 -0700, Darrick J. Wong wrote:
> On Thu, May 30, 2019 at 03:55:07PM +0000, Trond Myklebust wrote:
> > Hi Darrick,
> > 
> > On Thu, 2019-05-30 at 08:26 -0700, Darrick J. Wong wrote:
> > > On Thu, May 30, 2019 at 05:41:47PM +0800, Murphy Zhou wrote:
> > > > NFSv4.2 could pass _require_scratch_dedupe, since the test
> > > > offset
> > > > and
> > > > size are aligned, while generic/517 is performing unaligned
> > > > dedupe.
> > > > NFS does not support unaligned dedupe now, returns EINVAL.
> > > > 
> > > > Signed-off-by: Murphy Zhou <xzhou@redhat.com>
> > > > ---
> > > >  tests/generic/517 | 1 +
> > > >  1 file changed, 1 insertion(+)
> > > > 
> > > > diff --git a/tests/generic/517 b/tests/generic/517
> > > > index 601bb24e..23665782 100755
> > > > --- a/tests/generic/517
> > > > +++ b/tests/generic/517
> > > > @@ -30,6 +30,7 @@ _cleanup()
> > > >  _supported_fs generic
> > > >  _supported_os Linux
> > > >  _require_scratch_dedupe
> > > > +$FSTYP == "nfs"  && _notrun "NFS can't handle unaligned
> > > > deduplication"
> > > 
> > > Uh... NFS supports dedupe??
> > > 
> > > Let's see, we pass REMAP_FILE_DEDUP to nfs42_remap_file_range via
> > > @remap_flags.  That function checks remap_flags but never touches
> > > it
> > > again.  It's not passed to nfs42_proc_clone, which (AFAICT) means
> > > that
> > > the nfs client sends a CLONE request to the server on behalf of a
> > > FS_IOC_EXTENT_SAME ioctl.  That seems suspicious to me...
> > > 
> > > The nfs client also doesn't lock and compare the file contents
> > > itself
> > > (the server should be doing that anyway, right?) which means that
> > > dedupe
> > > doesn't fail if the file contents are different?
> > > 
> > > Oh, I see... Xiaoli Feng turned on dedupe for cifs
> > > (b073a08016a10f0)
> > > and
> > > nfs (ce96e888fe48e) even though (the last I heard) neither
> > > protocol
> > > supports dedupe and now will corrupt data in doing so.
> > > 
> > > Let's hold off on this for now while I go email Anna & Steve
> > > about
> > > whether or not nfs and cifs support dedupe.
> > > 
> > 
> > What is the VFS requirement for dedup support?
> > 
> > According to the RFC7862 spec for CLONE: "If SAVED_FH and
> > CURRENT_FH
> > refer to the same file and the source and target ranges overlap,
> > the
> > operation MUST fail with NFS4ERR_INVAL."
> > 
> > So clearly we may not support dedup if there is a requirement that
> > we
> > be able to clone between overlapping ranges on the same file.
> > However I
> > can find no restriction on using CLONE for non-overlapping ranges.
> 
> Heh, concurrent replies. :)
> 
> There isn't, except that the NFS client code doesn't check for
> identical
> contents, nor does it appear to ask the server to do the comparison.
> 
> The VFS can do such comparison via generic_remap_file_range_prep ->
> vfs_dedupe_file_range_compare, but NFS doesn't call the first
> function,
> it just forwards the request to the server and lets the server do all
> the work (including sending back "not supported"), right?
> 
> Admittedly I'm not sure you'd want to do the comparison on the client
> anyway since that involves having the client read /both/ file ranges
> while keeping both files locked against writes on the server.

There is no "atomic_compare_and_dedup()" operation in NFS. Only a
"CLONE" operation, which will support vfs_clone_file_range().

The problem here would appear to be the refactoring that squelched
range based clone and dedup into the same "remap_file_range()"
filesystem level method. That would appear to be confusing people if
the expectation is that filesystems should actually be providing two
different sets of functionality.

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com