cgroup: add xattr support

PATCH In review Li Zefan 4 participants 11 messages Mar 1, 2012 – Mar 8, 2012

Subsystems (4)

Memory management VFS cgroup tmpfs (shmem filesystem)

lore

Share & tools

lore

Li Zefan

author

2012-03-01 06:14 UTC #0 lore raw

v2 cover cgroup: add xattr support

This can be used to attach metadata to a cgroup, which is required by systemd.

As lazy as I am, here just quoted from Lennart for the detailed use cases:

Quoted 17 lines

Quoted 1 line
What would the use case be for this?
Attaching meta information to services, in an easily discoverable way. For example, in systemd we create one cgroup for each service, and could then store data like the main pid of the specific service as an xattr on the cgroup itself. That way we'd have almost all service state in the cgroupfs, which would make it possible to terminate systemd and later restart it without losing any state information. But there's more:
for example, some very peculiar services cannot be terminated on shutdown (i.e. fakeraid DM stuff) and it would be really nice if the services in question could just mark that on their cgroup, by setting an xattr. On the more desktopy side of things there are other
possibilities: for example there are plans defining what an application is along the lines of a cgroup (i.e. an app being a collection of processes). With xattrs one could then attach an icon or human readable program name on the cgroup.

Quoted 8 lines

The key idea is that this would allow attaching runtime meta information to cgroups and everything they model (services, apps, vms), that doesn't need any complex userspace infrastructure, has good access control (i.e. because the file system enforces that anyway, and there's the "trusted." xattr namespace), notifications (inotify), and can easily be shared among applications.
Lennart

Changelog v1->v2:
- add kmem_xattr APIs
- use these new APIs in both tmpfs and cgroup

Early discussions can be found in this thread:

https://lkml.org/lkml/2012/1/16/51

Note: this patchset is based on Tejun's cgroup.git/for-next

 fs/xattr.c               |  167 +++++++++++++++++++++++++++++++++++++++++
 include/linux/cgroup.h   |   18 +++-
 include/linux/shmem_fs.h |    3 +-
 include/linux/xattr.h    |   23 ++++++
 init/Kconfig             |   12 +++
 kernel/cgroup.c          |  184 ++++++++++++++++++++++++++++++++++++----------
 mm/shmem.c               |  151 +++-----------------------------------
 7 files changed, 371 insertions(+), 187 deletions(-)

Li Zefan

author

2012-03-01 06:14 UTC #1 lore raw

PATCH v2 1/3 xattr: extract kmem_xattr code from tmpfs

Extract in-memory xattr APIs from tmpfs. Will be used by cgroup.

$ size vmlinux.o
   text    data     bss     dec     hex filename
5480650  655744 7039512 13175906         c90c62 vmlinux.o
$ size vmlinux.o
   text    data     bss     dec     hex filename
5480427  655744 7039448 13175619         c90b43 vmlinux.o

Signed-off-by Li Zefan <[email protected]>

---
 fs/xattr.c               |  167 ++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/shmem_fs.h |    3 +-
 include/linux/xattr.h    |   23 ++++++
 mm/shmem.c               |  151 +++--------------------------------------
 4 files changed, 202 insertions(+), 142 deletions(-)

fs/xattr.c +167 -0

index 82f4337..8af1b7f 100644
@@ -757,3 +757,170 @@ EXPORT_SYMBOL(generic_getxattr);
 EXPORT_SYMBOL(generic_listxattr);
 EXPORT_SYMBOL(generic_setxattr);
 EXPORT_SYMBOL(generic_removexattr);
+
+/*
+ * initialize the kmem_xattrs structure
+ */
+void kmem_xattrs_init(struct kmem_xattrs *xattrs)
+{
+	INIT_LIST_HEAD(&xattrs->head);
+	spin_lock_init(&xattrs->lock);
+}
+
+/*
+ * free all the xattrs
+ */
+void kmem_xattrs_free(struct kmem_xattrs *xattrs)
+{
+	struct kmem_xattr *xattr, *node;
+
+	spin_lock(&xattrs->lock);
+	list_for_each_entry_safe(xattr, node, &xattrs->head, list) {
+		kfree(xattr->name);
+		kfree(xattr);
+	}
+	spin_unlock(&xattrs->lock);
+}
+
+/*
+ * xattr GET operation for in-memory/pseudo filesystems
+ */
+int kmem_xattr_get(struct kmem_xattrs *xattrs, const char *name,
+		   void *buffer, size_t size)
+{
+	struct kmem_xattr *xattr;
+	int ret = -ENODATA;
+
+	spin_lock(&xattrs->lock);
+	list_for_each_entry(xattr, &xattrs->head, list) {
+		if (strcmp(name, xattr->name))
+			continue;
+
+		ret = xattr->size;
+		if (buffer) {
+			if (size < xattr->size)
+				ret = -ERANGE;
+			else
+				memcpy(buffer, xattr->value, xattr->size);
+		}
+		break;
+	}
+	spin_unlock(&xattrs->lock);
+	return ret;
+}
+
+static int __kmem_xattr_set(struct kmem_xattrs *xattrs, const char *name,
+			    const void *value, size_t size, int flags)
+{
+	struct kmem_xattr *xattr;
+	struct kmem_xattr *new_xattr = NULL;
+	size_t len;
+	int err = 0;
+
+	/* value == NULL means remove */
+	if (value) {
+		/* wrap around? */
+		len = sizeof(*new_xattr) + size;
+		if (len <= sizeof(*new_xattr))
+			return -ENOMEM;
+
+		new_xattr = kmalloc(len, GFP_KERNEL);
+		if (!new_xattr)
+			return -ENOMEM;
+
+		new_xattr->name = kstrdup(name, GFP_KERNEL);
+		if (!new_xattr->name) {
+			kfree(new_xattr);
+			return -ENOMEM;
+		}
+
+		new_xattr->size = size;
+		memcpy(new_xattr->value, value, size);
+	}
+
+	spin_lock(&xattrs->lock);
+	list_for_each_entry(xattr, &xattrs->head, list) {
+		if (!strcmp(name, xattr->name)) {
+			if (flags & XATTR_CREATE) {
+				xattr = new_xattr;
+				err = -EEXIST;
+			} else if (new_xattr) {
+				list_replace(&xattr->list, &new_xattr->list);
+			} else {
+				list_del(&xattr->list);
+			}
+			goto out;
+		}
+	}
+	if (flags & XATTR_REPLACE) {
+		xattr = new_xattr;
+		err = -ENODATA;
+	} else {
+		list_add(&new_xattr->list, &xattrs->head);
+		xattr = NULL;
+	}
+out:
+	spin_unlock(&xattrs->lock);
+	if (xattr)
+		kfree(xattr->name);
+	kfree(xattr);
+	return err;
+
+}
+
+/*
+ * xattr SET operation for in-memory/pseudo filesystems
+ */
+int kmem_xattr_set(struct kmem_xattrs *xattrs, const char *name,
+		   const void *value, size_t size, int flags)
+{
+	if (size == 0)
+		value = ""; /* empty EA, do not remove */
+	return __kmem_xattr_set(xattrs, name, value, size, flags);
+}
+
+/*
+ * xattr REMOVE operation for in-memory/pseudo filesystems
+ */
+int kmem_xattr_remove(struct kmem_xattrs *xattrs, const char *name)
+{
+        return __kmem_xattr_set(xattrs, name, NULL, 0, XATTR_REPLACE);
+}
+
+static bool xattr_is_trusted(const char *name)
+{
+	return !strncmp(name, XATTR_TRUSTED_PREFIX, XATTR_TRUSTED_PREFIX_LEN);
+}
+
+/*
+ * xattr LIST operation for in-memory/pseudo filesystems
+ */
+ssize_t kmem_xattr_list(struct kmem_xattrs *xattrs, char *buffer, size_t size)
+{
+	bool trusted = capable(CAP_SYS_ADMIN);
+	struct kmem_xattr *xattr;
+	size_t used = 0;
+
+	spin_lock(&xattrs->lock);
+	list_for_each_entry(xattr, &xattrs->head, list) {
+		size_t len;
+
+		/* skip "trusted." attributes for unprivileged callers */
+		if (!trusted && xattr_is_trusted(xattr->name))
+			continue;
+
+		len = strlen(xattr->name) + 1;
+		used += len;
+		if (buffer) {
+			if (size < used) {
+				used = -ERANGE;
+				break;
+			}
+			memcpy(buffer, xattr->name, len);
+			buffer += len;
+		}
+	}
+	spin_unlock(&xattrs->lock);
+
+	return used;
+}

include/linux/shmem_fs.h +2 -1

index e4c711c..6d006b6 100644
@@ -5,6 +5,7 @@
 #include <linux/mempolicy.h>
 #include <linux/pagemap.h>
 #include <linux/percpu_counter.h>
+#include <linux/xattr.h>
 
 /* inode in-kernel data */

@@ -18,7 +19,7 @@ struct shmem_inode_info {
 	};
 	struct shared_policy	policy;		/* NUMA memory alloc policy */
 	struct list_head	swaplist;	/* chain of maybes on swap */
-	struct list_head	xattr_list;	/* list of shmem_xattr */
+	struct kmem_xattrs	xattrs;		/* list of xattrs */
 	struct inode		vfs_inode;
 };

include/linux/xattr.h +23 -0

index e5d1220..4d07f97 100644
@@ -60,6 +60,7 @@
 #ifdef  __KERNEL__
 
 #include <linux/types.h>
+#include <linux/spinlock.h>
 
 struct inode;
 struct dentry;

@@ -96,6 +97,28 @@ ssize_t vfs_getxattr_alloc(struct dentry *dentry, const char *name,
 			   char **xattr_value, size_t size, gfp_t flags);
 int vfs_xattr_cmp(struct dentry *dentry, const char *xattr_name,
 		  const char *value, size_t size, gfp_t flags);
+
+struct kmem_xattrs {
+	struct list_head head;
+	spinlock_t lock;
+};
+
+struct kmem_xattr {
+	struct list_head list;
+	char *name;
+	size_t size;
+	char value[0];
+};
+
+void kmem_xattrs_init(struct kmem_xattrs *xattrs);
+void kmem_xattrs_free(struct kmem_xattrs *xattrs);
+int kmem_xattr_get(struct kmem_xattrs *xattrs, const char *name,
+		   void *buffer, size_t size);
+int kmem_xattr_set(struct kmem_xattrs *xattrs, const char *name,
+		   const void *value, size_t size, int flags);
+int kmem_xattr_remove(struct kmem_xattrs *xattrs, const char *name);
+ssize_t kmem_xattr_list(struct kmem_xattrs *xattrs, char *buffer, size_t size);
+
 #endif  /*  __KERNEL__  */
 
 #endif	/* _LINUX_XATTR_H */

mm/shmem.c +10 ~~-141~~

218 lines not on this page · raw message

Signature · 1 line

1.7.3.1

Hillf Danton

2012-03-01 12:42 UTC #2 lore raw

Re: [PATCH v2 1/3] xattr: extract kmem_xattr code from tmpfs

fs/xattr.c EXPORT_SYMBOL(generic_getxattr); · line 782

Quoted 13 lines · Li Zefan

fs/xattr.c +13 -0

+/*
+ * free all the xattrs
+ */
+void kmem_xattrs_free(struct kmem_xattrs *xattrs)
+{
+       struct kmem_xattr *xattr, *node;
+
+       spin_lock(&xattrs->lock);
+       list_for_each_entry_safe(xattr, node, &xattrs->head, list) {
+               kfree(xattr->name);
+               kfree(xattr);
+       }
+       spin_unlock(&xattrs->lock);

In your work it is a library function, operations on &xattrs->head are no longer allowed, say list_empty(), after it called, though I dunno if such operations exist.

And kmem_xattrs is too close to kmem_xattr, kmem_xattr_list is not pron of typo.

-hd

Quoted 2 lines

(quoted diff) +2 -0
+}
+

[...]

Quoted 12 lines

mm/shmem.c +1 -4

@@ -559,10 +551,7 @@ static void shmem_evict_inode(struct inode *inode)
        } else
                kfree(info->symlink);
 
-       list_for_each_entry_safe(xattr, nxattr, &info->xattr_list, list) {
-               kfree(xattr->name);
-               kfree(xattr);
-       }
+       kmem_xattrs_free(&info->xattrs);
        BUG_ON(inode->i_blocks);
        shmem_free_inode(inode->i_sb);
        end_writeback(inode);

Li Zefan

author

2012-03-02 05:20 UTC #3 lore raw

Re: [PATCH v2 1/3] xattr: extract kmem_xattr code from tmpfs

Quoted 10 lines · Hillf Danton

Quoted 9 lines · Li Zefan

fs/xattr.c +9 -0

+/*
+ * free all the xattrs
+ */
+void kmem_xattrs_free(struct kmem_xattrs *xattrs)
+{
+       struct kmem_xattr *xattr, *node;
+
+       spin_lock(&xattrs->lock);
+       list_for_each_entry_safe(xattr, node, &xattrs->head, list) {

Quoted 9 lines

fs/xattr.c EXPORT_SYMBOL(generic_getxattr); · line 782
Quoted 4 lines
fs/xattr.c +4 -0
+               kfree(xattr->name);
+               kfree(xattr);
+       }
+       spin_unlock(&xattrs->lock);
In your work it is a library function, operations on &xattrs->head are no longer allowed, say list_empty(), after it called, though I dunno if such operations exist.

I'd prefer to make it safer to use, so I'll re-init the list head after freeing all the list items.

Quoted 3 lines

And kmem_xattrs is too close to kmem_xattr, kmem_xattr_list is not pron of typo.

Then you won't pass compile. ;)

kmem_xattr_list exposed the data structure we used internally. We might change to use an rbtree.

Li Zefan

author

2012-03-01 06:15 UTC #4 lore raw

PATCH v2 2/3 cgroup: revise how we re-populate root directory

In reply to Li Zefan

When remounting cgroupfs with some subsystems added to it and some removed, cgroup will remove all the files in root directory and then re-popluate it.

What I'm doing here is, only remove files which belong to subsystems that are to be unbinded, and only create files for newly-added subsystems. The purpose is to have all other files untouched.

This is a preparation for cgroup xattr support.

Signed-off-by Li Zefan <[email protected]>

---
 include/linux/cgroup.h |   11 +++---
 kernel/cgroup.c        |   92 +++++++++++++++++++++++++++++++----------------
 2 files changed, 67 insertions(+), 36 deletions(-)

include/linux/cgroup.h +6 -5

index 501adb1..9b93c9a 100644
@@ -295,6 +295,9 @@ struct cftype {
 	 */
 	size_t max_write_len;
 
+	/* The subsystem this cgroup file belongs to */
+	struct cgroup_subsys *subsys;
+
 	int (*open)(struct inode *inode, struct file *file);
 	ssize_t (*read)(struct cgroup *cgrp, struct cftype *cft,
 			struct file *file,

@@ -387,16 +390,14 @@ struct cgroup_scanner {
  * called by subsystems from within a populate() method
  */
 int cgroup_add_file(struct cgroup *cgrp, struct cgroup_subsys *subsys,
-		       const struct cftype *cft);
+		    struct cftype *cft);
 
 /*
  * Add a set of new files to the given cgroup directory. Should
  * only be called by subsystems from within a populate() method
  */
-int cgroup_add_files(struct cgroup *cgrp,
-			struct cgroup_subsys *subsys,
-			const struct cftype cft[],
-			int count);
+int cgroup_add_files(struct cgroup *cgrp, struct cgroup_subsys *subsys,
+		     struct cftype cft[], int count);
 
 int cgroup_is_removed(const struct cgroup *cgrp);

kernel/cgroup.c +61 ~~-31~~

186 lines not on this page · raw message

Signature · 1 line

1.7.3.1

Hillf Danton

2012-03-01 12:48 UTC #5 lore raw

Re: [PATCH v2 2/3] cgroup: revise how we re-populate root directory

Quoted 22 lines · Li Zefan

kernel/cgroup.c +2 -1
-static void cgroup_clear_directory(struct dentry *dentry)
+static void cgroup_clear_directory(struct dentry *dentry, bool remove_all,
+                                  unsigned long removed_bits)
{
- struct list_head *node;
+ LIST_HEAD(head);
+ struct dentry *d, *node;
BUG_ON(!mutex_is_locked(&dentry->d_inode->i_mutex));
+
spin_lock(&dentry->d_lock);
- node = dentry->d_subdirs.next;
- while (node != &dentry->d_subdirs) {
- struct dentry *d = list_entry(node, struct dentry, d_u.d_child);
+ list_for_each_entry_safe(d, node, &dentry->d_subdirs, d_u.d_child) {
+ struct cftype *cft = d->d_fsdata;
+
+ if (!remove_all && cft->subsys &&
+ !test_bit(cft->subsys->subsys_id, &removed_bits))
+ continue;

Quoted 15 lines

spin_lock_nested(&d->d_lock, DENTRY_D_LOCK_NESTED);
- list_del_init(node);
+ list_move(&d->d_u.d_child, &head);
if (d->d_inode) {
/* This should never be called on a cgroup
* directory with child cgroups */ BUG_ON(d->d_inode->i_mode & S_IFDIR); dget_dlock(d);
- spin_unlock(&d->d_lock);
- spin_unlock(&dentry->d_lock);
+ }
+ spin_unlock(&d->d_lock);
+ }
+ spin_unlock(&dentry->d_lock);
+

Safe to process isolated entries without dentry->d_lock held?

-hd

Quoted 17 lines

+ list_for_each_entry_safe(d, node, &head, d_u.d_child) {
+ spin_lock(&d->d_lock);
+ list_del_init(&d->d_u.d_child);
+ spin_unlock(&d->d_lock);
+ if (d->d_inode) {
d_delete(d); simple_unlink(dentry->d_inode, d); dput(d);
- spin_lock(&dentry->d_lock);
- } else
- spin_unlock(&d->d_lock);
- node = dentry->d_subdirs.next;
+ }
}
- spin_unlock(&dentry->d_lock);
}

Li Zefan

author

2012-03-02 05:46 UTC #6 lore raw

Re: [PATCH v2 2/3] cgroup: revise how we re-populate root directory

Quoted 32 lines · Hillf Danton

Quoted 31 lines · Li Zefan

kernel/cgroup.c +12 -7

-static void cgroup_clear_directory(struct dentry *dentry)
+static void cgroup_clear_directory(struct dentry *dentry, bool remove_all,
+                                  unsigned long removed_bits)
 {
-       struct list_head *node;
+       LIST_HEAD(head);
+       struct dentry *d, *node;
 
       BUG_ON(!mutex_is_locked(&dentry->d_inode->i_mutex));
+
       spin_lock(&dentry->d_lock);
-       node = dentry->d_subdirs.next;
-       while (node != &dentry->d_subdirs) {
-               struct dentry *d = list_entry(node, struct dentry, d_u.d_child);
+       list_for_each_entry_safe(d, node, &dentry->d_subdirs, d_u.d_child) {
+               struct cftype *cft = d->d_fsdata;
+
+               if (!remove_all && cft->subsys &&
+                   !test_bit(cft->subsys->subsys_id, &removed_bits))
+                       continue;
 
               spin_lock_nested(&d->d_lock, DENTRY_D_LOCK_NESTED);
-               list_del_init(node);
+               list_move(&d->d_u.d_child, &head);
               if (d->d_inode) {
                       /* This should never be called on a cgroup
                        * directory with child cgroups */
                       BUG_ON(d->d_inode->i_mode & S_IFDIR);
                       dget_dlock(d);
-                       spin_unlock(&d->d_lock);

Quoted 9 lines

kernel/cgroup.c static void remove_dir(struct dentry *d) · line 914
Quoted 6 lines
kernel/cgroup.c +5 -1
-                       spin_unlock(&dentry->d_lock);
+               }
+               spin_unlock(&d->d_lock);
+       }
+       spin_unlock(&dentry->d_lock);
+
Safe to process isolated entries without dentry->d_lock held?

no vfs expert, I need to check into the vfs code.

Quoted 19 lines

-hd

Quoted 17 lines

kernel/cgroup.c +6 -5

+       list_for_each_entry_safe(d, node, &head, d_u.d_child) {
+               spin_lock(&d->d_lock);
+               list_del_init(&d->d_u.d_child);
+               spin_unlock(&d->d_lock);
+               if (d->d_inode) {
                       d_delete(d);
                       simple_unlink(dentry->d_inode, d);
                       dput(d);
-                       spin_lock(&dentry->d_lock);
-               } else
-                       spin_unlock(&d->d_lock);
-               node = dentry->d_subdirs.next;
+               }
       }
-       spin_unlock(&dentry->d_lock);
 }

Li Zefan

author

2012-03-01 06:14 UTC #7 lore raw

PATCH v2 3/3 cgroup: add xattr support

In reply to Li Zefan

This is one of the items in the plumber's wish list.

For use cases:

Quoted 17 lines

Quoted 1 line
What would the use case be for this?
Attaching meta information to services, in an easily discoverable way. For example, in systemd we create one cgroup for each service, and could then store data like the main pid of the specific service as an xattr on the cgroup itself. That way we'd have almost all service state in the cgroupfs, which would make it possible to terminate systemd and later restart it without losing any state information. But there's more:
for example, some very peculiar services cannot be terminated on shutdown (i.e. fakeraid DM stuff) and it would be really nice if the services in question could just mark that on their cgroup, by setting an xattr. On the more desktopy side of things there are other
possibilities: for example there are plans defining what an application is along the lines of a cgroup (i.e. an app being a collection of processes). With xattrs one could then attach an icon or human readable program name on the cgroup.

Quoted 8 lines

The key idea is that this would allow attaching runtime meta information to cgroups and everything they model (services, apps, vms), that doesn't need any complex userspace infrastructure, has good access control (i.e. because the file system enforces that anyway, and there's the "trusted." xattr namespace), notifications (inotify), and can easily be shared among applications.
Lennart

Signed-off-by Li Zefan <[email protected]>

---
 include/linux/cgroup.h |    7 ++++
 init/Kconfig           |   12 ++++++
 kernel/cgroup.c        |   92 +++++++++++++++++++++++++++++++++++++++++++-----
 3 files changed, 102 insertions(+), 9 deletions(-)

include/linux/cgroup.h +7 -0

index 9b93c9a..141c3ad 100644
@@ -16,6 +16,7 @@
 #include <linux/prio_heap.h>
 #include <linux/rwsem.h>
 #include <linux/idr.h>
+#include <linux/xattr.h>
 
 #ifdef CONFIG_CGROUPS

@@ -211,6 +212,9 @@ struct cgroup {
 	/* List of events which userspace want to receive */
 	struct list_head event_list;
 	spinlock_t event_list_lock;
+
+	/* directory xattrs */
+	struct kmem_xattrs xattrs;
 };
 
 /*

@@ -298,6 +302,9 @@ struct cftype {
 	/* The subsystem this cgroup file belongs to */
 	struct cgroup_subsys *subsys;
 
+	/* file xattrs */
+	struct kmem_xattrs xattrs;
+
 	int (*open)(struct inode *inode, struct file *file);
 	ssize_t (*read)(struct cgroup *cgrp, struct cftype *cft,
 			struct file *file,

init/Kconfig +12 -0

index 3f42cd6..ba9a9dc 100644
@@ -601,6 +601,18 @@ menuconfig CGROUPS
 
 if CGROUPS
 
+config CGROUP_XATTR
+	bool "Cgroup extended attributes"
+	default n
+	help
+	  Extended attributes are name:value pairs associated with inodes by
+	  the kernel or by users (see the attr(5) manual page, or visit
+	  <http://acl.bestbits.at/> for details).
+
+	  Currently the system.* namespace is not supported.
+
+	  If unsure, say N.
+
 config CGROUP_DEBUG
 	bool "Example debug cgroup subsystem"
 	default n

kernel/cgroup.c +83 -9

133 lines not on this page · raw message

Signature · 1 line

1.7.3.1

Tejun Heo

maintainer

2012-03-04 18:05 UTC #8 lore raw

In reply to Li Zefan

Hello, Li.

Sorry about the delay. Was on vacation.

Quoted 5 lines · Li Zefan

This can be used to attach metadata to a cgroup, which is required by systemd.
As lazy as I am, here just quoted from Lennart for the detailed use cases:

I much prefer this version but would prefer to deal with this in the next merge window as we're already on rc6. On a cursory look...

* I'm not sure kmem_ is a good prefix. It sucks that xattr values are stored in kmem w/o size limit and we probably want to improve on both fronts in the future. Wouldn't it be better to name them simple_*() and put them in fs/libfs.c?

* I don't think we want CONFIG option for cgroupfs xattr. Let's just add a mount option.

Thanks.

Signature · 1 line

tejun

Hugh Dickins

maintainer

2012-03-06 21:37 UTC #9 lore raw

Quoted 3 lines · Tejun Heo

Quoted 2 lines · Li Zefan
This can be used to attach metadata to a cgroup, which is required by systemd.

I've not reviewed your patches in detail, but I'm very happy for most of the shmem xattr code to move out to a library for use elsewhere.

Note that the linux-next mm/shmem.c now includes Jarkko Sakkinen's shmem_initxattrs() work, maybe you can draw from that too.

Quoted 11 lines

Quoted 3 lines
As lazy as I am, here just quoted from Lennart for the detailed use cases:
I much prefer this version but would prefer to deal with this in the next merge window as we're already on rc6. On a cursory look...
* I'm not sure kmem_ is a good prefix. It sucks that xattr values are stored in kmem w/o size limit and we probably want to improve on both fronts in the future. Wouldn't it be better to name them simple_*() and put them in fs/libfs.c?

Yes, simple_*() in fs/libfs.c sounds much better to me than kmem_*().

On the size limit issue, shmem has its own shmem_xattr_validate(), which rejects XATTR_USER_PREFIX, and the patches leave that unchanged.

I don't know whether the cgroup xattrs need to support USER or not, and I don't know what permissions would be involved if they do; but it's an important concern.

Hugh

Quoted 3 lines

* I don't think we want CONFIG option for cgroupfs xattr. Let's just add a mount option.

Li Zefan

author

2012-03-08 09:00 UTC #10 lore raw

In reply to Tejun Heo

Quoted 4 lines · Tejun Heo

Hello, Li.
Sorry about the delay. Was on vacation.

Me too. ;)

Quoted 10 lines · Li Zefan

Quoted 5 lines
This can be used to attach metadata to a cgroup, which is required by systemd.
As lazy as I am, here just quoted from Lennart for the detailed use cases:
I much prefer this version but would prefer to deal with this in the next merge window as we're already on rc6. On a cursory look...

I'm fine with this, especially I'm busy with other stuff.

Quoted 5 lines

* I'm not sure kmem_ is a good prefix. It sucks that xattr values are stored in kmem w/o size limit and we probably want to improve on both fronts in the future. Wouldn't it be better to name them simple_*() and put them in fs/libfs.c?

Ok.

Quoted 2 lines

* I don't think we want CONFIG option for cgroupfs xattr. Let's just add a mount option.

Ok.

Quoted 3 lines

Thanks.