[PATCH] NFSD: Implement FATTR4_CLONE_BLKSIZE attribute

[PATCH] NFSD: Implement FATTR4_CLONE_BLKSIZE attribute

[cel]

From: Chuck Lever <chuck.lever@oracle.com>

RFC 7862 states that if an NFS server implements a CLONE operation,
it MUST also implement FATTR4_CLONE_BLKSIZE. NFSD implements CLONE,
but does not implement FATTR4_CLONE_BLKSIZE.

Note that in Section 12.2, RFC 7862 claims that
FATTR4_CLONE_BLKSIZE is RECOMMENDED, not REQUIRED. Likely this is
because a minor version is not permitted to add a REQUIRED
attribute. Confusing.

NFSD can return 0 here, as at least one client implementation we
are aware of (the Linux NFS client) treats 0 as meaning "CLONE has
no alignment restrictions".

Meanwhile we need to consult the nfsv4 Working Group to clarify the
meaning and use of the value of this attribute.

Reported-by: Roland Mainz <roland.mainz@nrubsig.org>
X-Cc: stable@vger.kernel.org
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 fs/nfsd/nfs4xdr.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index e67420729ecd..44e7fb34f433 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -3391,6 +3391,13 @@ static __be32 nfsd4_encode_fattr4_suppattr_exclcreat(struct xdr_stream *xdr,
 	return nfsd4_encode_bitmap4(xdr, supp[0], supp[1], supp[2]);
 }
 
+static __be32 nfsd4_encode_fattr4_clone_blksize(struct xdr_stream *xdr,
+						const struct nfsd4_fattr_args *args)
+{
+	/* Linux filesystems have no clone alignment restrictions */
+	return nfsd4_encode_uint32_t(xdr, 0);
+}
+
 #ifdef CONFIG_NFSD_V4_SECURITY_LABEL
 static __be32 nfsd4_encode_fattr4_sec_label(struct xdr_stream *xdr,
 					    const struct nfsd4_fattr_args *args)
@@ -3545,7 +3552,7 @@ static const nfsd4_enc_attr nfsd4_enc_fattr4_encode_ops[] = {
 	[FATTR4_MODE_SET_MASKED]	= nfsd4_encode_fattr4__noop,
 	[FATTR4_SUPPATTR_EXCLCREAT]	= nfsd4_encode_fattr4_suppattr_exclcreat,
 	[FATTR4_FS_CHARSET_CAP]		= nfsd4_encode_fattr4__noop,
-	[FATTR4_CLONE_BLKSIZE]		= nfsd4_encode_fattr4__noop,
+	[FATTR4_CLONE_BLKSIZE]		= nfsd4_encode_fattr4_clone_blksize,
 	[FATTR4_SPACE_FREED]		= nfsd4_encode_fattr4__noop,
 	[FATTR4_CHANGE_ATTR_TYPE]	= nfsd4_encode_fattr4__noop,
 
-- 
2.49.0

Re: [PATCH] NFSD: Implement FATTR4_CLONE_BLKSIZE attribute

[Christoph Hellwig]

On Sun, Apr 27, 2025 at 12:39:14PM -0400, cel@kernel.org wrote:
> NFSD can return 0 here, as at least one client implementation we
> are aware of (the Linux NFS client) treats 0 as meaning "CLONE has
> no alignment restrictions".

Usuaully clone does have a restriction, though.

> Meanwhile we need to consult the nfsv4 Working Group to clarify the
> meaning and use of the value of this attribute.

Yeah, the attribute seems to be severly underspecified, i.e. it does
not even provide a unit that the value is in.

I think the only sane way out is an errate that makes 0 mean
"not specified" and then provides the byte unit and maybe some
other quirks.

> +	/* Linux filesystems have no clone alignment restrictions */

That is absolutely untrue as said above.

Re: [PATCH] NFSD: Implement FATTR4_CLONE_BLKSIZE attribute

[Roland Mainz]

On Tue, May 6, 2025 at 10:10 AM Christoph Hellwig <hch@infradead.org> wrote:
> On Sun, Apr 27, 2025 at 12:39:14PM -0400, cel@kernel.org wrote:
> > NFSD can return 0 here, as at least one client implementation we
> > are aware of (the Linux NFS client) treats 0 as meaning "CLONE has
> > no alignment restrictions".
>
> Usuaully clone does have a restriction, though.
>
> > Meanwhile we need to consult the nfsv4 Working Group to clarify the
> > meaning and use of the value of this attribute.
>
> Yeah, the attribute seems to be severly underspecified, i.e. it does
> not even provide a unit that the value is in.

My preference would be that it counts in bytes, basically the same
what the Windows API |FSCTL_DUPLICATE_EXTENTS_TO_FILE| (see
https://learn.microsoft.com/en-us/windows/win32/api/winioctl/ni-winioctl-fsctl_duplicate_extents_to_file)
does.

> I think the only sane way out is an errate that makes 0 mean
> "not specified" and then provides the byte unit and maybe some
> other quirks.

I agree, Technically not even an "errata", it's just logic... :-)

FATTR4_CLONE_BLKSIZE==0 ----> no info
FATTR4_CLONE_BLKSIZE==1 ----> no alignment restrictions
FATTR4_CLONE_BLKSIZE>1 ----> use this alignment value

One thing to consider would be to make |FATTR4_CLONE_BLKSIZE|
perr-filesystem *AND* per-file, since modern filesystems may have a
per-file *preferred* block size.

> > +     /* Linux filesystems have no clone alignment restrictions */
>
> That is absolutely untrue as said above.

FYI I've implemented Win32 |FSCTL_DUPLICATE_EXTENTS_TO_FILE| support
in the Windows ms-nfs41-client NFSv4.2 driver via NFS CLONE, and so
far didn't hit any alignment restrictions with Linux 6.6 exporting a
btrfs filesystem...

----

Bye,
Roland
-- 
  __ .  . __
 (o.\ \/ /.o) roland.mainz@nrubsig.org
  \__\/\/__/  MPEG specialist, C&&JAVA&&Sun&&Unix programmer
  /O /==\ O\  TEL +49 641 3992797
 (;O/ \/ \O;)

Re: [PATCH] NFSD: Implement FATTR4_CLONE_BLKSIZE attribute

[Chuck Lever]

On 5/6/25 4:10 AM, Christoph Hellwig wrote:
> On Sun, Apr 27, 2025 at 12:39:14PM -0400, cel@kernel.org wrote:
>> NFSD can return 0 here, as at least one client implementation we
>> are aware of (the Linux NFS client) treats 0 as meaning "CLONE has
>> no alignment restrictions".
> 
> Usually clone does have a restriction, though.

Then should this patch extract that block size and report it? Pointer
to sample code would kickstart me to get that done.


>> Meanwhile we need to consult the nfsv4 Working Group to clarify the
>> meaning and use of the value of this attribute.
> 
> Yeah, the attribute seems to be severely underspecified, i.e. it does
> not even provide a unit that the value is in.

Agreed. Everyone is /assuming/ bytes, for now.


> I think the only sane way out is an errate that makes 0 mean
> "not specified" and then provides the byte unit and maybe some
> other quirks.
> 
>> +	/* Linux filesystems have no clone alignment restrictions */
> 
> That is absolutely untrue as said above.

-- 
Chuck Lever

Re: [PATCH] NFSD: Implement FATTR4_CLONE_BLKSIZE attribute

[Christoph Hellwig]

On Tue, May 06, 2025 at 09:45:56AM -0400, Chuck Lever wrote:
> On 5/6/25 4:10 AM, Christoph Hellwig wrote:
> > On Sun, Apr 27, 2025 at 12:39:14PM -0400, cel@kernel.org wrote:
> >> NFSD can return 0 here, as at least one client implementation we
> >> are aware of (the Linux NFS client) treats 0 as meaning "CLONE has
> >> no alignment restrictions".
> > 
> > Usually clone does have a restriction, though.
> 
> Then should this patch extract that block size and report it? Pointer
> to sample code would kickstart me to get that done.

I think reporting 0 as unknown makes perfect sense, just don't claim
that there is no limitation.

The generic remap range checks for file system block size alignment:

	if (!IS_ALIGNED(pos_in, bs) || !IS_ALIGNED(pos_out, bs))
		return -EINVAL;

in generic_remap_checks.