POSIX ACL kernel infrastructure

RFC Andreas Gruenbacher 3 participants 12 messages Aug 4, 2002 – Aug 9, 2002 VFS

Share & tools

Andreas Gruenbacher

author

2002-08-04 13:46 UTC #0 lore raw

Hello,

I want to propose adding some infrastructure to the VFS for POSIX ACL ssupport. There are already implementations for XFS and Ext2/Ext3; ReiserFS is to follow soon.

The design is as follows: POSIX ACLs are passed between the kernel and user space as Extended Attributes. The patches I've mentioned are doing exactly that. The filesystems are responsible for handling ACLs; they are not limited to POSIX ACLs.
Filesystems may choose to cache/manipulate ACLs in the kernel internal
format that this patch implements, or differently. In any case they should implement the get_posix_acl and set_posix_acl inode operations if mapping between native filesystem ACLs and POSIX ACLs makes sense.
Other parts of the kernel (such as nfsd) can use the get_posix_acl and
set_posix_acl inode operations to manipulate POSIX ACLs. This is currently required for nfsd, since nfsd must mask off permissions depending on the protocol and client version (a module parameter controls this behavior).

(This patch also requires the ENOTSUP and MS_NOUMASK issues to be resolved. I've posted messages on these topics to linux-kernel before, and I will send copies to linux-fsdevel.)

Regards,
Andreas.

------------------------------------------------------------------
Andreas Gruenbacher SuSE Linux AG
mailto:[email protected] Deutschherrnstr. 15-19
http://www.suse.de/ D-90429 Nuernberg, Germany

Christoph Hellwig

2002-08-04 14:04 UTC #1 lore raw

Quoted 5 lines · Andreas Gruenbacher

Hello,
I want to propose adding some infrastructure to the VFS for POSIX ACL ssupport. There are already implementations for XFS and Ext2/Ext3; ReiserFS is to follow soon.

Could you please resend without mime and gzip?

Quoted 5 lines

------------------------------------------------------------------
Andreas Gruenbacher SuSE Linux AG
mailto:[email protected] Deutschherrnstr. 15-19
http://www.suse.de/ D-90429 Nuernberg, Germany

---end quoted text---

Andreas Gruenbacher

author

2002-08-04 14:14 UTC #2 lore raw

Quoted 8 lines · you

Quoted 5 lines · Andreas Gruenbacher
Hello,
I want to propose adding some infrastructure to the VFS for POSIX ACL ssupport. There are already implementations for XFS and Ext2/Ext3; ReiserFS is to follow soon.
Could you please resend without mime and gzip?

Well, sorry the patch is rather large uncompressed.

Cheers,
Andreas.

diff -Nur --exclude-from=linux.excl linux-2.5.30/fs/Config.help linux-2.5.30.patch/fs/Config.help

fs/Config.help +17 -0

@@ -1,3 +1,20 @@
+CONFIG_FS_POSIX_ACL
+  POSIX Access Control Lists (ACLs) support permissions for users and
+  groups beyond the owner/group/world scheme.
+
+  To learn more about Access Control Lists, visit
+  <http://acl.bestbits.at/>.
+
+  You need an additional filesystem patch to get POSIX ACL support
+  on one of the built-in filesystems; such patches are available at
+  http://acl.bestbits.at/ for ext2 and ext3, and at
+  http://oss.sgi.com/projects/xfs/ for XFS.
+  
+  The getfacl and setfacl utilities for manipulating POSIX ACLs are also
+  available at these web sites.
+
+  If you don't know what Access Control Lists are, say N.
+
 CONFIG_QUOTA
   If you say Y here, you will be able to set per user limits for disk
   usage (also called disk quotas). Currently, it works for the

diff -Nur --exclude-from=linux.excl linux-2.5.30/fs/Config.in linux-2.5.30.patch/fs/Config.in

fs/Config.in +2 -0

@@ -4,6 +4,8 @@
 mainmenu_option next_comment
 comment 'File systems'
 
+bool 'POSIX Access Control Lists' CONFIG_FS_POSIX_ACL
+
 bool 'Quota support' CONFIG_QUOTA
 dep_tristate '  Old quota format support' CONFIG_QFMT_V1 $CONFIG_QUOTA
 dep_tristate '  Quota format v2 support' CONFIG_QFMT_V2 $CONFIG_QUOTA

diff -Nur --exclude-from=linux.excl linux-2.5.30/fs/Makefile linux-2.5.30.patch/fs/Makefile

fs/Makefile +4 -1

@@ -7,7 +7,8 @@
 
 O_TARGET := fs.o
 
-export-objs :=	open.o dcache.o buffer.o bio.o inode.o dquot.o mpage.o
+export-objs :=	open.o dcache.o buffer.o bio.o inode.o dquot.o mpage.o \
+		posix_acl.o
 
 obj-y :=	open.o read_write.o devices.o file_table.o buffer.o \
 		bio.o super.o block_dev.o char_dev.o stat.o exec.o pipe.o \

@@ -36,6 +37,8 @@
 obj-$(CONFIG_QFMT_V2)		+= quota_v2.o
 obj-$(CONFIG_QUOTACTL)		+= quota.o
 
+obj-$(CONFIG_FS_POSIX_ACL)	+= posix_acl.o
+
 obj-$(CONFIG_PROC_FS)		+= proc/
 obj-y				+= partitions/
 obj-y				+= driverfs/

diff -Nur --exclude-from=linux.excl linux-2.5.30/fs/posix_acl.c linux-2.5.30.patch/fs/posix_acl.c

fs/posix_acl.c +496 -0

496 lines not on this page · raw message

diff -Nur --exclude-from=linux.excl linux-2.5.30/include/linux/errno.h linux-2.5.30.patch/include/linux/errno.h

include/linux/errno.h +2 -0

@@ -11,6 +11,8 @@
 #define ERESTARTNOHAND	514	/* restart if no handler.. */
 #define ENOIOCTLCMD	515	/* No ioctl command */
 
+#define ENOTSUP		-1	/* UNRESOLVED ISSUE -- see other proposal */
+
 /* Defined for the NFSv3 protocol */
 #define EBADHANDLE	521	/* Illegal NFS file handle */
 #define ENOTSYNC	522	/* Update synchronization mismatch */

diff -Nur --exclude-from=linux.excl linux-2.5.30/include/linux/fs.h linux-2.5.30.patch/include/linux/fs.h

include/linux/fs.h +3 -0

@@ -23,6 +23,7 @@
 #include <linux/string.h>
 #include <linux/radix-tree.h>
 #include <linux/bitops.h>
+#include <linux/posix_acl.h>
 
 #include <asm/atomic.h>

@@ -787,6 +788,8 @@
 	ssize_t (*getxattr) (struct dentry *, const char *, void *, size_t);
 	ssize_t (*listxattr) (struct dentry *, char *, size_t);
 	int (*removexattr) (struct dentry *, const char *);
+	posix_acl_t *(*get_posix_acl) (struct inode *, int);
+	int (*set_posix_acl) (struct inode *, int, posix_acl_t *);
 };
 
 struct seq_file;

diff -Nur --exclude-from=linux.excl linux-2.5.30/include/linux/posix_acl.h linux-2.5.30.patch/include/linux/posix_acl.h

include/linux/posix_acl.h +69 -0

69 lines not on this page · raw message

Christoph Hellwig

2002-08-04 14:33 UTC #3 lore raw

fs/posix_acl.c line 30

Quoted 5 lines · Andreas Gruenbacher

fs/posix_acl.c +5 -0

+MODULE_AUTHOR("Andreas Gruenbacher <[email protected]>");
+MODULE_DESCRIPTION("Generic Posix Access Control List (ACL) Manipulation");
+#if LINUX_VERSION_CODE >= KERNEL_VERSION(2,4,0)
+MODULE_LICENSE("GPL");
+#endif

MODULE_LICENSE was new in 2.4.9 or 2.4.10, but certainly not present in 2.4.1.. I don't think kernel version checks in core code are a good idea though. especially if you aim for inclusion.

fs/posix_acl.c line 60

Quoted 11 lines

fs/posix_acl.c +11 -0

+posix_acl_t *
+posix_acl_alloc(int count)
+{
+	const size_t size = sizeof(posix_acl_t) +
+	                    count * sizeof(posix_acl_entry_t);
+	posix_acl_t *acl = kmalloc(size, GFP_KERNEL);
+	if (acl) {
+		atomic_set(&acl->a_refcount, 1);
+		acl->a_count = count;
+	}
+	return acl;

are you sure this is never called from filsystem transactions? passing a gfp flag down seems like a good idea to me.

fs/posix_acl.c line 72

Quoted 10 lines

fs/posix_acl.c +10 -0

+/*
+ * Duplicate an ACL handle.
+ */
+posix_acl_t *
+posix_acl_dup(posix_acl_t *acl)
+{
+	if (acl)
+		atomic_inc(&acl->a_refcount);
+	return acl;
+}

Make this an inline in a header? can acl really be NULL?

fs/posix_acl.c line 467

Quoted 10 lines

fs/posix_acl.c +10 -0

+/*
+ * Get the POSIX ACL of an inode.
+ */
+posix_acl_t *
+get_posix_acl(struct inode *inode, int type)
+{
+	posix_acl_t *acl;
+
+	if (!inode->i_op || !inode->i_op->get_posix_acl)
+		return ERR_PTR(-ENOTSUP);

inode->i_op is never NULL.

Review

Quoted 2 lines

(quoted diff) +2 -0

+	down(&inode->i_sem);
+	lock_kernel();  /* goes away in 2.5.x */

this patch _is_ for 2.5, isn't it?

Quoted 12 lines

linux-2.5.30.patch/include/linux/fs.h

include/linux/fs.h +1 -0

@@ -23,6 +23,7 @@
 #include <linux/string.h>
 #include <linux/radix-tree.h>
 #include <linux/bitops.h>
+#include <linux/posix_acl.h>
 
 #include <asm/atomic.h>

include/linux/fs.h line 792

Quoted 5 lines

include/linux/fs.h +2 -0

@@ -787,6 +788,8 @@
 	ssize_t (*getxattr) (struct dentry *, const char *, void *, size_t);
 	ssize_t (*listxattr) (struct dentry *, char *, size_t);
 	int (*removexattr) (struct dentry *, const char *);
+	posix_acl_t *(*get_posix_acl) (struct inode *, int);
+	int (*set_posix_acl) (struct inode *, int, posix_acl_t *);

If you had followed Documentation/CodingSyle and use struct osix_acl instead of posix_acl_t we wouldn't have to bloat fs.h with yet another indirect header..

Also what exactly are get_posix_acl/set_posix_acl for? We have wrappers for them in fs/posix_acl.c, but even in your 2.4 patch only get_posix_acl is ever used. Shouldn't we always set/get posix ACLs through the xattr inode operations?

include/linux/fs.h line 793

Quoted 1 line

include/linux/fs.h +1 -0
+#ifdef __KERNEL__

why the __KERNEL__?

include/linux/fs.h line 794

Quoted 1 line

include/linux/fs.h +1 -0
+/* pxacl.c */

Shouldn't this be posix_acl.c?

Andreas Gruenbacher

author

2002-08-05 12:11 UTC #4 lore raw

Quoted 10 lines · Christoph Hellwig

fs/posix_acl.c line 30
Quoted 5 lines · Andreas Gruenbacher
(quoted diff) +2 -0
+MODULE_AUTHOR("Andreas Gruenbacher <[email protected]>");
+MODULE_DESCRIPTION("Generic Posix Access Control List (ACL)
Manipulation"); +#if LINUX_VERSION_CODE >= KERNEL_VERSION(2,4,0)
fs/posix_acl.c +2 -0
+MODULE_LICENSE("GPL");
+#endif
MODULE_LICENSE was new in 2.4.9 or 2.4.10, but certainly not present in 2.4.1.. I don't think kernel version checks in core code are a good idea though. especially if you aim for inclusion.

It was 2.4.10; I've canged that. I wanted to use identical code from 2.2 to 2.4; the non-2.5 parts can be stripped off.

Quoted 6 lines

fs/posix_acl.c +6 -0

+posix_acl_t *
+posix_acl_alloc(int count)
+{
+	const size_t size = sizeof(posix_acl_t) +
+	                    count * sizeof(posix_acl_entry_t);
+	posix_acl_t *acl = kmalloc(size, GFP_KERNEL);

Quoted 8 lines

fs/posix_acl.c line 60
Quoted 5 lines
fs/posix_acl.c +5 -0
+	if (acl) {
+		atomic_set(&acl->a_refcount, 1);
+		acl->a_count = count;
+	}
+	return acl;
are you sure this is never called from filesystem transactions? passing a gfp flag down seems like a good idea to me.

Probably, yes.

Quoted 4 lines

Quoted 4 lines
fs/posix_acl.c +4 -0
+/*
+ * Duplicate an ACL handle.
+ */
+posix_acl_t *

Quoted 8 lines

fs/posix_acl.c line 72
Quoted 6 lines
fs/posix_acl.c +6 -0
+posix_acl_dup(posix_acl_t *acl)
+{
+	if (acl)
+		atomic_inc(&acl->a_refcount);
+	return acl;
+}
Make this an inline in a header? can acl really be NULL?

Yes it can, meaning `there are only the file permission bits'.

Quoted 4 lines

Quoted 4 lines
fs/posix_acl.c +4 -0
+/*
+ * Get the POSIX ACL of an inode.
+ */
+posix_acl_t *

Quoted 8 lines

fs/posix_acl.c line 467
Quoted 6 lines
fs/posix_acl.c +6 -0
+get_posix_acl(struct inode *inode, int type)
+{
+	posix_acl_t *acl;
+
+	if (!inode->i_op || !inode->i_op->get_posix_acl)
+		return ERR_PTR(-ENOTSUP);
inode->i_op is never NULL.

Changed.

Quoted 4 lines

Review
Quoted 2 lines
(quoted diff) +2 -0
+	down(&inode->i_sem);
+	lock_kernel();  /* goes away in 2.5.x */
this patch _is_ for 2.5, isn't it?

Removed.

Quoted 11 lines

linux-2.5.30.patch/include/linux/fs.h

include/linux/fs.h +1 -0

@@ -23,6 +23,7 @@
 #include <linux/string.h>
 #include <linux/radix-tree.h>
 #include <linux/bitops.h>
+#include <linux/posix_acl.h>
 
 #include <asm/atomic.h>

Quoted 10 lines

include/linux/fs.h line 792
Quoted 6 lines
include/linux/fs.h +2 -0
@@ -787,6 +788,8 @@
 	ssize_t (*getxattr) (struct dentry *, const char *, void *, size_t);
 	ssize_t (*listxattr) (struct dentry *, char *, size_t);
 	int (*removexattr) (struct dentry *, const char *);
+	posix_acl_t *(*get_posix_acl) (struct inode *, int);
+	int (*set_posix_acl) (struct inode *, int, posix_acl_t *);
If you had followed Documentation/CodingSyle and use struct posix_acl instead of posix_acl_t we wouldn't have to bloat fs.h with yet another indirect header..

Using `struct posix_acl' everywhere is so much more messy. Is an exception justified here? The core kernel code has typedefs all over the place.

Quoted 6 lines

Also what exactly are get_posix_acl/set_posix_acl for? We have wrappers for them in fs/posix_acl.c, but even in your 2.4 patch only get_posix_acl is ever used. Shouldn't we always set/get posix ACLs through the xattr inode operations?
From user space, yes. The get_posix_acl operation is currently used in nfsd;

going via the xattr operations would be too expensive here. The set_posix_acl operation is indeed not used so far; I think it makes sense to add it for completeness' sake.

Quoted 4 lines

include/linux/fs.h line 793
Quoted 2 lines
include/linux/fs.h +1 -0
+#ifdef __KERNEL__
why the __KERNEL__?

Not needed anymore.

Quoted 3 lines

include/linux/fs.h line 794
Quoted 1 line
include/linux/fs.h +1 -0
+/* pxacl.c */
Shouldn't this be posix_acl.c?

Yes.

I have put up an updated version at
<http://acl.bestbits.at/pre/2.5/linux-2.5.30-posix-acl-1.diff>.

Regards,
Andreas.

------------------------------------------------------------------
Andreas Gruenbacher SuSE Linux AG
mailto:[email protected] Deutschherrnstr. 15-19
http://www.suse.de/ D-90429 Nuernberg, Germany

Christoph Hellwig

2002-08-05 12:28 UTC #5 lore raw

Quoted 2 lines · Andreas Gruenbacher

It was 2.4.10; I've canged that. I wanted to use identical code from 2.2 to 2.4; the non-2.5 parts can be stripped off.

Then strip them off :) especially as you added even more ugly checks around the lock_kernel/unlock_kernel invocations..

Quoted 1 line

Using `struct posix_acl' everywhere is so much more messy.

Why?

Quoted 2 lines

Is an exception
justified here? The core kernel code has typedefs all over the place.

Ask Linus.. You should at least provide an alternate struct posix_acl definition so fs.h doesn't have to pull in posix_acl.h but can use a struct forward declaration.

Quoted 4 lines

Quoted 1 line
From user space, yes. The get_posix_acl operation is currently used in nfsd;
going via the xattr operations would be too expensive here. The set_posix_acl operation is indeed not used so far; I think it makes sense to add it for completeness' sake.

I'm not fully convienced. At least add the locking rules to Documentation/ filesystems/Locking. Documentation/filesystems/porting should also get a an entry about the ACL support.

Nathan Scott

2002-08-09 02:02 UTC #6 lore raw

hi Andreas,

Quoted 8 lines · Christoph Hellwig

Quoted 4 lines · Andreas Gruenbacher
Quoted 1 line
From user space, yes. The get_posix_acl operation is currently used in nfsd;
going via the xattr operations would be too expensive here. The set_posix_acl operation is indeed not used so far; I think it makes sense to add it for completeness' sake.
I'm not fully convienced.

Nor am I (I don't buy either the too expensive argument or the need for unused ops). It also begins to add unnecessary stuff to the VFS inode ops - when capabilities, MAC labels, and misc other attributes come along, we don't want to be adding in ops for each of them too (esp. considering many filesystems do not support extended attributes at all). IMO getxattr() should be all we ever need here.

MS_POSIXACL from your earlier mail is a Good Thing, I think - I'll switch XFS over to use that when I get some time (instead of the inode flag we are currently using).

cheers.

Signature · 1 line

Nathan

Andreas Gruenbacher

author

2002-08-09 10:53 UTC #7 lore raw

Quoted 7 lines · Nathan Scott

hi Andreas,
Quoted 4 lines · Christoph Hellwig
Quoted 3 lines · Andreas Gruenbacher
Quoted 2 lines
From user space, yes. The get_posix_acl operation is currently used in nfsd;

Quoted 8 lines

Quoted 5 lines
Quoted 3 lines
going via the xattr operations would be too expensive here. The set_posix_acl operation is indeed not used so far; I think it makes sense to add it for completeness' sake.
I'm not fully convienced.
Nor am I (I don't buy either the too expensive argument or the need for unused ops).

The kernel NFS daemon introduces a number of interesting problems in the kernel that no other component requires; the ACL permission masking hack is one of them. The hack affects NFSv2 and NFSv3

It is really important that encode_fattr and encode_fattr3, the functions that include the hack, are efficient; they are used in all the essential NFS RPCs.

The two implementation choices are:

* Use the get_posix_acl inode operation

On Ext2, Ext3, ReiserFS the inode operation passes a handle to the
ACL to the caller (no copying). The handle is in the cache after the
first access, the cache is very effective. XFS currently seems not to implement the *_posix_acl inode operations. That's fine; just the NFS hack won't be able to mask permissions.

* Go via the xattr ACL format

The xattr interface uses pass-by-value by design. So this requires allocating a buffer, conversion from the file system into the xattr format, parsing the xattr format. The xattr format is in
(with possible reverse endianness conversion).

The get_posix_acl approach has the added benefit that other parts of the kernel interested in manipulating ACLs can do so easily and efficiently (though I don't see which ones that could be right now). If set_posix_acl really _must_ be removed I won't bang my head against a wall.

Is this sufficient?

Quoted 6 lines

It also begins to add unnecessary stuff
to the VFS inode ops - when capabilities, MAC labels, and misc other attributes come along, we don't want to be adding in ops for each of them too (esp. considering many filesystems do not support extended attributes at all). IMO getxattr() should be all we ever need here.

Those file systems not supporting some of the options are not affected. We only need to add stuff to the VFS for things that other parts of the kernel need to access independently of the real filesystem, in those cases where the accesses need to be faster than the xattr interface can be.

Quoted 3 lines

MS_POSIXACL from your earlier mail is a Good Thing, I think - I'll switch XFS over to use that when I get some time (instead of the inode flag we are currently using).

Fine, I will also use that, instead of MS_NOUMASK. MS_POSIXACL is nice to have in the nfsd hack as well. Having it is not extremely clean design, and I will hate myself for giving in the day another mechanism requires the umask to be skipped which is not POSIX ACLs, though.

Cheers,
Andreas.

------------------------------------------------------------------
Andreas Gruenbacher SuSE Linux AG
mailto:[email protected] Deutschherrnstr. 15-19
http://www.suse.de/ D-90429 Nuernberg, Germany

Christoph Hellwig

2002-08-09 11:15 UTC #8 lore raw

Quoted 6 lines · Andreas Gruenbacher

The kernel NFS daemon introduces a number of interesting problems in the kernel that no other component requires; the ACL permission masking hack is one of them. The hack affects NFSv2 and NFSv3
It is really important that encode_fattr and encode_fattr3, the functions that include the hack, are efficient; they are used in all the essential NFS RPCs.

IF it's so essential why are you doing another IOP call then instead of adding inkernel A retrieving to ->getattr and struct kstat? encode_fattr* has to call it anyway and that's the logical place for ACLs..

Andreas Gruenbacher

author

2002-08-09 12:22 UTC #9 lore raw

Quoted 5 lines · Christoph Hellwig

Quoted 4 lines · Andreas Gruenbacher
The kernel NFS daemon introduces a number of interesting problems in the kernel that no other component requires; the ACL permission masking hack is one of them. The hack affects NFSv2 and NFSv3

Quoted 7 lines

Quoted 3 lines
It is really important that encode_fattr and encode_fattr3, the functions that include the hack, are efficient; they are used in all the essential NFS RPCs.
IF it's so essential why are you doing another IOP call then instead of adding inkernel A retrieving to ->getattr and struct kstat? encode_fattr* has to call it anyway and that's the logical place for ACLs..

I read this as `Why are you not returning the permissions nfsd should see in struct, which the inode->getattr IOP fills?'. Please correct me if this is not what you wanted to say.

The usual callers of getattr should see the unmasked permissions; it's only nfsd that needs this special treatment, and only on filesystems that have ACLs enabled. The getattr IOP is used by others as well.

If you had adding an extra field to struct kstat in mind, this just seems too intrusive to me for solving the nfsd problem. The problem will go away in the future when all NFSv3 clients are using the ACCESS RPC, anyway. It's just a hack right now.

-Andreas.

------------------------------------------------------------------
Andreas Gruenbacher SuSE Linux AG
mailto:[email protected] Deutschherrnstr. 15-19
http://www.suse.de/ D-90429 Nuernberg, Germany

Christoph Hellwig

2002-08-09 12:32 UTC #10 lore raw

Quoted 3 lines · Andreas Gruenbacher

I read this as `Why are you not returning the permissions nfsd should see in struct, which the inode->getattr IOP fills?'. Please correct me if this is not what you wanted to say.

I meant adding an acl field to struct kstat that is supposed to be filled by ->getattr.

Quoted 3 lines

The usual callers of getattr should see the unmasked permissions; it's only nfsd that needs this special treatment, and only on filesystems that have ACLs enabled. The getattr IOP is used by others as well.

So why the is a new IOP better in that respect?

Quoted 4 lines

If you had adding an extra field to struct kstat in mind, this just seems too intrusive to me for solving the nfsd problem. The problem will go away in the future when all NFSv3 clients are using the ACCESS RPC, anyway. It's just a hack right now.

So you want to add two inode operations for a hack that is only needed to support old NFS clients? Do you have any profiling data that shows major overhead when going through the xattr layer?

Andreas Gruenbacher

author

2002-08-09 13:17 UTC #11 lore raw

Quoted 7 lines · Christoph Hellwig

Quoted 3 lines · Andreas Gruenbacher
I read this as `Why are you not returning the permissions nfsd should see in struct, which the inode->getattr IOP fills?'. Please correct me if this is not what you wanted to say.
I meant adding an acl field to struct kstat that is supposed to be filled by ->getattr.

I see. That's problematic on most filesystems, because retrieving the ACL from disk is pretty expensive, and often is not needed. For things like stat() it is not needed, for example.

Quoted 5 lines

Quoted 3 lines
The usual callers of getattr should see the unmasked permissions; it's only nfsd that needs this special treatment, and only on filesystems that have ACLs enabled. The getattr IOP is used by others as well.
So why is the a new IOP better in that respect?

Only nfsd needs to pay the runtime cost.

Quoted 7 lines

Quoted 4 lines
If you had adding an extra field to struct kstat in mind, this just seems too intrusive to me for solving the nfsd problem. The problem will go away in the future when all NFSv3 clients are using the ACCESS RPC, anyway. It's just a hack right now.
So you want to add two inode operations for a hack that is only needed to support old NFS clients?

It will be needed for NFSv2 and NFSv3, until all NFSv3 clients support the ACCESS RPC properly. The ACCESS RPC patch for the client side in the current kernel still has some performance issues. The hack won't go away anytime soon, but a lot fewer people will be affected by the underlying problem in a year or so.

I also think the inode operations are a good thing to keep independent of the nfsd issue.

Quoted 1 line

Do you have any profiling data that shows major overhead when going through

the xattr layer?

No, this would be tricky to measure. The interface requires copying around ACLs, and endianness conversions on big endian machines; it's simply inappropriate for that task.

--Andreas.

------------------------------------------------------------------
Andreas Gruenbacher SuSE Linux AG
mailto:[email protected] Deutschherrnstr. 15-19
http://www.suse.de/ D-90429 Nuernberg, Germany