[PATCH 0/6] ceph: virtual extended attribute cleanup

[PATCH 0/6] ceph: virtual extended attribute cleanup

[Alex Elder]

This series cleans up some code involving ceph's virtual extended
attributes.  Three of them define some simple macros are set up to
help ensure the attributes are defined in a consistent way.  One
makes the size of certain constant values get defined at startup
time rather than repeatedly, and the remaining two are some very
small changes made for clarity.

					-Alex

[PATCH 1/6] ceph: use a symbolic name for "ceph." extended attribute namespace

[Alex Elder]

Use symbolic constants to define the top-level prefix for "ceph."
extended attribute names.

Signed-off-by: Alex Elder <elder@dreamhost.com>
---
  fs/ceph/xattr.c |   25 ++++++++++++++-----------
  1 files changed, 14 insertions(+), 11 deletions(-)

diff --git a/fs/ceph/xattr.c b/fs/ceph/xattr.c
index bec9c30..21ee6aa 100644
--- a/fs/ceph/xattr.c
+++ b/fs/ceph/xattr.c
@@ -8,9 +8,12 @@
  #include <linux/xattr.h>
  #include <linux/slab.h>

+#define XATTR_CEPH_PREFIX "ceph."
+#define XATTR_CEPH_PREFIX_LEN (sizeof (XATTR_CEPH_PREFIX) - 1)
+
  static bool ceph_is_valid_xattr(const char *name)
  {
-	return !strncmp(name, "ceph.", 5) ||
+	return !strncmp(name, XATTR_CEPH_PREFIX, XATTR_CEPH_PREFIX_LEN) ||
  	       !strncmp(name, XATTR_SECURITY_PREFIX,
  			XATTR_SECURITY_PREFIX_LEN) ||
  	       !strncmp(name, XATTR_TRUSTED_PREFIX, XATTR_TRUSTED_PREFIX_LEN) ||
@@ -80,14 +83,14 @@ static size_t ceph_vxattrcb_rctime(struct 
ceph_inode_info *ci, char *val,
  }

  static struct ceph_vxattr_cb ceph_dir_vxattrs[] = {
-	{ true, "ceph.dir.entries", ceph_vxattrcb_entries},
-	{ true, "ceph.dir.files", ceph_vxattrcb_files},
-	{ true, "ceph.dir.subdirs", ceph_vxattrcb_subdirs},
-	{ true, "ceph.dir.rentries", ceph_vxattrcb_rentries},
-	{ true, "ceph.dir.rfiles", ceph_vxattrcb_rfiles},
-	{ true, "ceph.dir.rsubdirs", ceph_vxattrcb_rsubdirs},
-	{ true, "ceph.dir.rbytes", ceph_vxattrcb_rbytes},
-	{ true, "ceph.dir.rctime", ceph_vxattrcb_rctime},
+	{ true, XATTR_CEPH_PREFIX "dir.entries", ceph_vxattrcb_entries},
+	{ true, XATTR_CEPH_PREFIX "dir.files", ceph_vxattrcb_files},
+	{ true, XATTR_CEPH_PREFIX "dir.subdirs", ceph_vxattrcb_subdirs},
+	{ true, XATTR_CEPH_PREFIX "dir.rentries", ceph_vxattrcb_rentries},
+	{ true, XATTR_CEPH_PREFIX "dir.rfiles", ceph_vxattrcb_rfiles},
+	{ true, XATTR_CEPH_PREFIX "dir.rsubdirs", ceph_vxattrcb_rsubdirs},
+	{ true, XATTR_CEPH_PREFIX "dir.rbytes", ceph_vxattrcb_rbytes},
+	{ true, XATTR_CEPH_PREFIX "dir.rctime", ceph_vxattrcb_rctime},
  	{ true, NULL, NULL }
  };

@@ -111,9 +114,9 @@ static size_t ceph_vxattrcb_layout(struct 
ceph_inode_info *ci, char *val,
  }

  static struct ceph_vxattr_cb ceph_file_vxattrs[] = {
-	{ true, "ceph.file.layout", ceph_vxattrcb_layout},
+	{ true, XATTR_CEPH_PREFIX "file.layout", ceph_vxattrcb_layout},
  	/* The following extended attribute name is deprecated */
-	{ true, "ceph.layout", ceph_vxattrcb_layout},
+	{ true, XATTR_CEPH_PREFIX "layout", ceph_vxattrcb_layout},
  	{ true, NULL, NULL }
  };

-- 
1.7.5.4

[PATCH 2/6] ceph: use macros to normalize vxattr table definitions

[Alex Elder]

Entries in the ceph virtual extended attribute tables all follow a
distinct pattern in their definition.  Enforce this pattern through
the use of a macro.

Also, a null name field signals the end of the table, so make that
be the first field in the ceph_vxattr_cb structure.

Signed-off-by: Alex Elder <elder@dreamhost.com>
---
  fs/ceph/xattr.c |   39 ++++++++++++++++++++++++++-------------
  1 files changed, 26 insertions(+), 13 deletions(-)

diff --git a/fs/ceph/xattr.c b/fs/ceph/xattr.c
index 21ee6aa..9ef0134 100644
--- a/fs/ceph/xattr.c
+++ b/fs/ceph/xattr.c
@@ -25,10 +25,10 @@ static bool ceph_is_valid_xattr(const char *name)
   * statistics and layout metadata.
   */
  struct ceph_vxattr_cb {
-	bool readonly;
  	char *name;
  	size_t (*getxattr_cb)(struct ceph_inode_info *ci, char *val,
  			      size_t size);
+	bool readonly;
  };

  /* directories */
@@ -82,16 +82,25 @@ static size_t ceph_vxattrcb_rctime(struct 
ceph_inode_info *ci, char *val,
  			(long)ci->i_rctime.tv_nsec);
  }

+#define CEPH_XATTR_NAME(_type, _name)	XATTR_CEPH_PREFIX #_type "." #_name
+
+#define XATTR_NAME_CEPH(_type, _name) \
+		{ \
+			.name = CEPH_XATTR_NAME(_type, _name), \
+			.getxattr_cb = ceph_vxattrcb_ ## _name, \
+			.readonly = true, \
+		}
+
  static struct ceph_vxattr_cb ceph_dir_vxattrs[] = {
-	{ true, XATTR_CEPH_PREFIX "dir.entries", ceph_vxattrcb_entries},
-	{ true, XATTR_CEPH_PREFIX "dir.files", ceph_vxattrcb_files},
-	{ true, XATTR_CEPH_PREFIX "dir.subdirs", ceph_vxattrcb_subdirs},
-	{ true, XATTR_CEPH_PREFIX "dir.rentries", ceph_vxattrcb_rentries},
-	{ true, XATTR_CEPH_PREFIX "dir.rfiles", ceph_vxattrcb_rfiles},
-	{ true, XATTR_CEPH_PREFIX "dir.rsubdirs", ceph_vxattrcb_rsubdirs},
-	{ true, XATTR_CEPH_PREFIX "dir.rbytes", ceph_vxattrcb_rbytes},
-	{ true, XATTR_CEPH_PREFIX "dir.rctime", ceph_vxattrcb_rctime},
-	{ true, NULL, NULL }
+	XATTR_NAME_CEPH(dir, entries),
+	XATTR_NAME_CEPH(dir, files),
+	XATTR_NAME_CEPH(dir, subdirs),
+	XATTR_NAME_CEPH(dir, rentries),
+	XATTR_NAME_CEPH(dir, rfiles),
+	XATTR_NAME_CEPH(dir, rsubdirs),
+	XATTR_NAME_CEPH(dir, rbytes),
+	XATTR_NAME_CEPH(dir, rctime),
+	{ 0 }	/* Required table terminator */
  };

  /* files */
@@ -114,10 +123,14 @@ static size_t ceph_vxattrcb_layout(struct 
ceph_inode_info *ci, char *val,
  }

  static struct ceph_vxattr_cb ceph_file_vxattrs[] = {
-	{ true, XATTR_CEPH_PREFIX "file.layout", ceph_vxattrcb_layout},
+	XATTR_NAME_CEPH(file, layout),
  	/* The following extended attribute name is deprecated */
-	{ true, XATTR_CEPH_PREFIX "layout", ceph_vxattrcb_layout},
-	{ true, NULL, NULL }
+	{
+		.name = XATTR_CEPH_PREFIX "layout",
+		.getxattr_cb = ceph_vxattrcb_layout,
+		.readonly = true,
+	},
+	{ 0 }	/* Required table terminator */
  };

  static struct ceph_vxattr_cb *ceph_inode_vxattrs(struct inode *inode)
-- 
1.7.5.4

[PATCH 3/6] ceph: drop "_cb" from name of struct ceph_vxattr_cb

[Alex Elder]

A struct ceph_vxattr_cb does not represent a callback at all, but
rather a virtual extended attribute itself.  Drop the "_cb" suffix
from its name to reflect that.

Signed-off-by: Alex Elder <elder@dreamhost.com>
---
  fs/ceph/xattr.c |   20 ++++++++++----------
  1 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/fs/ceph/xattr.c b/fs/ceph/xattr.c
index 9ef0134..3e0ef77 100644
--- a/fs/ceph/xattr.c
+++ b/fs/ceph/xattr.c
@@ -24,7 +24,7 @@ static bool ceph_is_valid_xattr(const char *name)
   * These define virtual xattrs exposing the recursive directory
   * statistics and layout metadata.
   */
-struct ceph_vxattr_cb {
+struct ceph_vxattr {
  	char *name;
  	size_t (*getxattr_cb)(struct ceph_inode_info *ci, char *val,
  			      size_t size);
@@ -91,7 +91,7 @@ static size_t ceph_vxattrcb_rctime(struct 
ceph_inode_info *ci, char *val,
  			.readonly = true, \
  		}

-static struct ceph_vxattr_cb ceph_dir_vxattrs[] = {
+static struct ceph_vxattr ceph_dir_vxattrs[] = {
  	XATTR_NAME_CEPH(dir, entries),
  	XATTR_NAME_CEPH(dir, files),
  	XATTR_NAME_CEPH(dir, subdirs),
@@ -122,7 +122,7 @@ static size_t ceph_vxattrcb_layout(struct 
ceph_inode_info *ci, char *val,
  	return ret;
  }

-static struct ceph_vxattr_cb ceph_file_vxattrs[] = {
+static struct ceph_vxattr ceph_file_vxattrs[] = {
  	XATTR_NAME_CEPH(file, layout),
  	/* The following extended attribute name is deprecated */
  	{
@@ -133,7 +133,7 @@ static struct ceph_vxattr_cb ceph_file_vxattrs[] = {
  	{ 0 }	/* Required table terminator */
  };

-static struct ceph_vxattr_cb *ceph_inode_vxattrs(struct inode *inode)
+static struct ceph_vxattr *ceph_inode_vxattrs(struct inode *inode)
  {
  	if (S_ISDIR(inode->i_mode))
  		return ceph_dir_vxattrs;
@@ -142,10 +142,10 @@ static struct ceph_vxattr_cb 
*ceph_inode_vxattrs(struct inode *inode)
  	return NULL;
  }

-static struct ceph_vxattr_cb *ceph_match_vxattr(struct inode *inode,
+static struct ceph_vxattr *ceph_match_vxattr(struct inode *inode,
  						const char *name)
  {
-	struct ceph_vxattr_cb *vxattr = ceph_inode_vxattrs(inode);
+	struct ceph_vxattr *vxattr = ceph_inode_vxattrs(inode);

  	if (vxattr)
  		while (vxattr->name) {
@@ -524,7 +524,7 @@ ssize_t ceph_getxattr(struct dentry *dentry, const 
char *name, void *value,
  	struct ceph_inode_info *ci = ceph_inode(inode);
  	int err;
  	struct ceph_inode_xattr *xattr;
-	struct ceph_vxattr_cb *vxattr = NULL;
+	struct ceph_vxattr *vxattr = NULL;

  	if (!ceph_is_valid_xattr(name))
  		return -ENODATA;
@@ -586,7 +586,7 @@ ssize_t ceph_listxattr(struct dentry *dentry, char 
*names, size_t size)
  {
  	struct inode *inode = dentry->d_inode;
  	struct ceph_inode_info *ci = ceph_inode(inode);
-	struct ceph_vxattr_cb *vxattrs = ceph_inode_vxattrs(inode);
+	struct ceph_vxattr *vxattrs = ceph_inode_vxattrs(inode);
  	u32 vir_namelen = 0;
  	u32 namelen;
  	int err;
@@ -716,7 +716,7 @@ int ceph_setxattr(struct dentry *dentry, const char 
*name,
  		  const void *value, size_t size, int flags)
  {
  	struct inode *inode = dentry->d_inode;
-	struct ceph_vxattr_cb *vxattr;
+	struct ceph_vxattr *vxattr;
  	struct ceph_inode_info *ci = ceph_inode(inode);
  	int err;
  	int name_len = strlen(name);
@@ -829,7 +829,7 @@ static int ceph_send_removexattr(struct dentry 
*dentry, const char *name)
  int ceph_removexattr(struct dentry *dentry, const char *name)
  {
  	struct inode *inode = dentry->d_inode;
-	struct ceph_vxattr_cb *vxattr;
+	struct ceph_vxattr *vxattr;
  	struct ceph_inode_info *ci = ceph_inode(inode);
  	int issued;
  	int err;
-- 
1.7.5.4

[PATCH 4/6] ceph: encode type in vxattr callback routines

[Alex Elder]

The names of the callback functions used for virtual extended
attributes are based only on the last component of the attribute
name.  Because of the way these are defined, this precludes allowing
a single (lowest) attribute name for different callbacks, dependent
on the type of file being operated on.  (For example, it might be
nice to support both "ceph.dir.layout" and "ceph.file.layout".)

Just change the callback names to avoid this problem.

Signed-off-by: Alex Elder <elder@dreamhost.com>
---
  fs/ceph/xattr.c |   22 +++++++++++-----------
  1 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/fs/ceph/xattr.c b/fs/ceph/xattr.c
index 3e0ef77..83366f9 100644
--- a/fs/ceph/xattr.c
+++ b/fs/ceph/xattr.c
@@ -33,49 +33,49 @@ struct ceph_vxattr {

  /* directories */

-static size_t ceph_vxattrcb_entries(struct ceph_inode_info *ci, char *val,
+static size_t ceph_vxattrcb_dir_entries(struct ceph_inode_info *ci, 
char *val,
  					size_t size)
  {
  	return snprintf(val, size, "%lld", ci->i_files + ci->i_subdirs);
  }

-static size_t ceph_vxattrcb_files(struct ceph_inode_info *ci, char *val,
+static size_t ceph_vxattrcb_dir_files(struct ceph_inode_info *ci, char 
*val,
  				      size_t size)
  {
  	return snprintf(val, size, "%lld", ci->i_files);
  }

-static size_t ceph_vxattrcb_subdirs(struct ceph_inode_info *ci, char *val,
+static size_t ceph_vxattrcb_dir_subdirs(struct ceph_inode_info *ci, 
char *val,
  					size_t size)
  {
  	return snprintf(val, size, "%lld", ci->i_subdirs);
  }

-static size_t ceph_vxattrcb_rentries(struct ceph_inode_info *ci, char *val,
+static size_t ceph_vxattrcb_dir_rentries(struct ceph_inode_info *ci, 
char *val,
  					 size_t size)
  {
  	return snprintf(val, size, "%lld", ci->i_rfiles + ci->i_rsubdirs);
  }

-static size_t ceph_vxattrcb_rfiles(struct ceph_inode_info *ci, char *val,
+static size_t ceph_vxattrcb_dir_rfiles(struct ceph_inode_info *ci, char 
*val,
  				       size_t size)
  {
  	return snprintf(val, size, "%lld", ci->i_rfiles);
  }

-static size_t ceph_vxattrcb_rsubdirs(struct ceph_inode_info *ci, char *val,
+static size_t ceph_vxattrcb_dir_rsubdirs(struct ceph_inode_info *ci, 
char *val,
  					 size_t size)
  {
  	return snprintf(val, size, "%lld", ci->i_rsubdirs);
  }

-static size_t ceph_vxattrcb_rbytes(struct ceph_inode_info *ci, char *val,
+static size_t ceph_vxattrcb_dir_rbytes(struct ceph_inode_info *ci, char 
*val,
  				       size_t size)
  {
  	return snprintf(val, size, "%lld", ci->i_rbytes);
  }

-static size_t ceph_vxattrcb_rctime(struct ceph_inode_info *ci, char *val,
+static size_t ceph_vxattrcb_dir_rctime(struct ceph_inode_info *ci, char 
*val,
  				       size_t size)
  {
  	return snprintf(val, size, "%ld.%ld", (long)ci->i_rctime.tv_sec,
@@ -87,7 +87,7 @@ static size_t ceph_vxattrcb_rctime(struct 
ceph_inode_info *ci, char *val,
  #define XATTR_NAME_CEPH(_type, _name) \
  		{ \
  			.name = CEPH_XATTR_NAME(_type, _name), \
-			.getxattr_cb = ceph_vxattrcb_ ## _name, \
+			.getxattr_cb = ceph_vxattrcb_ ## _type ## _ ## _name, \
  			.readonly = true, \
  		}

@@ -105,7 +105,7 @@ static struct ceph_vxattr ceph_dir_vxattrs[] = {

  /* files */

-static size_t ceph_vxattrcb_layout(struct ceph_inode_info *ci, char *val,
+static size_t ceph_vxattrcb_file_layout(struct ceph_inode_info *ci, 
char *val,
  				   size_t size)
  {
  	int ret;
@@ -127,7 +127,7 @@ static struct ceph_vxattr ceph_file_vxattrs[] = {
  	/* The following extended attribute name is deprecated */
  	{
  		.name = XATTR_CEPH_PREFIX "layout",
-		.getxattr_cb = ceph_vxattrcb_layout,
+		.getxattr_cb = ceph_vxattrcb_file_layout,
  		.readonly = true,
  	},
  	{ 0 }	/* Required table terminator */
-- 
1.7.5.4

[PATCH 5/6] ceph: avoid repeatedly computing the size of constant vxattr names

[Alex Elder]

All names defined in the directory and file virtual extended
attribute tables are constant, and the size of each is known at
compile time.  So there's no need to compute their length every
time any file's attribute is listed.

Record the length of each string and use it when needed to determine
the space need to represent them.  In addition, compute the
aggregate size of strings in each table just once at initialization
time.

Signed-off-by: Alex Elder <elder@dreamhost.com>
---
  fs/ceph/super.c |    3 ++
  fs/ceph/super.h |    2 +
  fs/ceph/xattr.c |   56 
++++++++++++++++++++++++++++++++++++++++++++++++++----
  3 files changed, 56 insertions(+), 5 deletions(-)

diff --git a/fs/ceph/super.c b/fs/ceph/super.c
index b48f15f..e6325f0 100644
--- a/fs/ceph/super.c
+++ b/fs/ceph/super.c
@@ -906,6 +906,7 @@ static int __init init_ceph(void)
  	if (ret)
  		goto out;

+	ceph_xattr_init();
  	ret = register_filesystem(&ceph_fs_type);
  	if (ret)
  		goto out_icache;
@@ -915,6 +916,7 @@ static int __init init_ceph(void)
  	return 0;

  out_icache:
+	ceph_xattr_exit();
  	destroy_caches();
  out:
  	return ret;
@@ -924,6 +926,7 @@ static void __exit exit_ceph(void)
  {
  	dout("exit_ceph\n");
  	unregister_filesystem(&ceph_fs_type);
+	ceph_xattr_exit();
  	destroy_caches();
  }

diff --git a/fs/ceph/super.h b/fs/ceph/super.h
index 5f00e82..4be2dc4 100644
--- a/fs/ceph/super.h
+++ b/fs/ceph/super.h
@@ -732,6 +732,8 @@ extern ssize_t ceph_listxattr(struct dentry *, char 
*, size_t);
  extern int ceph_removexattr(struct dentry *, const char *);
  extern void __ceph_build_xattrs_blob(struct ceph_inode_info *ci);
  extern void __ceph_destroy_xattrs(struct ceph_inode_info *ci);
+extern void __init ceph_xattr_init(void);
+extern void ceph_xattr_exit(void);

  /* caps.c */
  extern const char *ceph_cap_string(int c);
diff --git a/fs/ceph/xattr.c b/fs/ceph/xattr.c
index 83366f9..984c863 100644
--- a/fs/ceph/xattr.c
+++ b/fs/ceph/xattr.c
@@ -26,6 +26,7 @@ static bool ceph_is_valid_xattr(const char *name)
   */
  struct ceph_vxattr {
  	char *name;
+	size_t name_size;	/* strlen(name) + 1 (for '\0') */
  	size_t (*getxattr_cb)(struct ceph_inode_info *ci, char *val,
  			      size_t size);
  	bool readonly;
@@ -87,6 +88,7 @@ static size_t ceph_vxattrcb_dir_rctime(struct 
ceph_inode_info *ci, char *val,
  #define XATTR_NAME_CEPH(_type, _name) \
  		{ \
  			.name = CEPH_XATTR_NAME(_type, _name), \
+			.name_size = sizeof CEPH_XATTR_NAME(_type, _name), \
  			.getxattr_cb = ceph_vxattrcb_ ## _type ## _ ## _name, \
  			.readonly = true, \
  		}
@@ -102,6 +104,7 @@ static struct ceph_vxattr ceph_dir_vxattrs[] = {
  	XATTR_NAME_CEPH(dir, rctime),
  	{ 0 }	/* Required table terminator */
  };
+static size_t ceph_dir_vxattrs_name_size;	/* total size of all names */

  /* files */

@@ -127,11 +130,13 @@ static struct ceph_vxattr ceph_file_vxattrs[] = {
  	/* The following extended attribute name is deprecated */
  	{
  		.name = XATTR_CEPH_PREFIX "layout",
+		.name_size = sizeof XATTR_CEPH_PREFIX "layout",
  		.getxattr_cb = ceph_vxattrcb_file_layout,
  		.readonly = true,
  	},
  	{ 0 }	/* Required table terminator */
  };
+static size_t ceph_file_vxattrs_name_size;	/* total size of all names */

  static struct ceph_vxattr *ceph_inode_vxattrs(struct inode *inode)
  {
@@ -142,6 +147,46 @@ static struct ceph_vxattr 
*ceph_inode_vxattrs(struct inode *inode)
  	return NULL;
  }

+static size_t ceph_vxattrs_name_size(struct ceph_vxattr *vxattrs)
+{
+	if (vxattrs == ceph_dir_vxattrs)
+		return ceph_dir_vxattrs_name_size;
+	if (vxattrs == ceph_file_vxattrs)
+		return ceph_file_vxattrs_name_size;
+	BUG();
+
+	return 0;
+}
+
+/*
+ * Compute the aggregate size (including terminating '\0') of all
+ * virtual extended attribute names in the given vxattr table.
+ */
+static size_t __init vxattrs_name_size(struct ceph_vxattr *vxattrs)
+{
+	struct ceph_vxattr *vxattr;
+	size_t size = 0;
+
+	for (vxattr = vxattrs; vxattr->name; vxattr++)
+		size += vxattr->name_size;
+
+	return size;
+}
+
+/* Routines called at initialization and exit time */
+
+void __init ceph_xattr_init(void)
+{
+	ceph_dir_vxattrs_name_size = vxattrs_name_size(ceph_dir_vxattrs);
+	ceph_file_vxattrs_name_size = vxattrs_name_size(ceph_file_vxattrs);
+}
+
+void ceph_xattr_exit(void)
+{
+	ceph_dir_vxattrs_name_size = 0;
+	ceph_file_vxattrs_name_size = 0;
+}
+
  static struct ceph_vxattr *ceph_match_vxattr(struct inode *inode,
  						const char *name)
  {
@@ -614,11 +659,12 @@ ssize_t ceph_listxattr(struct dentry *dentry, char 
*names, size_t size)
  		goto out;

  list_xattr:
-	vir_namelen = 0;
-	/* include virtual dir xattrs */
-	if (vxattrs)
-		for (i = 0; vxattrs[i].name; i++)
-			vir_namelen += strlen(vxattrs[i].name) + 1;
+	/*
+	 * Start with virtual dir xattr names (if any) (including
+	 * terminating '\0' characters for each).
+	 */
+	vir_namelen = ceph_vxattrs_name_size(vxattrs);
+
  	/* adding 1 byte per each variable due to the null termination */
  	namelen = vir_namelen + ci->i_xattrs.names_size + ci->i_xattrs.count;
  	err = -ERANGE;
-- 
1.7.5.4

Re: [PATCH 5/6] ceph: avoid repeatedly computing the size of constant vxattr names

[Yehuda Sadeh Weinraub]

On Tue, Feb 28, 2012 at 7:21 PM, Alex Elder <elder@dreamhost.com> wrote:
> All names defined in the directory and file virtual extended
> attribute tables are constant, and the size of each is known at
> compile time.  So there's no need to compute their length every
> time any file's attribute is listed.
>
> Record the length of each string and use it when needed to determine
> the space need to represent them.  In addition, compute the
> aggregate size of strings in each table just once at initialization
> time.
>
> Signed-off-by: Alex Elder <elder@dreamhost.com>
> ---
>  fs/ceph/super.c |    3 ++
>  fs/ceph/super.h |    2 +
>  fs/ceph/xattr.c |   56
> ++++++++++++++++++++++++++++++++++++++++++++++++++----
>  3 files changed, 56 insertions(+), 5 deletions(-)
>
> diff --git a/fs/ceph/super.c b/fs/ceph/super.c
> index b48f15f..e6325f0 100644
> --- a/fs/ceph/super.c
> +++ b/fs/ceph/super.c
> @@ -906,6 +906,7 @@ static int __init init_ceph(void)
>        if (ret)
>                goto out;
>
> +       ceph_xattr_init();
>        ret = register_filesystem(&ceph_fs_type);
>        if (ret)
>                goto out_icache;
> @@ -915,6 +916,7 @@ static int __init init_ceph(void)
>        return 0;
>
>  out_icache:
> +       ceph_xattr_exit();
>        destroy_caches();
>  out:
>        return ret;
> @@ -924,6 +926,7 @@ static void __exit exit_ceph(void)
>  {
>        dout("exit_ceph\n");
>        unregister_filesystem(&ceph_fs_type);
> +       ceph_xattr_exit();
>        destroy_caches();
>  }
>
> diff --git a/fs/ceph/super.h b/fs/ceph/super.h
> index 5f00e82..4be2dc4 100644
> --- a/fs/ceph/super.h
> +++ b/fs/ceph/super.h
> @@ -732,6 +732,8 @@ extern ssize_t ceph_listxattr(struct dentry *, char *,
> size_t);
>  extern int ceph_removexattr(struct dentry *, const char *);
>  extern void __ceph_build_xattrs_blob(struct ceph_inode_info *ci);
>  extern void __ceph_destroy_xattrs(struct ceph_inode_info *ci);
> +extern void __init ceph_xattr_init(void);
> +extern void ceph_xattr_exit(void);
>
>  /* caps.c */
>  extern const char *ceph_cap_string(int c);
> diff --git a/fs/ceph/xattr.c b/fs/ceph/xattr.c
> index 83366f9..984c863 100644
> --- a/fs/ceph/xattr.c
> +++ b/fs/ceph/xattr.c
> @@ -26,6 +26,7 @@ static bool ceph_is_valid_xattr(const char *name)
>  */
>  struct ceph_vxattr {
>        char *name;
> +       size_t name_size;       /* strlen(name) + 1 (for '\0') */
>        size_t (*getxattr_cb)(struct ceph_inode_info *ci, char *val,
>                              size_t size);
>        bool readonly;
> @@ -87,6 +88,7 @@ static size_t ceph_vxattrcb_dir_rctime(struct
> ceph_inode_info *ci, char *val,
>  #define XATTR_NAME_CEPH(_type, _name) \
>                { \
>                        .name = CEPH_XATTR_NAME(_type, _name), \
> +                       .name_size = sizeof CEPH_XATTR_NAME(_type, _name), \

You should use sizeof(x) instead of sizeof x.

>                        .getxattr_cb = ceph_vxattrcb_ ## _type ## _ ## _name,
> \
>                        .readonly = true, \
>                }
> @@ -102,6 +104,7 @@ static struct ceph_vxattr ceph_dir_vxattrs[] = {
>        XATTR_NAME_CEPH(dir, rctime),
>        { 0 }   /* Required table terminator */
>  };
> +static size_t ceph_dir_vxattrs_name_size;      /* total size of all names
> */
>
>  /* files */
>
> @@ -127,11 +130,13 @@ static struct ceph_vxattr ceph_file_vxattrs[] = {
>        /* The following extended attribute name is deprecated */
>        {
>                .name = XATTR_CEPH_PREFIX "layout",
> +               .name_size = sizeof XATTR_CEPH_PREFIX "layout",

here too.

>                .getxattr_cb = ceph_vxattrcb_file_layout,
>                .readonly = true,
>        },
>        { 0 }   /* Required table terminator */
>  };
> +static size_t ceph_file_vxattrs_name_size;     /* total size of all names
> */
>
>  static struct ceph_vxattr *ceph_inode_vxattrs(struct inode *inode)
>  {
> @@ -142,6 +147,46 @@ static struct ceph_vxattr *ceph_inode_vxattrs(struct
> inode *inode)
>        return NULL;
>  }
>
> +static size_t ceph_vxattrs_name_size(struct ceph_vxattr *vxattrs)
> +{
> +       if (vxattrs == ceph_dir_vxattrs)
> +               return ceph_dir_vxattrs_name_size;
> +       if (vxattrs == ceph_file_vxattrs)
> +               return ceph_file_vxattrs_name_size;
> +       BUG();
> +
> +       return 0;
> +}
> +
> +/*
> + * Compute the aggregate size (including terminating '\0') of all
> + * virtual extended attribute names in the given vxattr table.
> + */
> +static size_t __init vxattrs_name_size(struct ceph_vxattr *vxattrs)
> +{sizeof
> +       struct ceph_vxattr *vxattr;
> +       size_t size = 0;
> +
> +       for (vxattr = vxattrs; vxattr->name; vxattr++)
> +               size += vxattr->name_size;
> +
> +       return size;
> +}
> +
> +/* Routines called at initialization and exit time */
> +
> +void __init ceph_xattr_init(void)
> +{
> +       ceph_dir_vxattrs_name_size = vxattrs_name_size(ceph_dir_vxattrs);
> +       ceph_file_vxattrs_name_size = vxattrs_name_size(ceph_file_vxattrs);
> +}
> +
> +void ceph_xattr_exit(void)
> +{
> +       ceph_dir_vxattrs_name_size = 0;
> +       ceph_file_vxattrs_name_size = 0;
> +}
> +
>  static struct ceph_vxattr *ceph_match_vxattr(struct inode *inode,
>                                                const char *name)
>  {
> @@ -614,11 +659,12 @@ ssize_t ceph_listxattr(struct dentry *dentry, char
> *names, size_t size)
>                goto out;
>
>  list_xattr:
> -       vir_namelen = 0;
> -       /* include virtual dir xattrs */
> -       if (vxattrs)
> -               for (i = 0; vxattrs[i].name; i++)
> -                       vir_namelen += strlen(vxattrs[i].name) + 1;
> +       /*
> +        * Start with virtual dir xattr names (if any) (including
> +        * terminating '\0' characters for each).
> +        */
> +       vir_namelen = ceph_vxattrs_name_size(vxattrs);
> +
>        /* adding 1 byte per each variable due to the null termination */
>        namelen = vir_namelen + ci->i_xattrs.names_size + ci->i_xattrs.count;
>        err = -ERANGE;
> --
> 1.7.5.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/6] ceph: avoid repeatedly computing the size of constant vxattr names

[Alex Elder]

On 02/28/2012 08:47 PM, Yehuda Sadeh Weinraub wrote:
> On Tue, Feb 28, 2012 at 7:21 PM, Alex Elder<elder@dreamhost.com>  wrote:
>> All names defined in the directory and file virtual extended
>> attribute tables are constant, and the size of each is known at
>> compile time.  So there's no need to compute their length every
>> time any file's attribute is listed.
>>
>> Record the length of each string and use it when needed to determine
>> the space need to represent them.  In addition, compute the
>> aggregate size of strings in each table just once at initialization
>> time.
>>
>> Signed-off-by: Alex Elder<elder@dreamhost.com>
>> ---
>>   fs/ceph/super.c |    3 ++
>>   fs/ceph/super.h |    2 +
>>   fs/ceph/xattr.c |   56
>> ++++++++++++++++++++++++++++++++++++++++++++++++++----
>>   3 files changed, 56 insertions(+), 5 deletions(-)
. . .
>> @@ -87,6 +88,7 @@ static size_t ceph_vxattrcb_dir_rctime(struct
>> ceph_inode_info *ci, char *val,
>>   #define XATTR_NAME_CEPH(_type, _name) \
>>                 { \
>>                         .name = CEPH_XATTR_NAME(_type, _name), \
>> +                       .name_size = sizeof CEPH_XATTR_NAME(_type, _name), \
>
> You should use sizeof(x) instead of sizeof x.

I got the same comment when I tried using this form on XFS code.

I personally prefer this form--it tells you that "x" in this case
is an object, not a type.

However I will fix all references to "sizeof object" to use
sizeof(object) instead--here and in the future.

					-Alex


>>                         .getxattr_cb = ceph_vxattrcb_ ## _type ## _ ## _name,
>> \
>>                         .readonly = true, \
>>                 }
>> @@ -102,6 +104,7 @@ static struct ceph_vxattr ceph_dir_vxattrs[] = {
>>         XATTR_NAME_CEPH(dir, rctime),
>>         { 0 }   /* Required table terminator */
>>   };
>> +static size_t ceph_dir_vxattrs_name_size;      /* total size of all names
>> */
>>
>>   /* files */
>>
>> @@ -127,11 +130,13 @@ static struct ceph_vxattr ceph_file_vxattrs[] = {
>>         /* The following extended attribute name is deprecated */
>>         {
>>                 .name = XATTR_CEPH_PREFIX "layout",
>> +               .name_size = sizeof XATTR_CEPH_PREFIX "layout",
>
> here too.
>
>>                 .getxattr_cb = ceph_vxattrcb_file_layout,
>>                 .readonly = true,
>>         },
>>         { 0 }   /* Required table terminator */
>>   };
>> +static size_t ceph_file_vxattrs_name_size;     /* total size of all names
>> */
>>
>>   static struct ceph_vxattr *ceph_inode_vxattrs(struct inode *inode)
>>   {
>> @@ -142,6 +147,46 @@ static struct ceph_vxattr *ceph_inode_vxattrs(struct
>> inode *inode)
>>         return NULL;
>>   }
>>
>> +static size_t ceph_vxattrs_name_size(struct ceph_vxattr *vxattrs)
>> +{
>> +       if (vxattrs == ceph_dir_vxattrs)
>> +               return ceph_dir_vxattrs_name_size;
>> +       if (vxattrs == ceph_file_vxattrs)
>> +               return ceph_file_vxattrs_name_size;
>> +       BUG();
>> +
>> +       return 0;
>> +}
>> +
>> +/*
>> + * Compute the aggregate size (including terminating '\0') of all
>> + * virtual extended attribute names in the given vxattr table.
>> + */
>> +static size_t __init vxattrs_name_size(struct ceph_vxattr *vxattrs)
>> +{sizeof
>> +       struct ceph_vxattr *vxattr;
>> +       size_t size = 0;
>> +
>> +       for (vxattr = vxattrs; vxattr->name; vxattr++)
>> +               size += vxattr->name_size;
>> +
>> +       return size;
>> +}
>> +
>> +/* Routines called at initialization and exit time */
>> +
>> +void __init ceph_xattr_init(void)
>> +{
>> +       ceph_dir_vxattrs_name_size = vxattrs_name_size(ceph_dir_vxattrs);
>> +       ceph_file_vxattrs_name_size = vxattrs_name_size(ceph_file_vxattrs);
>> +}
>> +
>> +void ceph_xattr_exit(void)
>> +{
>> +       ceph_dir_vxattrs_name_size = 0;
>> +       ceph_file_vxattrs_name_size = 0;
>> +}
>> +
>>   static struct ceph_vxattr *ceph_match_vxattr(struct inode *inode,
>>                                                 const char *name)
>>   {
>> @@ -614,11 +659,12 @@ ssize_t ceph_listxattr(struct dentry *dentry, char
>> *names, size_t size)
>>                 goto out;
>>
>>   list_xattr:
>> -       vir_namelen = 0;
>> -       /* include virtual dir xattrs */
>> -       if (vxattrs)
>> -               for (i = 0; vxattrs[i].name; i++)
>> -                       vir_namelen += strlen(vxattrs[i].name) + 1;
>> +       /*
>> +        * Start with virtual dir xattr names (if any) (including
>> +        * terminating '\0' characters for each).
>> +        */
>> +       vir_namelen = ceph_vxattrs_name_size(vxattrs);
>> +
>>         /* adding 1 byte per each variable due to the null termination */
>>         namelen = vir_namelen + ci->i_xattrs.names_size + ci->i_xattrs.count;
>>         err = -ERANGE;
>> --
>> 1.7.5.4
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 6/6] ceph: make ceph_setxattr() and ceph_removexattr() more alike

[Alex Elder]

This patch just rearranges a few bits of code to make more
portions of ceph_setxattr() and ceph_removexattr() identical.

Signed-off-by: Alex Elder <elder@dreamhost.com>
---
  fs/ceph/xattr.c |   15 ++++++++-------
  1 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/fs/ceph/xattr.c b/fs/ceph/xattr.c
index 984c863..3fccd3b 100644
--- a/fs/ceph/xattr.c
+++ b/fs/ceph/xattr.c
@@ -764,15 +764,15 @@ int ceph_setxattr(struct dentry *dentry, const 
char *name,
  	struct inode *inode = dentry->d_inode;
  	struct ceph_vxattr *vxattr;
  	struct ceph_inode_info *ci = ceph_inode(inode);
+	int issued;
  	int err;
+	int dirty;
  	int name_len = strlen(name);
  	int val_len = size;
  	char *newname = NULL;
  	char *newval = NULL;
  	struct ceph_inode_xattr *xattr = NULL;
-	int issued;
  	int required_blob_size;
-	int dirty;

  	if (ceph_snap(inode) != CEPH_NOSNAP)
  		return -EROFS;
@@ -803,6 +803,7 @@ int ceph_setxattr(struct dentry *dentry, const char 
*name,
  	spin_lock(&ci->i_ceph_lock);
  retry:
  	issued = __ceph_caps_issued(ci, NULL);
+	dout("setxattr %p issued %s\n", inode, ceph_cap_string(issued));
  	if (!(issued & CEPH_CAP_XATTR_EXCL))
  		goto do_sync;
  	__build_xattrs(inode);
@@ -811,7 +812,7 @@ retry:

  	if (!ci->i_xattrs.prealloc_blob ||
  	    required_blob_size > ci->i_xattrs.prealloc_blob->alloc_len) {
-		struct ceph_buffer *blob = NULL;
+		struct ceph_buffer *blob;

  		spin_unlock(&ci->i_ceph_lock);
  		dout(" preaallocating new blob size=%d\n", required_blob_size);
@@ -825,12 +826,13 @@ retry:
  		goto retry;
  	}

-	dout("setxattr %p issued %s\n", inode, ceph_cap_string(issued));
  	err = __set_xattr(ci, newname, name_len, newval,
  			  val_len, 1, 1, 1, &xattr);
+
  	dirty = __ceph_mark_dirty_caps(ci, CEPH_CAP_XATTR_EXCL);
  	ci->i_xattrs.dirty = true;
  	inode->i_ctime = CURRENT_TIME;
+
  	spin_unlock(&ci->i_ceph_lock);
  	if (dirty)
  		__mark_inode_dirty(inode, dirty);
@@ -892,18 +894,17 @@ int ceph_removexattr(struct dentry *dentry, const 
char *name)
  		return -EOPNOTSUPP;

  	spin_lock(&ci->i_ceph_lock);
-	__build_xattrs(inode);
  	issued = __ceph_caps_issued(ci, NULL);
  	dout("removexattr %p issued %s\n", inode, ceph_cap_string(issued));
-
  	if (!(issued & CEPH_CAP_XATTR_EXCL))
  		goto do_sync;
+	__build_xattrs(inode);

  	err = __remove_xattr_by_name(ceph_inode(inode), name);
+
  	dirty = __ceph_mark_dirty_caps(ci, CEPH_CAP_XATTR_EXCL);
  	ci->i_xattrs.dirty = true;
  	inode->i_ctime = CURRENT_TIME;
-
  	spin_unlock(&ci->i_ceph_lock);
  	if (dirty)
  		__mark_inode_dirty(inode, dirty);
-- 
1.7.5.4

Re: [PATCH 6/6] ceph: make ceph_setxattr() and ceph_removexattr() more alike

[Sage Weil]

On Tue, 28 Feb 2012, Alex Elder wrote:

> This patch just rearranges a few bits of code to make more
> portions of ceph_setxattr() and ceph_removexattr() identical.
> 
> Signed-off-by: Alex Elder <elder@dreamhost.com>
> ---
>  fs/ceph/xattr.c |   15 ++++++++-------
>  1 files changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/ceph/xattr.c b/fs/ceph/xattr.c
> index 984c863..3fccd3b 100644
> --- a/fs/ceph/xattr.c
> +++ b/fs/ceph/xattr.c
> @@ -764,15 +764,15 @@ int ceph_setxattr(struct dentry *dentry, const char
> *name,
>  	struct inode *inode = dentry->d_inode;
>  	struct ceph_vxattr *vxattr;
>  	struct ceph_inode_info *ci = ceph_inode(inode);
> +	int issued;
>  	int err;
> +	int dirty;
>  	int name_len = strlen(name);
>  	int val_len = size;
>  	char *newname = NULL;
>  	char *newval = NULL;
>  	struct ceph_inode_xattr *xattr = NULL;
> -	int issued;
>  	int required_blob_size;
> -	int dirty;
> 
>  	if (ceph_snap(inode) != CEPH_NOSNAP)
>  		return -EROFS;
> @@ -803,6 +803,7 @@ int ceph_setxattr(struct dentry *dentry, const char *name,
>  	spin_lock(&ci->i_ceph_lock);
>  retry:

This:

>  	issued = __ceph_caps_issued(ci, NULL);
> +	dout("setxattr %p issued %s\n", inode, ceph_cap_string(issued));
>  	if (!(issued & CEPH_CAP_XATTR_EXCL))
>  		goto do_sync;

needs to go after this:

>  	__build_xattrs(inode);

All of the __build_xattrs() calls should go before the cap check, because 
they drop i_ceph_lock.  

Can probably do that in a new patch, since it was broken for some of these 
before.

Otherwise, this series looks good!

Reviewed-by: Sage Weil <sage@newdream.net>

> @@ -811,7 +812,7 @@ retry:
> 
>  	if (!ci->i_xattrs.prealloc_blob ||
>  	    required_blob_size > ci->i_xattrs.prealloc_blob->alloc_len) {
> -		struct ceph_buffer *blob = NULL;
> +		struct ceph_buffer *blob;
> 
>  		spin_unlock(&ci->i_ceph_lock);
>  		dout(" preaallocating new blob size=%d\n",
> required_blob_size);
> @@ -825,12 +826,13 @@ retry:
>  		goto retry;
>  	}
> 
> -	dout("setxattr %p issued %s\n", inode, ceph_cap_string(issued));
>  	err = __set_xattr(ci, newname, name_len, newval,
>  			  val_len, 1, 1, 1, &xattr);
> +
>  	dirty = __ceph_mark_dirty_caps(ci, CEPH_CAP_XATTR_EXCL);
>  	ci->i_xattrs.dirty = true;
>  	inode->i_ctime = CURRENT_TIME;
> +
>  	spin_unlock(&ci->i_ceph_lock);
>  	if (dirty)
>  		__mark_inode_dirty(inode, dirty);
> @@ -892,18 +894,17 @@ int ceph_removexattr(struct dentry *dentry, const char
> *name)
>  		return -EOPNOTSUPP;
> 
>  	spin_lock(&ci->i_ceph_lock);
> -	__build_xattrs(inode);
>  	issued = __ceph_caps_issued(ci, NULL);
>  	dout("removexattr %p issued %s\n", inode, ceph_cap_string(issued));
> -
>  	if (!(issued & CEPH_CAP_XATTR_EXCL))
>  		goto do_sync;
> +	__build_xattrs(inode);
> 
>  	err = __remove_xattr_by_name(ceph_inode(inode), name);
> +
>  	dirty = __ceph_mark_dirty_caps(ci, CEPH_CAP_XATTR_EXCL);
>  	ci->i_xattrs.dirty = true;
>  	inode->i_ctime = CURRENT_TIME;
> -
>  	spin_unlock(&ci->i_ceph_lock);
>  	if (dirty)
>  		__mark_inode_dirty(inode, dirty);
> -- 
> 1.7.5.4
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 

Re: [PATCH 6/6] ceph: make ceph_setxattr() and ceph_removexattr() more alike

[Alex Elder]

On 03/02/2012 11:35 AM, Sage Weil wrote:
> On Tue, 28 Feb 2012, Alex Elder wrote:
>
>> This patch just rearranges a few bits of code to make more
>> portions of ceph_setxattr() and ceph_removexattr() identical.
>>
>> Signed-off-by: Alex Elder<elder@dreamhost.com>
>> ---

. . .

>> @@ -803,6 +803,7 @@ int ceph_setxattr(struct dentry *dentry, const char *name,
>>   	spin_lock(&ci->i_ceph_lock);
>>   retry:
>
> This:
>
>>   	issued = __ceph_caps_issued(ci, NULL);
>> +	dout("setxattr %p issued %s\n", inode, ceph_cap_string(issued));
>>   	if (!(issued&  CEPH_CAP_XATTR_EXCL))
>>   		goto do_sync;
>
> needs to go after this:
>
>>   	__build_xattrs(inode);
>
> All of the __build_xattrs() calls should go before the cap check, because
> they drop i_ceph_lock.
>
> Can probably do that in a new patch, since it was broken for some of these
> before.

I've created this to (try to) document your observation:
     http://tracker.newdream.net/issues/2129

I am committing this patch as-is for now.

					-Alex

> Otherwise, this series looks good!
>
> Reviewed-by: Sage Weil<sage@newdream.net>
>
>> @@ -811,7 +812,7 @@ retry:
>>
>>   	if (!ci->i_xattrs.prealloc_blob ||
>>   	    required_blob_size>  ci->i_xattrs.prealloc_blob->alloc_len) {
>> -		struct ceph_buffer *blob = NULL;
>> +		struct ceph_buffer *blob;
>>
>>   		spin_unlock(&ci->i_ceph_lock);
>>   		dout(" preaallocating new blob size=%d\n",
>> required_blob_size);
>> @@ -825,12 +826,13 @@ retry:
>>   		goto retry;
>>   	}
>>
>> -	dout("setxattr %p issued %s\n", inode, ceph_cap_string(issued));
>>   	err = __set_xattr(ci, newname, name_len, newval,
>>   			  val_len, 1, 1, 1,&xattr);
>> +
>>   	dirty = __ceph_mark_dirty_caps(ci, CEPH_CAP_XATTR_EXCL);
>>   	ci->i_xattrs.dirty = true;
>>   	inode->i_ctime = CURRENT_TIME;
>> +
>>   	spin_unlock(&ci->i_ceph_lock);
>>   	if (dirty)
>>   		__mark_inode_dirty(inode, dirty);
>> @@ -892,18 +894,17 @@ int ceph_removexattr(struct dentry *dentry, const char
>> *name)
>>   		return -EOPNOTSUPP;
>>
>>   	spin_lock(&ci->i_ceph_lock);
>> -	__build_xattrs(inode);
>>   	issued = __ceph_caps_issued(ci, NULL);
>>   	dout("removexattr %p issued %s\n", inode, ceph_cap_string(issued));
>> -
>>   	if (!(issued&  CEPH_CAP_XATTR_EXCL))
>>   		goto do_sync;
>> +	__build_xattrs(inode);
>>
>>   	err = __remove_xattr_by_name(ceph_inode(inode), name);
>> +
>>   	dirty = __ceph_mark_dirty_caps(ci, CEPH_CAP_XATTR_EXCL);
>>   	ci->i_xattrs.dirty = true;
>>   	inode->i_ctime = CURRENT_TIME;
>> -
>>   	spin_unlock(&ci->i_ceph_lock);
>>   	if (dirty)
>>   		__mark_inode_dirty(inode, dirty);
>> --
>> 1.7.5.4
>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>>

Re: [PATCH 0/6] ceph: virtual extended attribute cleanup

[Christoph Hellwig]

On Tue, Feb 28, 2012 at 07:17:41PM -0800, Alex Elder wrote:
> This series cleans up some code involving ceph's virtual extended
> attributes.  Three of them define some simple macros are set up to
> help ensure the attributes are defined in a consistent way.  One
> makes the size of certain constant values get defined at startup
> time rather than repeatedly, and the remaining two are some very
> small changes made for clarity.

Is there any reason you can't use the generic_*xattr helpers that
parse attribute names and use the handler array in the superblock?

I'm still vaguely planning on getting all remaining filesystems
converted over to it.

Re: [PATCH 0/6] ceph: virtual extended attribute cleanup

[Alex Elder]

On 02/28/2012 08:20 PM, Christoph Hellwig wrote:
> On Tue, Feb 28, 2012 at 07:17:41PM -0800, Alex Elder wrote:
>> This series cleans up some code involving ceph's virtual extended
>> attributes.  Three of them define some simple macros are set up to
>> help ensure the attributes are defined in a consistent way.  One
>> makes the size of certain constant values get defined at startup
>> time rather than repeatedly, and the remaining two are some very
>> small changes made for clarity.
>
> Is there any reason you can't use the generic_*xattr helpers that
> parse attribute names and use the handler array in the superblock?

We should.  And I started to look at that but decided to at least
start moving the code in a better, more regular direction.  I've
got it on a list but haven't gotten around to it.  Lots of this
stuff is just the result of taking my first walk through some
of this code.

> I'm still vaguely planning on getting all remaining filesystems
> converted over to it.

OK.  I'll look into converting this stuff soon.

					-Alex