ceph: virtual extended attribute cleanup

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.

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 <[email protected]>

---

 fs/ceph/super.c |    3 ++
 fs/ceph/super.h |    2 +
 fs/ceph/xattr.c |   56
++++++++++++++++++++++++++++++++++++++++++++++++++----
 3 files changed, 56 insertions(+), 5 deletions(-)

fs/ceph/super.c +1 -0

index b48f15f..e6325f0 100644
@@ -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;

fs/ceph/super.c +1 -0

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

fs/ceph/super.h +2 -0

index 5f00e82..4be2dc4 100644
@@ -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);

fs/ceph/xattr.c +1 -0

index 83366f9..984c863 100644
@@ -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;

fs/ceph/xattr.c +0 -0

                        .getxattr_cb = ceph_vxattrcb_ ## _type ## _ ## _name,
 \

                       .readonly = true, \
               }

fs/ceph/xattr.c +0 -0

@@ -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 */

fs/ceph/xattr.c +0 -0

                .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) {

fs/ceph/xattr.c +40 -0

@@ -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)

 {

fs/ceph/xattr.c +0 -0

@@ -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 [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Quoted 6 lines · 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.

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.

Quoted 10 lines · 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.

Quoted 7 lines

Signed-off-by Alex Elder<[email protected]>

---
  fs/ceph/super.c |    3 ++
  fs/ceph/super.h |    2 +
  fs/ceph/xattr.c |   56
++++++++++++++++++++++++++++++++++++++++++++++++++----
  3 files changed, 56 insertions(+), 5 deletions(-)

fs/ceph/xattr.c static size_t ceph_vxattrcb_dir_rctime(struct · line 92

Quoted 6 lines

fs/ceph/xattr.c +1 -0

@@ -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.

Quoted 14 lines

(quoted diff) +0 -0

                        .getxattr_cb = ceph_vxattrcb_ ## _type ## _ ## _name,
 \

                        .readonly = true, \
                }

fs/ceph/xattr.c +1 -0

@@ -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 */

fs/ceph/xattr.c static struct ceph_vxattr ceph_file_vxattrs[] = { · line 133

Quoted 4 lines

fs/ceph/xattr.c +1 -0

@@ -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.

Quoted 84 lines

fs/ceph/xattr.c +1 -0

                .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)

  {

fs/ceph/xattr.c +40 -0

@@ -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)

  {

fs/ceph/xattr.c +6 -5

@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

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 <[email protected]>

---
 fs/ceph/xattr.c |   15 ++++++++-------
 1 files changed, 8 insertions(+), 7 deletions(-)

fs/ceph/xattr.c +2 -2

index 984c863..3fccd3b 100644
@@ -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)

fs/ceph/xattr.c +0 -0

 		return -EROFS;

fs/ceph/xattr.c +1 -1

@@ -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);

fs/ceph/xattr.c +4 -4

@@ -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);

Signature · 7 lines

1.7.5.4


--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Quoted 5 lines · 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<[email protected]>

---

fs/ceph/xattr.c int ceph_setxattr(struct dentry *dentry, const char *name, · line 804

Quoted 3 lines

fs/ceph/xattr.c +0 -0

@@ -803,6 +803,7 @@ int ceph_setxattr(struct dentry *dentry, const char *name,
  	spin_lock(&ci->i_ceph_lock);
  retry:

This:

fs/ceph/xattr.c int ceph_setxattr(struct dentry *dentry, const char *name, · line 808

Quoted 4 lines

fs/ceph/xattr.c +1 -0

  	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:

fs/ceph/xattr.c int ceph_setxattr(struct dentry *dentry, const char *name, · line 809

Quoted 1 line

fs/ceph/xattr.c +0 -0

  	__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<[email protected]>

Quoted 57 lines

fs/ceph/xattr.c +1 -1

@@ -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);

fs/ceph/xattr.c +4 -4

@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html