From: Li Zefan <[email protected]>
Extract in-memory xattr APIs from tmpfs. Will be used by cgroup.
$ size vmlinux.o
text data bss dec hex filename
4658782 880729 5195032 10734543 a3cbcf vmlinux.o
$ size vmlinux.o
text data bss dec hex filename
4658957 880729 5195032 10734718 a3cc7e vmlinux.o
v6:
- no changes
v5:
- no changes
v4:
- move simple_xattrs_free() to fs/xattr.c
v3:
- in kmem_xattrs_free(), reinitialize the list
- use simple_xattr_* prefix
- introduce simple_xattr_add() to prevent direct list usage
Cc: Li Zefan <[email protected]>
Cc: Tejun Heo <[email protected]>
Cc: Hugh Dickins <[email protected]>
Cc: Hillf Danton <[email protected]>
Cc: Lennart Poettering <[email protected]>
Signed-off-by: Li Zefan <[email protected]>
Signed-off-by: Aristeu Rozanski <[email protected]>
---
fs/xattr.c | 200 +++++++++++++++++++++++++++++++++++++++++++++++
include/linux/shmem_fs.h | 3
include/linux/xattr.h | 25 +++++
mm/shmem.c | 171 +++-------------------------------------
4 files changed, 240 insertions(+), 159 deletions(-)
Index: github/fs/xattr.c
===================================================================
--- github.orig/fs/xattr.c 2012-08-16 11:28:12.719273435 -0400
+++ github/fs/xattr.c 2012-08-16 11:28:13.975307743 -0400
@@ -791,3 +791,203 @@
EXPORT_SYMBOL(generic_listxattr);
EXPORT_SYMBOL(generic_setxattr);
EXPORT_SYMBOL(generic_removexattr);
+
+/*
+ * initialize the simple_xattrs structure
+ */
+void simple_xattrs_init(struct simple_xattrs *xattrs)
+{
+ INIT_LIST_HEAD(&xattrs->head);
+ spin_lock_init(&xattrs->lock);
+}
+
+/*
+ * Allocate new xattr and copy in the value; but leave the name to callers.
+ */
+struct simple_xattr *simple_xattr_alloc(const void *value, size_t size)
+{
+ struct simple_xattr *new_xattr;
+ size_t len;
+
+ /* wrap around? */
+ len = sizeof(*new_xattr) + size;
+ if (len <= sizeof(*new_xattr))
+ return NULL;
+
+ new_xattr = kmalloc(len, GFP_KERNEL);
+ if (!new_xattr)
+ return NULL;
+
+ new_xattr->size = size;
+ memcpy(new_xattr->value, value, size);
+ return new_xattr;
+}
+
+/*
+ * free all the xattrs
+ */
+void simple_xattrs_free(struct simple_xattrs *xattrs)
+{
+ struct simple_xattr *xattr, *node;
+
+ spin_lock(&xattrs->lock);
+ list_for_each_entry_safe(xattr, node, &xattrs->head, list) {
+ kfree(xattr->name);
+ kfree(xattr);
+ }
+ INIT_LIST_HEAD(&xattrs->head);
+ spin_unlock(&xattrs->lock);
+}
+
+/*
+ * xattr GET operation for in-memory/pseudo filesystems
+ */
+int simple_xattr_get(struct simple_xattrs *xattrs, const char *name,
+ void *buffer, size_t size)
+{
+ struct simple_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 __simple_xattr_set(struct simple_xattrs *xattrs, const char *name,
+ const void *value, size_t size, int flags)
+{
+ struct simple_xattr *xattr;
+ struct simple_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 simple_xattr_set(struct simple_xattrs *xattrs, const char *name,
+ const void *value, size_t size, int flags)
+{
+ if (size == 0)
+ value = ""; /* empty EA, do not remove */
+ return __simple_xattr_set(xattrs, name, value, size, flags);
+}
+
+/*
+ * xattr REMOVE operation for in-memory/pseudo filesystems
+ */
+int simple_xattr_remove(struct simple_xattrs *xattrs, const char *name)
+{
+ return __simple_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 simple_xattr_list(struct simple_xattrs *xattrs, char *buffer, size_t size)
+{
+ bool trusted = capable(CAP_SYS_ADMIN);
+ struct simple_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;
+}
+
+void simple_xattr_list_add(struct simple_xattrs *xattrs,
+ struct simple_xattr *new_xattr)
+{
+ spin_lock(&xattrs->lock);
+ list_add(&new_xattr->list, &xattrs->head);
+ spin_unlock(&xattrs->lock);
+}
+
Index: github/include/linux/shmem_fs.h
===================================================================
--- github.orig/include/linux/shmem_fs.h 2012-08-16 11:28:12.719273435 -0400
+++ github/include/linux/shmem_fs.h 2012-08-16 11:28:13.975307743 -0400
@@ -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 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 simple_xattrs xattrs; /* list of xattrs */
struct inode vfs_inode;
};
Index: github/include/linux/xattr.h
===================================================================
--- github.orig/include/linux/xattr.h 2012-08-16 11:28:12.719273435 -0400
+++ github/include/linux/xattr.h 2012-08-16 11:28:13.975307743 -0400
@@ -60,6 +60,7 @@
#ifdef __KERNEL__
#include <linux/types.h>
+#include <linux/spinlock.h>
struct inode;
struct dentry;
@@ -96,6 +97,30 @@
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 simple_xattrs {
+ struct list_head head;
+ spinlock_t lock;
+};
+
+struct simple_xattr {
+ struct list_head list;
+ char *name;
+ size_t size;
+ char value[0];
+};
+
+void simple_xattrs_init(struct simple_xattrs *xattrs);
+struct simple_xattr *simple_xattr_alloc(const void *value, size_t size);
+void simple_xattrs_free(struct simple_xattrs *xattrs);
+int simple_xattr_get(struct simple_xattrs *xattrs, const char *name,
+ void *buffer, size_t size);
+int simple_xattr_set(struct simple_xattrs *xattrs, const char *name,
+ const void *value, size_t size, int flags);
+int simple_xattr_remove(struct simple_xattrs *xattrs, const char *name);
+ssize_t simple_xattr_list(struct simple_xattrs *xattrs, char *buffer, size_t size);
+void simple_xattr_list_add(struct simple_xattrs *xattrs, struct simple_xattr *new_xattr);
+
#endif /* __KERNEL__ */
#endif /* _LINUX_XATTR_H */
Index: github/mm/shmem.c
===================================================================
--- github.orig/mm/shmem.c 2012-08-16 11:28:12.719273435 -0400
+++ github/mm/shmem.c 2012-08-16 11:28:13.975307743 -0400
@@ -77,13 +77,6 @@
/* Symlink up to this size is kmalloc'ed instead of using a swappable page */
#define SHORT_SYMLINK_LEN 128
-struct shmem_xattr {
- struct list_head list; /* anchored by shmem_inode_info->xattr_list */
- char *name; /* xattr name */
- size_t size;
- char value[0];
-};
-
/*
* shmem_fallocate and shmem_writepage communicate via inode->i_private
* (with i_mutex making sure that it has only one user at a time):
@@ -636,7 +629,6 @@
static void shmem_evict_inode(struct inode *inode)
{
struct shmem_inode_info *info = SHMEM_I(inode);
- struct shmem_xattr *xattr, *nxattr;
if (inode->i_mapping->a_ops == &shmem_aops) {
shmem_unacct_size(info->flags, inode->i_size);
@@ -650,10 +642,7 @@
} else
kfree(info->symlink);
- list_for_each_entry_safe(xattr, nxattr, &info->xattr_list, list) {
- kfree(xattr->name);
- kfree(xattr);
- }
+ simple_xattrs_free(&info->xattrs);
BUG_ON(inode->i_blocks);
shmem_free_inode(inode->i_sb);
clear_inode(inode);
@@ -1377,7 +1366,7 @@
spin_lock_init(&info->lock);
info->flags = flags & VM_NORESERVE;
INIT_LIST_HEAD(&info->swaplist);
- INIT_LIST_HEAD(&info->xattr_list);
+ simple_xattrs_init(&info->xattrs);
cache_no_acl(inode);
switch (mode & S_IFMT) {
@@ -2060,28 +2049,6 @@
*/
/*
- * Allocate new xattr and copy in the value; but leave the name to callers.
- */
-static struct shmem_xattr *shmem_xattr_alloc(const void *value, size_t size)
-{
- struct shmem_xattr *new_xattr;
- size_t len;
-
- /* wrap around? */
- len = sizeof(*new_xattr) + size;
- if (len <= sizeof(*new_xattr))
- return NULL;
-
- new_xattr = kmalloc(len, GFP_KERNEL);
- if (!new_xattr)
- return NULL;
-
- new_xattr->size = size;
- memcpy(new_xattr->value, value, size);
- return new_xattr;
-}
-
-/*
* Callback for security_inode_init_security() for acquiring xattrs.
*/
static int shmem_initxattrs(struct inode *inode,
@@ -2090,11 +2057,11 @@
{
struct shmem_inode_info *info = SHMEM_I(inode);
const struct xattr *xattr;
- struct shmem_xattr *new_xattr;
+ struct simple_xattr *new_xattr;
size_t len;
for (xattr = xattr_array; xattr->name != NULL; xattr++) {
- new_xattr = shmem_xattr_alloc(xattr->value, xattr->value_len);
+ new_xattr = simple_xattr_alloc(xattr->value, xattr->value_len);
if (!new_xattr)
return -ENOMEM;
@@ -2111,91 +2078,12 @@
memcpy(new_xattr->name + XATTR_SECURITY_PREFIX_LEN,
xattr->name, len);
- spin_lock(&info->lock);
- list_add(&new_xattr->list, &info->xattr_list);
- spin_unlock(&info->lock);
+ simple_xattr_list_add(&info->xattrs, new_xattr);
}
return 0;
}
-static int shmem_xattr_get(struct dentry *dentry, const char *name,
- void *buffer, size_t size)
-{
- struct shmem_inode_info *info;
- struct shmem_xattr *xattr;
- int ret = -ENODATA;
-
- info = SHMEM_I(dentry->d_inode);
-
- spin_lock(&info->lock);
- list_for_each_entry(xattr, &info->xattr_list, 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(&info->lock);
- return ret;
-}
-
-static int shmem_xattr_set(struct inode *inode, const char *name,
- const void *value, size_t size, int flags)
-{
- struct shmem_inode_info *info = SHMEM_I(inode);
- struct shmem_xattr *xattr;
- struct shmem_xattr *new_xattr = NULL;
- int err = 0;
-
- /* value == NULL means remove */
- if (value) {
- new_xattr = shmem_xattr_alloc(value, size);
- if (!new_xattr)
- return -ENOMEM;
-
- new_xattr->name = kstrdup(name, GFP_KERNEL);
- if (!new_xattr->name) {
- kfree(new_xattr);
- return -ENOMEM;
- }
- }
-
- spin_lock(&info->lock);
- list_for_each_entry(xattr, &info->xattr_list, 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, &info->xattr_list);
- xattr = NULL;
- }
-out:
- spin_unlock(&info->lock);
- if (xattr)
- kfree(xattr->name);
- kfree(xattr);
- return err;
-}
-
static const struct xattr_handler *shmem_xattr_handlers[] = {
#ifdef CONFIG_TMPFS_POSIX_ACL
&generic_acl_access_handler,
@@ -2226,6 +2114,7 @@
static ssize_t shmem_getxattr(struct dentry *dentry, const char *name,
void *buffer, size_t size)
{
+ struct shmem_inode_info *info = SHMEM_I(dentry->d_inode);
int err;
/*
@@ -2240,12 +2129,13 @@
if (err)
return err;
- return shmem_xattr_get(dentry, name, buffer, size);
+ return simple_xattr_get(&info->xattrs, name, buffer, size);
}
static int shmem_setxattr(struct dentry *dentry, const char *name,
const void *value, size_t size, int flags)
{
+ struct shmem_inode_info *info = SHMEM_I(dentry->d_inode);
int err;
/*
@@ -2260,15 +2150,12 @@
if (err)
return err;
- if (size == 0)
- value = ""; /* empty EA, do not remove */
-
- return shmem_xattr_set(dentry->d_inode, name, value, size, flags);
-
+ return simple_xattr_set(&info->xattrs, name, value, size, flags);
}
static int shmem_removexattr(struct dentry *dentry, const char *name)
{
+ struct shmem_inode_info *info = SHMEM_I(dentry->d_inode);
int err;
/*
@@ -2283,45 +2170,13 @@
if (err)
return err;
- return shmem_xattr_set(dentry->d_inode, name, NULL, 0, XATTR_REPLACE);
-}
-
-static bool xattr_is_trusted(const char *name)
-{
- return !strncmp(name, XATTR_TRUSTED_PREFIX, XATTR_TRUSTED_PREFIX_LEN);
+ return simple_xattr_remove(&info->xattrs, name);
}
static ssize_t shmem_listxattr(struct dentry *dentry, char *buffer, size_t size)
{
- bool trusted = capable(CAP_SYS_ADMIN);
- struct shmem_xattr *xattr;
- struct shmem_inode_info *info;
- size_t used = 0;
-
- info = SHMEM_I(dentry->d_inode);
-
- spin_lock(&info->lock);
- list_for_each_entry(xattr, &info->xattr_list, 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(&info->lock);
-
- return used;
+ struct shmem_inode_info *info = SHMEM_I(dentry->d_inode);
+ return simple_xattr_list(&info->xattrs, buffer, size);
}
#endif /* CONFIG_TMPFS_XATTR */
On Thu, Aug 16, 2012 at 01:44:54PM -0400, [email protected] wrote:
> From: Li Zefan <[email protected]>
>
> Extract in-memory xattr APIs from tmpfs. Will be used by cgroup.
>
> $ size vmlinux.o
> text data bss dec hex filename
> 4658782 880729 5195032 10734543 a3cbcf vmlinux.o
> $ size vmlinux.o
> text data bss dec hex filename
> 4658957 880729 5195032 10734718 a3cc7e vmlinux.o
>
> v6:
> - no changes
> v5:
> - no changes
> v4:
> - move simple_xattrs_free() to fs/xattr.c
> v3:
> - in kmem_xattrs_free(), reinitialize the list
> - use simple_xattr_* prefix
> - introduce simple_xattr_add() to prevent direct list usage
>
> Cc: Li Zefan <[email protected]>
> Cc: Tejun Heo <[email protected]>
> Cc: Hugh Dickins <[email protected]>
Hugh, can you please review and ack this one?
Thanks.
--
tejun
On Thu, 16 Aug 2012, Tejun Heo wrote:
> On Thu, Aug 16, 2012 at 01:44:54PM -0400, [email protected] wrote:
> > From: Li Zefan <[email protected]>
> >
> > Extract in-memory xattr APIs from tmpfs. Will be used by cgroup.
> >
> > $ size vmlinux.o
> > text data bss dec hex filename
> > 4658782 880729 5195032 10734543 a3cbcf vmlinux.o
> > $ size vmlinux.o
> > text data bss dec hex filename
> > 4658957 880729 5195032 10734718 a3cc7e vmlinux.o
> >
> > v6:
> > - no changes
> > v5:
> > - no changes
> > v4:
> > - move simple_xattrs_free() to fs/xattr.c
> > v3:
> > - in kmem_xattrs_free(), reinitialize the list
> > - use simple_xattr_* prefix
> > - introduce simple_xattr_add() to prevent direct list usage
> >
> > Cc: Li Zefan <[email protected]>
> > Cc: Tejun Heo <[email protected]>
> > Cc: Hugh Dickins <[email protected]>
>
> Hugh, can you please review and ack this one?
Yes, it looks nice to me. I might have preferred more as inlines in
the header file to lower the slight init/evict overhead, and I don't
see why __simple_xattr_set() isn't using simple_xattr_alloc() in the
same way that shmem_xattr_set() used shmem_xattr_alloc(). But none
of that matters:
Acked-by: Hugh Dickins <[email protected]>
On Mon, Aug 20, 2012 at 12:10:09AM -0700, Hugh Dickins wrote:
> Yes, it looks nice to me. I might have preferred more as inlines in
> the header file to lower the slight init/evict overhead, and I don't
> see why __simple_xattr_set() isn't using simple_xattr_alloc() in the
> same way that shmem_xattr_set() used shmem_xattr_alloc(). But none
> of that matters:
>
> Acked-by: Hugh Dickins <[email protected]>
I can submit additional patches to fix these. What functions you want
inlined?
On why __simple_xattr_set() is not using simple_xattr_alloc(), there's
no reason to be that way, I missed it.
Thanks for reviewing!
--
Aristeu
On Mon, 20 Aug 2012, Aristeu Rozanski wrote:
> On Mon, Aug 20, 2012 at 12:10:09AM -0700, Hugh Dickins wrote:
> > Yes, it looks nice to me. I might have preferred more as inlines in
> > the header file to lower the slight init/evict overhead, and I don't
> > see why __simple_xattr_set() isn't using simple_xattr_alloc() in the
> > same way that shmem_xattr_set() used shmem_xattr_alloc(). But none
> > of that matters:
> >
> > Acked-by: Hugh Dickins <[email protected]>
>
> I can submit additional patches to fix these. What functions you want
> inlined?
Oh, thank you. I was thinking that it's uncommon for tmpfs files to
have xattrs (and the same probably true of other filesystems), so it's
best to minimize xattrs impact on shared paths. If simple_xattrs_init()
and simple_xattrs_free() can be static inline functions in linux/xattr.h,
that would be nice.
Probably more important would be to remove spin_lock() and spin_unlock()
(and INIT_LIST_HEAD) from simple_xattrs_free() - those are unnecessary
in shmem_evict_inode(), and wouldn't they be unnecessary whenever
simple_xattrs_free() gets called?
Hugh
>
> On why __simple_xattr_set() is not using simple_xattr_alloc(), there's
> no reason to be that way, I missed it.
>
> Thanks for reviewing!
>
> --
> Aristeu
>
>
On Mon, Aug 20, 2012 at 09:47:15PM -0700, Hugh Dickins wrote:
> On Mon, 20 Aug 2012, Aristeu Rozanski wrote:
> > On Mon, Aug 20, 2012 at 12:10:09AM -0700, Hugh Dickins wrote:
> > > Yes, it looks nice to me. I might have preferred more as inlines in
> > > the header file to lower the slight init/evict overhead, and I don't
> > > see why __simple_xattr_set() isn't using simple_xattr_alloc() in the
> > > same way that shmem_xattr_set() used shmem_xattr_alloc(). But none
> > > of that matters:
> > >
> > > Acked-by: Hugh Dickins <[email protected]>
> >
> > I can submit additional patches to fix these. What functions you want
> > inlined?
>
> Oh, thank you. I was thinking that it's uncommon for tmpfs files to
> have xattrs (and the same probably true of other filesystems), so it's
> best to minimize xattrs impact on shared paths. If simple_xattrs_init()
> and simple_xattrs_free() can be static inline functions in linux/xattr.h,
> that would be nice.
ok, done. and since Tejun is waiting for those before integrating, I'll
merge them in the original patches and resubmit everything.
> Probably more important would be to remove spin_lock() and spin_unlock()
> (and INIT_LIST_HEAD) from simple_xattrs_free() - those are unnecessary
> in shmem_evict_inode(), and wouldn't they be unnecessary whenever
> simple_xattrs_free() gets called?
Removing INIT_LIST_HEAD() it's possible by actually unlinking each xattr
inside the loop before freeing them. still, it'll have to check if the list is
empty or not, which might end up being the same?
About the locking, I'm not sure, I'm investigating it.
--
Aristeu
On Wed, 22 Aug 2012, Aristeu Rozanski wrote:
> On Mon, Aug 20, 2012 at 09:47:15PM -0700, Hugh Dickins wrote:
> > On Mon, 20 Aug 2012, Aristeu Rozanski wrote:
> > > On Mon, Aug 20, 2012 at 12:10:09AM -0700, Hugh Dickins wrote:
> > > > Yes, it looks nice to me. I might have preferred more as inlines in
> > > > the header file to lower the slight init/evict overhead, and I don't
> > > > see why __simple_xattr_set() isn't using simple_xattr_alloc() in the
> > > > same way that shmem_xattr_set() used shmem_xattr_alloc(). But none
> > > > of that matters:
> > > >
> > > > Acked-by: Hugh Dickins <[email protected]>
> > >
> > > I can submit additional patches to fix these. What functions you want
> > > inlined?
> >
> > Oh, thank you. I was thinking that it's uncommon for tmpfs files to
> > have xattrs (and the same probably true of other filesystems), so it's
> > best to minimize xattrs impact on shared paths. If simple_xattrs_init()
> > and simple_xattrs_free() can be static inline functions in linux/xattr.h,
> > that would be nice.
>
> ok, done. and since Tejun is waiting for those before integrating, I'll
> merge them in the original patches and resubmit everything.
Thanks.
>
> > Probably more important would be to remove spin_lock() and spin_unlock()
> > (and INIT_LIST_HEAD) from simple_xattrs_free() - those are unnecessary
> > in shmem_evict_inode(), and wouldn't they be unnecessary whenever
> > simple_xattrs_free() gets called?
>
> Removing INIT_LIST_HEAD() it's possible by actually unlinking each xattr
> inside the loop before freeing them. still, it'll have to check if the list is
> empty or not, which might end up being the same?
>
> About the locking, I'm not sure, I'm investigating it.
I think we have a misunderstanding.
INIT_LIST_HEAD() is not expensive, I just meant to remove it because
I thought it unnecessary by that point.
Do you envisage anywhere that would call simple_xattrs_free() except
a filesystem's evict_inode()?
By that point, the inode is on its way out of the system: nothing
much (yes, I am being a bit vague there ;) can get to it any more,
there's no need to reinitialize the list head and there's no need for
locking, because nothing else can be playing with those xattrs now.
If I'm wrong, then the current shmem_evict_inode() is already wrong
not to be locking there.
Would renaming simple_xattrs_free() to simple_xattrs_evict() help?
Hugh
On Wed, Aug 22, 2012 at 01:25:06PM -0700, Hugh Dickins wrote:
> > > Probably more important would be to remove spin_lock() and spin_unlock()
> > > (and INIT_LIST_HEAD) from simple_xattrs_free() - those are unnecessary
> > > in shmem_evict_inode(), and wouldn't they be unnecessary whenever
> > > simple_xattrs_free() gets called?
> >
> > Removing INIT_LIST_HEAD() it's possible by actually unlinking each xattr
> > inside the loop before freeing them. still, it'll have to check if the list is
> > empty or not, which might end up being the same?
> >
> > About the locking, I'm not sure, I'm investigating it.
>
> I think we have a misunderstanding.
>
> INIT_LIST_HEAD() is not expensive, I just meant to remove it because
> I thought it unnecessary by that point.
ah, I see.
> Do you envisage anywhere that would call simple_xattrs_free() except
> a filesystem's evict_inode()?
cgroup does it differently and it's called in d_iput() path (see cgroup_diput),
because it needs to selectively remove files upon remount.
> By that point, the inode is on its way out of the system: nothing
> much (yes, I am being a bit vague there ;) can get to it any more,
> there's no need to reinitialize the list head and there's no need for
> locking, because nothing else can be playing with those xattrs now.
I agree with you. That's why I'm looking into it because I'm pretty sure
I removed it at some point in the past and decided to put it back after
investigating the easily reproducible oops. Sadly I managed to forget
the analisys I did at the time.
--
Aristeu