Simplify copy-reflink code

[David Disseldorp]

On Thu, 4 Apr 2024 18:46:11 +0200, Ralph Boehme via samba-technical wrote:

> On 4/4/24 03:58, David Disseldorp wrote:
> > I had a quick play with your changes. BTRFS_IOC_CLONE_RANGE fallback is
> > triggered by the existing smb2.ioctl.copy_chunk_tiny test. strace
> > indicates that it's doing the right thing on the syscall side before and
> > after your 48d8b9c7ad5 ("vfs_btrfs: fix BTRFS_IOC_CLONE_RANGE fallback")
> > change (with ret = -1 reverted back to ret = ioctl...):
> >    ioctl(30, BTRFS_IOC_CLONE_RANGE , {src_fd=28, src_offset=0, src_length=48, dest_offset=0}) = -1 EINVAL
> >    ...
> >    copy_file_range(28, [0], 30, [0], 48, 0) = 48  
> 
> oh, yes, sure, the fallback for a copy-chunk request will work, because 
> that goes via VFS_NEXT in the READ_SEND function, so vfs_default known 
> about the token.
> 
> What will not work is the fallback for an DUP_EXTENTS ioctl. If you eg 
> modify the test smb2.ioctl.dup_extents_simple to use a lenght of eg 1 
> bytes, the ioctl will fail and then calls 
> SMB_VFS_NEXT_OFFLOAD_WRITE_SEND() which will then fail in vfs_default as 
> that doesn't know about the token, because in READ_SEND in vfs_btrfs we 
> create the token/fsp association via the db ourselves for the 
> DUP_EXTENTS case.

Yeah, it's the dup-extents->CLONE_RANGE failure handling which is indeed
missing test coverage.

> Now the interesting question is whether the fallback to normal data copy 
> is actually correct if the client explicitly requests DUP_EXTENTS. 
> Afaict it is clearly not correct and my change to consolidate 
> DUP_EXTENTS implementation in vfs_default actually doesn't have a 
> fallback if an SMB2 level DUP_EXTENTS fails in the ioctl().

I don't think dup-extents should fallback to copy; with the initial
implementation we had VFS_COPY_CHUNK_FL_MUST_CLONE to make this
explicit. However, the MS-FSCC spec doesn't appear to state that cloning
is a hard requirement, only that it should be supported alongside
FILE_SUPPORTS_BLOCK_REFCOUNTING and that offsets+lengths need to be
"logical cluster boundary" aligned.
We probably need to do some testing against modern ReFS to check some of
these questions.

> But as the current code had this behaviour until I broke it with 
> af6cbc7a441e05f71ae4e97c7d82c27868633e53 and friends, I wanted to bring 
> this to your attention and get your feedback before proceeding. :)

I appreciate you raising this :) The rest of the changes look reasonable.
I'll follow up there in the gitlab MR.

Cheers, David