Re: reflink support and Samba running over XFS

[Jeremy Allison <jra@samba.org>]

On Wed, Nov 09, 2022 at 10:38:02AM -0800, Jeremy Allison via samba-technical wrote:
>On Wed, Nov 09, 2022 at 11:32:30AM +0200, Amir Goldstein wrote:
>>On Tue, Nov 8, 2022 at 7:53 PM Jeremy Allison via samba-technical
>><samba-technical@lists.samba.org> wrote:
>>>
>>>On Mon, Nov 07, 2022 at 10:47:48PM -0800, Christoph Hellwig wrote:
>>>>On Mon, Nov 07, 2022 at 04:53:42PM -0800, Jeremy Allison via samba-technical wrote:
>>>>> ret = ioctl(fsp_get_io_fd(dest_fsp), BTRFS_IOC_CLONE_RANGE, &cr_args);
>>>>>
>>>>> what ioctls are used for this in XFS ?
>>>>>
>>>>> We'd need a VFS module that implements them for XFS.
>>>>
>>>>That ioctl is now implemented in the Linux VFS and supported by btrfs,
>>>>ocfs2, xfs, nfs (v4.2), cifs and overlayfs.
>>>
>>>I'm assuming it's this:
>>>
>>>https://man7.org/linux/man-pages/man2/ioctl_ficlonerange.2.html
>>>
>>>Yeah ? I'll write some test code and see if I can get it
>>>into the vfs_default code.
>>>
>>
>>Looks like this was already discussed during the work on generic
>>implementation of FSCTL_SRV_COPYCHUNK:
>>https://bugzilla.samba.org/show_bug.cgi?id=12033#c3
>>
>>Forgotten?
>
>Yep :-).
>
>>Left for later?
>
>So looks like we do copy_file_range(), but not CLONE_RANGE,
>or rather CLONE_RANGE only in btrfs.
>
>So the code change needed is to move the logic in vfs_btrfs.c
>into vfs_default.c, and change the call in vfs_btrfs.c:btrfs_offload_write_send()
>to SMB_VFS_NEXT_OFFLOAD_WRITE_SEND() to call the old fallback code
>inside vfs_default.c (vfswrap_offload_write_send()).

Although looking at the current Linux kernel I find inside:

ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in,
			    struct file *file_out, loff_t pos_out,
			    size_t len, unsigned int flags)
{

https://github.com/torvalds/linux/blob/0adc313c4f20639f7e235b8d6719d96a2024cf91/fs/read_write.c#L1506

	/*
	 * Try cloning first, this is supported by more file systems, and
	 * more efficient if both clone and copy are supported (e.g. NFS).
	 */
	if (file_in->f_op->remap_file_range &&
	    file_inode(file_in)->i_sb == file_inode(file_out)->i_sb) {
		loff_t cloned;

		cloned = file_in->f_op->remap_file_range(file_in, pos_in,
				file_out, pos_out,
				min_t(loff_t, MAX_RW_COUNT, len),
				REMAP_FILE_CAN_SHORTEN);
		if (cloned > 0) {
			ret = cloned;

and looking at the code supporting int ioctl(int dest_fd, FICLONERANGE, struct file_clone_range *arg);
we have:

loff_t do_clone_file_range(struct file *file_in, loff_t pos_in,
			   struct file *file_out, loff_t pos_out,
			   loff_t len, unsigned int remap_flags)
...
	ret = file_in->f_op->remap_file_range(file_in, pos_in,
			file_out, pos_out, len, remap_flags);

So it *looks* like the copy_file_range() syscall will internally
call the equivalent of FICLONERANGE if the underlying file
system supports it.

So maybe the right fix is to remove the FICLONERANGE specific
code from our vfs_btrfs.c and just always use copy_file_range().

Any comments from other Samba Team members ?

Jeremy.