* Re: reflink support and Samba running over XFS
2022-11-09 18:47 ` Jeremy Allison
@ 2022-11-09 19:24 ` Jeremy Allison
2022-11-09 22:30 ` David Disseldorp
2022-11-09 19:50 ` Amir Goldstein
` (2 subsequent siblings)
3 siblings, 1 reply; 15+ messages in thread
From: Jeremy Allison @ 2022-11-09 19:24 UTC (permalink / raw)
To: Amir Goldstein, Steve French, samba-technical, CIFS, slow, vl,
metze
Cc: slow, vl, metze
On Wed, Nov 09, 2022 at 10:47:41AM -0800, Jeremy Allison wrote:
>
>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 ?
So right now Steve what is preventing FSCTL_DUP_EXTENTS_TO_FILE
from working against anything other then btrfs on Samba is the
following code:
source3/smbd/smb2_ioctl_filesys.c:fsctl_dup_extents_send()
180 if ((dst_fsp->conn->fs_capabilities
181 & FILE_SUPPORTS_BLOCK_REFCOUNTING) == 0) {
182 DBG_INFO("FS does not advertise block refcounting support\n");
183 tevent_req_nterror(req, NT_STATUS_INVALID_DEVICE_REQUEST);
184 return tevent_req_post(req, ev);
185 }
because currently only the vfs_btrfs module reports FILE_SUPPORTS_BLOCK_REFCOUNTING,
not vfs_default.
and also in:
source3/modules/vfs_default.c:vfswrap_offload_write_send()
2194 case FSCTL_DUP_EXTENTS_TO_FILE:
2195 DBG_DEBUG("COW clones not supported by vfs_default\n");
2196 tevent_req_nterror(req, NT_STATUS_INVALID_PARAMETER);
2197 return tevent_req_post(req, ev);
but looking at vfs_btrfs it looks like that code should
probably also be in vfswrap_offload_read_send() as well
and the error code should be NT_STATUS_INVALID_DEVICE_REQUEST.
We also need to duplicate the logic in vfs_btrfs for
handling FSCTL_DUP_EXTENTS_TO_FILE into VFS default,
gated on support for the copy_file_range() system
call (which would set FILE_SUPPORTS_BLOCK_REFCOUNTING
in the fs_capabilities return from vfs_default).
I think this is doable with some work...
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: reflink support and Samba running over XFS
2022-11-09 19:24 ` Jeremy Allison
@ 2022-11-09 22:30 ` David Disseldorp
2022-11-10 11:42 ` Christoph Hellwig
0 siblings, 1 reply; 15+ messages in thread
From: David Disseldorp @ 2022-11-09 22:30 UTC (permalink / raw)
To: Jeremy Allison via samba-technical
Cc: Jeremy Allison, Amir Goldstein, Steve French, CIFS, slow, vl,
metze
Hi Jeremy,
On Wed, 9 Nov 2022 11:24:26 -0800, Jeremy Allison via samba-technical wrote:
> On Wed, Nov 09, 2022 at 10:47:41AM -0800, Jeremy Allison wrote:
> >
> >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 ?
>
> So right now Steve what is preventing FSCTL_DUP_EXTENTS_TO_FILE
> from working against anything other then btrfs on Samba is the
> following code:
>
> source3/smbd/smb2_ioctl_filesys.c:fsctl_dup_extents_send()
>
> 180 if ((dst_fsp->conn->fs_capabilities
> 181 & FILE_SUPPORTS_BLOCK_REFCOUNTING) == 0) {
> 182 DBG_INFO("FS does not advertise block refcounting support\n");
> 183 tevent_req_nterror(req, NT_STATUS_INVALID_DEVICE_REQUEST);
> 184 return tevent_req_post(req, ev);
> 185 }
>
> because currently only the vfs_btrfs module reports FILE_SUPPORTS_BLOCK_REFCOUNTING,
> not vfs_default.
>
> and also in:
>
> source3/modules/vfs_default.c:vfswrap_offload_write_send()
>
> 2194 case FSCTL_DUP_EXTENTS_TO_FILE:
> 2195 DBG_DEBUG("COW clones not supported by vfs_default\n");
> 2196 tevent_req_nterror(req, NT_STATUS_INVALID_PARAMETER);
> 2197 return tevent_req_post(req, ev);
>
> but looking at vfs_btrfs it looks like that code should
> probably also be in vfswrap_offload_read_send() as well
> and the error code should be NT_STATUS_INVALID_DEVICE_REQUEST.
>
> We also need to duplicate the logic in vfs_btrfs for
> handling FSCTL_DUP_EXTENTS_TO_FILE into VFS default,
> gated on support for the copy_file_range() system
> call (which would set FILE_SUPPORTS_BLOCK_REFCOUNTING
> in the fs_capabilities return from vfs_default).
>
> I think this is doable with some work...
I think it's doable too :-). As indicated in my other mail, I think
FICLONERANGE will need to be used for FSCTL_DUP_EXTENTS_TO_FILE instead
of copy_file_range().
I'm not sure how to best handle FILE_SUPPORTS_BLOCK_REFCOUNTING -
ideally we'd set it for Btrfs and XFS(reflink=1) backed shares only.
Another option might be to advertise FILE_SUPPORTS_BLOCK_REFCOUNTING and
then propagate errors up to the client if FICLONERANGE fails for
FSCTL_DUP_EXTENTS_TO_FILE. Client copy fallback would work, but the
extra request overhead would be ugly.
Cheers, David
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: reflink support and Samba running over XFS
2022-11-09 22:30 ` David Disseldorp
@ 2022-11-10 11:42 ` Christoph Hellwig
2022-11-10 15:05 ` David Disseldorp
0 siblings, 1 reply; 15+ messages in thread
From: Christoph Hellwig @ 2022-11-10 11:42 UTC (permalink / raw)
To: David Disseldorp
Cc: Jeremy Allison via samba-technical, CIFS, Amir Goldstein, metze,
Steve French, vl, Jeremy Allison
On Wed, Nov 09, 2022 at 11:30:55PM +0100, David Disseldorp via samba-technical wrote:
> I think it's doable too :-). As indicated in my other mail, I think
> FICLONERANGE will need to be used for FSCTL_DUP_EXTENTS_TO_FILE instead
> of copy_file_range().
> I'm not sure how to best handle FILE_SUPPORTS_BLOCK_REFCOUNTING -
> ideally we'd set it for Btrfs and XFS(reflink=1) backed shares only.
> Another option might be to advertise FILE_SUPPORTS_BLOCK_REFCOUNTING and
> then propagate errors up to the client if FICLONERANGE fails for
> FSCTL_DUP_EXTENTS_TO_FILE. Client copy fallback would work, but the
> extra request overhead would be ugly.
We could probably add a bit to struct statx if that is helpful for
samba.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: reflink support and Samba running over XFS
2022-11-10 11:42 ` Christoph Hellwig
@ 2022-11-10 15:05 ` David Disseldorp
0 siblings, 0 replies; 15+ messages in thread
From: David Disseldorp @ 2022-11-10 15:05 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Jeremy Allison via samba-technical, CIFS, Amir Goldstein, metze,
Steve French, vl, Jeremy Allison
On Thu, 10 Nov 2022 03:42:28 -0800, Christoph Hellwig wrote:
> On Wed, Nov 09, 2022 at 11:30:55PM +0100, David Disseldorp via samba-technical wrote:
> > I think it's doable too :-). As indicated in my other mail, I think
> > FICLONERANGE will need to be used for FSCTL_DUP_EXTENTS_TO_FILE instead
> > of copy_file_range().
> > I'm not sure how to best handle FILE_SUPPORTS_BLOCK_REFCOUNTING -
> > ideally we'd set it for Btrfs and XFS(reflink=1) backed shares only.
> > Another option might be to advertise FILE_SUPPORTS_BLOCK_REFCOUNTING and
> > then propagate errors up to the client if FICLONERANGE fails for
> > FSCTL_DUP_EXTENTS_TO_FILE. Client copy fallback would work, but the
> > extra request overhead would be ugly.
>
> We could probably add a bit to struct statx if that is helpful for
> samba.
I think that'd be helpful for the above example.
The corresponding MS-FSA spec states:
2.1.5.9.4 FSCTL_DUPLICATE_EXTENTS_TO_FILE
...
The purpose of this operation is to make it look like a copy of a region from the source stream to the
target stream has occurred when in reality no data is actually copied. This operation modifies the
target stream's extent list such that, the same clusters are pointed to by both the source and target
streams' extent lists for the region being copied.
So it would appear that SMB clients can expect metadata I/O only.
There's one other SMB caveat which should be considered with any statx
changes, from MS-FSCC:
2.3.9 FSCTL_DUPLICATE_EXTENTS_TO_FILE_EX Request
...
When the DUPLICATE_EXTENTS_DATA_EX_SOURCE_ATOMIC
flag isn’t set, the behavior is identical to FSCTL_DUPLICATE_EXTENTS_TO_FILE. When the flag is set,
duplication is atomic from the source's point of view. It means duplication fully succeeds or fails
without side effect (when only part of source file region is duplicated).
Cheers, David
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: reflink support and Samba running over XFS
2022-11-09 18:47 ` Jeremy Allison
2022-11-09 19:24 ` Jeremy Allison
@ 2022-11-09 19:50 ` Amir Goldstein
2022-11-09 21:50 ` David Disseldorp
2022-11-10 11:41 ` Christoph Hellwig
3 siblings, 0 replies; 15+ messages in thread
From: Amir Goldstein @ 2022-11-09 19:50 UTC (permalink / raw)
To: Jeremy Allison; +Cc: Steve French, samba-technical, CIFS, slow, vl, metze
On Wed, Nov 9, 2022 at 8:47 PM Jeremy Allison <jra@samba.org> wrote:
>
> 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.
>
It's true.
Unless you have some SMB command that requires the clone to be
a clone (what is VFS_COPY_CHUNK_FL_MUST_CLONE in the
referred comment?)
because currently there is no flag that can be passed to
copy_file_range() to request only clone.
Thanks,
Amir.
> 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.
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: reflink support and Samba running over XFS
2022-11-09 18:47 ` Jeremy Allison
2022-11-09 19:24 ` Jeremy Allison
2022-11-09 19:50 ` Amir Goldstein
@ 2022-11-09 21:50 ` David Disseldorp
2022-11-10 11:41 ` Christoph Hellwig
3 siblings, 0 replies; 15+ messages in thread
From: David Disseldorp @ 2022-11-09 21:50 UTC (permalink / raw)
To: Jeremy Allison via samba-technical
Cc: Jeremy Allison, Amir Goldstein, Steve French, CIFS, vl, metze
Hi Jeremy,
On Wed, 9 Nov 2022 10:47:41 -0800, Jeremy Allison via samba-technical wrote:
...
> >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().
copy_file_range() should be okay for FSCTL_SRV_COPYCHUNK, but I don't
think it's an option for FSCTL_DUP_EXTENTS_TO_FILE, as (IIRC) the latter
explicitly requires block cloning so can't fallback to regular copy.
Cheers, David
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: reflink support and Samba running over XFS
2022-11-09 18:47 ` Jeremy Allison
` (2 preceding siblings ...)
2022-11-09 21:50 ` David Disseldorp
@ 2022-11-10 11:41 ` Christoph Hellwig
3 siblings, 0 replies; 15+ messages in thread
From: Christoph Hellwig @ 2022-11-10 11:41 UTC (permalink / raw)
To: Jeremy Allison
Cc: Amir Goldstein, Steve French, samba-technical, CIFS, vl, metze
On Wed, Nov 09, 2022 at 10:47:41AM -0800, Jeremy Allison via samba-technical wrote:
> So it *looks* like the copy_file_range() syscall will internally
> call the equivalent of FICLONERANGE if the underlying file
> system supports it.
Yes. The separate clone interface exists for the cases where
applications only want to do the fast metadata only operation and
not fall back toan actual copy. I'll leave it to people with more
SMB experience if and how that matters to the protocol. For NFS
CLONE and COPY primitives exist on the wire with a similar distinction.
^ permalink raw reply [flat|nested] 15+ messages in thread