On Wed, Sep 24, 2008 at 03:00:54PM -0700, Mark Fasheh wrote:
> xattr.o \
> + xattr_user.o \
> + xattr_trusted.o
Please don't split this up, it's always been a really stupid idea in
extN. The only difference between secure, trusted and user attrs is
that they go into a different namespace bit (and have different
permission checking, but that's handled in the VFS). I have some
upcoming patches to store a fs private flag in struct xattr_handler
so that even those flags wrappers can go away, and each of the
namespaces will just be five lines of code for the xattr_handler
declaration.
> +static inline struct xattr_handler *ocfs2_xattr_handler(int name_index)
> +{
> + struct xattr_handler *handler = NULL;
> +
> + if (name_index > 0 && name_index < OCFS2_XATTR_MAX)
> + handler = ocfs2_xattr_handler_map[name_index];
> +
> + return handler;
> +}
You seem to need the handler mostly for getting back to the prefix
from the handler. This is a pretty clear indicator that you don't
want to use the xattr_handler splitting but deal with the whole
attr name. Take a look at the btrfs code after my recent xattr changes
on how to handle this more nicely.
> +
> +static inline u32 ocfs2_xattr_name_hash(struct inode *inode,
> + char *prefix,
> + int prefix_len,
> + char *name,
> + int name_len)
And I think there's far too much inlining going on in here..
On Thu, Oct 02, 2008 at 10:16:44AM +0200, Christoph Hellwig wrote:
> On Wed, Sep 24, 2008 at 03:00:54PM -0700, Mark Fasheh wrote:
> > xattr.o \
> > + xattr_user.o \
> > + xattr_trusted.o
>
> Please don't split this up, it's always been a really stupid idea in
> extN. The only difference between secure, trusted and user attrs is
> that they go into a different namespace bit (and have different
> permission checking, but that's handled in the VFS). I have some
> upcoming patches to store a fs private flag in struct xattr_handler
> so that even those flags wrappers can go away, and each of the
> namespaces will just be five lines of code for the xattr_handler
> declaration.
Ok. The following patch (in ocfs2.git now) removes those two files, and puts
the code for user and trusted xattrs at the bottom of xattr.c. Is that
mainly what you were getting at here?
> > +static inline struct xattr_handler *ocfs2_xattr_handler(int name_index)
> > +{
> > + struct xattr_handler *handler = NULL;
> > +
> > + if (name_index > 0 && name_index < OCFS2_XATTR_MAX)
> > + handler = ocfs2_xattr_handler_map[name_index];
> > +
> > + return handler;
> > +}
>
> You seem to need the handler mostly for getting back to the prefix
> from the handler. This is a pretty clear indicator that you don't
> want to use the xattr_handler splitting but deal with the whole
> attr name. Take a look at the btrfs code after my recent xattr changes
> on how to handle this more nicely.
Tao, Can you look into this?
> > +
> > +static inline u32 ocfs2_xattr_name_hash(struct inode *inode,
> > + char *prefix,
> > + int prefix_len,
> > + char *name,
> > + int name_len)
>
> And I think there's far too much inlining going on in here..
Yep, I went ahead and un-inlined that function.
Thanks for the review,
--Mark
--
Mark Fasheh
From: Mark Fasheh <[email protected]>
ocfs2: Move trusted and user attribute support into xattr.c
Per Christoph Hellwig's suggestion - don't split these up. It's not like we
gained much by having the two tiny files around.
Signed-off-by: Mark Fasheh <[email protected]>
---
fs/ocfs2/Makefile | 4 +-
fs/ocfs2/xattr.c | 110 ++++++++++++++++++++++++++++++++++++++++++++++
fs/ocfs2/xattr_trusted.c | 82 ----------------------------------
fs/ocfs2/xattr_user.c | 94 ---------------------------------------
4 files changed, 111 insertions(+), 179 deletions(-)
delete mode 100644 fs/ocfs2/xattr_trusted.c
delete mode 100644 fs/ocfs2/xattr_user.c
diff --git a/fs/ocfs2/Makefile b/fs/ocfs2/Makefile
index 21323da..589dcdf 100644
--- a/fs/ocfs2/Makefile
+++ b/fs/ocfs2/Makefile
@@ -35,9 +35,7 @@ ocfs2-objs := \
sysfile.o \
uptodate.o \
ver.o \
- xattr.o \
- xattr_user.o \
- xattr_trusted.o
+ xattr.o
ocfs2_stackglue-objs := stackglue.o
ocfs2_stack_o2cb-objs := stack_o2cb.o
diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
index e21a1a8..0f556b0 100644
--- a/fs/ocfs2/xattr.c
+++ b/fs/ocfs2/xattr.c
@@ -37,6 +37,9 @@
#include <linux/writeback.h>
#include <linux/falloc.h>
#include <linux/sort.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/string.h>
#define MLOG_MASK_PREFIX ML_XATTR
#include <cluster/masklog.h>
@@ -4740,3 +4743,110 @@ static int ocfs2_delete_xattr_index_block(struct inode *inode,
out:
return ret;
}
+
+/*
+ * 'trusted' attributes support
+ */
+
+#define XATTR_TRUSTED_PREFIX "trusted."
+
+static size_t ocfs2_xattr_trusted_list(struct inode *inode, char *list,
+ size_t list_size, const char *name,
+ size_t name_len)
+{
+ const size_t prefix_len = sizeof(XATTR_TRUSTED_PREFIX) - 1;
+ const size_t total_len = prefix_len + name_len + 1;
+
+ if (list && total_len <= list_size) {
+ memcpy(list, XATTR_TRUSTED_PREFIX, prefix_len);
+ memcpy(list + prefix_len, name, name_len);
+ list[prefix_len + name_len] = '\0';
+ }
+ return total_len;
+}
+
+static int ocfs2_xattr_trusted_get(struct inode *inode, const char *name,
+ void *buffer, size_t size)
+{
+ if (strcmp(name, "") == 0)
+ return -EINVAL;
+ return ocfs2_xattr_get(inode, OCFS2_XATTR_INDEX_TRUSTED, name,
+ buffer, size);
+}
+
+static int ocfs2_xattr_trusted_set(struct inode *inode, const char *name,
+ const void *value, size_t size, int flags)
+{
+ if (strcmp(name, "") == 0)
+ return -EINVAL;
+
+ return ocfs2_xattr_set(inode, OCFS2_XATTR_INDEX_TRUSTED, name, value,
+ size, flags);
+}
+
+struct xattr_handler ocfs2_xattr_trusted_handler = {
+ .prefix = XATTR_TRUSTED_PREFIX,
+ .list = ocfs2_xattr_trusted_list,
+ .get = ocfs2_xattr_trusted_get,
+ .set = ocfs2_xattr_trusted_set,
+};
+
+
+/*
+ * 'user' attributes support
+ */
+
+#define XATTR_USER_PREFIX "user."
+
+static size_t ocfs2_xattr_user_list(struct inode *inode, char *list,
+ size_t list_size, const char *name,
+ size_t name_len)
+{
+ const size_t prefix_len = sizeof(XATTR_USER_PREFIX) - 1;
+ const size_t total_len = prefix_len + name_len + 1;
+ struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
+
+ if (osb->s_mount_opt & OCFS2_MOUNT_NOUSERXATTR)
+ return 0;
+
+ if (list && total_len <= list_size) {
+ memcpy(list, XATTR_USER_PREFIX, prefix_len);
+ memcpy(list + prefix_len, name, name_len);
+ list[prefix_len + name_len] = '\0';
+ }
+ return total_len;
+}
+
+static int ocfs2_xattr_user_get(struct inode *inode, const char *name,
+ void *buffer, size_t size)
+{
+ struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
+
+ if (strcmp(name, "") == 0)
+ return -EINVAL;
+ if (osb->s_mount_opt & OCFS2_MOUNT_NOUSERXATTR)
+ return -EOPNOTSUPP;
+ return ocfs2_xattr_get(inode, OCFS2_XATTR_INDEX_USER, name,
+ buffer, size);
+}
+
+static int ocfs2_xattr_user_set(struct inode *inode, const char *name,
+ const void *value, size_t size, int flags)
+{
+ struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
+
+ if (strcmp(name, "") == 0)
+ return -EINVAL;
+ if (osb->s_mount_opt & OCFS2_MOUNT_NOUSERXATTR)
+ return -EOPNOTSUPP;
+
+ return ocfs2_xattr_set(inode, OCFS2_XATTR_INDEX_USER, name, value,
+ size, flags);
+}
+
+struct xattr_handler ocfs2_xattr_user_handler = {
+ .prefix = XATTR_USER_PREFIX,
+ .list = ocfs2_xattr_user_list,
+ .get = ocfs2_xattr_user_get,
+ .set = ocfs2_xattr_user_set,
+};
diff --git a/fs/ocfs2/xattr_trusted.c b/fs/ocfs2/xattr_trusted.c
deleted file mode 100644
index 4c589c4..0000000
--- a/fs/ocfs2/xattr_trusted.c
+++ /dev/null
@@ -1,82 +0,0 @@
-/* -*- mode: c; c-basic-offset: 8; -*-
- * vim: noexpandtab sw=8 ts=8 sts=0:
- *
- * xattr_trusted.c
- *
- * Copyright (C) 2008 Oracle. All rights reserved.
- *
- * CREDITS:
- * Lots of code in this file is taken from ext3.
- *
- * This program is free software; you can redistribute it and/or
- * modify it under the terms of the GNU General Public
- * License as published by the Free Software Foundation; either
- * version 2 of the License, or (at your option) any later version.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
- * General Public License for more details.
- *
- * You should have received a copy of the GNU General Public
- * License along with this program; if not, write to the
- * Free Software Foundation, Inc., 59 Temple Place - Suite 330,
- * Boston, MA 021110-1307, USA.
- */
-
-#include <linux/init.h>
-#include <linux/module.h>
-#include <linux/string.h>
-
-#define MLOG_MASK_PREFIX ML_INODE
-#include <cluster/masklog.h>
-
-#include "ocfs2.h"
-#include "alloc.h"
-#include "dlmglue.h"
-#include "file.h"
-#include "ocfs2_fs.h"
-#include "xattr.h"
-
-#define XATTR_TRUSTED_PREFIX "trusted."
-
-static size_t ocfs2_xattr_trusted_list(struct inode *inode, char *list,
- size_t list_size, const char *name,
- size_t name_len)
-{
- const size_t prefix_len = sizeof(XATTR_TRUSTED_PREFIX) - 1;
- const size_t total_len = prefix_len + name_len + 1;
-
- if (list && total_len <= list_size) {
- memcpy(list, XATTR_TRUSTED_PREFIX, prefix_len);
- memcpy(list + prefix_len, name, name_len);
- list[prefix_len + name_len] = '\0';
- }
- return total_len;
-}
-
-static int ocfs2_xattr_trusted_get(struct inode *inode, const char *name,
- void *buffer, size_t size)
-{
- if (strcmp(name, "") == 0)
- return -EINVAL;
- return ocfs2_xattr_get(inode, OCFS2_XATTR_INDEX_TRUSTED, name,
- buffer, size);
-}
-
-static int ocfs2_xattr_trusted_set(struct inode *inode, const char *name,
- const void *value, size_t size, int flags)
-{
- if (strcmp(name, "") == 0)
- return -EINVAL;
-
- return ocfs2_xattr_set(inode, OCFS2_XATTR_INDEX_TRUSTED, name, value,
- size, flags);
-}
-
-struct xattr_handler ocfs2_xattr_trusted_handler = {
- .prefix = XATTR_TRUSTED_PREFIX,
- .list = ocfs2_xattr_trusted_list,
- .get = ocfs2_xattr_trusted_get,
- .set = ocfs2_xattr_trusted_set,
-};
diff --git a/fs/ocfs2/xattr_user.c b/fs/ocfs2/xattr_user.c
deleted file mode 100644
index 93ba716..0000000
--- a/fs/ocfs2/xattr_user.c
+++ /dev/null
@@ -1,94 +0,0 @@
-/* -*- mode: c; c-basic-offset: 8; -*-
- * vim: noexpandtab sw=8 ts=8 sts=0:
- *
- * xattr_user.c
- *
- * Copyright (C) 2008 Oracle. All rights reserved.
- *
- * CREDITS:
- * Lots of code in this file is taken from ext3.
- *
- * This program is free software; you can redistribute it and/or
- * modify it under the terms of the GNU General Public
- * License as published by the Free Software Foundation; either
- * version 2 of the License, or (at your option) any later version.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
- * General Public License for more details.
- *
- * You should have received a copy of the GNU General Public
- * License along with this program; if not, write to the
- * Free Software Foundation, Inc., 59 Temple Place - Suite 330,
- * Boston, MA 021110-1307, USA.
- */
-
-#include <linux/init.h>
-#include <linux/module.h>
-#include <linux/string.h>
-
-#define MLOG_MASK_PREFIX ML_INODE
-#include <cluster/masklog.h>
-
-#include "ocfs2.h"
-#include "alloc.h"
-#include "dlmglue.h"
-#include "file.h"
-#include "ocfs2_fs.h"
-#include "xattr.h"
-
-#define XATTR_USER_PREFIX "user."
-
-static size_t ocfs2_xattr_user_list(struct inode *inode, char *list,
- size_t list_size, const char *name,
- size_t name_len)
-{
- const size_t prefix_len = sizeof(XATTR_USER_PREFIX) - 1;
- const size_t total_len = prefix_len + name_len + 1;
- struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
-
- if (osb->s_mount_opt & OCFS2_MOUNT_NOUSERXATTR)
- return 0;
-
- if (list && total_len <= list_size) {
- memcpy(list, XATTR_USER_PREFIX, prefix_len);
- memcpy(list + prefix_len, name, name_len);
- list[prefix_len + name_len] = '\0';
- }
- return total_len;
-}
-
-static int ocfs2_xattr_user_get(struct inode *inode, const char *name,
- void *buffer, size_t size)
-{
- struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
-
- if (strcmp(name, "") == 0)
- return -EINVAL;
- if (osb->s_mount_opt & OCFS2_MOUNT_NOUSERXATTR)
- return -EOPNOTSUPP;
- return ocfs2_xattr_get(inode, OCFS2_XATTR_INDEX_USER, name,
- buffer, size);
-}
-
-static int ocfs2_xattr_user_set(struct inode *inode, const char *name,
- const void *value, size_t size, int flags)
-{
- struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
-
- if (strcmp(name, "") == 0)
- return -EINVAL;
- if (osb->s_mount_opt & OCFS2_MOUNT_NOUSERXATTR)
- return -EOPNOTSUPP;
-
- return ocfs2_xattr_set(inode, OCFS2_XATTR_INDEX_USER, name, value,
- size, flags);
-}
-
-struct xattr_handler ocfs2_xattr_user_handler = {
- .prefix = XATTR_USER_PREFIX,
- .list = ocfs2_xattr_user_list,
- .get = ocfs2_xattr_user_get,
- .set = ocfs2_xattr_user_set,
-};
--
1.5.4.1
Mark Fasheh wrote:
>>> +static inline struct xattr_handler *ocfs2_xattr_handler(int name_index)
>>> +{
>>> + struct xattr_handler *handler = NULL;
>>> +
>>> + if (name_index > 0 && name_index < OCFS2_XATTR_MAX)
>>> + handler = ocfs2_xattr_handler_map[name_index];
>>> +
>>> + return handler;
>>> +}
>> You seem to need the handler mostly for getting back to the prefix
>> from the handler. This is a pretty clear indicator that you don't
>> want to use the xattr_handler splitting but deal with the whole
>> attr name. Take a look at the btrfs code after my recent xattr changes
>> on how to handle this more nicely.
>
> Tao, Can you look into this?
I have looked the patch for btrfs about this. We are different.
Btrfs store the whole xattr name including the prefix "user."
"trusted.", we store index number instead of it.
regards,
tiger
On Wed, Oct 08, 2008 at 09:56:41AM +0800, Tiger Yang wrote:
> Mark Fasheh wrote:
> >>>+static inline struct xattr_handler *ocfs2_xattr_handler(int name_index)
> >>>+{
> >>>+ struct xattr_handler *handler = NULL;
> >>>+
> >>>+ if (name_index > 0 && name_index < OCFS2_XATTR_MAX)
> >>>+ handler = ocfs2_xattr_handler_map[name_index];
> >>>+
> >>>+ return handler;
> >>>+}
> >>You seem to need the handler mostly for getting back to the prefix
> >>from the handler. This is a pretty clear indicator that you don't
> >>want to use the xattr_handler splitting but deal with the whole
> >>attr name. Take a look at the btrfs code after my recent xattr changes
> >>on how to handle this more nicely.
> >
> >Tao, Can you look into this?
>
> I have looked the patch for btrfs about this. We are different.
> Btrfs store the whole xattr name including the prefix "user."
> "trusted.", we store index number instead of it.
In which case you shouldn't need to look the handler up anyway. I'll
re-review the code once you post the next version.
On Tue, Oct 07, 2008 at 03:08:11PM -0700, Mark Fasheh wrote:
> On Thu, Oct 02, 2008 at 10:16:44AM +0200, Christoph Hellwig wrote:
> > On Wed, Sep 24, 2008 at 03:00:54PM -0700, Mark Fasheh wrote:
> > > xattr.o \
> > > + xattr_user.o \
> > > + xattr_trusted.o
> >
> > Please don't split this up, it's always been a really stupid idea in
> > extN. The only difference between secure, trusted and user attrs is
> > that they go into a different namespace bit (and have different
> > permission checking, but that's handled in the VFS). I have some
> > upcoming patches to store a fs private flag in struct xattr_handler
> > so that even those flags wrappers can go away, and each of the
> > namespaces will just be five lines of code for the xattr_handler
> > declaration.
>
> Ok. The following patch (in ocfs2.git now) removes those two files, and puts
> the code for user and trusted xattrs at the bottom of xattr.c. Is that
> mainly what you were getting at here?
Yeah.
On Wed, Oct 08, 2008 at 09:56:41AM +0800, Tiger Yang wrote:
> I have looked the patch for btrfs about this. We are different.
> Btrfs store the whole xattr name including the prefix "user."
> "trusted.", we store index number instead of it.
I looked at the git tree and there are two users of
ocfs2_xattr_handler().
(1) for using the ->list handler in listattr. That's something I fixed
in btrfs that I wanted to point you to. The whole concept of a
->list handler is stupid, and it was only added as a hack for
the tmpfs "generic" xattr support which is a mess. Instead of
looking up a handler that would only do the same thing anyway
for all on-disk attributes just call the code directly and
have a map from index to prefix (look at
fs/xfs/linux-2.6/xfs_xattr.c for an example). You
also have a check for OCFS2_MOUNT_NOUSERXATTR for the user
attributes, but that's much easier done by just checking the
index in an if (and I'd personally just kill it completely, the
options doesn't seem useful - but that's an unrelated bit)
(2) For generating the hash. I don't quite understand why you want to
also hash the prefix if it's not store on disk anyway but sorted
into the numeric buckets.
Christoph Hellwig wrote:
> On Wed, Oct 08, 2008 at 09:56:41AM +0800, Tiger Yang wrote:
>> I have looked the patch for btrfs about this. We are different.
>> Btrfs store the whole xattr name including the prefix "user."
>> "trusted.", we store index number instead of it.
>
> I looked at the git tree and there are two users of
> ocfs2_xattr_handler().
>
> (1) for using the ->list handler in listattr. That's something I fixed
> in btrfs that I wanted to point you to. The whole concept of a
> ->list handler is stupid, and it was only added as a hack for
> the tmpfs "generic" xattr support which is a mess. Instead of
> looking up a handler that would only do the same thing anyway
> for all on-disk attributes just call the code directly and
> have a map from index to prefix (look at
> fs/xfs/linux-2.6/xfs_xattr.c for an example). You
> also have a check for OCFS2_MOUNT_NOUSERXATTR for the user
> attributes, but that's much easier done by just checking the
> index in an if (and I'd personally just kill it completely, the
> options doesn't seem useful - but that's an unrelated bit)
yes, you are right. The handler for list is borrowed from ext3 and
somewhat ugly. We just need the prefix name but use such a complicated
method. Just a map from index to prefix should work fine.
>
> (2) For generating the hash. I don't quite understand why you want to
> also hash the prefix if it's not store on disk anyway but sorted
> into the numeric buckets.
This is done intentionally. See the design doc
http://oss.oracle.com/osswiki/OCFS2/DesignDocs/ExtendedAttributes.
"Each entry has a 32-bit hash value associated with it. The hash value
is calculated using the full (prefix.suffix) name of the xattr to avoid
hash collisions when the same suffix is used in multiple attribute
namespaces. "
So Mark, do you think we need this prefix hash?
Anyway, if we make consensus that the hash calculation doesn't need
prefix any more, we can remove the ocfs2_xattr_handler safely.
Regards,
Tao
On Wed, Oct 08, 2008 at 10:04:40PM +0800, Tao Ma wrote:
>
>
> Christoph Hellwig wrote:
> > On Wed, Oct 08, 2008 at 09:56:41AM +0800, Tiger Yang wrote:
> >> I have looked the patch for btrfs about this. We are different.
> >> Btrfs store the whole xattr name including the prefix "user."
> >> "trusted.", we store index number instead of it.
> >
> > I looked at the git tree and there are two users of
> > ocfs2_xattr_handler().
> >
> > (1) for using the ->list handler in listattr. That's something I fixed
> > in btrfs that I wanted to point you to. The whole concept of a
> > ->list handler is stupid, and it was only added as a hack for
> > the tmpfs "generic" xattr support which is a mess. Instead of
> > looking up a handler that would only do the same thing anyway
> > for all on-disk attributes just call the code directly and
> > have a map from index to prefix (look at
> > fs/xfs/linux-2.6/xfs_xattr.c for an example). You
> > also have a check for OCFS2_MOUNT_NOUSERXATTR for the user
> > attributes, but that's much easier done by just checking the
> > index in an if (and I'd personally just kill it completely, the
> > options doesn't seem useful - but that's an unrelated bit)
> yes, you are right. The handler for list is borrowed from ext3 and
> somewhat ugly. We just need the prefix name but use such a complicated
> method. Just a map from index to prefix should work fine.
> >
> > (2) For generating the hash. I don't quite understand why you want to
> > also hash the prefix if it's not store on disk anyway but sorted
> > into the numeric buckets.
> This is done intentionally. See the design doc
> http://oss.oracle.com/osswiki/OCFS2/DesignDocs/ExtendedAttributes.
> "Each entry has a 32-bit hash value associated with it. The hash value
> is calculated using the full (prefix.suffix) name of the xattr to avoid
> hash collisions when the same suffix is used in multiple attribute
> namespaces. "
> So Mark, do you think we need this prefix hash?
> Anyway, if we make consensus that the hash calculation doesn't need
> prefix any more, we can remove the ocfs2_xattr_handler safely.
Removing the prefix hash should be fine. Technically, this changes the disk
format, but nobody should be using this for production yet anyway.
--Mark
--
Mark Fasheh