[PATCHSET] support FSCTL_DUPLICATE_EXTENTS_TO_FILE

[David Disseldorp]

Thanks a lot for the review, Ralph!

Please find a version 2 patchset attached. Further comments below...

On Sun, 7 May 2017 13:30:48 +0200, Ralph Böhme wrote:

> Hi David,
> 
> nice work! Some nitpicks below...
> 
> On Thu, May 04, 2017 at 05:30:35PM +0200, David Disseldorp via samba-technical wrote:
> > Please find attached a patchset from Aurélien and myself, which adds
> > support for FSCTL_DUPLICATE_EXTENTS_TO_FILE - a new reflink/clone FSCTL
> > supported on Windows Server 2016 shares backed by ReFS.
> > 
> > A couple of things that I'm still a little unsure about:
> > - It might be cleaner to move the locking calls out of the copy-chunk
> >   VFS function (drop VFS_COPY_CHUNK_FL_IGNORE_LOCKS), and move them
> >   into fsctl_srv_copychunk_loop(). Thoughts?  
> 
> Yeah, probably makes sense, but I'd have to take a closer look. As I'm going to
> do some larger changes here in the near future for offload read/write as, I can
> move the locking stuff around as part of that.

I've left it as is for now (with the flag).

> > - The server behaviour for success on truncated clone (where the
> >   dest file's allocation size is less than the size of the clone) seems
> >   prone to races, especially given that the operation ignores locks.
> >   + See test_ioctl_dup_extents_len_beyond_dest(). Dochelp are aware of
> >     this inconsistency.
> >   + There's not much we can do here, as we need to match Windows Server
> >     2016 + ReFS behaviour.  
> 
> ok.
> 
> [PATCH 03/10] vfs: add parameter to copy chunk VFS function to handle dup_extents
> 
> TODO: please move "[ddiss at samba.org: split VFS API change from  ioctl code]" to subject body

For the kernel this is a pretty common way of flagging changes which
have been made after an author (aaptel) has added his sign off:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst#n466
If Aurélien says that he's fine with the change as it is now, then I'll
drop the line completely.

> otherwise rb: me.
> 
> [PATCH 4/10] smbd/smb2_ioctl: add support for FSCTL_DUPLICATE_EXTENTS_TO_FILE
> 
> - initialize all pointer vars to NULL
> - use DEBUG helper macros DBG_ERR, DBG_INFO, ...
> - fsctl_dup_extents_send/recv() must create a tevent_req and return that
>   and not just return the SMB_VFS_COPY_CHUNK_SEND subreq
> - empty line between var declaration and subsequent first statement

I've added a SQUASH commit with these changes. If you and Aurélien are
okay with it then I'll squash it in.

> Everything else: reviewed-by: me.
> 
> Fwiw, I tested copy behaviour of a Windows 10 client against Windows 2016
> sharing a refs filesystem, as discussed last week. What I see is:
> 
> - server returns FILE_SUPPORTS_BLOCK_REFCOUNTING in volume attributes
> 
> - copying a 100 MB file with c-c -> c-v in the same share
> 
> - client issues offload read ioctl
> 
> - server returns invalid device request
> 
> - client falls back to copy-chunk
> 
> I don't see clone-range on the wire, trace attached. Am I missing something?

Interesting, thanks for checking this. I've been using the cifs.ko
client until now. I don't know whether Explorer makes use of it at this
stage. Will try with robocopy.

Cheers, David
-------------- next part --------------
A non-text attachment was scrubbed...
Name: dup_extents_xp2017_v2.patchset
Type: text/x-patch
Size: 49064 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20170508/0d743348/dup_extents_xp2017_v2.bin>