[Pvfs2-developers] ACL/getxattr overhead in ls

[Murali Vilayannur]

Hi David,
Interesting. Must have slipped through some of the configure kernel
feature checks or something.

> --- This is a strace on RHEL3
> getxattr("/mnt/datagrid1", "system.posix_acl_access", (nil), 0) = -1 ENODATA
> (No data available)
> getxattr("/mnt/datagrid1", "system.posix_acl_default", (nil), 0) = -1
> ENODATA (No data available)

On this system, what does HAVE_GENERIC_GETXATTR have? Presumably it is set
to 0. Here, I think we are issuing network calls since pvfs2_getxattr()
directly calls pvfs2_inode_getxattr() without any checks for whether acl's
are supported...
Does this patch help prevent the network calls?
There are just way too many ways to get into the xattr routines. I am
not sure if/why we even need to support xattr's if HAVE_GENERIC_GETXATTR
is set to 0. :(


> --- This is a strace on RHEL4
> getxattr("/mnt/datagrid1", "system.posix_acl_access", 0x0, 0) = -1
> EOPNOTSUPP (Operation not supported)

and here, I'd imagine HAVE_GENERIC_GETXATTR being set to 1.
In this case, I dont think we are issuing any network getxattr calls..
Can you confirm that?
Thanks,
Murali

>
>
> > -----Original Message-----
> > From: pvfs2-developers-bounces at beowulf-underground.org
> > [mailto:pvfs2-developers-bounces at beowulf-underground.org] On
> > Behalf Of Murali Vilayannur
> > Sent: Thursday, May 04, 2006 10:31 AM
> > To: Phil Carns
> > Cc: PVFS2-developers
> > Subject: Re: [Pvfs2-developers] ACL/getxattr overhead in ls
> >
> > Hi Phil,
> >
> > > We have noticed on RedHat RHEL3 and RHEL4 that when you do a long
> > > listing of a directory (on any file system, not just pvfs2)
> > that the
> > > ls triggers a getxattr of "system.posix_acl_access" and
> > > "system.posix_acl_default" for every single entry that is listed.
> > > This happens regardless of whether ACLs are enabled for the
> > file system or not.
> > >
> > > Digging in the coreutils/ls source, it turns out that ls (with -l
> > > options at least) is checking to see if any acl's are set on each
> > > entry so that it can display them differently, maybe with a
> > "+" character.
> > >
> > > On RHEL3 at least (2.4 kernel) these extra getxattrs are causing 2
> > > serialized getxattr requests that are probably
> > significantly slowing
> > > down the listing, which is a shame because the 2.4 kernel module
> > > doesn't even support ACLs so there is never any point in checking.
> > >
> > > Does it seem reasonable to add a test in the kernel module
> > so that if
> > > a) acl's are not enabled (which can be determined by checking the
> > > super block acl flag), and b) the getxattr name matches
> > "system.posix_acl"
> > > then we just immediately return an appropriate error code
> > rather than
> > > servicing the getxattr?
> >
> > I don't understand how this situation even happens..
> > There is already such a check in acl.c in the function
> > pvfs2_xattr_get_acl() to see if we support acl's or not
> > before calling the underlying getxattr.
> > Can you find out if the strace of an ls -l causes the
> > getxattr() to return -EOPNOTSUPP instead of -ENODATA?
> >
> > Thanks,
> > Murali
> >
> > >
> > > The stock coreutils package doesn't currently have this problem,
> > > because their acl check is broken for Linux.  RedHat
> > supplies a patch
> > > with their rpm that adds acl checks that work on linux (using the
> > > acl_extended_file() function rather than the unsupported acl()
> > > function).  I imagine there are other distributions that have done
> > > likewise, and probably it will be "fixed" in the official
> > gnu version
> > > eventually also.
> > >
> > > -Phil
> > >
> > > _______________________________________________
> > > Pvfs2-developers mailing list
> > > Pvfs2-developers at beowulf-underground.org
> > > http://www.beowulf-underground.org/mailman/listinfo/pvfs2-developers
> > >
> > >
> > _______________________________________________
> > Pvfs2-developers mailing list
> > Pvfs2-developers at beowulf-underground.org
> > http://www.beowulf-underground.org/mailman/listinfo/pvfs2-developers
> >
>
>
-------------- next part --------------
Index: src/kernel/linux-2.6/xattr.c
===================================================================
RCS file: /anoncvs/pvfs2/src/kernel/linux-2.6/xattr.c,v
retrieving revision 1.9
diff -u -r1.9 xattr.c
--- src/kernel/linux-2.6/xattr.c	9 Mar 2006 22:48:35 -0000	1.9
+++ src/kernel/linux-2.6/xattr.c	4 May 2006 16:04:15 -0000
@@ -72,12 +72,18 @@
     if((n = pvfs2_strcmp_prefix(name, PVFS2_XATTR_NAME_TRUSTED_PREFIX)))
     {
         ret = pvfs2_xattr_set_trusted(inode, n, value, size, flags);
+        goto out;
     }
-    else
+    else if ((n = pvfs2_strcmp_prefix(name, PVFS2_XATTR_NAME_ACL_DEFAULT)) ||
+            (n = pvfs2_strcmp_prefix(name, PVFS2_XATTR_NAME_ACL_ACCESS)))
     {
-        ret = pvfs2_xattr_set_default(inode, name, value, size, flags);
+        if (get_acl_flag(inode) == 0) {
+            ret = -EOPNOTSUPP;
+            goto out;
+        }
     }
-
+    ret = pvfs2_xattr_set_default(inode, name, value, size, flags);
+out:
     return ret;
 }
 
@@ -91,12 +97,19 @@
     if((n = pvfs2_strcmp_prefix(name, PVFS2_XATTR_NAME_TRUSTED_PREFIX)))
     {
         ret = pvfs2_xattr_get_trusted(inode, n, buffer, size);
+        goto out;
     }
-    else
+    else if ((n = pvfs2_strcmp_prefix(name, PVFS2_XATTR_NAME_ACL_DEFAULT)) ||
+            (n = pvfs2_strcmp_prefix(name, PVFS2_XATTR_NAME_ACL_ACCESS)))
     {
-        ret = pvfs2_xattr_get_default(inode, name, buffer, size);
+        if (get_acl_flag(inode) == 0) {
+            ret = -EOPNOTSUPP;
+            goto out;
+        }
     }
+    ret = pvfs2_xattr_get_default(inode, name, buffer, size);
 
+out:
     return ret;
 }