Skip to contentSkip to conversation

NFSD: Implement FATTR4_CLONE_BLKSIZE attribute

PATCH In review [email protected] 3 participants 4 messages May 7, 2025

Subsystems (3)
lore
Share & tools
lore

[email protected]

author maintainer
#0 lore raw

PATCH v3 1/1 NFSD: Implement FATTR4_CLONE_BLKSIZE attribute

From: Chuck Lever <[email protected]>

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.

We assume this attribute reports a block size as a count of bytes, as RFC 7862 does not specify a unit.

---
 fs/nfsd/nfs4xdr.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)
fs/nfsd/nfs4xdr.c +18 -1
index e67420729ecd..9eb8e5704622 100644
@@ -3391,6 +3391,23 @@ static __be32 nfsd4_encode_fattr4_suppattr_exclcreat(struct xdr_stream *xdr,
 	return nfsd4_encode_bitmap4(xdr, supp[0], supp[1], supp[2]);
 }
 
+/*
+ * Copied from generic_remap_checks/generic_remap_file_range_prep.
+ *
+ * These generic functions use the file system's s_blocksize, but
+ * individual file systems aren't required to use
+ * generic_remap_file_range_prep. Until there is a mechanism for
+ * determining a particular file system's (or file's) clone block
+ * size, this is the best NFSD can do.
+ */
+static __be32 nfsd4_encode_fattr4_clone_blksize(struct xdr_stream *xdr,
+						const struct nfsd4_fattr_args *args)
+{
+	struct inode *inode = d_inode(args->dentry);
+
+	return nfsd4_encode_uint32_t(xdr, inode->i_sb->s_blocksize);
+}
+
 #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 +3562,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,
 
Signature · 1 line
2.49.0
maintainer Reviewed-by on v3 1/1
#1 lore raw
Quoted 11 lines · On Wed, 2025-05-07 at 10:45 -0400, [email protected] wrote:

From: Chuck Lever <[email protected]>

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.

Isn't CLONE itself an optional operation? It wouldn't make sense to REQUIRE this attribute on servers that don't support CLONE, so I think it makes sense that it should be optional. Anyway, I'm just being pedantic.

Quoted 41 lines

We assume this attribute reports a block size as a count of bytes, as RFC 7862 does not specify a unit.

---
 fs/nfsd/nfs4xdr.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)
fs/nfsd/nfs4xdr.c +17 -0
index e67420729ecd..9eb8e5704622 100644
@@ -3391,6 +3391,23 @@ static __be32 nfsd4_encode_fattr4_suppattr_exclcreat(struct xdr_stream *xdr,
 	return nfsd4_encode_bitmap4(xdr, supp[0], supp[1], supp[2]);
 }
 
+/*
+ * Copied from generic_remap_checks/generic_remap_file_range_prep.
+ *
+ * These generic functions use the file system's s_blocksize, but
+ * individual file systems aren't required to use
+ * generic_remap_file_range_prep. Until there is a mechanism for
+ * determining a particular file system's (or file's) clone block
+ * size, this is the best NFSD can do.
+ */
+static __be32 nfsd4_encode_fattr4_clone_blksize(struct xdr_stream *xdr,
+						const struct nfsd4_fattr_args *args)
+{
+	struct inode *inode = d_inode(args->dentry);
+
+	return nfsd4_encode_uint32_t(xdr, inode->i_sb->s_blocksize);
+}
+
 #ifdef CONFIG_NFSD_V4_SECURITY_LABEL
 static __be32 nfsd4_encode_fattr4_sec_label(struct xdr_stream *xdr,
 					    const struct nfsd4_fattr_args *args)

fs/nfsd/nfs4xdr.c static const nfsd4_enc_attr nfsd4_enc_fattr4_encode_ops[] = { · line 3568

Quoted 8 lines
fs/nfsd/nfs4xdr.c +1 -1
@@ -3545,7 +3562,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,
 
author maintainer
#2 lore raw
Quoted 12 lines · Jeff Layton
Quoted 10 lines · On Wed, 2025-05-07 at 10:45 -0400, [email protected] wrote:

From: Chuck Lever <[email protected]>

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.

Quoted 4 lines

Isn't CLONE itself an optional operation? It wouldn't make sense to REQUIRE this attribute on servers that don't support CLONE, so I think it makes sense that it should be optional. Anyway, I'm just being pedantic.

My take:

It's problematic that one part of the specification states that FATTR4_CLONE_BLKSIZE is mandatory-to-implement (with a MUST), and another categorizes the attribute as RECOMMENDED.

I understand the reasons why this might be necessary, but IMO implementers who do not read the whole document might see one or the other of these (because they are in widely separated sections) and then do the wrong thing.

Section 12.2 needs to provide an explicit explanation to make it clear.

Signature · 1 line
Chuck Lever
#3 lore raw
In reply to Jeff Layton
Quoted 14 lines · Jeff Layton
Quoted 11 lines · On Wed, 2025-05-07 at 10:45 -0400, [email protected] wrote:

From: Chuck Lever <[email protected]>

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.

Quoted 4 lines

Isn't CLONE itself an optional operation? It wouldn't make sense to REQUIRE this attribute on servers that don't support CLONE, so I think it makes sense that it should be optional. Anyway, I'm just being pedantic.

AFAIK (unfortunately) *everything* in NFSv4.2 is optional, basically a NFSv4.1 client or server can claim NFSv4.2 conformance by implementing just protocol minor version negotiation... ;-(

Would a protocol minor version check suffice in this case ?

----

Bye,
Roland

Signature · 5 lines
  __ .  . __
 (o.\ \/ /.o) [email protected]
  \__\/\/__/  MPEG specialist, C&&JAVA&&Sun&&Unix programmer
  /O /==\ O\  TEL +49 641 3992797
 (;O/ \/ \O;)