[PATCH 0/1] ceph: add getvxattr support

[PATCH 0/1] ceph: add getvxattr support

[Milind Changire]

Add getvxattr op support to handle fetching xattr values for:
 * ceph.dir.pin*
 * ceph.dir.layout*
 * ceph.file.layout*

Milind Changire (1):
  ceph: add getvxattr op

 fs/ceph/inode.c              | 51 ++++++++++++++++++++++++++++
 fs/ceph/mds_client.c         | 27 ++++++++++++++-
 fs/ceph/mds_client.h         | 12 ++++++-
 fs/ceph/strings.c            |  1 +
 fs/ceph/super.h              |  1 +
 fs/ceph/xattr.c              | 65 ++++++++++++++++++++++++++++++++++++
 include/linux/ceph/ceph_fs.h |  1 +
 7 files changed, 156 insertions(+), 2 deletions(-)

-- 
2.34.1

[PATCH 1/1] ceph: add getvxattr op

[Milind Changire]

Problem:
Directory vxattrs like ceph.dir.pin* and ceph.dir.layout* may not be
propagated to the client as frequently to keep them updated. This
creates vxattr availability problems.

Solution:
Adds new getvxattr op to fetch ceph.dir.pin*, ceph.dir.layout* and
ceph.file.layout* vxattrs.
If the entire layout for a dir or a file is being set, then it is
expected that the layout be set in standard JSON format. Individual
field value retrieval is not wrapped in JSON. The JSON format also
applies while setting the vxattr if the entire layout is being set in
one go.
As a temporary measure, setting a vxattr can also be done in the old
format. The old format will be deprecated in the future.

URL: https://tracker.ceph.com/issues/51062
Signed-off-by: Milind Changire <mchangir@redhat.com>
---
 fs/ceph/inode.c              | 51 ++++++++++++++++++++++++++++
 fs/ceph/mds_client.c         | 27 ++++++++++++++-
 fs/ceph/mds_client.h         | 12 ++++++-
 fs/ceph/strings.c            |  1 +
 fs/ceph/super.h              |  1 +
 fs/ceph/xattr.c              | 65 ++++++++++++++++++++++++++++++++++++
 include/linux/ceph/ceph_fs.h |  1 +
 7 files changed, 156 insertions(+), 2 deletions(-)

diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
index e3322fcb2e8d..f29d59b63c52 100644
--- a/fs/ceph/inode.c
+++ b/fs/ceph/inode.c
@@ -2291,6 +2291,57 @@ int __ceph_do_getattr(struct inode *inode, struct page *locked_page,
 	return err;
 }
 
+int ceph_do_getvxattr(struct inode *inode, const char *name, void *value,
+		      size_t size)
+{
+	struct ceph_fs_client *fsc = ceph_sb_to_client(inode->i_sb);
+	struct ceph_mds_client *mdsc = fsc->mdsc;
+	struct ceph_mds_request *req;
+	int mode = USE_AUTH_MDS;
+	int err;
+	char *xattr_value;
+	size_t xattr_value_len;
+
+	req = ceph_mdsc_create_request(mdsc, CEPH_MDS_OP_GETVXATTR, mode);
+	if (IS_ERR(req)) {
+		err = -ENOMEM;
+		goto out;
+	}
+
+	req->r_path2 = kstrdup(name, GFP_NOFS);
+	if (!req->r_path2) {
+		err = -ENOMEM;
+		goto put;
+	}
+
+	ihold(inode);
+	req->r_inode = inode;
+	err = ceph_mdsc_do_request(mdsc, NULL, req);
+	if (err < 0)
+		goto put;
+
+	xattr_value = req->r_reply_info.xattr_info.xattr_value;
+	xattr_value_len = req->r_reply_info.xattr_info.xattr_value_len;
+
+	dout("do_getvxattr xattr_value_len:%lu, size:%lu\n", xattr_value_len, size);
+
+	err = xattr_value_len;
+	if (size == 0)
+		goto put;
+
+	if (xattr_value_len > size) {
+		err = -ERANGE;
+		goto put;
+	}
+
+	memcpy(value, xattr_value, xattr_value_len);
+put:
+	ceph_mdsc_put_request(req);
+out:
+	dout("do_getvxattr result=%d\n", err);
+	return err;
+}
+
 
 /*
  * Check inode permissions.  We verify we have a valid value for
diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
index c30eefc0ac19..a5eafc71d976 100644
--- a/fs/ceph/mds_client.c
+++ b/fs/ceph/mds_client.c
@@ -555,6 +555,29 @@ static int parse_reply_info_create(void **p, void *end,
 	return -EIO;
 }
 
+static int parse_reply_info_getvxattr(void **p, void *end,
+				      struct ceph_mds_reply_info_parsed *info,
+				      u64 features)
+{
+	u8 struct_v, struct_compat;
+	u32 struct_len;
+	u32 value_len;
+
+	ceph_decode_8_safe(p, end, struct_v, bad);
+	ceph_decode_8_safe(p, end, struct_compat, bad);
+	ceph_decode_32_safe(p, end, struct_len, bad);
+	ceph_decode_32_safe(p, end, value_len, bad);
+
+	if (value_len == end - *p) {
+	  info->xattr_info.xattr_value = *p;
+	  info->xattr_info.xattr_value_len = end - *p;
+	  *p = end;
+	  return info->xattr_info.xattr_value_len;
+	}
+bad:
+	return -EIO;
+}
+
 /*
  * parse extra results
  */
@@ -570,6 +593,8 @@ static int parse_reply_info_extra(void **p, void *end,
 		return parse_reply_info_readdir(p, end, info, features);
 	else if (op == CEPH_MDS_OP_CREATE)
 		return parse_reply_info_create(p, end, info, features, s);
+	else if (op == CEPH_MDS_OP_GETVXATTR)
+		return parse_reply_info_getvxattr(p, end, info, features);
 	else
 		return -EIO;
 }
@@ -615,7 +640,7 @@ static int parse_reply_info(struct ceph_mds_session *s, struct ceph_msg *msg,
 
 	if (p != end)
 		goto bad;
-	return 0;
+	return err;
 
 bad:
 	err = -EIO;
diff --git a/fs/ceph/mds_client.h b/fs/ceph/mds_client.h
index 97c7f7bfa55f..f2a8e5af3c2e 100644
--- a/fs/ceph/mds_client.h
+++ b/fs/ceph/mds_client.h
@@ -29,8 +29,10 @@ enum ceph_feature_type {
 	CEPHFS_FEATURE_MULTI_RECONNECT,
 	CEPHFS_FEATURE_DELEG_INO,
 	CEPHFS_FEATURE_METRIC_COLLECT,
+	CEPHFS_FEATURE_ALTERNATE_NAME,
+	CEPHFS_FEATURE_GETVXATTR,
 
-	CEPHFS_FEATURE_MAX = CEPHFS_FEATURE_METRIC_COLLECT,
+	CEPHFS_FEATURE_MAX = CEPHFS_FEATURE_GETVXATTR,
 };
 
 /*
@@ -45,6 +47,8 @@ enum ceph_feature_type {
 	CEPHFS_FEATURE_MULTI_RECONNECT,		\
 	CEPHFS_FEATURE_DELEG_INO,		\
 	CEPHFS_FEATURE_METRIC_COLLECT,		\
+	CEPHFS_FEATURE_ALTERNATE_NAME,		\
+	CEPHFS_FEATURE_GETVXATTR,		\
 						\
 	CEPHFS_FEATURE_MAX,			\
 }
@@ -100,6 +104,11 @@ struct ceph_mds_reply_dir_entry {
 	loff_t			      offset;
 };
 
+struct ceph_mds_reply_xattr {
+	char *xattr_value;
+	size_t xattr_value_len;
+};
+
 /*
  * parsed info about an mds reply, including information about
  * either: 1) the target inode and/or its parent directory and dentry,
@@ -115,6 +124,7 @@ struct ceph_mds_reply_info_parsed {
 	char                          *dname;
 	u32                           dname_len;
 	struct ceph_mds_reply_lease   *dlease;
+	struct ceph_mds_reply_xattr   xattr_info;
 
 	/* extra */
 	union {
diff --git a/fs/ceph/strings.c b/fs/ceph/strings.c
index 573bb9556fb5..e36e8948e728 100644
--- a/fs/ceph/strings.c
+++ b/fs/ceph/strings.c
@@ -60,6 +60,7 @@ const char *ceph_mds_op_name(int op)
 	case CEPH_MDS_OP_LOOKUPINO:  return "lookupino";
 	case CEPH_MDS_OP_LOOKUPNAME:  return "lookupname";
 	case CEPH_MDS_OP_GETATTR:  return "getattr";
+	case CEPH_MDS_OP_GETVXATTR:  return "getvxattr";
 	case CEPH_MDS_OP_SETXATTR: return "setxattr";
 	case CEPH_MDS_OP_SETATTR: return "setattr";
 	case CEPH_MDS_OP_RMXATTR: return "rmxattr";
diff --git a/fs/ceph/super.h b/fs/ceph/super.h
index ac331aa07cfa..a627fa69668e 100644
--- a/fs/ceph/super.h
+++ b/fs/ceph/super.h
@@ -1043,6 +1043,7 @@ static inline bool ceph_inode_is_shutdown(struct inode *inode)
 
 /* xattr.c */
 int __ceph_setxattr(struct inode *, const char *, const void *, size_t, int);
+int ceph_do_getvxattr(struct inode *inode, const char *name, void *value, size_t size);
 ssize_t __ceph_getxattr(struct inode *, const char *, void *, size_t);
 extern ssize_t ceph_listxattr(struct dentry *, char *, size_t);
 extern struct ceph_buffer *__ceph_build_xattrs_blob(struct ceph_inode_info *ci);
diff --git a/fs/ceph/xattr.c b/fs/ceph/xattr.c
index fcf7dfdecf96..fdde8ffa71bb 100644
--- a/fs/ceph/xattr.c
+++ b/fs/ceph/xattr.c
@@ -918,6 +918,64 @@ static inline int __get_request_mask(struct inode *in) {
 	return mask;
 }
 
+static inline bool ceph_is_passthrough_vxattr(const char *name)
+{
+	const char *vxattr_list[] = {
+		"ceph.dir.pin",
+		"ceph.dir.pin.random",
+		"ceph.dir.pin.distributed",
+		"ceph.dir.layout",
+		"ceph.dir.layout.object_size",
+		"ceph.dir.layout.stripe_count",
+		"ceph.dir.layout.stripe_unit",
+		"ceph.dir.layout.pool",
+		"ceph.dir.layout.pool_name",
+		"ceph.dir.layout.pool_id",
+		"ceph.dir.layout.pool_namespace",
+		"ceph.file.layout",
+		"ceph.file.layout.object_size",
+		"ceph.file.layout.stripe_count",
+		"ceph.file.layout.stripe_unit",
+		"ceph.file.layout.pool",
+		"ceph.file.layout.pool_name",
+		"ceph.file.layout.pool_id",
+		"ceph.file.layout.pool_namespace",
+	};
+	int i = 0;
+	int n = sizeof(vxattr_list)/sizeof(vxattr_list[0]);
+
+	while (i < n) {
+		if (!strcmp(name, vxattr_list[i]))
+			return true;
+		i++;
+	}
+	return false;
+}
+
+/* check if the entire cluster supports the given feature */
+static inline bool ceph_cluster_has_feature(struct inode *inode, int feature_bit)
+{
+	int64_t i;
+	struct ceph_fs_client *fsc = ceph_inode_to_client(inode);
+	struct ceph_mds_session **sessions = fsc->mdsc->sessions;
+	int64_t num_sessions = atomic_read(&fsc->mdsc->num_sessions);
+
+	if (fsc->mdsc->stopping)
+		return false;
+
+	if (!sessions)
+		return false;
+
+	for (i = 0; i < num_sessions; i++) {
+		struct ceph_mds_session *session = sessions[i];
+		if (!session)
+			return false;
+		if (!test_bit(feature_bit, &session->s_features))
+			return false;
+	}
+	return true;
+}
+
 ssize_t __ceph_getxattr(struct inode *inode, const char *name, void *value,
 		      size_t size)
 {
@@ -927,6 +985,13 @@ ssize_t __ceph_getxattr(struct inode *inode, const char *name, void *value,
 	int req_mask;
 	ssize_t err;
 
+	if (ceph_is_passthrough_vxattr(name) &&
+	    ceph_cluster_has_feature(inode, CEPHFS_FEATURE_GETVXATTR)) {
+		err = ceph_do_getvxattr(inode, name, value, size);
+		return err;
+	}
+	dout("vxattr is not passthrough or kclient doesn't support GETVXATTR\n");
+
 	/* let's see if a virtual xattr was requested */
 	vxattr = ceph_match_vxattr(inode, name);
 	if (vxattr) {
diff --git a/include/linux/ceph/ceph_fs.h b/include/linux/ceph/ceph_fs.h
index 7ad6c3d0db7d..66db21ac5f0c 100644
--- a/include/linux/ceph/ceph_fs.h
+++ b/include/linux/ceph/ceph_fs.h
@@ -328,6 +328,7 @@ enum {
 	CEPH_MDS_OP_LOOKUPPARENT = 0x00103,
 	CEPH_MDS_OP_LOOKUPINO  = 0x00104,
 	CEPH_MDS_OP_LOOKUPNAME = 0x00105,
+	CEPH_MDS_OP_GETVXATTR  = 0x00106,
 
 	CEPH_MDS_OP_SETXATTR   = 0x01105,
 	CEPH_MDS_OP_RMXATTR    = 0x01106,
-- 
2.34.1

Re: [PATCH 1/1] ceph: add getvxattr op

[Jeff Layton]

On Tue, 2022-01-11 at 17:54 +0530, Milind Changire wrote:
> Problem:
> Directory vxattrs like ceph.dir.pin* and ceph.dir.layout* may not be
> propagated to the client as frequently to keep them updated. This
> creates vxattr availability problems.
> 
> Solution:
> Adds new getvxattr op to fetch ceph.dir.pin*, ceph.dir.layout* and
> ceph.file.layout* vxattrs.
> If the entire layout for a dir or a file is being set, then it is
> expected that the layout be set in standard JSON format. Individual
> field value retrieval is not wrapped in JSON. The JSON format also
> applies while setting the vxattr if the entire layout is being set in
> one go.
> As a temporary measure, setting a vxattr can also be done in the old
> format. The old format will be deprecated in the future.
> 
> URL: https://tracker.ceph.com/issues/51062
> Signed-off-by: Milind Changire <mchangir@redhat.com>
> ---
>  fs/ceph/inode.c              | 51 ++++++++++++++++++++++++++++
>  fs/ceph/mds_client.c         | 27 ++++++++++++++-
>  fs/ceph/mds_client.h         | 12 ++++++-
>  fs/ceph/strings.c            |  1 +
>  fs/ceph/super.h              |  1 +
>  fs/ceph/xattr.c              | 65 ++++++++++++++++++++++++++++++++++++
>  include/linux/ceph/ceph_fs.h |  1 +
>  7 files changed, 156 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
> index e3322fcb2e8d..f29d59b63c52 100644
> --- a/fs/ceph/inode.c
> +++ b/fs/ceph/inode.c
> @@ -2291,6 +2291,57 @@ int __ceph_do_getattr(struct inode *inode, struct page *locked_page,
>  	return err;
>  }
>  
> +int ceph_do_getvxattr(struct inode *inode, const char *name, void *value,
> +		      size_t size)
> +{
> +	struct ceph_fs_client *fsc = ceph_sb_to_client(inode->i_sb);
> +	struct ceph_mds_client *mdsc = fsc->mdsc;
> +	struct ceph_mds_request *req;
> +	int mode = USE_AUTH_MDS;
> +	int err;
> +	char *xattr_value;
> +	size_t xattr_value_len;
> +
> +	req = ceph_mdsc_create_request(mdsc, CEPH_MDS_OP_GETVXATTR, mode);
> +	if (IS_ERR(req)) {
> +		err = -ENOMEM;
> +		goto out;
> +	}
> +
> +	req->r_path2 = kstrdup(name, GFP_NOFS);
> +	if (!req->r_path2) {
> +		err = -ENOMEM;
> +		goto put;
> +	}
> +
> +	ihold(inode);
> +	req->r_inode = inode;
> +	err = ceph_mdsc_do_request(mdsc, NULL, req);
> +	if (err < 0)
> +		goto put;
> +
> +	xattr_value = req->r_reply_info.xattr_info.xattr_value;
> +	xattr_value_len = req->r_reply_info.xattr_info.xattr_value_len;
> +
> +	dout("do_getvxattr xattr_value_len:%lu, size:%lu\n", xattr_value_len, size);
> +
> +	err = xattr_value_len;
> +	if (size == 0)
> +		goto put;
> +
> +	if (xattr_value_len > size) {
> +		err = -ERANGE;
> +		goto put;
> +	}
> +
> +	memcpy(value, xattr_value, xattr_value_len);
> +put:
> +	ceph_mdsc_put_request(req);
> +out:
> +	dout("do_getvxattr result=%d\n", err);
> +	return err;
> +}
> +
>  
>  /*
>   * Check inode permissions.  We verify we have a valid value for
> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> index c30eefc0ac19..a5eafc71d976 100644
> --- a/fs/ceph/mds_client.c
> +++ b/fs/ceph/mds_client.c
> @@ -555,6 +555,29 @@ static int parse_reply_info_create(void **p, void *end,
>  	return -EIO;
>  }
>  
> +static int parse_reply_info_getvxattr(void **p, void *end,
> +				      struct ceph_mds_reply_info_parsed *info,
> +				      u64 features)
> +{
> +	u8 struct_v, struct_compat;
> +	u32 struct_len;
> +	u32 value_len;
> +
> +	ceph_decode_8_safe(p, end, struct_v, bad);
> +	ceph_decode_8_safe(p, end, struct_compat, bad);
> +	ceph_decode_32_safe(p, end, struct_len, bad);
> +	ceph_decode_32_safe(p, end, value_len, bad);
> +
> +	if (value_len == end - *p) {
> +	  info->xattr_info.xattr_value = *p;
> +	  info->xattr_info.xattr_value_len = end - *p;
> +	  *p = end;
> +	  return info->xattr_info.xattr_value_len;
> +	}

The kernel uses tab indent almost exclusively. Can you fix the indention
above (and anywhere else I missed)?

> +bad:
> +	return -EIO;
> +}
> +
>  /*
>   * parse extra results
>   */
> @@ -570,6 +593,8 @@ static int parse_reply_info_extra(void **p, void *end,
>  		return parse_reply_info_readdir(p, end, info, features);
>  	else if (op == CEPH_MDS_OP_CREATE)
>  		return parse_reply_info_create(p, end, info, features, s);
> +	else if (op == CEPH_MDS_OP_GETVXATTR)
> +		return parse_reply_info_getvxattr(p, end, info, features);
>  	else
>  		return -EIO;
>  }
> @@ -615,7 +640,7 @@ static int parse_reply_info(struct ceph_mds_session *s, struct ceph_msg *msg,
>  
>  	if (p != end)
>  		goto bad;
> -	return 0;
> +	return err;
>  
>  bad:
>  	err = -EIO;
> diff --git a/fs/ceph/mds_client.h b/fs/ceph/mds_client.h
> index 97c7f7bfa55f..f2a8e5af3c2e 100644
> --- a/fs/ceph/mds_client.h
> +++ b/fs/ceph/mds_client.h
> @@ -29,8 +29,10 @@ enum ceph_feature_type {
>  	CEPHFS_FEATURE_MULTI_RECONNECT,
>  	CEPHFS_FEATURE_DELEG_INO,
>  	CEPHFS_FEATURE_METRIC_COLLECT,
> +	CEPHFS_FEATURE_ALTERNATE_NAME,
> +	CEPHFS_FEATURE_GETVXATTR,
>  
> -	CEPHFS_FEATURE_MAX = CEPHFS_FEATURE_METRIC_COLLECT,
> +	CEPHFS_FEATURE_MAX = CEPHFS_FEATURE_GETVXATTR,
>  };
>  
>  /*
> @@ -45,6 +47,8 @@ enum ceph_feature_type {
>  	CEPHFS_FEATURE_MULTI_RECONNECT,		\
>  	CEPHFS_FEATURE_DELEG_INO,		\
>  	CEPHFS_FEATURE_METRIC_COLLECT,		\
> +	CEPHFS_FEATURE_ALTERNATE_NAME,		\
> +	CEPHFS_FEATURE_GETVXATTR,		\
>  						\
>  	CEPHFS_FEATURE_MAX,			\
>  }
> @@ -100,6 +104,11 @@ struct ceph_mds_reply_dir_entry {
>  	loff_t			      offset;
>  };
>  
> +struct ceph_mds_reply_xattr {
> +	char *xattr_value;
> +	size_t xattr_value_len;
> +};
> +
>  /*
>   * parsed info about an mds reply, including information about
>   * either: 1) the target inode and/or its parent directory and dentry,
> @@ -115,6 +124,7 @@ struct ceph_mds_reply_info_parsed {
>  	char                          *dname;
>  	u32                           dname_len;
>  	struct ceph_mds_reply_lease   *dlease;
> +	struct ceph_mds_reply_xattr   xattr_info;
>  
>  	/* extra */
>  	union {
> diff --git a/fs/ceph/strings.c b/fs/ceph/strings.c
> index 573bb9556fb5..e36e8948e728 100644
> --- a/fs/ceph/strings.c
> +++ b/fs/ceph/strings.c
> @@ -60,6 +60,7 @@ const char *ceph_mds_op_name(int op)
>  	case CEPH_MDS_OP_LOOKUPINO:  return "lookupino";
>  	case CEPH_MDS_OP_LOOKUPNAME:  return "lookupname";
>  	case CEPH_MDS_OP_GETATTR:  return "getattr";
> +	case CEPH_MDS_OP_GETVXATTR:  return "getvxattr";
>  	case CEPH_MDS_OP_SETXATTR: return "setxattr";
>  	case CEPH_MDS_OP_SETATTR: return "setattr";
>  	case CEPH_MDS_OP_RMXATTR: return "rmxattr";
> diff --git a/fs/ceph/super.h b/fs/ceph/super.h
> index ac331aa07cfa..a627fa69668e 100644
> --- a/fs/ceph/super.h
> +++ b/fs/ceph/super.h
> @@ -1043,6 +1043,7 @@ static inline bool ceph_inode_is_shutdown(struct inode *inode)
>  
>  /* xattr.c */
>  int __ceph_setxattr(struct inode *, const char *, const void *, size_t, int);
> +int ceph_do_getvxattr(struct inode *inode, const char *name, void *value, size_t size);
>  ssize_t __ceph_getxattr(struct inode *, const char *, void *, size_t);
>  extern ssize_t ceph_listxattr(struct dentry *, char *, size_t);
>  extern struct ceph_buffer *__ceph_build_xattrs_blob(struct ceph_inode_info *ci);
> diff --git a/fs/ceph/xattr.c b/fs/ceph/xattr.c
> index fcf7dfdecf96..fdde8ffa71bb 100644
> --- a/fs/ceph/xattr.c
> +++ b/fs/ceph/xattr.c
> @@ -918,6 +918,64 @@ static inline int __get_request_mask(struct inode *in) {
>  	return mask;
>  }
>  
> +static inline bool ceph_is_passthrough_vxattr(const char *name)
> +{
> +	const char *vxattr_list[] = {
> +		"ceph.dir.pin",
> +		"ceph.dir.pin.random",
> +		"ceph.dir.pin.distributed",
> +		"ceph.dir.layout",
> +		"ceph.dir.layout.object_size",
> +		"ceph.dir.layout.stripe_count",
> +		"ceph.dir.layout.stripe_unit",
> +		"ceph.dir.layout.pool",
> +		"ceph.dir.layout.pool_name",
> +		"ceph.dir.layout.pool_id",
> +		"ceph.dir.layout.pool_namespace",
> +		"ceph.file.layout",
> +		"ceph.file.layout.object_size",
> +		"ceph.file.layout.stripe_count",
> +		"ceph.file.layout.stripe_unit",
> +		"ceph.file.layout.pool",
> +		"ceph.file.layout.pool_name",
> +		"ceph.file.layout.pool_id",
> +		"ceph.file.layout.pool_namespace",
> +	};
> +	int i = 0;
> +	int n = sizeof(vxattr_list)/sizeof(vxattr_list[0]);
> +
> +	while (i < n) {
> +		if (!strcmp(name, vxattr_list[i]))
> +			return true;
> +		i++;
> +	}
> +	return false;
> +}

This part, I'm not so crazy about. This list will be hard to keep in
sync with the userland code and you're almost guaranteed to have a
mismatch between what the kernel and userland supports since they're on
different release schedules.

I think what we probably ought to do is run the ceph_match_vxattr call
first, and if it doesn't match the name and the name starts with
"ceph.", then we'll send a GETVXATTR call to the MDS.

Sound ok?

> +
> +/* check if the entire cluster supports the given feature */
> +static inline bool ceph_cluster_has_feature(struct inode *inode, int feature_bit)
> +{
> +	int64_t i;
> +	struct ceph_fs_client *fsc = ceph_inode_to_client(inode);
> +	struct ceph_mds_session **sessions = fsc->mdsc->sessions;
> +	int64_t num_sessions = atomic_read(&fsc->mdsc->num_sessions);
> +
> +	if (fsc->mdsc->stopping)
> +		return false;
> +
> +	if (!sessions)
> +		return false;
> +
> +	for (i = 0; i < num_sessions; i++) {
> +		struct ceph_mds_session *session = sessions[i];
> +		if (!session)
> +			return false;
> +		if (!test_bit(feature_bit, &session->s_features))
> +			return false;
> +	}
> +	return true;
> +}
> +
>  ssize_t __ceph_getxattr(struct inode *inode, const char *name, void *value,
>  		      size_t size)
>  {
> @@ -927,6 +985,13 @@ ssize_t __ceph_getxattr(struct inode *inode, const char *name, void *value,
>  	int req_mask;
>  	ssize_t err;
>  
> +	if (ceph_is_passthrough_vxattr(name) &&
> +	    ceph_cluster_has_feature(inode, CEPHFS_FEATURE_GETVXATTR)) {
> +		err = ceph_do_getvxattr(inode, name, value, size);
> +		return err;
> +	}
> +	dout("vxattr is not passthrough or kclient doesn't support GETVXATTR\n");
> +
>  	/* let's see if a virtual xattr was requested */
>  	vxattr = ceph_match_vxattr(inode, name);
>  	if (vxattr) {
> diff --git a/include/linux/ceph/ceph_fs.h b/include/linux/ceph/ceph_fs.h
> index 7ad6c3d0db7d..66db21ac5f0c 100644
> --- a/include/linux/ceph/ceph_fs.h
> +++ b/include/linux/ceph/ceph_fs.h
> @@ -328,6 +328,7 @@ enum {
>  	CEPH_MDS_OP_LOOKUPPARENT = 0x00103,
>  	CEPH_MDS_OP_LOOKUPINO  = 0x00104,
>  	CEPH_MDS_OP_LOOKUPNAME = 0x00105,
> +	CEPH_MDS_OP_GETVXATTR  = 0x00106,
>  
>  	CEPH_MDS_OP_SETXATTR   = 0x01105,
>  	CEPH_MDS_OP_RMXATTR    = 0x01106,

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH 1/1] ceph: add getvxattr op

[Milind Changire]

On Tue, Jan 11, 2022 at 6:19 PM Jeff Layton <jlayton@kernel.org> wrote:
>
> On Tue, 2022-01-11 at 17:54 +0530, Milind Changire wrote:
> > Problem:
> > Directory vxattrs like ceph.dir.pin* and ceph.dir.layout* may not be
> > propagated to the client as frequently to keep them updated. This
> > creates vxattr availability problems.
> >
> > Solution:
> > Adds new getvxattr op to fetch ceph.dir.pin*, ceph.dir.layout* and
> > ceph.file.layout* vxattrs.
> > If the entire layout for a dir or a file is being set, then it is
> > expected that the layout be set in standard JSON format. Individual
> > field value retrieval is not wrapped in JSON. The JSON format also
> > applies while setting the vxattr if the entire layout is being set in
> > one go.
> > As a temporary measure, setting a vxattr can also be done in the old
> > format. The old format will be deprecated in the future.
> >
> > URL: https://tracker.ceph.com/issues/51062
> > Signed-off-by: Milind Changire <mchangir@redhat.com>
> > ---
> >  fs/ceph/inode.c              | 51 ++++++++++++++++++++++++++++
> >  fs/ceph/mds_client.c         | 27 ++++++++++++++-
> >  fs/ceph/mds_client.h         | 12 ++++++-
> >  fs/ceph/strings.c            |  1 +
> >  fs/ceph/super.h              |  1 +
> >  fs/ceph/xattr.c              | 65 ++++++++++++++++++++++++++++++++++++
> >  include/linux/ceph/ceph_fs.h |  1 +
> >  7 files changed, 156 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
> > index e3322fcb2e8d..f29d59b63c52 100644
> > --- a/fs/ceph/inode.c
> > +++ b/fs/ceph/inode.c
> > @@ -2291,6 +2291,57 @@ int __ceph_do_getattr(struct inode *inode, struct page *locked_page,
> >       return err;
> >  }
> >
> > +int ceph_do_getvxattr(struct inode *inode, const char *name, void *value,
> > +                   size_t size)
> > +{
> > +     struct ceph_fs_client *fsc = ceph_sb_to_client(inode->i_sb);
> > +     struct ceph_mds_client *mdsc = fsc->mdsc;
> > +     struct ceph_mds_request *req;
> > +     int mode = USE_AUTH_MDS;
> > +     int err;
> > +     char *xattr_value;
> > +     size_t xattr_value_len;
> > +
> > +     req = ceph_mdsc_create_request(mdsc, CEPH_MDS_OP_GETVXATTR, mode);
> > +     if (IS_ERR(req)) {
> > +             err = -ENOMEM;
> > +             goto out;
> > +     }
> > +
> > +     req->r_path2 = kstrdup(name, GFP_NOFS);
> > +     if (!req->r_path2) {
> > +             err = -ENOMEM;
> > +             goto put;
> > +     }
> > +
> > +     ihold(inode);
> > +     req->r_inode = inode;
> > +     err = ceph_mdsc_do_request(mdsc, NULL, req);
> > +     if (err < 0)
> > +             goto put;
> > +
> > +     xattr_value = req->r_reply_info.xattr_info.xattr_value;
> > +     xattr_value_len = req->r_reply_info.xattr_info.xattr_value_len;
> > +
> > +     dout("do_getvxattr xattr_value_len:%lu, size:%lu\n", xattr_value_len, size);
> > +
> > +     err = xattr_value_len;
> > +     if (size == 0)
> > +             goto put;
> > +
> > +     if (xattr_value_len > size) {
> > +             err = -ERANGE;
> > +             goto put;
> > +     }
> > +
> > +     memcpy(value, xattr_value, xattr_value_len);
> > +put:
> > +     ceph_mdsc_put_request(req);
> > +out:
> > +     dout("do_getvxattr result=%d\n", err);
> > +     return err;
> > +}
> > +
> >
> >  /*
> >   * Check inode permissions.  We verify we have a valid value for
> > diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> > index c30eefc0ac19..a5eafc71d976 100644
> > --- a/fs/ceph/mds_client.c
> > +++ b/fs/ceph/mds_client.c
> > @@ -555,6 +555,29 @@ static int parse_reply_info_create(void **p, void *end,
> >       return -EIO;
> >  }
> >
> > +static int parse_reply_info_getvxattr(void **p, void *end,
> > +                                   struct ceph_mds_reply_info_parsed *info,
> > +                                   u64 features)
> > +{
> > +     u8 struct_v, struct_compat;
> > +     u32 struct_len;
> > +     u32 value_len;
> > +
> > +     ceph_decode_8_safe(p, end, struct_v, bad);
> > +     ceph_decode_8_safe(p, end, struct_compat, bad);
> > +     ceph_decode_32_safe(p, end, struct_len, bad);
> > +     ceph_decode_32_safe(p, end, value_len, bad);
> > +
> > +     if (value_len == end - *p) {
> > +       info->xattr_info.xattr_value = *p;
> > +       info->xattr_info.xattr_value_len = end - *p;
> > +       *p = end;
> > +       return info->xattr_info.xattr_value_len;
> > +     }
>
> The kernel uses tab indent almost exclusively. Can you fix the indention
> above (and anywhere else I missed)?

My bad; newbie issues
I'll see how can I fix this.

>
> > +bad:
> > +     return -EIO;
> > +}
> > +
> >  /*
> >   * parse extra results
> >   */
> > @@ -570,6 +593,8 @@ static int parse_reply_info_extra(void **p, void *end,
> >               return parse_reply_info_readdir(p, end, info, features);
> >       else if (op == CEPH_MDS_OP_CREATE)
> >               return parse_reply_info_create(p, end, info, features, s);
> > +     else if (op == CEPH_MDS_OP_GETVXATTR)
> > +             return parse_reply_info_getvxattr(p, end, info, features);
> >       else
> >               return -EIO;
> >  }
> > @@ -615,7 +640,7 @@ static int parse_reply_info(struct ceph_mds_session *s, struct ceph_msg *msg,
> >
> >       if (p != end)
> >               goto bad;
> > -     return 0;
> > +     return err;
> >
> >  bad:
> >       err = -EIO;
> > diff --git a/fs/ceph/mds_client.h b/fs/ceph/mds_client.h
> > index 97c7f7bfa55f..f2a8e5af3c2e 100644
> > --- a/fs/ceph/mds_client.h
> > +++ b/fs/ceph/mds_client.h
> > @@ -29,8 +29,10 @@ enum ceph_feature_type {
> >       CEPHFS_FEATURE_MULTI_RECONNECT,
> >       CEPHFS_FEATURE_DELEG_INO,
> >       CEPHFS_FEATURE_METRIC_COLLECT,
> > +     CEPHFS_FEATURE_ALTERNATE_NAME,
> > +     CEPHFS_FEATURE_GETVXATTR,
> >
> > -     CEPHFS_FEATURE_MAX = CEPHFS_FEATURE_METRIC_COLLECT,
> > +     CEPHFS_FEATURE_MAX = CEPHFS_FEATURE_GETVXATTR,
> >  };
> >
> >  /*
> > @@ -45,6 +47,8 @@ enum ceph_feature_type {
> >       CEPHFS_FEATURE_MULTI_RECONNECT,         \
> >       CEPHFS_FEATURE_DELEG_INO,               \
> >       CEPHFS_FEATURE_METRIC_COLLECT,          \
> > +     CEPHFS_FEATURE_ALTERNATE_NAME,          \
> > +     CEPHFS_FEATURE_GETVXATTR,               \
> >                                               \
> >       CEPHFS_FEATURE_MAX,                     \
> >  }
> > @@ -100,6 +104,11 @@ struct ceph_mds_reply_dir_entry {
> >       loff_t                        offset;
> >  };
> >
> > +struct ceph_mds_reply_xattr {
> > +     char *xattr_value;
> > +     size_t xattr_value_len;
> > +};
> > +
> >  /*
> >   * parsed info about an mds reply, including information about
> >   * either: 1) the target inode and/or its parent directory and dentry,
> > @@ -115,6 +124,7 @@ struct ceph_mds_reply_info_parsed {
> >       char                          *dname;
> >       u32                           dname_len;
> >       struct ceph_mds_reply_lease   *dlease;
> > +     struct ceph_mds_reply_xattr   xattr_info;
> >
> >       /* extra */
> >       union {
> > diff --git a/fs/ceph/strings.c b/fs/ceph/strings.c
> > index 573bb9556fb5..e36e8948e728 100644
> > --- a/fs/ceph/strings.c
> > +++ b/fs/ceph/strings.c
> > @@ -60,6 +60,7 @@ const char *ceph_mds_op_name(int op)
> >       case CEPH_MDS_OP_LOOKUPINO:  return "lookupino";
> >       case CEPH_MDS_OP_LOOKUPNAME:  return "lookupname";
> >       case CEPH_MDS_OP_GETATTR:  return "getattr";
> > +     case CEPH_MDS_OP_GETVXATTR:  return "getvxattr";
> >       case CEPH_MDS_OP_SETXATTR: return "setxattr";
> >       case CEPH_MDS_OP_SETATTR: return "setattr";
> >       case CEPH_MDS_OP_RMXATTR: return "rmxattr";
> > diff --git a/fs/ceph/super.h b/fs/ceph/super.h
> > index ac331aa07cfa..a627fa69668e 100644
> > --- a/fs/ceph/super.h
> > +++ b/fs/ceph/super.h
> > @@ -1043,6 +1043,7 @@ static inline bool ceph_inode_is_shutdown(struct inode *inode)
> >
> >  /* xattr.c */
> >  int __ceph_setxattr(struct inode *, const char *, const void *, size_t, int);
> > +int ceph_do_getvxattr(struct inode *inode, const char *name, void *value, size_t size);
> >  ssize_t __ceph_getxattr(struct inode *, const char *, void *, size_t);
> >  extern ssize_t ceph_listxattr(struct dentry *, char *, size_t);
> >  extern struct ceph_buffer *__ceph_build_xattrs_blob(struct ceph_inode_info *ci);
> > diff --git a/fs/ceph/xattr.c b/fs/ceph/xattr.c
> > index fcf7dfdecf96..fdde8ffa71bb 100644
> > --- a/fs/ceph/xattr.c
> > +++ b/fs/ceph/xattr.c
> > @@ -918,6 +918,64 @@ static inline int __get_request_mask(struct inode *in) {
> >       return mask;
> >  }
> >
> > +static inline bool ceph_is_passthrough_vxattr(const char *name)
> > +{
> > +     const char *vxattr_list[] = {
> > +             "ceph.dir.pin",
> > +             "ceph.dir.pin.random",
> > +             "ceph.dir.pin.distributed",
> > +             "ceph.dir.layout",
> > +             "ceph.dir.layout.object_size",
> > +             "ceph.dir.layout.stripe_count",
> > +             "ceph.dir.layout.stripe_unit",
> > +             "ceph.dir.layout.pool",
> > +             "ceph.dir.layout.pool_name",
> > +             "ceph.dir.layout.pool_id",
> > +             "ceph.dir.layout.pool_namespace",
> > +             "ceph.file.layout",
> > +             "ceph.file.layout.object_size",
> > +             "ceph.file.layout.stripe_count",
> > +             "ceph.file.layout.stripe_unit",
> > +             "ceph.file.layout.pool",
> > +             "ceph.file.layout.pool_name",
> > +             "ceph.file.layout.pool_id",
> > +             "ceph.file.layout.pool_namespace",
> > +     };
> > +     int i = 0;
> > +     int n = sizeof(vxattr_list)/sizeof(vxattr_list[0]);
> > +
> > +     while (i < n) {
> > +             if (!strcmp(name, vxattr_list[i]))
> > +                     return true;
> > +             i++;
> > +     }
> > +     return false;
> > +}
>
> This part, I'm not so crazy about. This list will be hard to keep in
> sync with the userland code and you're almost guaranteed to have a
> mismatch between what the kernel and userland supports since they're on
> different release schedules.
>
> I think what we probably ought to do is run the ceph_match_vxattr call
> first, and if it doesn't match the name and the name starts with
> "ceph.", then we'll send a GETVXATTR call to the MDS.

I was trying to preempt a getvxattr call if the full xattr name was invalid.
I could just test for the prefixes: ceph.dir.pin, ceph.dir.layout and
ceph.file.layout and send them over. Eventually, we'll get an ENODATA
if xattr name validation fails on the MDS side.

The problem with doing a ceph_match_vxattr() first is that it will succeed
for any of these vxattrs, but we'll land in the same place as the one you've
described in the tracker.
The very idea of implementing getvxattr is to bypass the existing "cached"
service mechanism for special vxattrs.

>
> Sound ok?
>
> > +
> > +/* check if the entire cluster supports the given feature */
> > +static inline bool ceph_cluster_has_feature(struct inode *inode, int feature_bit)
> > +{
> > +     int64_t i;
> > +     struct ceph_fs_client *fsc = ceph_inode_to_client(inode);
> > +     struct ceph_mds_session **sessions = fsc->mdsc->sessions;
> > +     int64_t num_sessions = atomic_read(&fsc->mdsc->num_sessions);
> > +
> > +     if (fsc->mdsc->stopping)
> > +             return false;
> > +
> > +     if (!sessions)
> > +             return false;
> > +
> > +     for (i = 0; i < num_sessions; i++) {
> > +             struct ceph_mds_session *session = sessions[i];
> > +             if (!session)
> > +                     return false;
> > +             if (!test_bit(feature_bit, &session->s_features))
> > +                     return false;
> > +     }
> > +     return true;
> > +}
> > +
> >  ssize_t __ceph_getxattr(struct inode *inode, const char *name, void *value,
> >                     size_t size)
> >  {
> > @@ -927,6 +985,13 @@ ssize_t __ceph_getxattr(struct inode *inode, const char *name, void *value,
> >       int req_mask;
> >       ssize_t err;
> >
> > +     if (ceph_is_passthrough_vxattr(name) &&
> > +         ceph_cluster_has_feature(inode, CEPHFS_FEATURE_GETVXATTR)) {
> > +             err = ceph_do_getvxattr(inode, name, value, size);
> > +             return err;
> > +     }
> > +     dout("vxattr is not passthrough or kclient doesn't support GETVXATTR\n");
> > +
> >       /* let's see if a virtual xattr was requested */
> >       vxattr = ceph_match_vxattr(inode, name);
> >       if (vxattr) {
> > diff --git a/include/linux/ceph/ceph_fs.h b/include/linux/ceph/ceph_fs.h
> > index 7ad6c3d0db7d..66db21ac5f0c 100644
> > --- a/include/linux/ceph/ceph_fs.h
> > +++ b/include/linux/ceph/ceph_fs.h
> > @@ -328,6 +328,7 @@ enum {
> >       CEPH_MDS_OP_LOOKUPPARENT = 0x00103,
> >       CEPH_MDS_OP_LOOKUPINO  = 0x00104,
> >       CEPH_MDS_OP_LOOKUPNAME = 0x00105,
> > +     CEPH_MDS_OP_GETVXATTR  = 0x00106,
> >
> >       CEPH_MDS_OP_SETXATTR   = 0x01105,
> >       CEPH_MDS_OP_RMXATTR    = 0x01106,
>
> --
> Jeff Layton <jlayton@kernel.org>

Re: [PATCH 1/1] ceph: add getvxattr op

[Jeff Layton]

On Tue, 2022-01-11 at 18:53 +0530, Milind Changire wrote:
> responses below ...
> 
> On Tue, Jan 11, 2022 at 6:19 PM Jeff Layton <jlayton@kernel.org>
> wrote:
> > On Tue, 2022-01-11 at 17:54 +0530, Milind Changire wrote:
> > > Problem:
> > > Directory vxattrs like ceph.dir.pin* and ceph.dir.layout* may not
> > > be
> > > propagated to the client as frequently to keep them updated. This
> > > creates vxattr availability problems.
> > > 
> > > Solution:
> > > Adds new getvxattr op to fetch ceph.dir.pin*, ceph.dir.layout* and
> > > ceph.file.layout* vxattrs.
> > > If the entire layout for a dir or a file is being set, then it is
> > > expected that the layout be set in standard JSON format.
> > > Individual
> > > field value retrieval is not wrapped in JSON. The JSON format also
> > > applies while setting the vxattr if the entire layout is being set
> > > in
> > > one go.
> > > As a temporary measure, setting a vxattr can also be done in the
> > > old
> > > format. The old format will be deprecated in the future.
> > > 
> > > URL: https://tracker.ceph.com/issues/51062
> > > Signed-off-by: Milind Changire <mchangir@redhat.com>
> > > ---
> > >   fs/ceph/inode.c              | 51 ++++++++++++++++++++++++++++
> > >   fs/ceph/mds_client.c         | 27 ++++++++++++++-
> > >   fs/ceph/mds_client.h         | 12 ++++++-
> > >   fs/ceph/strings.c            |  1 +
> > >   fs/ceph/super.h              |  1 +
> > >   fs/ceph/xattr.c              | 65
> > > ++++++++++++++++++++++++++++++++++++
> > >   include/linux/ceph/ceph_fs.h |  1 +
> > >   7 files changed, 156 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
> > > index e3322fcb2e8d..f29d59b63c52 100644
> > > --- a/fs/ceph/inode.c
> > > +++ b/fs/ceph/inode.c
> > > @@ -2291,6 +2291,57 @@ int __ceph_do_getattr(struct inode *inode,
> > > struct page *locked_page,
> > >        return err;
> > >   }
> > >   
> > > +int ceph_do_getvxattr(struct inode *inode, const char *name, void
> > > *value,
> > > +                   size_t size)
> > > +{
> > > +     struct ceph_fs_client *fsc = ceph_sb_to_client(inode->i_sb);
> > > +     struct ceph_mds_client *mdsc = fsc->mdsc;
> > > +     struct ceph_mds_request *req;
> > > +     int mode = USE_AUTH_MDS;
> > > +     int err;
> > > +     char *xattr_value;
> > > +     size_t xattr_value_len;
> > > +
> > > +     req = ceph_mdsc_create_request(mdsc, CEPH_MDS_OP_GETVXATTR,
> > > mode);
> > > +     if (IS_ERR(req)) {
> > > +             err = -ENOMEM;
> > > +             goto out;
> > > +     }
> > > +
> > > +     req->r_path2 = kstrdup(name, GFP_NOFS);
> > > +     if (!req->r_path2) {
> > > +             err = -ENOMEM;
> > > +             goto put;
> > > +     }
> > > +
> > > +     ihold(inode);
> > > +     req->r_inode = inode;
> > > +     err = ceph_mdsc_do_request(mdsc, NULL, req);
> > > +     if (err < 0)
> > > +             goto put;
> > > +
> > > +     xattr_value = req->r_reply_info.xattr_info.xattr_value;
> > > +     xattr_value_len = req-
> > > >r_reply_info.xattr_info.xattr_value_len;
> > > +
> > > +     dout("do_getvxattr xattr_value_len:%lu, size:%lu\n",
> > > xattr_value_len, size);
> > > +
> > > +     err = xattr_value_len;
> > > +     if (size == 0)
> > > +             goto put;
> > > +
> > > +     if (xattr_value_len > size) {
> > > +             err = -ERANGE;
> > > +             goto put;
> > > +     }
> > > +
> > > +     memcpy(value, xattr_value, xattr_value_len);
> > > +put:
> > > +     ceph_mdsc_put_request(req);
> > > +out:
> > > +     dout("do_getvxattr result=%d\n", err);
> > > +     return err;
> > > +}
> > > +
> > >   
> > >   /*
> > >    * Check inode permissions.  We verify we have a valid value for
> > > diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> > > index c30eefc0ac19..a5eafc71d976 100644
> > > --- a/fs/ceph/mds_client.c
> > > +++ b/fs/ceph/mds_client.c
> > > @@ -555,6 +555,29 @@ static int parse_reply_info_create(void **p,
> > > void *end,
> > >        return -EIO;
> > >   }
> > >   
> > > +static int parse_reply_info_getvxattr(void **p, void *end,
> > > +                                   struct
> > > ceph_mds_reply_info_parsed *info,
> > > +                                   u64 features)
> > > +{
> > > +     u8 struct_v, struct_compat;
> > > +     u32 struct_len;
> > > +     u32 value_len;
> > > +
> > > +     ceph_decode_8_safe(p, end, struct_v, bad);
> > > +     ceph_decode_8_safe(p, end, struct_compat, bad);
> > > +     ceph_decode_32_safe(p, end, struct_len, bad);
> > > +     ceph_decode_32_safe(p, end, value_len, bad);
> > > +
> > > +     if (value_len == end - *p) {
> > > +       info->xattr_info.xattr_value = *p;
> > > +       info->xattr_info.xattr_value_len = end - *p;
> > > +       *p = end;
> > > +       return info->xattr_info.xattr_value_len;
> > > +     }
> > 
> > The kernel uses tab indent almost exclusively. Can you fix the
> > indention
> > above (and anywhere else I missed)?
> > 
> 
> 
> I'll fix this. newbie issues :P
>  
> > 
> > > +bad:
> > > +     return -EIO;
> > > +}
> > > +
> > >   /*
> > >    * parse extra results
> > >    */
> > > @@ -570,6 +593,8 @@ static int parse_reply_info_extra(void **p,
> > > void *end,
> > >                return parse_reply_info_readdir(p, end, info,
> > > features);
> > >        else if (op == CEPH_MDS_OP_CREATE)
> > >                return parse_reply_info_create(p, end, info,
> > > features, s);
> > > +     else if (op == CEPH_MDS_OP_GETVXATTR)
> > > +             return parse_reply_info_getvxattr(p, end, info,
> > > features);
> > >        else
> > >                return -EIO;
> > >   }
> > > @@ -615,7 +640,7 @@ static int parse_reply_info(struct
> > > ceph_mds_session *s, struct ceph_msg *msg,
> > >   
> > >        if (p != end)
> > >                goto bad;
> > > -     return 0;
> > > +     return err;
> > >   
> > >   bad:
> > >        err = -EIO;
> > > diff --git a/fs/ceph/mds_client.h b/fs/ceph/mds_client.h
> > > index 97c7f7bfa55f..f2a8e5af3c2e 100644
> > > --- a/fs/ceph/mds_client.h
> > > +++ b/fs/ceph/mds_client.h
> > > @@ -29,8 +29,10 @@ enum ceph_feature_type {
> > >        CEPHFS_FEATURE_MULTI_RECONNECT,
> > >        CEPHFS_FEATURE_DELEG_INO,
> > >        CEPHFS_FEATURE_METRIC_COLLECT,
> > > +     CEPHFS_FEATURE_ALTERNATE_NAME,
> > > +     CEPHFS_FEATURE_GETVXATTR,
> > >   
> > > -     CEPHFS_FEATURE_MAX = CEPHFS_FEATURE_METRIC_COLLECT,
> > > +     CEPHFS_FEATURE_MAX = CEPHFS_FEATURE_GETVXATTR,
> > >   };
> > >   
> > >   /*
> > > @@ -45,6 +47,8 @@ enum ceph_feature_type {
> > >        CEPHFS_FEATURE_MULTI_RECONNECT,         \
> > >        CEPHFS_FEATURE_DELEG_INO,               \
> > >        CEPHFS_FEATURE_METRIC_COLLECT,          \
> > > +     CEPHFS_FEATURE_ALTERNATE_NAME,          \
> > > +     CEPHFS_FEATURE_GETVXATTR,               \
> > >                                                \
> > >        CEPHFS_FEATURE_MAX,                     \
> > >   }
> > > @@ -100,6 +104,11 @@ struct ceph_mds_reply_dir_entry {
> > >        loff_t                        offset;
> > >   };
> > >   
> > > +struct ceph_mds_reply_xattr {
> > > +     char *xattr_value;
> > > +     size_t xattr_value_len;
> > > +};
> > > +
> > >   /*
> > >    * parsed info about an mds reply, including information about
> > >    * either: 1) the target inode and/or its parent directory and
> > > dentry,
> > > @@ -115,6 +124,7 @@ struct ceph_mds_reply_info_parsed {
> > >        char                          *dname;
> > >        u32                           dname_len;
> > >        struct ceph_mds_reply_lease   *dlease;
> > > +     struct ceph_mds_reply_xattr   xattr_info;
> > >   
> > >        /* extra */
> > >        union {
> > > diff --git a/fs/ceph/strings.c b/fs/ceph/strings.c
> > > index 573bb9556fb5..e36e8948e728 100644
> > > --- a/fs/ceph/strings.c
> > > +++ b/fs/ceph/strings.c
> > > @@ -60,6 +60,7 @@ const char *ceph_mds_op_name(int op)
> > >        case CEPH_MDS_OP_LOOKUPINO:  return "lookupino";
> > >        case CEPH_MDS_OP_LOOKUPNAME:  return "lookupname";
> > >        case CEPH_MDS_OP_GETATTR:  return "getattr";
> > > +     case CEPH_MDS_OP_GETVXATTR:  return "getvxattr";
> > >        case CEPH_MDS_OP_SETXATTR: return "setxattr";
> > >        case CEPH_MDS_OP_SETATTR: return "setattr";
> > >        case CEPH_MDS_OP_RMXATTR: return "rmxattr";
> > > diff --git a/fs/ceph/super.h b/fs/ceph/super.h
> > > index ac331aa07cfa..a627fa69668e 100644
> > > --- a/fs/ceph/super.h
> > > +++ b/fs/ceph/super.h
> > > @@ -1043,6 +1043,7 @@ static inline bool
> > > ceph_inode_is_shutdown(struct inode *inode)
> > >   
> > >   /* xattr.c */
> > >   int __ceph_setxattr(struct inode *, const char *, const void *,
> > > size_t, int);
> > > +int ceph_do_getvxattr(struct inode *inode, const char *name, void
> > > *value, size_t size);
> > >   ssize_t __ceph_getxattr(struct inode *, const char *, void *,
> > > size_t);
> > >   extern ssize_t ceph_listxattr(struct dentry *, char *, size_t);
> > >   extern struct ceph_buffer *__ceph_build_xattrs_blob(struct
> > > ceph_inode_info *ci);
> > > diff --git a/fs/ceph/xattr.c b/fs/ceph/xattr.c
> > > index fcf7dfdecf96..fdde8ffa71bb 100644
> > > --- a/fs/ceph/xattr.c
> > > +++ b/fs/ceph/xattr.c
> > > @@ -918,6 +918,64 @@ static inline int __get_request_mask(struct
> > > inode *in) {
> > >        return mask;
> > >   }
> > >   
> > > +static inline bool ceph_is_passthrough_vxattr(const char *name)
> > > +{
> > > +     const char *vxattr_list[] = {
> > > +             "ceph.dir.pin",
> > > +             "ceph.dir.pin.random",
> > > +             "ceph.dir.pin.distributed",
> > > +             "ceph.dir.layout",
> > > +             "ceph.dir.layout.object_size",
> > > +             "ceph.dir.layout.stripe_count",
> > > +             "ceph.dir.layout.stripe_unit",
> > > +             "ceph.dir.layout.pool",
> > > +             "ceph.dir.layout.pool_name",
> > > +             "ceph.dir.layout.pool_id",
> > > +             "ceph.dir.layout.pool_namespace",
> > > +             "ceph.file.layout",
> > > +             "ceph.file.layout.object_size",
> > > +             "ceph.file.layout.stripe_count",
> > > +             "ceph.file.layout.stripe_unit",
> > > +             "ceph.file.layout.pool",
> > > +             "ceph.file.layout.pool_name",
> > > +             "ceph.file.layout.pool_id",
> > > +             "ceph.file.layout.pool_namespace",
> > > +     };
> > > +     int i = 0;
> > > +     int n = sizeof(vxattr_list)/sizeof(vxattr_list[0]);
> > > +
> > > +     while (i < n) {
> > > +             if (!strcmp(name, vxattr_list[i]))
> > > +                     return true;
> > > +             i++;
> > > +     }
> > > +     return false;
> > > +}
> > 
> > This part, I'm not so crazy about. This list will be hard to keep in
> > sync with the userland code and you're almost guaranteed to have a
> > mismatch between what the kernel and userland supports since they're
> > on
> > different release schedules.
> > 
> > I think what we probably ought to do is run the ceph_match_vxattr
> > call
> > first, and if it doesn't match the name and the name starts with
> > "ceph.", then we'll send a GETVXATTR call to the MDS.
> > 
> > Sound ok?
> > 
> 
> 
> I was trying to preempt a getvxattr call if the full xattr name was
> invalid.

Huh, why? That seems like the point of the whole bug. We need a way to
hand vxattrs that the client doesn't recognize off to the MDS so it can
satisfy the request.

> I could just test for the prefixes: ceph.dir.pin, ceph.dir.layout and
> ceph.file.layout and send them over. Eventually, we'll get an ENODATA
> if xattr name validation fails on the MDS side.
> 
> The problem with doing a ceph_match_vxattr() first is that it will
> succeed
> for any of these vxattrs, but we'll land in the same place as the one
> you've
> described in the tracker.
> The very idea of implementing getvxattr is to bypass the existing
> "cached"
> service mechanism for special vxattrs.
> 

I don't get it. What's the benefit of calling out to the MDS to ask for
ceph.file.layout.pool instead of just satisfying it from the info we
have?

I'd prefer we just add a fallback for "ceph.*" vxattrs that for which we
don't have a local handler.


> > 
> > > +
> > > +/* check if the entire cluster supports the given feature */
> > > +static inline bool ceph_cluster_has_feature(struct inode *inode,
> > > int feature_bit)
> > > +{
> > > +     int64_t i;
> > > +     struct ceph_fs_client *fsc = ceph_inode_to_client(inode);
> > > +     struct ceph_mds_session **sessions = fsc->mdsc->sessions;
> > > +     int64_t num_sessions = atomic_read(&fsc->mdsc-
> > > >num_sessions);
> > > +
> > > +     if (fsc->mdsc->stopping)
> > > +             return false;
> > > +
> > > +     if (!sessions)
> > > +             return false;
> > > +
> > > +     for (i = 0; i < num_sessions; i++) {
> > > +             struct ceph_mds_session *session = sessions[i];
> > > +             if (!session)
> > > +                     return false;
> > > +             if (!test_bit(feature_bit, &session->s_features))
> > > +                     return false;
> > > +     }
> > > +     return true;
> > > +}
> > > +
> > >   ssize_t __ceph_getxattr(struct inode *inode, const char *name,
> > > void *value,
> > >                      size_t size)
> > >   {
> > > @@ -927,6 +985,13 @@ ssize_t __ceph_getxattr(struct inode *inode,
> > > const char *name, void *value,
> > >        int req_mask;
> > >        ssize_t err;
> > >   
> > > +     if (ceph_is_passthrough_vxattr(name) &&
> > > +         ceph_cluster_has_feature(inode,
> > > CEPHFS_FEATURE_GETVXATTR)) {
> > > +             err = ceph_do_getvxattr(inode, name, value, size);
> > > +             return err;
> > > +     }
> > > +     dout("vxattr is not passthrough or kclient doesn't support
> > > GETVXATTR\n");
> > > +
> > >        /* let's see if a virtual xattr was requested */
> > >        vxattr = ceph_match_vxattr(inode, name);
> > >        if (vxattr) {
> > > diff --git a/include/linux/ceph/ceph_fs.h
> > > b/include/linux/ceph/ceph_fs.h
> > > index 7ad6c3d0db7d..66db21ac5f0c 100644
> > > --- a/include/linux/ceph/ceph_fs.h
> > > +++ b/include/linux/ceph/ceph_fs.h
> > > @@ -328,6 +328,7 @@ enum {
> > >        CEPH_MDS_OP_LOOKUPPARENT = 0x00103,
> > >        CEPH_MDS_OP_LOOKUPINO  = 0x00104,
> > >        CEPH_MDS_OP_LOOKUPNAME = 0x00105,
> > > +     CEPH_MDS_OP_GETVXATTR  = 0x00106,
> > >   
> > >        CEPH_MDS_OP_SETXATTR   = 0x01105,
> > >        CEPH_MDS_OP_RMXATTR    = 0x01106,
> > 

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH 1/1] ceph: add getvxattr op

[Milind Changire]

On Tue, Jan 11, 2022 at 7:18 PM Jeff Layton <jlayton@kernel.org> wrote:
>
> On Tue, 2022-01-11 at 18:53 +0530, Milind Changire wrote:
> > responses below ...
> >
> > On Tue, Jan 11, 2022 at 6:19 PM Jeff Layton <jlayton@kernel.org>
> > wrote:
> > > On Tue, 2022-01-11 at 17:54 +0530, Milind Changire wrote:
> > > > Problem:
> > > > Directory vxattrs like ceph.dir.pin* and ceph.dir.layout* may not
> > > > be
> > > > propagated to the client as frequently to keep them updated. This
> > > > creates vxattr availability problems.
> > > >
> > > > Solution:
> > > > Adds new getvxattr op to fetch ceph.dir.pin*, ceph.dir.layout* and
> > > > ceph.file.layout* vxattrs.
> > > > If the entire layout for a dir or a file is being set, then it is
> > > > expected that the layout be set in standard JSON format.
> > > > Individual
> > > > field value retrieval is not wrapped in JSON. The JSON format also
> > > > applies while setting the vxattr if the entire layout is being set
> > > > in
> > > > one go.
> > > > As a temporary measure, setting a vxattr can also be done in the
> > > > old
> > > > format. The old format will be deprecated in the future.
> > > >
> > > > URL: https://tracker.ceph.com/issues/51062
> > > > Signed-off-by: Milind Changire <mchangir@redhat.com>
> > > > ---
> > > >   fs/ceph/inode.c              | 51 ++++++++++++++++++++++++++++
> > > >   fs/ceph/mds_client.c         | 27 ++++++++++++++-
> > > >   fs/ceph/mds_client.h         | 12 ++++++-
> > > >   fs/ceph/strings.c            |  1 +
> > > >   fs/ceph/super.h              |  1 +
> > > >   fs/ceph/xattr.c              | 65
> > > > ++++++++++++++++++++++++++++++++++++
> > > >   include/linux/ceph/ceph_fs.h |  1 +
> > > >   7 files changed, 156 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
> > > > index e3322fcb2e8d..f29d59b63c52 100644
> > > > --- a/fs/ceph/inode.c
> > > > +++ b/fs/ceph/inode.c
> > > > @@ -2291,6 +2291,57 @@ int __ceph_do_getattr(struct inode *inode,
> > > > struct page *locked_page,
> > > >        return err;
> > > >   }
> > > >
> > > > +int ceph_do_getvxattr(struct inode *inode, const char *name, void
> > > > *value,
> > > > +                   size_t size)
> > > > +{
> > > > +     struct ceph_fs_client *fsc = ceph_sb_to_client(inode->i_sb);
> > > > +     struct ceph_mds_client *mdsc = fsc->mdsc;
> > > > +     struct ceph_mds_request *req;
> > > > +     int mode = USE_AUTH_MDS;
> > > > +     int err;
> > > > +     char *xattr_value;
> > > > +     size_t xattr_value_len;
> > > > +
> > > > +     req = ceph_mdsc_create_request(mdsc, CEPH_MDS_OP_GETVXATTR,
> > > > mode);
> > > > +     if (IS_ERR(req)) {
> > > > +             err = -ENOMEM;
> > > > +             goto out;
> > > > +     }
> > > > +
> > > > +     req->r_path2 = kstrdup(name, GFP_NOFS);
> > > > +     if (!req->r_path2) {
> > > > +             err = -ENOMEM;
> > > > +             goto put;
> > > > +     }
> > > > +
> > > > +     ihold(inode);
> > > > +     req->r_inode = inode;
> > > > +     err = ceph_mdsc_do_request(mdsc, NULL, req);
> > > > +     if (err < 0)
> > > > +             goto put;
> > > > +
> > > > +     xattr_value = req->r_reply_info.xattr_info.xattr_value;
> > > > +     xattr_value_len = req-
> > > > >r_reply_info.xattr_info.xattr_value_len;
> > > > +
> > > > +     dout("do_getvxattr xattr_value_len:%lu, size:%lu\n",
> > > > xattr_value_len, size);
> > > > +
> > > > +     err = xattr_value_len;
> > > > +     if (size == 0)
> > > > +             goto put;
> > > > +
> > > > +     if (xattr_value_len > size) {
> > > > +             err = -ERANGE;
> > > > +             goto put;
> > > > +     }
> > > > +
> > > > +     memcpy(value, xattr_value, xattr_value_len);
> > > > +put:
> > > > +     ceph_mdsc_put_request(req);
> > > > +out:
> > > > +     dout("do_getvxattr result=%d\n", err);
> > > > +     return err;
> > > > +}
> > > > +
> > > >
> > > >   /*
> > > >    * Check inode permissions.  We verify we have a valid value for
> > > > diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> > > > index c30eefc0ac19..a5eafc71d976 100644
> > > > --- a/fs/ceph/mds_client.c
> > > > +++ b/fs/ceph/mds_client.c
> > > > @@ -555,6 +555,29 @@ static int parse_reply_info_create(void **p,
> > > > void *end,
> > > >        return -EIO;
> > > >   }
> > > >
> > > > +static int parse_reply_info_getvxattr(void **p, void *end,
> > > > +                                   struct
> > > > ceph_mds_reply_info_parsed *info,
> > > > +                                   u64 features)
> > > > +{
> > > > +     u8 struct_v, struct_compat;
> > > > +     u32 struct_len;
> > > > +     u32 value_len;
> > > > +
> > > > +     ceph_decode_8_safe(p, end, struct_v, bad);
> > > > +     ceph_decode_8_safe(p, end, struct_compat, bad);
> > > > +     ceph_decode_32_safe(p, end, struct_len, bad);
> > > > +     ceph_decode_32_safe(p, end, value_len, bad);
> > > > +
> > > > +     if (value_len == end - *p) {
> > > > +       info->xattr_info.xattr_value = *p;
> > > > +       info->xattr_info.xattr_value_len = end - *p;
> > > > +       *p = end;
> > > > +       return info->xattr_info.xattr_value_len;
> > > > +     }
> > >
> > > The kernel uses tab indent almost exclusively. Can you fix the
> > > indention
> > > above (and anywhere else I missed)?
> > >
> >
> >
> > I'll fix this. newbie issues :P
> >
> > >
> > > > +bad:
> > > > +     return -EIO;
> > > > +}
> > > > +
> > > >   /*
> > > >    * parse extra results
> > > >    */
> > > > @@ -570,6 +593,8 @@ static int parse_reply_info_extra(void **p,
> > > > void *end,
> > > >                return parse_reply_info_readdir(p, end, info,
> > > > features);
> > > >        else if (op == CEPH_MDS_OP_CREATE)
> > > >                return parse_reply_info_create(p, end, info,
> > > > features, s);
> > > > +     else if (op == CEPH_MDS_OP_GETVXATTR)
> > > > +             return parse_reply_info_getvxattr(p, end, info,
> > > > features);
> > > >        else
> > > >                return -EIO;
> > > >   }
> > > > @@ -615,7 +640,7 @@ static int parse_reply_info(struct
> > > > ceph_mds_session *s, struct ceph_msg *msg,
> > > >
> > > >        if (p != end)
> > > >                goto bad;
> > > > -     return 0;
> > > > +     return err;
> > > >
> > > >   bad:
> > > >        err = -EIO;
> > > > diff --git a/fs/ceph/mds_client.h b/fs/ceph/mds_client.h
> > > > index 97c7f7bfa55f..f2a8e5af3c2e 100644
> > > > --- a/fs/ceph/mds_client.h
> > > > +++ b/fs/ceph/mds_client.h
> > > > @@ -29,8 +29,10 @@ enum ceph_feature_type {
> > > >        CEPHFS_FEATURE_MULTI_RECONNECT,
> > > >        CEPHFS_FEATURE_DELEG_INO,
> > > >        CEPHFS_FEATURE_METRIC_COLLECT,
> > > > +     CEPHFS_FEATURE_ALTERNATE_NAME,
> > > > +     CEPHFS_FEATURE_GETVXATTR,
> > > >
> > > > -     CEPHFS_FEATURE_MAX = CEPHFS_FEATURE_METRIC_COLLECT,
> > > > +     CEPHFS_FEATURE_MAX = CEPHFS_FEATURE_GETVXATTR,
> > > >   };
> > > >
> > > >   /*
> > > > @@ -45,6 +47,8 @@ enum ceph_feature_type {
> > > >        CEPHFS_FEATURE_MULTI_RECONNECT,         \
> > > >        CEPHFS_FEATURE_DELEG_INO,               \
> > > >        CEPHFS_FEATURE_METRIC_COLLECT,          \
> > > > +     CEPHFS_FEATURE_ALTERNATE_NAME,          \
> > > > +     CEPHFS_FEATURE_GETVXATTR,               \
> > > >                                                \
> > > >        CEPHFS_FEATURE_MAX,                     \
> > > >   }
> > > > @@ -100,6 +104,11 @@ struct ceph_mds_reply_dir_entry {
> > > >        loff_t                        offset;
> > > >   };
> > > >
> > > > +struct ceph_mds_reply_xattr {
> > > > +     char *xattr_value;
> > > > +     size_t xattr_value_len;
> > > > +};
> > > > +
> > > >   /*
> > > >    * parsed info about an mds reply, including information about
> > > >    * either: 1) the target inode and/or its parent directory and
> > > > dentry,
> > > > @@ -115,6 +124,7 @@ struct ceph_mds_reply_info_parsed {
> > > >        char                          *dname;
> > > >        u32                           dname_len;
> > > >        struct ceph_mds_reply_lease   *dlease;
> > > > +     struct ceph_mds_reply_xattr   xattr_info;
> > > >
> > > >        /* extra */
> > > >        union {
> > > > diff --git a/fs/ceph/strings.c b/fs/ceph/strings.c
> > > > index 573bb9556fb5..e36e8948e728 100644
> > > > --- a/fs/ceph/strings.c
> > > > +++ b/fs/ceph/strings.c
> > > > @@ -60,6 +60,7 @@ const char *ceph_mds_op_name(int op)
> > > >        case CEPH_MDS_OP_LOOKUPINO:  return "lookupino";
> > > >        case CEPH_MDS_OP_LOOKUPNAME:  return "lookupname";
> > > >        case CEPH_MDS_OP_GETATTR:  return "getattr";
> > > > +     case CEPH_MDS_OP_GETVXATTR:  return "getvxattr";
> > > >        case CEPH_MDS_OP_SETXATTR: return "setxattr";
> > > >        case CEPH_MDS_OP_SETATTR: return "setattr";
> > > >        case CEPH_MDS_OP_RMXATTR: return "rmxattr";
> > > > diff --git a/fs/ceph/super.h b/fs/ceph/super.h
> > > > index ac331aa07cfa..a627fa69668e 100644
> > > > --- a/fs/ceph/super.h
> > > > +++ b/fs/ceph/super.h
> > > > @@ -1043,6 +1043,7 @@ static inline bool
> > > > ceph_inode_is_shutdown(struct inode *inode)
> > > >
> > > >   /* xattr.c */
> > > >   int __ceph_setxattr(struct inode *, const char *, const void *,
> > > > size_t, int);
> > > > +int ceph_do_getvxattr(struct inode *inode, const char *name, void
> > > > *value, size_t size);
> > > >   ssize_t __ceph_getxattr(struct inode *, const char *, void *,
> > > > size_t);
> > > >   extern ssize_t ceph_listxattr(struct dentry *, char *, size_t);
> > > >   extern struct ceph_buffer *__ceph_build_xattrs_blob(struct
> > > > ceph_inode_info *ci);
> > > > diff --git a/fs/ceph/xattr.c b/fs/ceph/xattr.c
> > > > index fcf7dfdecf96..fdde8ffa71bb 100644
> > > > --- a/fs/ceph/xattr.c
> > > > +++ b/fs/ceph/xattr.c
> > > > @@ -918,6 +918,64 @@ static inline int __get_request_mask(struct
> > > > inode *in) {
> > > >        return mask;
> > > >   }
> > > >
> > > > +static inline bool ceph_is_passthrough_vxattr(const char *name)
> > > > +{
> > > > +     const char *vxattr_list[] = {
> > > > +             "ceph.dir.pin",
> > > > +             "ceph.dir.pin.random",
> > > > +             "ceph.dir.pin.distributed",
> > > > +             "ceph.dir.layout",
> > > > +             "ceph.dir.layout.object_size",
> > > > +             "ceph.dir.layout.stripe_count",
> > > > +             "ceph.dir.layout.stripe_unit",
> > > > +             "ceph.dir.layout.pool",
> > > > +             "ceph.dir.layout.pool_name",
> > > > +             "ceph.dir.layout.pool_id",
> > > > +             "ceph.dir.layout.pool_namespace",
> > > > +             "ceph.file.layout",
> > > > +             "ceph.file.layout.object_size",
> > > > +             "ceph.file.layout.stripe_count",
> > > > +             "ceph.file.layout.stripe_unit",
> > > > +             "ceph.file.layout.pool",
> > > > +             "ceph.file.layout.pool_name",
> > > > +             "ceph.file.layout.pool_id",
> > > > +             "ceph.file.layout.pool_namespace",
> > > > +     };
> > > > +     int i = 0;
> > > > +     int n = sizeof(vxattr_list)/sizeof(vxattr_list[0]);
> > > > +
> > > > +     while (i < n) {
> > > > +             if (!strcmp(name, vxattr_list[i]))
> > > > +                     return true;
> > > > +             i++;
> > > > +     }
> > > > +     return false;
> > > > +}
> > >
> > > This part, I'm not so crazy about. This list will be hard to keep in
> > > sync with the userland code and you're almost guaranteed to have a
> > > mismatch between what the kernel and userland supports since they're
> > > on
> > > different release schedules.
> > >
> > > I think what we probably ought to do is run the ceph_match_vxattr
> > > call
> > > first, and if it doesn't match the name and the name starts with
> > > "ceph.", then we'll send a GETVXATTR call to the MDS.
> > >
> > > Sound ok?
> > >
> >
> >
> > I was trying to preempt a getvxattr call if the full xattr name was
> > invalid.
>
> Huh, why? That seems like the point of the whole bug. We need a way to
> hand vxattrs that the client doesn't recognize off to the MDS so it can
> satisfy the request.

File and dir layout vxattrs are server-side xattrs. So, it seemed prudent to
delegate them to the server. The idea was also to help identify layouts and
their inheritance for a dir hierarchy or a file at the server side and possibly
avoiding client round trips to the server for learning about the same (and
maintain consistency?)

Here's some discussion about the same:
https://tracker.ceph.com/issues/51062#note-3

>
> > I could just test for the prefixes: ceph.dir.pin, ceph.dir.layout and
> > ceph.file.layout and send them over. Eventually, we'll get an ENODATA
> > if xattr name validation fails on the MDS side.
> >
> > The problem with doing a ceph_match_vxattr() first is that it will
> > succeed
> > for any of these vxattrs, but we'll land in the same place as the one
> > you've
> > described in the tracker.
> > The very idea of implementing getvxattr is to bypass the existing
> > "cached"
> > service mechanism for special vxattrs.
> >
>
> I don't get it. What's the benefit of calling out to the MDS to ask for
> ceph.file.layout.pool instead of just satisfying it from the info we
> have?
>
> I'd prefer we just add a fallback for "ceph.*" vxattrs that for which we
> don't have a local handler.

See my explanation above.

>
>
> > >
> > > > +
> > > > +/* check if the entire cluster supports the given feature */
> > > > +static inline bool ceph_cluster_has_feature(struct inode *inode,
> > > > int feature_bit)
> > > > +{
> > > > +     int64_t i;
> > > > +     struct ceph_fs_client *fsc = ceph_inode_to_client(inode);
> > > > +     struct ceph_mds_session **sessions = fsc->mdsc->sessions;
> > > > +     int64_t num_sessions = atomic_read(&fsc->mdsc-
> > > > >num_sessions);
> > > > +
> > > > +     if (fsc->mdsc->stopping)
> > > > +             return false;
> > > > +
> > > > +     if (!sessions)
> > > > +             return false;
> > > > +
> > > > +     for (i = 0; i < num_sessions; i++) {
> > > > +             struct ceph_mds_session *session = sessions[i];
> > > > +             if (!session)
> > > > +                     return false;
> > > > +             if (!test_bit(feature_bit, &session->s_features))
> > > > +                     return false;
> > > > +     }
> > > > +     return true;
> > > > +}
> > > > +
> > > >   ssize_t __ceph_getxattr(struct inode *inode, const char *name,
> > > > void *value,
> > > >                      size_t size)
> > > >   {
> > > > @@ -927,6 +985,13 @@ ssize_t __ceph_getxattr(struct inode *inode,
> > > > const char *name, void *value,
> > > >        int req_mask;
> > > >        ssize_t err;
> > > >
> > > > +     if (ceph_is_passthrough_vxattr(name) &&
> > > > +         ceph_cluster_has_feature(inode,
> > > > CEPHFS_FEATURE_GETVXATTR)) {
> > > > +             err = ceph_do_getvxattr(inode, name, value, size);
> > > > +             return err;
> > > > +     }
> > > > +     dout("vxattr is not passthrough or kclient doesn't support
> > > > GETVXATTR\n");
> > > > +
> > > >        /* let's see if a virtual xattr was requested */
> > > >        vxattr = ceph_match_vxattr(inode, name);
> > > >        if (vxattr) {
> > > > diff --git a/include/linux/ceph/ceph_fs.h
> > > > b/include/linux/ceph/ceph_fs.h
> > > > index 7ad6c3d0db7d..66db21ac5f0c 100644
> > > > --- a/include/linux/ceph/ceph_fs.h
> > > > +++ b/include/linux/ceph/ceph_fs.h
> > > > @@ -328,6 +328,7 @@ enum {
> > > >        CEPH_MDS_OP_LOOKUPPARENT = 0x00103,
> > > >        CEPH_MDS_OP_LOOKUPINO  = 0x00104,
> > > >        CEPH_MDS_OP_LOOKUPNAME = 0x00105,
> > > > +     CEPH_MDS_OP_GETVXATTR  = 0x00106,
> > > >
> > > >        CEPH_MDS_OP_SETXATTR   = 0x01105,
> > > >        CEPH_MDS_OP_RMXATTR    = 0x01106,
> > >
>
> --
> Jeff Layton <jlayton@kernel.org>

Re: [PATCH 1/1] ceph: add getvxattr op

[Jeff Layton]

On Wed, 2022-01-12 at 18:00 +0530, Milind Changire wrote:
> On Tue, Jan 11, 2022 at 7:18 PM Jeff Layton <jlayton@kernel.org> wrote:
> > 
> > On Tue, 2022-01-11 at 18:53 +0530, Milind Changire wrote:
> > > responses below ...
> > > 
> > > On Tue, Jan 11, 2022 at 6:19 PM Jeff Layton <jlayton@kernel.org>
> > > wrote:
> > > > On Tue, 2022-01-11 at 17:54 +0530, Milind Changire wrote:
> > > > > Problem:
> > > > > Directory vxattrs like ceph.dir.pin* and ceph.dir.layout* may not
> > > > > be
> > > > > propagated to the client as frequently to keep them updated. This
> > > > > creates vxattr availability problems.
> > > > > 
> > > > > Solution:
> > > > > Adds new getvxattr op to fetch ceph.dir.pin*, ceph.dir.layout* and
> > > > > ceph.file.layout* vxattrs.
> > > > > If the entire layout for a dir or a file is being set, then it is
> > > > > expected that the layout be set in standard JSON format.
> > > > > Individual
> > > > > field value retrieval is not wrapped in JSON. The JSON format also
> > > > > applies while setting the vxattr if the entire layout is being set
> > > > > in
> > > > > one go.
> > > > > As a temporary measure, setting a vxattr can also be done in the
> > > > > old
> > > > > format. The old format will be deprecated in the future.
> > > > > 
> > > > > URL: https://tracker.ceph.com/issues/51062
> > > > > Signed-off-by: Milind Changire <mchangir@redhat.com>
> > > > > ---
> > > > >   fs/ceph/inode.c              | 51 ++++++++++++++++++++++++++++
> > > > >   fs/ceph/mds_client.c         | 27 ++++++++++++++-
> > > > >   fs/ceph/mds_client.h         | 12 ++++++-
> > > > >   fs/ceph/strings.c            |  1 +
> > > > >   fs/ceph/super.h              |  1 +
> > > > >   fs/ceph/xattr.c              | 65
> > > > > ++++++++++++++++++++++++++++++++++++
> > > > >   include/linux/ceph/ceph_fs.h |  1 +
> > > > >   7 files changed, 156 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
> > > > > index e3322fcb2e8d..f29d59b63c52 100644
> > > > > --- a/fs/ceph/inode.c
> > > > > +++ b/fs/ceph/inode.c
> > > > > @@ -2291,6 +2291,57 @@ int __ceph_do_getattr(struct inode *inode,
> > > > > struct page *locked_page,
> > > > >        return err;
> > > > >   }
> > > > > 
> > > > > +int ceph_do_getvxattr(struct inode *inode, const char *name, void
> > > > > *value,
> > > > > +                   size_t size)
> > > > > +{
> > > > > +     struct ceph_fs_client *fsc = ceph_sb_to_client(inode->i_sb);
> > > > > +     struct ceph_mds_client *mdsc = fsc->mdsc;
> > > > > +     struct ceph_mds_request *req;
> > > > > +     int mode = USE_AUTH_MDS;
> > > > > +     int err;
> > > > > +     char *xattr_value;
> > > > > +     size_t xattr_value_len;
> > > > > +
> > > > > +     req = ceph_mdsc_create_request(mdsc, CEPH_MDS_OP_GETVXATTR,
> > > > > mode);
> > > > > +     if (IS_ERR(req)) {
> > > > > +             err = -ENOMEM;
> > > > > +             goto out;
> > > > > +     }
> > > > > +
> > > > > +     req->r_path2 = kstrdup(name, GFP_NOFS);
> > > > > +     if (!req->r_path2) {
> > > > > +             err = -ENOMEM;
> > > > > +             goto put;
> > > > > +     }
> > > > > +
> > > > > +     ihold(inode);
> > > > > +     req->r_inode = inode;
> > > > > +     err = ceph_mdsc_do_request(mdsc, NULL, req);
> > > > > +     if (err < 0)
> > > > > +             goto put;
> > > > > +
> > > > > +     xattr_value = req->r_reply_info.xattr_info.xattr_value;
> > > > > +     xattr_value_len = req-
> > > > > > r_reply_info.xattr_info.xattr_value_len;
> > > > > +
> > > > > +     dout("do_getvxattr xattr_value_len:%lu, size:%lu\n",
> > > > > xattr_value_len, size);
> > > > > +
> > > > > +     err = xattr_value_len;
> > > > > +     if (size == 0)
> > > > > +             goto put;
> > > > > +
> > > > > +     if (xattr_value_len > size) {
> > > > > +             err = -ERANGE;
> > > > > +             goto put;
> > > > > +     }
> > > > > +
> > > > > +     memcpy(value, xattr_value, xattr_value_len);
> > > > > +put:
> > > > > +     ceph_mdsc_put_request(req);
> > > > > +out:
> > > > > +     dout("do_getvxattr result=%d\n", err);
> > > > > +     return err;
> > > > > +}
> > > > > +
> > > > > 
> > > > >   /*
> > > > >    * Check inode permissions.  We verify we have a valid value for
> > > > > diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> > > > > index c30eefc0ac19..a5eafc71d976 100644
> > > > > --- a/fs/ceph/mds_client.c
> > > > > +++ b/fs/ceph/mds_client.c
> > > > > @@ -555,6 +555,29 @@ static int parse_reply_info_create(void **p,
> > > > > void *end,
> > > > >        return -EIO;
> > > > >   }
> > > > > 
> > > > > +static int parse_reply_info_getvxattr(void **p, void *end,
> > > > > +                                   struct
> > > > > ceph_mds_reply_info_parsed *info,
> > > > > +                                   u64 features)
> > > > > +{
> > > > > +     u8 struct_v, struct_compat;
> > > > > +     u32 struct_len;
> > > > > +     u32 value_len;
> > > > > +
> > > > > +     ceph_decode_8_safe(p, end, struct_v, bad);
> > > > > +     ceph_decode_8_safe(p, end, struct_compat, bad);
> > > > > +     ceph_decode_32_safe(p, end, struct_len, bad);
> > > > > +     ceph_decode_32_safe(p, end, value_len, bad);
> > > > > +
> > > > > +     if (value_len == end - *p) {
> > > > > +       info->xattr_info.xattr_value = *p;
> > > > > +       info->xattr_info.xattr_value_len = end - *p;
> > > > > +       *p = end;
> > > > > +       return info->xattr_info.xattr_value_len;
> > > > > +     }
> > > > 
> > > > The kernel uses tab indent almost exclusively. Can you fix the
> > > > indention
> > > > above (and anywhere else I missed)?
> > > > 
> > > 
> > > 
> > > I'll fix this. newbie issues :P
> > > 
> > > > 
> > > > > +bad:
> > > > > +     return -EIO;
> > > > > +}
> > > > > +
> > > > >   /*
> > > > >    * parse extra results
> > > > >    */
> > > > > @@ -570,6 +593,8 @@ static int parse_reply_info_extra(void **p,
> > > > > void *end,
> > > > >                return parse_reply_info_readdir(p, end, info,
> > > > > features);
> > > > >        else if (op == CEPH_MDS_OP_CREATE)
> > > > >                return parse_reply_info_create(p, end, info,
> > > > > features, s);
> > > > > +     else if (op == CEPH_MDS_OP_GETVXATTR)
> > > > > +             return parse_reply_info_getvxattr(p, end, info,
> > > > > features);
> > > > >        else
> > > > >                return -EIO;
> > > > >   }
> > > > > @@ -615,7 +640,7 @@ static int parse_reply_info(struct
> > > > > ceph_mds_session *s, struct ceph_msg *msg,
> > > > > 
> > > > >        if (p != end)
> > > > >                goto bad;
> > > > > -     return 0;
> > > > > +     return err;
> > > > > 
> > > > >   bad:
> > > > >        err = -EIO;
> > > > > diff --git a/fs/ceph/mds_client.h b/fs/ceph/mds_client.h
> > > > > index 97c7f7bfa55f..f2a8e5af3c2e 100644
> > > > > --- a/fs/ceph/mds_client.h
> > > > > +++ b/fs/ceph/mds_client.h
> > > > > @@ -29,8 +29,10 @@ enum ceph_feature_type {
> > > > >        CEPHFS_FEATURE_MULTI_RECONNECT,
> > > > >        CEPHFS_FEATURE_DELEG_INO,
> > > > >        CEPHFS_FEATURE_METRIC_COLLECT,
> > > > > +     CEPHFS_FEATURE_ALTERNATE_NAME,
> > > > > +     CEPHFS_FEATURE_GETVXATTR,
> > > > > 
> > > > > -     CEPHFS_FEATURE_MAX = CEPHFS_FEATURE_METRIC_COLLECT,
> > > > > +     CEPHFS_FEATURE_MAX = CEPHFS_FEATURE_GETVXATTR,
> > > > >   };
> > > > > 
> > > > >   /*
> > > > > @@ -45,6 +47,8 @@ enum ceph_feature_type {
> > > > >        CEPHFS_FEATURE_MULTI_RECONNECT,         \
> > > > >        CEPHFS_FEATURE_DELEG_INO,               \
> > > > >        CEPHFS_FEATURE_METRIC_COLLECT,          \
> > > > > +     CEPHFS_FEATURE_ALTERNATE_NAME,          \
> > > > > +     CEPHFS_FEATURE_GETVXATTR,               \
> > > > >                                                \
> > > > >        CEPHFS_FEATURE_MAX,                     \
> > > > >   }
> > > > > @@ -100,6 +104,11 @@ struct ceph_mds_reply_dir_entry {
> > > > >        loff_t                        offset;
> > > > >   };
> > > > > 
> > > > > +struct ceph_mds_reply_xattr {
> > > > > +     char *xattr_value;
> > > > > +     size_t xattr_value_len;
> > > > > +};
> > > > > +
> > > > >   /*
> > > > >    * parsed info about an mds reply, including information about
> > > > >    * either: 1) the target inode and/or its parent directory and
> > > > > dentry,
> > > > > @@ -115,6 +124,7 @@ struct ceph_mds_reply_info_parsed {
> > > > >        char                          *dname;
> > > > >        u32                           dname_len;
> > > > >        struct ceph_mds_reply_lease   *dlease;
> > > > > +     struct ceph_mds_reply_xattr   xattr_info;
> > > > > 
> > > > >        /* extra */
> > > > >        union {
> > > > > diff --git a/fs/ceph/strings.c b/fs/ceph/strings.c
> > > > > index 573bb9556fb5..e36e8948e728 100644
> > > > > --- a/fs/ceph/strings.c
> > > > > +++ b/fs/ceph/strings.c
> > > > > @@ -60,6 +60,7 @@ const char *ceph_mds_op_name(int op)
> > > > >        case CEPH_MDS_OP_LOOKUPINO:  return "lookupino";
> > > > >        case CEPH_MDS_OP_LOOKUPNAME:  return "lookupname";
> > > > >        case CEPH_MDS_OP_GETATTR:  return "getattr";
> > > > > +     case CEPH_MDS_OP_GETVXATTR:  return "getvxattr";
> > > > >        case CEPH_MDS_OP_SETXATTR: return "setxattr";
> > > > >        case CEPH_MDS_OP_SETATTR: return "setattr";
> > > > >        case CEPH_MDS_OP_RMXATTR: return "rmxattr";
> > > > > diff --git a/fs/ceph/super.h b/fs/ceph/super.h
> > > > > index ac331aa07cfa..a627fa69668e 100644
> > > > > --- a/fs/ceph/super.h
> > > > > +++ b/fs/ceph/super.h
> > > > > @@ -1043,6 +1043,7 @@ static inline bool
> > > > > ceph_inode_is_shutdown(struct inode *inode)
> > > > > 
> > > > >   /* xattr.c */
> > > > >   int __ceph_setxattr(struct inode *, const char *, const void *,
> > > > > size_t, int);
> > > > > +int ceph_do_getvxattr(struct inode *inode, const char *name, void
> > > > > *value, size_t size);
> > > > >   ssize_t __ceph_getxattr(struct inode *, const char *, void *,
> > > > > size_t);
> > > > >   extern ssize_t ceph_listxattr(struct dentry *, char *, size_t);
> > > > >   extern struct ceph_buffer *__ceph_build_xattrs_blob(struct
> > > > > ceph_inode_info *ci);
> > > > > diff --git a/fs/ceph/xattr.c b/fs/ceph/xattr.c
> > > > > index fcf7dfdecf96..fdde8ffa71bb 100644
> > > > > --- a/fs/ceph/xattr.c
> > > > > +++ b/fs/ceph/xattr.c
> > > > > @@ -918,6 +918,64 @@ static inline int __get_request_mask(struct
> > > > > inode *in) {
> > > > >        return mask;
> > > > >   }
> > > > > 
> > > > > +static inline bool ceph_is_passthrough_vxattr(const char *name)
> > > > > +{
> > > > > +     const char *vxattr_list[] = {
> > > > > +             "ceph.dir.pin",
> > > > > +             "ceph.dir.pin.random",
> > > > > +             "ceph.dir.pin.distributed",
> > > > > +             "ceph.dir.layout",
> > > > > +             "ceph.dir.layout.object_size",
> > > > > +             "ceph.dir.layout.stripe_count",
> > > > > +             "ceph.dir.layout.stripe_unit",
> > > > > +             "ceph.dir.layout.pool",
> > > > > +             "ceph.dir.layout.pool_name",
> > > > > +             "ceph.dir.layout.pool_id",
> > > > > +             "ceph.dir.layout.pool_namespace",
> > > > > +             "ceph.file.layout",
> > > > > +             "ceph.file.layout.object_size",
> > > > > +             "ceph.file.layout.stripe_count",
> > > > > +             "ceph.file.layout.stripe_unit",
> > > > > +             "ceph.file.layout.pool",
> > > > > +             "ceph.file.layout.pool_name",
> > > > > +             "ceph.file.layout.pool_id",
> > > > > +             "ceph.file.layout.pool_namespace",
> > > > > +     };
> > > > > +     int i = 0;
> > > > > +     int n = sizeof(vxattr_list)/sizeof(vxattr_list[0]);
> > > > > +
> > > > > +     while (i < n) {
> > > > > +             if (!strcmp(name, vxattr_list[i]))
> > > > > +                     return true;
> > > > > +             i++;
> > > > > +     }
> > > > > +     return false;
> > > > > +}
> > > > 
> > > > This part, I'm not so crazy about. This list will be hard to keep in
> > > > sync with the userland code and you're almost guaranteed to have a
> > > > mismatch between what the kernel and userland supports since they're
> > > > on
> > > > different release schedules.
> > > > 
> > > > I think what we probably ought to do is run the ceph_match_vxattr
> > > > call
> > > > first, and if it doesn't match the name and the name starts with
> > > > "ceph.", then we'll send a GETVXATTR call to the MDS.
> > > > 
> > > > Sound ok?
> > > > 
> > > 
> > > 
> > > I was trying to preempt a getvxattr call if the full xattr name was
> > > invalid.
> > 
> > Huh, why? That seems like the point of the whole bug. We need a way to
> > hand vxattrs that the client doesn't recognize off to the MDS so it can
> > satisfy the request.
> 
> File and dir layout vxattrs are server-side xattrs. So, it seemed prudent to
> delegate them to the server. The idea was also to help identify layouts and
> their inheritance for a dir hierarchy or a file at the server side and possibly
> avoiding client round trips to the server for learning about the same (and
> maintain consistency?)
> 

I don't see how this solution results in fewer calls to the MDS, and I
don't agree that it's prudent to call up the MDS to satisfy a request
for info that we already have.

> Here's some discussion about the same:
> https://tracker.ceph.com/issues/51062#note-3
> 

I opened the bug and have been following it. I don't see any discussion
about delegating getxattr for a bunch of existing vxattrs to the MDS.
The bug is that you can set some vxattrs without being able to query
them.

> > 
> > > I could just test for the prefixes: ceph.dir.pin, ceph.dir.layout and
> > > ceph.file.layout and send them over. Eventually, we'll get an ENODATA
> > > if xattr name validation fails on the MDS side.
> > > 
> > > The problem with doing a ceph_match_vxattr() first is that it will
> > > succeed
> > > for any of these vxattrs, but we'll land in the same place as the one
> > > you've
> > > described in the tracker.
> > > The very idea of implementing getvxattr is to bypass the existing
> > > "cached"
> > > service mechanism for special vxattrs.
> > > 
> > 
> > I don't get it. What's the benefit of calling out to the MDS to ask for
> > ceph.file.layout.pool instead of just satisfying it from the info we
> > have?
> > 
> > I'd prefer we just add a fallback for "ceph.*" vxattrs that for which we
> > don't have a local handler.
> 
> See my explanation above.
> 

I see your explanation, but it doesn't outline the benefit of sending
these requests to the MDS instead of just satisfying them locally. What
good does that do?

I also think you're missing another point: someone will need to keep
this vxattr whitelist in sync, and it will be a maintenance headache.
It'll also result in a worse experience for users.

Suppose we add some new ceph.dir.foo vxattr on the MDS and backport that
to a point release. That attribute will not be accessible until someone
also adds it to the whitelist in the client.

At that point, it may be *years* before users can access that vxattr.
They'll not only have to wait for the MDS to get the patch that adds the
new vxattr, but also for a client that has it in the whitelist.

If however, you do this the way I'm suggesting, once the MDS supports
the new vxattr, it'll be usable, and we won't have to continually patch
the kernel to support new attributes.
-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH 1/1] ceph: add getvxattr op

[Milind Changire]

On Wed, Jan 12, 2022 at 6:25 PM Jeff Layton <jlayton@kernel.org> wrote:
>
> On Wed, 2022-01-12 at 18:00 +0530, Milind Changire wrote:
> > On Tue, Jan 11, 2022 at 7:18 PM Jeff Layton <jlayton@kernel.org> wrote:
> > >
> > > On Tue, 2022-01-11 at 18:53 +0530, Milind Changire wrote:
> > > > responses below ...
> > > >
> > > > On Tue, Jan 11, 2022 at 6:19 PM Jeff Layton <jlayton@kernel.org>
> > > > wrote:
> > > > > On Tue, 2022-01-11 at 17:54 +0530, Milind Changire wrote:
> > > > > > Problem:
> > > > > > Directory vxattrs like ceph.dir.pin* and ceph.dir.layout* may not
> > > > > > be
> > > > > > propagated to the client as frequently to keep them updated. This
> > > > > > creates vxattr availability problems.
> > > > > >
> > > > > > Solution:
> > > > > > Adds new getvxattr op to fetch ceph.dir.pin*, ceph.dir.layout* and
> > > > > > ceph.file.layout* vxattrs.
> > > > > > If the entire layout for a dir or a file is being set, then it is
> > > > > > expected that the layout be set in standard JSON format.
> > > > > > Individual
> > > > > > field value retrieval is not wrapped in JSON. The JSON format also
> > > > > > applies while setting the vxattr if the entire layout is being set
> > > > > > in
> > > > > > one go.
> > > > > > As a temporary measure, setting a vxattr can also be done in the
> > > > > > old
> > > > > > format. The old format will be deprecated in the future.
> > > > > >
> > > > > > URL: https://tracker.ceph.com/issues/51062
> > > > > > Signed-off-by: Milind Changire <mchangir@redhat.com>
> > > > > > ---
> > > > > >   fs/ceph/inode.c              | 51 ++++++++++++++++++++++++++++
> > > > > >   fs/ceph/mds_client.c         | 27 ++++++++++++++-
> > > > > >   fs/ceph/mds_client.h         | 12 ++++++-
> > > > > >   fs/ceph/strings.c            |  1 +
> > > > > >   fs/ceph/super.h              |  1 +
> > > > > >   fs/ceph/xattr.c              | 65
> > > > > > ++++++++++++++++++++++++++++++++++++
> > > > > >   include/linux/ceph/ceph_fs.h |  1 +
> > > > > >   7 files changed, 156 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
> > > > > > index e3322fcb2e8d..f29d59b63c52 100644
> > > > > > --- a/fs/ceph/inode.c
> > > > > > +++ b/fs/ceph/inode.c
> > > > > > @@ -2291,6 +2291,57 @@ int __ceph_do_getattr(struct inode *inode,
> > > > > > struct page *locked_page,
> > > > > >        return err;
> > > > > >   }
> > > > > >
> > > > > > +int ceph_do_getvxattr(struct inode *inode, const char *name, void
> > > > > > *value,
> > > > > > +                   size_t size)
> > > > > > +{
> > > > > > +     struct ceph_fs_client *fsc = ceph_sb_to_client(inode->i_sb);
> > > > > > +     struct ceph_mds_client *mdsc = fsc->mdsc;
> > > > > > +     struct ceph_mds_request *req;
> > > > > > +     int mode = USE_AUTH_MDS;
> > > > > > +     int err;
> > > > > > +     char *xattr_value;
> > > > > > +     size_t xattr_value_len;
> > > > > > +
> > > > > > +     req = ceph_mdsc_create_request(mdsc, CEPH_MDS_OP_GETVXATTR,
> > > > > > mode);
> > > > > > +     if (IS_ERR(req)) {
> > > > > > +             err = -ENOMEM;
> > > > > > +             goto out;
> > > > > > +     }
> > > > > > +
> > > > > > +     req->r_path2 = kstrdup(name, GFP_NOFS);
> > > > > > +     if (!req->r_path2) {
> > > > > > +             err = -ENOMEM;
> > > > > > +             goto put;
> > > > > > +     }
> > > > > > +
> > > > > > +     ihold(inode);
> > > > > > +     req->r_inode = inode;
> > > > > > +     err = ceph_mdsc_do_request(mdsc, NULL, req);
> > > > > > +     if (err < 0)
> > > > > > +             goto put;
> > > > > > +
> > > > > > +     xattr_value = req->r_reply_info.xattr_info.xattr_value;
> > > > > > +     xattr_value_len = req-
> > > > > > > r_reply_info.xattr_info.xattr_value_len;
> > > > > > +
> > > > > > +     dout("do_getvxattr xattr_value_len:%lu, size:%lu\n",
> > > > > > xattr_value_len, size);
> > > > > > +
> > > > > > +     err = xattr_value_len;
> > > > > > +     if (size == 0)
> > > > > > +             goto put;
> > > > > > +
> > > > > > +     if (xattr_value_len > size) {
> > > > > > +             err = -ERANGE;
> > > > > > +             goto put;
> > > > > > +     }
> > > > > > +
> > > > > > +     memcpy(value, xattr_value, xattr_value_len);
> > > > > > +put:
> > > > > > +     ceph_mdsc_put_request(req);
> > > > > > +out:
> > > > > > +     dout("do_getvxattr result=%d\n", err);
> > > > > > +     return err;
> > > > > > +}
> > > > > > +
> > > > > >
> > > > > >   /*
> > > > > >    * Check inode permissions.  We verify we have a valid value for
> > > > > > diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> > > > > > index c30eefc0ac19..a5eafc71d976 100644
> > > > > > --- a/fs/ceph/mds_client.c
> > > > > > +++ b/fs/ceph/mds_client.c
> > > > > > @@ -555,6 +555,29 @@ static int parse_reply_info_create(void **p,
> > > > > > void *end,
> > > > > >        return -EIO;
> > > > > >   }
> > > > > >
> > > > > > +static int parse_reply_info_getvxattr(void **p, void *end,
> > > > > > +                                   struct
> > > > > > ceph_mds_reply_info_parsed *info,
> > > > > > +                                   u64 features)
> > > > > > +{
> > > > > > +     u8 struct_v, struct_compat;
> > > > > > +     u32 struct_len;
> > > > > > +     u32 value_len;
> > > > > > +
> > > > > > +     ceph_decode_8_safe(p, end, struct_v, bad);
> > > > > > +     ceph_decode_8_safe(p, end, struct_compat, bad);
> > > > > > +     ceph_decode_32_safe(p, end, struct_len, bad);
> > > > > > +     ceph_decode_32_safe(p, end, value_len, bad);
> > > > > > +
> > > > > > +     if (value_len == end - *p) {
> > > > > > +       info->xattr_info.xattr_value = *p;
> > > > > > +       info->xattr_info.xattr_value_len = end - *p;
> > > > > > +       *p = end;
> > > > > > +       return info->xattr_info.xattr_value_len;
> > > > > > +     }
> > > > >
> > > > > The kernel uses tab indent almost exclusively. Can you fix the
> > > > > indention
> > > > > above (and anywhere else I missed)?
> > > > >
> > > >
> > > >
> > > > I'll fix this. newbie issues :P
> > > >
> > > > >
> > > > > > +bad:
> > > > > > +     return -EIO;
> > > > > > +}
> > > > > > +
> > > > > >   /*
> > > > > >    * parse extra results
> > > > > >    */
> > > > > > @@ -570,6 +593,8 @@ static int parse_reply_info_extra(void **p,
> > > > > > void *end,
> > > > > >                return parse_reply_info_readdir(p, end, info,
> > > > > > features);
> > > > > >        else if (op == CEPH_MDS_OP_CREATE)
> > > > > >                return parse_reply_info_create(p, end, info,
> > > > > > features, s);
> > > > > > +     else if (op == CEPH_MDS_OP_GETVXATTR)
> > > > > > +             return parse_reply_info_getvxattr(p, end, info,
> > > > > > features);
> > > > > >        else
> > > > > >                return -EIO;
> > > > > >   }
> > > > > > @@ -615,7 +640,7 @@ static int parse_reply_info(struct
> > > > > > ceph_mds_session *s, struct ceph_msg *msg,
> > > > > >
> > > > > >        if (p != end)
> > > > > >                goto bad;
> > > > > > -     return 0;
> > > > > > +     return err;
> > > > > >
> > > > > >   bad:
> > > > > >        err = -EIO;
> > > > > > diff --git a/fs/ceph/mds_client.h b/fs/ceph/mds_client.h
> > > > > > index 97c7f7bfa55f..f2a8e5af3c2e 100644
> > > > > > --- a/fs/ceph/mds_client.h
> > > > > > +++ b/fs/ceph/mds_client.h
> > > > > > @@ -29,8 +29,10 @@ enum ceph_feature_type {
> > > > > >        CEPHFS_FEATURE_MULTI_RECONNECT,
> > > > > >        CEPHFS_FEATURE_DELEG_INO,
> > > > > >        CEPHFS_FEATURE_METRIC_COLLECT,
> > > > > > +     CEPHFS_FEATURE_ALTERNATE_NAME,
> > > > > > +     CEPHFS_FEATURE_GETVXATTR,
> > > > > >
> > > > > > -     CEPHFS_FEATURE_MAX = CEPHFS_FEATURE_METRIC_COLLECT,
> > > > > > +     CEPHFS_FEATURE_MAX = CEPHFS_FEATURE_GETVXATTR,
> > > > > >   };
> > > > > >
> > > > > >   /*
> > > > > > @@ -45,6 +47,8 @@ enum ceph_feature_type {
> > > > > >        CEPHFS_FEATURE_MULTI_RECONNECT,         \
> > > > > >        CEPHFS_FEATURE_DELEG_INO,               \
> > > > > >        CEPHFS_FEATURE_METRIC_COLLECT,          \
> > > > > > +     CEPHFS_FEATURE_ALTERNATE_NAME,          \
> > > > > > +     CEPHFS_FEATURE_GETVXATTR,               \
> > > > > >                                                \
> > > > > >        CEPHFS_FEATURE_MAX,                     \
> > > > > >   }
> > > > > > @@ -100,6 +104,11 @@ struct ceph_mds_reply_dir_entry {
> > > > > >        loff_t                        offset;
> > > > > >   };
> > > > > >
> > > > > > +struct ceph_mds_reply_xattr {
> > > > > > +     char *xattr_value;
> > > > > > +     size_t xattr_value_len;
> > > > > > +};
> > > > > > +
> > > > > >   /*
> > > > > >    * parsed info about an mds reply, including information about
> > > > > >    * either: 1) the target inode and/or its parent directory and
> > > > > > dentry,
> > > > > > @@ -115,6 +124,7 @@ struct ceph_mds_reply_info_parsed {
> > > > > >        char                          *dname;
> > > > > >        u32                           dname_len;
> > > > > >        struct ceph_mds_reply_lease   *dlease;
> > > > > > +     struct ceph_mds_reply_xattr   xattr_info;
> > > > > >
> > > > > >        /* extra */
> > > > > >        union {
> > > > > > diff --git a/fs/ceph/strings.c b/fs/ceph/strings.c
> > > > > > index 573bb9556fb5..e36e8948e728 100644
> > > > > > --- a/fs/ceph/strings.c
> > > > > > +++ b/fs/ceph/strings.c
> > > > > > @@ -60,6 +60,7 @@ const char *ceph_mds_op_name(int op)
> > > > > >        case CEPH_MDS_OP_LOOKUPINO:  return "lookupino";
> > > > > >        case CEPH_MDS_OP_LOOKUPNAME:  return "lookupname";
> > > > > >        case CEPH_MDS_OP_GETATTR:  return "getattr";
> > > > > > +     case CEPH_MDS_OP_GETVXATTR:  return "getvxattr";
> > > > > >        case CEPH_MDS_OP_SETXATTR: return "setxattr";
> > > > > >        case CEPH_MDS_OP_SETATTR: return "setattr";
> > > > > >        case CEPH_MDS_OP_RMXATTR: return "rmxattr";
> > > > > > diff --git a/fs/ceph/super.h b/fs/ceph/super.h
> > > > > > index ac331aa07cfa..a627fa69668e 100644
> > > > > > --- a/fs/ceph/super.h
> > > > > > +++ b/fs/ceph/super.h
> > > > > > @@ -1043,6 +1043,7 @@ static inline bool
> > > > > > ceph_inode_is_shutdown(struct inode *inode)
> > > > > >
> > > > > >   /* xattr.c */
> > > > > >   int __ceph_setxattr(struct inode *, const char *, const void *,
> > > > > > size_t, int);
> > > > > > +int ceph_do_getvxattr(struct inode *inode, const char *name, void
> > > > > > *value, size_t size);
> > > > > >   ssize_t __ceph_getxattr(struct inode *, const char *, void *,
> > > > > > size_t);
> > > > > >   extern ssize_t ceph_listxattr(struct dentry *, char *, size_t);
> > > > > >   extern struct ceph_buffer *__ceph_build_xattrs_blob(struct
> > > > > > ceph_inode_info *ci);
> > > > > > diff --git a/fs/ceph/xattr.c b/fs/ceph/xattr.c
> > > > > > index fcf7dfdecf96..fdde8ffa71bb 100644
> > > > > > --- a/fs/ceph/xattr.c
> > > > > > +++ b/fs/ceph/xattr.c
> > > > > > @@ -918,6 +918,64 @@ static inline int __get_request_mask(struct
> > > > > > inode *in) {
> > > > > >        return mask;
> > > > > >   }
> > > > > >
> > > > > > +static inline bool ceph_is_passthrough_vxattr(const char *name)
> > > > > > +{
> > > > > > +     const char *vxattr_list[] = {
> > > > > > +             "ceph.dir.pin",
> > > > > > +             "ceph.dir.pin.random",
> > > > > > +             "ceph.dir.pin.distributed",
> > > > > > +             "ceph.dir.layout",
> > > > > > +             "ceph.dir.layout.object_size",
> > > > > > +             "ceph.dir.layout.stripe_count",
> > > > > > +             "ceph.dir.layout.stripe_unit",
> > > > > > +             "ceph.dir.layout.pool",
> > > > > > +             "ceph.dir.layout.pool_name",
> > > > > > +             "ceph.dir.layout.pool_id",
> > > > > > +             "ceph.dir.layout.pool_namespace",
> > > > > > +             "ceph.file.layout",
> > > > > > +             "ceph.file.layout.object_size",
> > > > > > +             "ceph.file.layout.stripe_count",
> > > > > > +             "ceph.file.layout.stripe_unit",
> > > > > > +             "ceph.file.layout.pool",
> > > > > > +             "ceph.file.layout.pool_name",
> > > > > > +             "ceph.file.layout.pool_id",
> > > > > > +             "ceph.file.layout.pool_namespace",
> > > > > > +     };
> > > > > > +     int i = 0;
> > > > > > +     int n = sizeof(vxattr_list)/sizeof(vxattr_list[0]);
> > > > > > +
> > > > > > +     while (i < n) {
> > > > > > +             if (!strcmp(name, vxattr_list[i]))
> > > > > > +                     return true;
> > > > > > +             i++;
> > > > > > +     }
> > > > > > +     return false;
> > > > > > +}
> > > > >
> > > > > This part, I'm not so crazy about. This list will be hard to keep in
> > > > > sync with the userland code and you're almost guaranteed to have a
> > > > > mismatch between what the kernel and userland supports since they're
> > > > > on
> > > > > different release schedules.
> > > > >
> > > > > I think what we probably ought to do is run the ceph_match_vxattr
> > > > > call
> > > > > first, and if it doesn't match the name and the name starts with
> > > > > "ceph.", then we'll send a GETVXATTR call to the MDS.
> > > > >
> > > > > Sound ok?
> > > > >
> > > >
> > > >
> > > > I was trying to preempt a getvxattr call if the full xattr name was
> > > > invalid.
> > >
> > > Huh, why? That seems like the point of the whole bug. We need a way to
> > > hand vxattrs that the client doesn't recognize off to the MDS so it can
> > > satisfy the request.
> >
> > File and dir layout vxattrs are server-side xattrs. So, it seemed prudent to
> > delegate them to the server. The idea was also to help identify layouts and
> > their inheritance for a dir hierarchy or a file at the server side and possibly
> > avoiding client round trips to the server for learning about the same (and
> > maintain consistency?)
> >
>
> I don't see how this solution results in fewer calls to the MDS, and I
> don't agree that it's prudent to call up the MDS to satisfy a request
> for info that we already have.
>
> > Here's some discussion about the same:
> > https://tracker.ceph.com/issues/51062#note-3
> >
>
> I opened the bug and have been following it. I don't see any discussion
> about delegating getxattr for a bunch of existing vxattrs to the MDS.
> The bug is that you can set some vxattrs without being able to query
> them.
>
> > >
> > > > I could just test for the prefixes: ceph.dir.pin, ceph.dir.layout and
> > > > ceph.file.layout and send them over. Eventually, we'll get an ENODATA
> > > > if xattr name validation fails on the MDS side.
> > > >
> > > > The problem with doing a ceph_match_vxattr() first is that it will
> > > > succeed
> > > > for any of these vxattrs, but we'll land in the same place as the one
> > > > you've
> > > > described in the tracker.
> > > > The very idea of implementing getvxattr is to bypass the existing
> > > > "cached"
> > > > service mechanism for special vxattrs.
> > > >
> > >
> > > I don't get it. What's the benefit of calling out to the MDS to ask for
> > > ceph.file.layout.pool instead of just satisfying it from the info we
> > > have?
> > >
> > > I'd prefer we just add a fallback for "ceph.*" vxattrs that for which we
> > > don't have a local handler.
> >
> > See my explanation above.
> >
>
> I see your explanation, but it doesn't outline the benefit of sending
> these requests to the MDS instead of just satisfying them locally. What
> good does that do?
>
> I also think you're missing another point: someone will need to keep
> this vxattr whitelist in sync, and it will be a maintenance headache.
> It'll also result in a worse experience for users.
>
> Suppose we add some new ceph.dir.foo vxattr on the MDS and backport that
> to a point release. That attribute will not be accessible until someone
> also adds it to the whitelist in the client.
>
> At that point, it may be *years* before users can access that vxattr.
> They'll not only have to wait for the MDS to get the patch that adds the
> new vxattr, but also for a client that has it in the whitelist.
>
> If however, you do this the way I'm suggesting, once the MDS supports
> the new vxattr, it'll be usable, and we won't have to continually patch
> the kernel to support new attributes.
> --
> Jeff Layton <jlayton@kernel.org>

hmm ... well, then I'll just have to remove layout and pin vxattrs from the
list of vxattrs being supported by the client instead of maintaining the
whitelist for getvxattr.
In the code, if the vxatrr is not found in the client supported list, the code
will drop down to the else case with getvxattr support.
I think I finally got around to what you were trying to say.

Re: [PATCH 1/1] ceph: add getvxattr op

[kernel test robot]

Hi Milind,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on ceph-client/for-linus]
[also build test WARNING on v5.16 next-20220112]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Milind-Changire/ceph-add-getvxattr-support/20220111-202533
base:   https://github.com/ceph/ceph-client.git for-linus
config: i386-allyesconfig (https://download.01.org/0day-ci/archive/20220112/202201122237.Lspc8TH5-lkp@intel.com/config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
        # https://github.com/0day-ci/linux/commit/9e670d02ce9f9d6e1ac3e234a89d305c85302338
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Milind-Changire/ceph-add-getvxattr-support/20220111-202533
        git checkout 9e670d02ce9f9d6e1ac3e234a89d305c85302338
        # save the config file to linux build tree
        mkdir build_dir
        make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/gpu/drm/vmwgfx/ drivers/iio/ fs/ceph/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   fs/ceph/inode.c: In function 'ceph_do_getvxattr':
>> <command-line>: warning: format '%lu' expects argument of type 'long unsigned int', but argument 7 has type 'size_t' {aka 'unsigned int'} [-Wformat=]
   include/linux/ceph/ceph_debug.h:5:21: note: in expansion of macro 'KBUILD_MODNAME'
       5 | #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
         |                     ^~~~~~~~~~~~~~
   include/linux/dynamic_debug.h:134:15: note: in expansion of macro 'pr_fmt'
     134 |   func(&id, ##__VA_ARGS__);  \
         |               ^~~~~~~~~~~
   include/linux/dynamic_debug.h:152:2: note: in expansion of macro '__dynamic_func_call'
     152 |  __dynamic_func_call(__UNIQUE_ID(ddebug), fmt, func, ##__VA_ARGS__)
         |  ^~~~~~~~~~~~~~~~~~~
   include/linux/dynamic_debug.h:162:2: note: in expansion of macro '_dynamic_func_call'
     162 |  _dynamic_func_call(fmt, __dynamic_pr_debug,  \
         |  ^~~~~~~~~~~~~~~~~~
   include/linux/printk.h:574:2: note: in expansion of macro 'dynamic_pr_debug'
     574 |  dynamic_pr_debug(fmt, ##__VA_ARGS__)
         |  ^~~~~~~~~~~~~~~~
   include/linux/ceph/ceph_debug.h:19:2: note: in expansion of macro 'pr_debug'
      19 |  pr_debug("%.*s %12.12s:%-4d : " fmt,    \
         |  ^~~~~~~~
   fs/ceph/inode.c:2326:2: note: in expansion of macro 'dout'
    2326 |  dout("do_getvxattr xattr_value_len:%lu, size:%lu\n", xattr_value_len, size);
         |  ^~~~
   <command-line>: warning: format '%lu' expects argument of type 'long unsigned int', but argument 8 has type 'size_t' {aka 'unsigned int'} [-Wformat=]
   include/linux/ceph/ceph_debug.h:5:21: note: in expansion of macro 'KBUILD_MODNAME'
       5 | #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
         |                     ^~~~~~~~~~~~~~
   include/linux/dynamic_debug.h:134:15: note: in expansion of macro 'pr_fmt'
     134 |   func(&id, ##__VA_ARGS__);  \
         |               ^~~~~~~~~~~
   include/linux/dynamic_debug.h:152:2: note: in expansion of macro '__dynamic_func_call'
     152 |  __dynamic_func_call(__UNIQUE_ID(ddebug), fmt, func, ##__VA_ARGS__)
         |  ^~~~~~~~~~~~~~~~~~~
   include/linux/dynamic_debug.h:162:2: note: in expansion of macro '_dynamic_func_call'
     162 |  _dynamic_func_call(fmt, __dynamic_pr_debug,  \
         |  ^~~~~~~~~~~~~~~~~~
   include/linux/printk.h:574:2: note: in expansion of macro 'dynamic_pr_debug'
     574 |  dynamic_pr_debug(fmt, ##__VA_ARGS__)
         |  ^~~~~~~~~~~~~~~~
   include/linux/ceph/ceph_debug.h:19:2: note: in expansion of macro 'pr_debug'
      19 |  pr_debug("%.*s %12.12s:%-4d : " fmt,    \
         |  ^~~~~~~~
   fs/ceph/inode.c:2326:2: note: in expansion of macro 'dout'
    2326 |  dout("do_getvxattr xattr_value_len:%lu, size:%lu\n", xattr_value_len, size);
         |  ^~~~

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

Re: [PATCH 1/1] ceph: add getvxattr op

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 4326 bytes --]

Hi Milind,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on ceph-client/for-linus]
[also build test WARNING on v5.16 next-20220112]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Milind-Changire/ceph-add-getvxattr-support/20220111-202533
base:   https://github.com/ceph/ceph-client.git for-linus
config: i386-allyesconfig (https://download.01.org/0day-ci/archive/20220112/202201122237.Lspc8TH5-lkp(a)intel.com/config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
        # https://github.com/0day-ci/linux/commit/9e670d02ce9f9d6e1ac3e234a89d305c85302338
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Milind-Changire/ceph-add-getvxattr-support/20220111-202533
        git checkout 9e670d02ce9f9d6e1ac3e234a89d305c85302338
        # save the config file to linux build tree
        mkdir build_dir
        make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/gpu/drm/vmwgfx/ drivers/iio/ fs/ceph/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   fs/ceph/inode.c: In function 'ceph_do_getvxattr':
>> <command-line>: warning: format '%lu' expects argument of type 'long unsigned int', but argument 7 has type 'size_t' {aka 'unsigned int'} [-Wformat=]
   include/linux/ceph/ceph_debug.h:5:21: note: in expansion of macro 'KBUILD_MODNAME'
       5 | #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
         |                     ^~~~~~~~~~~~~~
   include/linux/dynamic_debug.h:134:15: note: in expansion of macro 'pr_fmt'
     134 |   func(&id, ##__VA_ARGS__);  \
         |               ^~~~~~~~~~~
   include/linux/dynamic_debug.h:152:2: note: in expansion of macro '__dynamic_func_call'
     152 |  __dynamic_func_call(__UNIQUE_ID(ddebug), fmt, func, ##__VA_ARGS__)
         |  ^~~~~~~~~~~~~~~~~~~
   include/linux/dynamic_debug.h:162:2: note: in expansion of macro '_dynamic_func_call'
     162 |  _dynamic_func_call(fmt, __dynamic_pr_debug,  \
         |  ^~~~~~~~~~~~~~~~~~
   include/linux/printk.h:574:2: note: in expansion of macro 'dynamic_pr_debug'
     574 |  dynamic_pr_debug(fmt, ##__VA_ARGS__)
         |  ^~~~~~~~~~~~~~~~
   include/linux/ceph/ceph_debug.h:19:2: note: in expansion of macro 'pr_debug'
      19 |  pr_debug("%.*s %12.12s:%-4d : " fmt,    \
         |  ^~~~~~~~
   fs/ceph/inode.c:2326:2: note: in expansion of macro 'dout'
    2326 |  dout("do_getvxattr xattr_value_len:%lu, size:%lu\n", xattr_value_len, size);
         |  ^~~~
   <command-line>: warning: format '%lu' expects argument of type 'long unsigned int', but argument 8 has type 'size_t' {aka 'unsigned int'} [-Wformat=]
   include/linux/ceph/ceph_debug.h:5:21: note: in expansion of macro 'KBUILD_MODNAME'
       5 | #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
         |                     ^~~~~~~~~~~~~~
   include/linux/dynamic_debug.h:134:15: note: in expansion of macro 'pr_fmt'
     134 |   func(&id, ##__VA_ARGS__);  \
         |               ^~~~~~~~~~~
   include/linux/dynamic_debug.h:152:2: note: in expansion of macro '__dynamic_func_call'
     152 |  __dynamic_func_call(__UNIQUE_ID(ddebug), fmt, func, ##__VA_ARGS__)
         |  ^~~~~~~~~~~~~~~~~~~
   include/linux/dynamic_debug.h:162:2: note: in expansion of macro '_dynamic_func_call'
     162 |  _dynamic_func_call(fmt, __dynamic_pr_debug,  \
         |  ^~~~~~~~~~~~~~~~~~
   include/linux/printk.h:574:2: note: in expansion of macro 'dynamic_pr_debug'
     574 |  dynamic_pr_debug(fmt, ##__VA_ARGS__)
         |  ^~~~~~~~~~~~~~~~
   include/linux/ceph/ceph_debug.h:19:2: note: in expansion of macro 'pr_debug'
      19 |  pr_debug("%.*s %12.12s:%-4d : " fmt,    \
         |  ^~~~~~~~
   fs/ceph/inode.c:2326:2: note: in expansion of macro 'dout'
    2326 |  dout("do_getvxattr xattr_value_len:%lu, size:%lu\n", xattr_value_len, size);
         |  ^~~~

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

Re: [PATCH 1/1] ceph: add getvxattr op

[kernel test robot]

Hi Milind,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on ceph-client/for-linus]
[also build test WARNING on v5.16 next-20220112]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Milind-Changire/ceph-add-getvxattr-support/20220111-202533
base:   https://github.com/ceph/ceph-client.git for-linus
config: i386-randconfig-a006 (https://download.01.org/0day-ci/archive/20220113/202201130325.UBjYGkoh-lkp@intel.com/config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 244dd2913a43a200f5a6544d424cdc37b771028b)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/9e670d02ce9f9d6e1ac3e234a89d305c85302338
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Milind-Changire/ceph-add-getvxattr-support/20220111-202533
        git checkout 9e670d02ce9f9d6e1ac3e234a89d305c85302338
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash fs/ceph/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> fs/ceph/inode.c:2326:55: warning: format specifies type 'unsigned long' but the argument has type 'size_t' (aka 'unsigned int') [-Wformat]
           dout("do_getvxattr xattr_value_len:%lu, size:%lu\n", xattr_value_len, size);
                                              ~~~               ^~~~~~~~~~~~~~~
                                              %u
   include/linux/ceph/ceph_debug.h:35:45: note: expanded from macro 'dout'
   # define dout(fmt, ...) pr_debug(" " fmt, ##__VA_ARGS__)
                                        ~~~    ^~~~~~~~~~~
   include/linux/printk.h:580:38: note: expanded from macro 'pr_debug'
           no_printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
                                       ~~~     ^~~~~~~~~~~
   include/linux/printk.h:132:17: note: expanded from macro 'no_printk'
                   printk(fmt, ##__VA_ARGS__);             \
                          ~~~    ^~~~~~~~~~~
   include/linux/printk.h:450:60: note: expanded from macro 'printk'
   #define printk(fmt, ...) printk_index_wrap(_printk, fmt, ##__VA_ARGS__)
                                                       ~~~    ^~~~~~~~~~~
   include/linux/printk.h:422:19: note: expanded from macro 'printk_index_wrap'
                   _p_func(_fmt, ##__VA_ARGS__);                           \
                           ~~~~    ^~~~~~~~~~~
   fs/ceph/inode.c:2326:72: warning: format specifies type 'unsigned long' but the argument has type 'size_t' (aka 'unsigned int') [-Wformat]
           dout("do_getvxattr xattr_value_len:%lu, size:%lu\n", xattr_value_len, size);
                                                        ~~~                      ^~~~
                                                        %u
   include/linux/ceph/ceph_debug.h:35:45: note: expanded from macro 'dout'
   # define dout(fmt, ...) pr_debug(" " fmt, ##__VA_ARGS__)
                                        ~~~    ^~~~~~~~~~~
   include/linux/printk.h:580:38: note: expanded from macro 'pr_debug'
           no_printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
                                       ~~~     ^~~~~~~~~~~
   include/linux/printk.h:132:17: note: expanded from macro 'no_printk'
                   printk(fmt, ##__VA_ARGS__);             \
                          ~~~    ^~~~~~~~~~~
   include/linux/printk.h:450:60: note: expanded from macro 'printk'
   #define printk(fmt, ...) printk_index_wrap(_printk, fmt, ##__VA_ARGS__)
                                                       ~~~    ^~~~~~~~~~~
   include/linux/printk.h:422:19: note: expanded from macro 'printk_index_wrap'
                   _p_func(_fmt, ##__VA_ARGS__);                           \
                           ~~~~    ^~~~~~~~~~~
   2 warnings generated.


vim +2326 fs/ceph/inode.c

  2293	
  2294	int ceph_do_getvxattr(struct inode *inode, const char *name, void *value,
  2295			      size_t size)
  2296	{
  2297		struct ceph_fs_client *fsc = ceph_sb_to_client(inode->i_sb);
  2298		struct ceph_mds_client *mdsc = fsc->mdsc;
  2299		struct ceph_mds_request *req;
  2300		int mode = USE_AUTH_MDS;
  2301		int err;
  2302		char *xattr_value;
  2303		size_t xattr_value_len;
  2304	
  2305		req = ceph_mdsc_create_request(mdsc, CEPH_MDS_OP_GETVXATTR, mode);
  2306		if (IS_ERR(req)) {
  2307			err = -ENOMEM;
  2308			goto out;
  2309		}
  2310	
  2311		req->r_path2 = kstrdup(name, GFP_NOFS);
  2312		if (!req->r_path2) {
  2313			err = -ENOMEM;
  2314			goto put;
  2315		}
  2316	
  2317		ihold(inode);
  2318		req->r_inode = inode;
  2319		err = ceph_mdsc_do_request(mdsc, NULL, req);
  2320		if (err < 0)
  2321			goto put;
  2322	
  2323		xattr_value = req->r_reply_info.xattr_info.xattr_value;
  2324		xattr_value_len = req->r_reply_info.xattr_info.xattr_value_len;
  2325	
> 2326		dout("do_getvxattr xattr_value_len:%lu, size:%lu\n", xattr_value_len, size);
  2327	
  2328		err = xattr_value_len;
  2329		if (size == 0)
  2330			goto put;
  2331	
  2332		if (xattr_value_len > size) {
  2333			err = -ERANGE;
  2334			goto put;
  2335		}
  2336	
  2337		memcpy(value, xattr_value, xattr_value_len);
  2338	put:
  2339		ceph_mdsc_put_request(req);
  2340	out:
  2341		dout("do_getvxattr result=%d\n", err);
  2342		return err;
  2343	}
  2344	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

Re: [PATCH 1/1] ceph: add getvxattr op

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 6064 bytes --]

Hi Milind,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on ceph-client/for-linus]
[also build test WARNING on v5.16 next-20220112]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Milind-Changire/ceph-add-getvxattr-support/20220111-202533
base:   https://github.com/ceph/ceph-client.git for-linus
config: i386-randconfig-a006 (https://download.01.org/0day-ci/archive/20220113/202201130325.UBjYGkoh-lkp(a)intel.com/config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 244dd2913a43a200f5a6544d424cdc37b771028b)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/9e670d02ce9f9d6e1ac3e234a89d305c85302338
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Milind-Changire/ceph-add-getvxattr-support/20220111-202533
        git checkout 9e670d02ce9f9d6e1ac3e234a89d305c85302338
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash fs/ceph/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> fs/ceph/inode.c:2326:55: warning: format specifies type 'unsigned long' but the argument has type 'size_t' (aka 'unsigned int') [-Wformat]
           dout("do_getvxattr xattr_value_len:%lu, size:%lu\n", xattr_value_len, size);
                                              ~~~               ^~~~~~~~~~~~~~~
                                              %u
   include/linux/ceph/ceph_debug.h:35:45: note: expanded from macro 'dout'
   # define dout(fmt, ...) pr_debug(" " fmt, ##__VA_ARGS__)
                                        ~~~    ^~~~~~~~~~~
   include/linux/printk.h:580:38: note: expanded from macro 'pr_debug'
           no_printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
                                       ~~~     ^~~~~~~~~~~
   include/linux/printk.h:132:17: note: expanded from macro 'no_printk'
                   printk(fmt, ##__VA_ARGS__);             \
                          ~~~    ^~~~~~~~~~~
   include/linux/printk.h:450:60: note: expanded from macro 'printk'
   #define printk(fmt, ...) printk_index_wrap(_printk, fmt, ##__VA_ARGS__)
                                                       ~~~    ^~~~~~~~~~~
   include/linux/printk.h:422:19: note: expanded from macro 'printk_index_wrap'
                   _p_func(_fmt, ##__VA_ARGS__);                           \
                           ~~~~    ^~~~~~~~~~~
   fs/ceph/inode.c:2326:72: warning: format specifies type 'unsigned long' but the argument has type 'size_t' (aka 'unsigned int') [-Wformat]
           dout("do_getvxattr xattr_value_len:%lu, size:%lu\n", xattr_value_len, size);
                                                        ~~~                      ^~~~
                                                        %u
   include/linux/ceph/ceph_debug.h:35:45: note: expanded from macro 'dout'
   # define dout(fmt, ...) pr_debug(" " fmt, ##__VA_ARGS__)
                                        ~~~    ^~~~~~~~~~~
   include/linux/printk.h:580:38: note: expanded from macro 'pr_debug'
           no_printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
                                       ~~~     ^~~~~~~~~~~
   include/linux/printk.h:132:17: note: expanded from macro 'no_printk'
                   printk(fmt, ##__VA_ARGS__);             \
                          ~~~    ^~~~~~~~~~~
   include/linux/printk.h:450:60: note: expanded from macro 'printk'
   #define printk(fmt, ...) printk_index_wrap(_printk, fmt, ##__VA_ARGS__)
                                                       ~~~    ^~~~~~~~~~~
   include/linux/printk.h:422:19: note: expanded from macro 'printk_index_wrap'
                   _p_func(_fmt, ##__VA_ARGS__);                           \
                           ~~~~    ^~~~~~~~~~~
   2 warnings generated.


vim +2326 fs/ceph/inode.c

  2293	
  2294	int ceph_do_getvxattr(struct inode *inode, const char *name, void *value,
  2295			      size_t size)
  2296	{
  2297		struct ceph_fs_client *fsc = ceph_sb_to_client(inode->i_sb);
  2298		struct ceph_mds_client *mdsc = fsc->mdsc;
  2299		struct ceph_mds_request *req;
  2300		int mode = USE_AUTH_MDS;
  2301		int err;
  2302		char *xattr_value;
  2303		size_t xattr_value_len;
  2304	
  2305		req = ceph_mdsc_create_request(mdsc, CEPH_MDS_OP_GETVXATTR, mode);
  2306		if (IS_ERR(req)) {
  2307			err = -ENOMEM;
  2308			goto out;
  2309		}
  2310	
  2311		req->r_path2 = kstrdup(name, GFP_NOFS);
  2312		if (!req->r_path2) {
  2313			err = -ENOMEM;
  2314			goto put;
  2315		}
  2316	
  2317		ihold(inode);
  2318		req->r_inode = inode;
  2319		err = ceph_mdsc_do_request(mdsc, NULL, req);
  2320		if (err < 0)
  2321			goto put;
  2322	
  2323		xattr_value = req->r_reply_info.xattr_info.xattr_value;
  2324		xattr_value_len = req->r_reply_info.xattr_info.xattr_value_len;
  2325	
> 2326		dout("do_getvxattr xattr_value_len:%lu, size:%lu\n", xattr_value_len, size);
  2327	
  2328		err = xattr_value_len;
  2329		if (size == 0)
  2330			goto put;
  2331	
  2332		if (xattr_value_len > size) {
  2333			err = -ERANGE;
  2334			goto put;
  2335		}
  2336	
  2337		memcpy(value, xattr_value, xattr_value_len);
  2338	put:
  2339		ceph_mdsc_put_request(req);
  2340	out:
  2341		dout("do_getvxattr result=%d\n", err);
  2342		return err;
  2343	}
  2344	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org