On Thu, May 30, 2019 at 04:28:42PM +0000, Trond Myklebust wrote: > 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@xxxxxxxxxx> > > > > > --- > > > > > 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(). <nod> > 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. Documentation/filesystems/vfs.txt says of remap_file_range: "If REMAP_FILE_DEDUP is set then the implementation must only remap if the requested file ranges have identical contents." So yes, there is an expectation that the implementation provide a piece of functionality (remapping extents) and a variation on the theme (remapping extents if they're identical). Anyway, I'll send patches for nfs (and cifs if I hear back from Steve)... --D > -- > Trond Myklebust > Linux NFS client maintainer, Hammerspace > trond.myklebust@xxxxxxxxxxxxxxx > >
