Skip to contentSkip to conversation

vfs pile 1.5

GIT PULL In review Al Viro 9 participants 20 messages Jul 25, 2011 – Jul 31, 2011

Subsystems (2)
VFS XFS filesystem
lore
Share & tools
lore

Al Viro

author maintainer
#0 lore raw

Regression fixes (devtmpfs race, cifs ->d_revalidate() breakage, fsync fallout), ACL stuff (me/Linus/Christoph), fixes for bugs found while wading through the mode_t handling. There'll be another pull request with assorted stuff from over the last cycle.

Please, pull from
git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs-2.6.git/ for-linus

Shortlog:

Al Viro (11):
ocfs2_init_acl(): fix a leak
9p: close ACL leaks
fix jffs2 ACLs on big-endian with 16bit mode_t
asus-wmi: ->is_visible() can't return negative
get rid of create_proc_entry() abuses - proc_mkdir() is there for purpose
caam: don't pass bogus S_IFCHR to debugfs_create_...()
fix devtmpfs race
kill boilerplate around posix_acl_chmod_masq()
generic_acl: no need to clone acl just to push it to set_cached_acl() kill boilerplates around posix_acl_create_masq()
bury posix_acl_..._masq() variants

Christoph Hellwig (4):
9p: do no return 0 from ->check_acl without actually checking
xfs: cache negative ACLs if there is no attribute fork
reiserfs: cache negative ACLs for v1 stat format
fs: take the ACL checks to common code

Linus Torvalds (1):
vfs: move ACL cache lookup into generic code

Markus Trippelsdorf (1):
xfs: Fix wrong return value of xfs_file_aio_write

Pavel Shilovsky (1):
CIFS: Fix oops while mounting with prefixpath

Tim Chen (1):
VFS : mount lock scalability for internal mounts

Diffstat:

 Documentation/filesystems/Locking                  |    4 +-
 Documentation/filesystems/porting                  |    7 +-
 Documentation/filesystems/vfs.txt                  |    2 +-
 arch/arm/mach-bcmring/dma.c                        |    2 +-
 drivers/base/devtmpfs.c                            |    3 +-
 drivers/crypto/caam/ctrl.c                         |   26 +++---
 drivers/mtd/mtdchar.c                              |    2 +-
 drivers/platform/x86/asus-wmi.c                    |    2 +-
 drivers/staging/rtl8187se/r8180_core.c             |    2 +-
 .../staging/rtl8192e/ieee80211/ieee80211_module.c  |    2 +-
 drivers/staging/rtl8192e/r8192E_core.c             |    6 +-
 .../staging/rtl8192u/ieee80211/ieee80211_module.c  |    2 +-
 drivers/staging/rtl8192u/r8192U_core.c             |    6 +-
 fs/9p/acl.c                                        |   68 +++++---------
 fs/9p/acl.h                                        |   10 +-
 fs/9p/vfs_inode_dotl.c                             |   13 ++-
 fs/anon_inodes.c                                   |    2 +-
 fs/btrfs/acl.c                                     |   63 +++----------
 fs/btrfs/ctree.h                                   |    4 +-
 fs/btrfs/inode.c                                   |   10 +-
 fs/cifs/dir.c                                      |    2 +-
 fs/ext2/acl.c                                      |   63 +++----------
 fs/ext2/acl.h                                      |    4 +-
 fs/ext2/file.c                                     |    2 +-
 fs/ext2/namei.c                                    |    4 +-
 fs/ext3/acl.c                                      |   94 ++++++-------------
 fs/ext3/acl.h                                      |    4 +-
 fs/ext3/file.c                                     |    2 +-
 fs/ext3/namei.c                                    |    4 +-
 fs/ext4/acl.c                                      |   95 ++++++-------------
 fs/ext4/acl.h                                      |    4 +-
 fs/ext4/file.c                                     |    2 +-
 fs/ext4/namei.c                                    |    4 +-
 fs/generic_acl.c                                   |   62 +++----------
 fs/gfs2/acl.c                                      |   75 ++++------------
 fs/gfs2/acl.h                                      |    2 +-
 fs/gfs2/inode.c                                    |    6 +-
 fs/hugetlbfs/inode.c                               |    1 +
 fs/jffs2/acl.c                                     |   49 +++--------
 fs/jffs2/acl.h                                     |    6 +-
 fs/jffs2/dir.c                                     |    2 +-
 fs/jffs2/file.c                                    |    2 +-
 fs/jffs2/fs.c                                      |    2 +-
 fs/jffs2/os-linux.h                                |    2 +-
 fs/jffs2/symlink.c                                 |    2 +-
 fs/jfs/acl.c                                       |   75 +++++-----------
 fs/jfs/file.c                                      |    2 +-
 fs/jfs/jfs_acl.h                                   |    2 +-
 fs/jfs/namei.c                                     |    2 +-
 fs/namei.c                                         |   54 +++++++++++-
 fs/namespace.c                                     |   21 ++++-
 fs/nfs/nfs3acl.c                                   |   10 +--
 fs/ocfs2/acl.c                                     |   77 ++++++-----------
 fs/ocfs2/acl.h                                     |    2 +-
 fs/ocfs2/file.c                                    |    4 +-
 fs/ocfs2/namei.c                                   |    2 +-
 fs/pipe.c                                          |    2 +-
 fs/posix_acl.c                                     |   48 ++++++++--
 fs/reiserfs/file.c                                 |    2 +-
 fs/reiserfs/inode.c                                |    5 +
 fs/reiserfs/namei.c                                |    6 +-
 fs/reiserfs/xattr.c                                |   27 ------
 fs/reiserfs/xattr_acl.c                            |   71 +++++----------
 fs/xfs/linux-2.6/xfs_acl.c                         |   80 ++++-------------
 fs/xfs/linux-2.6/xfs_file.c                        |    5 +-
 fs/xfs/linux-2.6/xfs_iops.c                        |   14 ++-
 fs/xfs/linux-2.6/xfs_trace.h                       |    2 +-
 fs/xfs/xfs_acl.h                                   |    2 -
 include/linux/fs.h                                 |    3 +-
 include/linux/generic_acl.h                        |    1 -
 include/linux/posix_acl.h                          |    5 +-
 include/linux/reiserfs_acl.h                       |    6 +-
 include/linux/reiserfs_xattr.h                     |    2 -
 mm/shmem.c                                         |    6 --
 security/selinux/selinuxfs.c                       |    1 +
 sound/core/info.c                                  |    2 +-
 76 files changed, 477 insertions(+), 795 deletions(-)
#1 lore raw
Quoted 5 lines · Al Viro

Regression fixes (devtmpfs race, cifs ->d_revalidate() breakage, fsync fallout), ACL stuff (me/Linus/Christoph), fixes for bugs found while

Linus Torvalds (1):
vfs: move ACL cache lookup into generic code

This breaks the build for "# CONFIG_FS_POSIX_ACL is not set" configs:

CC fs/namei.o
fs/namei.c: In function ‘check_acl’:
fs/namei.c:191:10: error: implicit declaration of function ‘negative_cached_acl’ [-Werror=implicit-function-declaration] fs/namei.c:196:2: error: implicit declaration of function ‘get_cached_acl’ [-Werror=implicit-function-declaration] fs/namei.c:196:6: warning: assignment makes pointer from integer without a cast [enabled by default] fs/namei.c:212:11: error: implicit declaration of function ‘set_cached_acl’ [-Werror=implicit-function-declaration]
cc1: some warnings being treated as errors

Signature · 5 lines
Markus
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
#2 lore raw
Quoted 2 lines · Markus Trippelsdorf

This breaks the build for "# CONFIG_FS_POSIX_ACL is not set" configs:

Of course it does. And as usual, my exhaustive testing was for the allyesconfig case, rather than doing both allyesconfig and allnoconfig like I should.

Bah.

Does the attached trivial patch fix everything for you?

Linus

 fs/namei.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)
fs/namei.c +2 -0
index ef00b984fb20..f8c69d373793 100644
@@ -176,6 +176,7 @@ EXPORT_SYMBOL(putname);
 
 static int check_acl(struct inode *inode, int mask)
 {
+#ifdef CONFIG_FS_POSIX_ACL
 	struct posix_acl *acl;
 
 	/*

@@ -219,6 +220,7 @@ static int check_acl(struct inode *inode, int mask)
 	        posix_acl_release(acl);
 	        return error;
 	}
+#endif
 
 	return -EAGAIN;
 }
Acked-by
#3 lore raw

Hi Linus,

Quoted 6 lines · On Mon, 25 Jul 2011 21:52:57 -0700 Linus Torvalds…
Quoted 2 lines · Markus Trippelsdorf

This breaks the build for "# CONFIG_FS_POSIX_ACL is not set" configs:

Quoted 7 lines

Of course it does. And as usual, my exhaustive testing was for the allyesconfig case, rather than doing both allyesconfig and allnoconfig like I should.

Bah.

Does the attached trivial patch fix everything for you?

It fixes my allnoconfig builds in linux-next today (which were broken).

Acked-by Stephen Rothwell <[email protected]>
Signature · 3 lines
Cheers,
Stephen Rothwell                    [email protected]
http://www.canb.auug.org.au/~sfr/
#4 lore raw
In reply to Linus Torvalds
Quoted 5 lines · Linus Torvalds
Quoted 2 lines · Markus Trippelsdorf

This breaks the build for "# CONFIG_FS_POSIX_ACL is not set" configs:

Quoted 7 lines

Of course it does. And as usual, my exhaustive testing was for the allyesconfig case, rather than doing both allyesconfig and allnoconfig like I should.

Bah.

Does the attached trivial patch fix everything for you?

Yes, thanks.

But now xfs fails to build when CONFIG_FS_POSIX_ACL is not set:

CC fs/xfs/linux-2.6/xfs_xattr.o
fs/xfs/linux-2.6/xfs_iops.c:1025:14: error: ‘xfs_get_acl’ undeclared here (not in a function)

Signature · 5 lines
Markus
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
#5 lore raw

maybe some one have not use #ifdef CONFIG_FS_POSIX_ACL with fs/xfs/linux-2.6/xfs_iops.c:1025:14

:)

2011/7/26 Markus Trippelsdorf <[email protected]>:

Quoted 21 lines · Linus Torvalds
Quoted 5 lines
Quoted 2 lines · Markus Trippelsdorf

This breaks the build for "# CONFIG_FS_POSIX_ACL is not set" configs:

Quoted 7 lines

Of course it does. And as usual, my exhaustive testing was for the allyesconfig case, rather than doing both allyesconfig and allnoconfig like I should.

Bah.

Does the attached trivial patch fix everything for you?

Yes, thanks.

But now xfs fails to build when CONFIG_FS_POSIX_ACL is not set:

 CC      fs/xfs/linux-2.6/xfs_xattr.o
fs/xfs/linux-2.6/xfs_iops.c:1025:14: error: ‘xfs_get_acl’ undeclared here (not in a function)

Quoted 7 lines

--
Markus
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#6 lore raw
Quoted 17 lines · Steven Liu

2011/7/26 Markus Trippelsdorf <[email protected]>:

Quoted 16 lines · Linus Torvalds
Quoted 5 lines
Quoted 2 lines · Markus Trippelsdorf

This breaks the build for "# CONFIG_FS_POSIX_ACL is not set" configs:

Quoted 7 lines

Of course it does. And as usual, my exhaustive testing was for the allyesconfig case, rather than doing both allyesconfig and allnoconfig like I should.

Bah.

Does the attached trivial patch fix everything for you?

Yes, thanks.

Quoted 7 lines
Quoted 4 lines

But now xfs fails to build when CONFIG_FS_POSIX_ACL is not set:

 CC      fs/xfs/linux-2.6/xfs_xattr.o
fs/xfs/linux-2.6/xfs_iops.c:1025:14: error: ‘xfs_get_acl’ undeclared here (not in a function)

maybe some one have not use #ifdef CONFIG_FS_POSIX_ACL with fs/xfs/linux-2.6/xfs_iops.c:1025:14

#ifdefs are ugly. The following fixes the issue for me:

fs/xfs/xfs_acl.h +4 -1
index 2c656ef..39632d9 100644
@@ -51,7 +51,10 @@ extern int posix_acl_default_exists(struct inode *inode);
 extern const struct xattr_handler xfs_xattr_acl_access_handler;
 extern const struct xattr_handler xfs_xattr_acl_default_handler;
 #else
-# define xfs_get_acl(inode, type)			NULL
+static inline struct posix_acl *xfs_get_acl(struct inode *inode, int type)
+{
+	return NULL;
+}
 # define xfs_inherit_acl(inode, default_acl)		0
 # define xfs_acl_chmod(inode)				0
 # define posix_acl_access_exists(inode)			0
Signature · 5 lines
Markus
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
#7 lore raw

PATCH 1/1 xfs: Fix build breakage in xfs_iops.c when CONFIG_FS_POSIX_ACL is not set

commit 4e34e719e45, that takes the ACL checks to common code, accidentely broke the build when CONFIG_FS_POSIX_ACL is not set:

CC fs/xfs/linux-2.6/xfs_iops.o
fs/xfs/linux-2.6/xfs_iops.c:1025:14: error: ‘xfs_get_acl’ undeclared here (not in a function)

Fix this by declaring xfs_get_acl a static inline function.

Signed-off-by Markus Trippelsdorf <[email protected]>
---
 fs/xfs/xfs_acl.h |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)
fs/xfs/xfs_acl.h +4 -1
index 2c656ef..39632d9 100644
@@ -51,7 +51,10 @@ extern int posix_acl_default_exists(struct inode *inode);
 extern const struct xattr_handler xfs_xattr_acl_access_handler;
 extern const struct xattr_handler xfs_xattr_acl_default_handler;
 #else
-# define xfs_get_acl(inode, type)			NULL
+static inline struct posix_acl *xfs_get_acl(struct inode *inode, int type)
+{
+	return NULL;
+}
 # define xfs_inherit_acl(inode, default_acl)		0
 # define xfs_acl_chmod(inode)				0
 # define posix_acl_access_exists(inode)			0
Signature · 5 lines
Markus
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
#8 lore raw

Re: [PATCH] xfs: Fix build breakage in xfs_iops.c when CONFIG_FS_POSIX_ACL is not set

Quoted 7 lines · Markus Trippelsdorf

commit 4e34e719e45, that takes the ACL checks to common code, accidentely broke the build when CONFIG_FS_POSIX_ACL is not set:

CC fs/xfs/linux-2.6/xfs_iops.o
fs/xfs/linux-2.6/xfs_iops.c:1025:14: error: ???xfs_get_acl??? undeclared here (not in a function)

Fix this by declaring xfs_get_acl a static inline function.

Do you have OPTIMIZE_INLINING enabled? In general I dont think we can take the address of an inline function, so without this option I far the code might not compile, in which case we'll need ifdefs around the inode operation assignment in addition to your patch.

#9 lore raw

Re: [PATCH] xfs: Fix build breakage in xfs_iops.c when CONFIG_FS_POSIX_ACL is not set

Quoted 7 lines · Christoph Hellwig
Quoted 6 lines · Markus Trippelsdorf

commit 4e34e719e45, that takes the ACL checks to common code, accidentely broke the build when CONFIG_FS_POSIX_ACL is not set:

CC fs/xfs/linux-2.6/xfs_iops.o
fs/xfs/linux-2.6/xfs_iops.c:1025:14: error: ???xfs_get_acl??? undeclared here (not in a function)

Quoted 6 lines
Quoted 1 line

Fix this by declaring xfs_get_acl a static inline function.

Do you have OPTIMIZE_INLINING enabled? In general I dont think we can take the address of an inline function, so without this option I far the code might not compile, in which case we'll need ifdefs around the inode operation assignment in addition to your patch.

No, "# CONFIG_OPTIMIZE_INLINING is not set".
I've tested this patch with gcc 4.5.2, 4.6.1 and 4.7.0 and in all cases the kernel builds just fine.

Signature · 1 line
Markus
#10 lore raw

Re: [PATCH] xfs: Fix build breakage in xfs_iops.c when CONFIG_FS_POSIX_ACL is not set

Quoted 3 lines · Markus Trippelsdorf

No, "# CONFIG_OPTIMIZE_INLINING is not set".
I've tested this patch with gcc 4.5.2, 4.6.1 and 4.7.0 and in all cases the kernel builds just fine.

In that case we might as well just throw it in and try it. In the worst case we'll get one expansion per site that takes the address, but no-acl builds aren't really something worth optimizing for.

#11 lore raw

Re: [PATCH] xfs: Fix build breakage in xfs_iops.c when CONFIG_FS_POSIX_ACL is not set

Quoted 4 lines · Christoph Hellwig

In that case we might as well just throw it in and try it.  In the worst case we'll get one expansion per site that takes the address, but no-acl builds aren't really something worth optimizing for.

If ACL's are enabled on a system level, but not in XFS, this is more than a small optimization. It kills the RCU walk entirely if you just return NULL without setting the cache.

Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#12 lore raw

Re: [PATCH] xfs: Fix build breakage in xfs_iops.c when CONFIG_FS_POSIX_ACL is not set

In reply to Christoph Hellwig
Quoted 7 lines · Christoph Hellwig
Quoted 6 lines · Markus Trippelsdorf

commit 4e34e719e45, that takes the ACL checks to common code, accidentely broke the build when CONFIG_FS_POSIX_ACL is not set:

CC fs/xfs/linux-2.6/xfs_iops.o
fs/xfs/linux-2.6/xfs_iops.c:1025:14: error: ???xfs_get_acl??? undeclared here (not in a function)

Quoted 6 lines
Quoted 1 line

Fix this by declaring xfs_get_acl a static inline function.

Do you have OPTIMIZE_INLINING enabled? In general I dont think we can take the address of an inline function, so without this option I far the code might not compile, in which case we'll need ifdefs around the inode operation assignment in addition to your patch.

FYI, this build bug is still present upstream and triggers rather frequently in randconfig testing.

Also, according to the timestamps of the guilty commit (4e34e719e45) it had near zero linux-next testing time before it was pushed upstream - could we improve on that please?

Thanks,

Ingo

#13 lore raw

Re: [PATCH] xfs: Fix build breakage in xfs_iops.c when CONFIG_FS_POSIX_ACL is not set

In reply to Markus Trippelsdorf
Quoted 19 lines · Markus Trippelsdorf

commit 4e34e719e45, that takes the ACL checks to common code, accidentely broke the build when CONFIG_FS_POSIX_ACL is not set:

CC fs/xfs/linux-2.6/xfs_iops.o
fs/xfs/linux-2.6/xfs_iops.c:1025:14: error: ‘xfs_get_acl’ undeclared here (not in a function)

Fix this by declaring xfs_get_acl a static inline function.

Signed-off-by Markus Trippelsdorf <[email protected]>
---
 fs/xfs/xfs_acl.h |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)
fs/xfs/xfs_acl.h +0 -0
index 2c656ef..39632d9 100644
@@ -51,7 +51,10 @@ extern int posix_acl_default_exists(struct inode *inode);
 extern const struct xattr_handler xfs_xattr_acl_access_handler;

fs/xfs/xfs_acl.h extern int posix_acl_default_exists(struct inode *inode); · line 60

Quoted 10 lines
fs/xfs/xfs_acl.h +4 -1
 extern const struct xattr_handler xfs_xattr_acl_default_handler;
 #else
-# define xfs_get_acl(inode, type)			NULL
+static inline struct posix_acl *xfs_get_acl(struct inode *inode, int type)
+{
+	return NULL;
+}
 # define xfs_inherit_acl(inode, default_acl)		0
 # define xfs_acl_chmod(inode)				0
 # define posix_acl_access_exists(inode)			0

Linus, I have done basic testing of this patch (build, boot, and some simple XFS tests that touch ACLs) using your latest master branch (55f9c40). I tested three combinations:
CONFIG_XFS_POSIX_ACL (and CONFIG_FS_POSIX_ACL) on CONFIG_XFS_POSIX_ACL off, CONFIG_FS_POSIX_ACL on CONFIG_XFS_POSIX_ACL and CONFIG_FS_POSIX_ACL both off

I confirmed the build problem in the last two configs without this patch, and verified it went away with the patch applied.

So all is well from my perspective. Please either take his original patch, or pull from here:

git://oss.sgi.com/xfs/xfs for-linus

Thanks.

Signed-off-by Alex Elder <[email protected]>

--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#14 lore raw
In reply to Markus Trippelsdorf
Quoted 2 lines · Markus Trippelsdorf

#ifdefs are ugly. The following fixes the issue for me:

Actually, I think the proper fix is just the following (whitespace-damaged). It's better to just set ".get_acl" to NULL, and it's what we used to do back when it was called "check_acl" (see commit 4e34e719e45).

Christoph?

Linus

---
 fs/xfs/xfs_acl.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
fs/xfs/xfs_acl.h +1 -1
index 2c656ef49473..b97a6edcab9d 100644
@@ -51,7 +51,7 @@ extern int posix_acl_default_exists(struct inode *inode);
 extern const struct xattr_handler xfs_xattr_acl_access_handler;
 extern const struct xattr_handler xfs_xattr_acl_default_handler;
 #else
-# define xfs_get_acl(inode, type)                      NULL
+# define xfs_get_acl                                   NULL
 # define xfs_inherit_acl(inode, default_acl)           0
 # define xfs_acl_chmod(inode)                          0
 # define posix_acl_access_exists(inode)                        0
#15 lore raw
Quoted 10 lines · Linus Torvalds
Quoted 2 lines · Markus Trippelsdorf

#ifdefs are ugly. The following fixes the issue for me:

Actually, I think the proper fix is just the following (whitespace-damaged). It's better to just set ".get_acl" to NULL, and it's what we used to do back when it was called "check_acl" (see commit 4e34e719e45).

Yes, that was my first idea, too. But unfortunately it doesn't work:

CC fs/xfs/linux-2.6/xfs_iops.o
fs/xfs/linux-2.6/xfs_iops.c: In function ‘xfs_vn_mknod’:
fs/xfs/linux-2.6/xfs_iops.c:184:28: error: called object ‘0u’ is not a function make[2]: *** [fs/xfs/linux-2.6/xfs_iops.o] Error 1

Signature · 5 lines
Markus
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
#16 lore raw
Quoted 7 lines · Markus Trippelsdorf

Yes, that was my first idea, too. But unfortunately it doesn't work:

 CC      fs/xfs/linux-2.6/xfs_iops.o
fs/xfs/linux-2.6/xfs_iops.c: In function ‘xfs_vn_mknod’:
fs/xfs/linux-2.6/xfs_iops.c:184:28: error: called object ‘0u’ is not a function make[2]: *** [fs/xfs/linux-2.6/xfs_iops.o] Error 1

Ahh, we use that elsewhere too.

However, if we *do* have POSIX ACL's enabled, but we just have the XFS ACL's disabled, then just returning NULL from xfs_get_acl() is actually a horrible idea, because then the VFS code will always call that function and it never gets cached as "I have no ACL's". Which is why I didn't like the inline function approach. Well, at least not the one that only returns NULL - we also want to fill in the cache.

I'm actually getting more and more convinced that we should just do that "set_acl_cache()" in generic code for the "get_acl()" case. Even without any locking it's no worse than all the current filesystems, since they *also* do it with no locking. And then this kind of issue wouldn't come up.

Christoph, Al, comments?

Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#17 lore raw
Quoted 6 lines · Linus Torvalds

However, if we *do* have POSIX ACL's enabled, but we just have the XFS ACL's disabled, then just returning NULL from xfs_get_acl() is actually a horrible idea, because then the VFS code will always call that function and it never gets cached as "I have no ACL's". Which is why I didn't like the inline function approach. Well, at least not the one that only returns NULL - we also want to fill in the cache.

Even getting to check_acl() requires MS_POSIXACL to be set on the superblock, which it won't be if CONFIG_XFS_ACL isn't set.

These days we can probably kill CONFIG_FOOFS_ACL given that everyone now uses the generic code, and due to the deeper VFS interaction is more or less forced to anyway. Historically that wasn't the case.

Quoted 5 lines

I'm actually getting more and more convinced that we should just do that "set_acl_cache()" in generic code for the "get_acl()" case. Even without any locking it's no worse than all the current filesystems, since they *also* do it with no locking. And then this kind of issue wouldn't come up.

While I agree with you that we should life it into the common code, the common caching is not related at all to this particular issue.

The code that fails to compile is where XFS implements the default ACL inheritance when creating new inodes. Move filesystems do this a bit simpler and dumber and do the ACL-induced i_mode modifications after creating the inode. XFS tries to create the inode with the correct mode from the first moment and thus has a slightly broader interface between the "normal" inode ops and its ACL code.

Due to various interactions with the fs transaction code the inheritance code is at the very end of my list of ACL pieces to move to common code.

#18 lore raw
Quoted 3 lines · Christoph Hellwig

Even getting to check_acl() requires MS_POSIXACL to be set on the superblock, which it won't be if CONFIG_XFS_ACL isn't set.

Ahh, ok. That thing is well-hidden behind that IS_POSIXACL() thing.. And it turns out that now that I see it, I shudder. That's another of those potentially expensive inode indirect pointer accesses - but I haven't seen it because my normal test-workload is as the owner of the tree I'm traversing.

Quoted 3 lines

These days we can probably kill CONFIG_FOOFS_ACL given that everyone now uses the generic code, and due to the deeper VFS interaction is more or less forced to anyway.  Historically that wasn't the case.

Yeah. And these days pretty much every single distribution uses ACL's, which also didn't use to be the case (or even if they used them, it wasn't a requirement: now it tends to be).

Quoted 2 lines

While I agree with you that we should life it into the common code, the common caching is not related at all to this particular issue.

Yeah, I had missed the hidden MS_POSIX_ACL test.

So I agree, the plain NULL return for "no ACL" works fine.

Do we have a signed-off version with a changelog etc?

Linus

#19 lore raw
In reply to Markus Trippelsdorf
Quoted 9 lines · Markus Trippelsdorf
Quoted 5 lines · Al Viro

Regression fixes (devtmpfs race, cifs ->d_revalidate() breakage, fsync fallout), ACL stuff (me/Linus/Christoph), fixes for bugs found while

Linus Torvalds (1):
vfs: move ACL cache lookup into generic code

This breaks the build for "# CONFIG_FS_POSIX_ACL is not set" configs:

Quoted 7 lines

CC fs/namei.o
fs/namei.c: In function ‘check_acl’:
fs/namei.c:191:10: error: implicit declaration of function ‘negative_cached_acl’ [-Werror=implicit-function-declaration] fs/namei.c:196:2: error: implicit declaration of function ‘get_cached_acl’ [-Werror=implicit-function-declaration] fs/namei.c:196:6: warning: assignment makes pointer from integer without a cast [enabled by default] fs/namei.c:212:11: error: implicit declaration of function ‘set_cached_acl’ [-Werror=implicit-function-declaration]
cc1: some warnings being treated as errors

Ditto.
(I'm using the staging-2.6 tree.)
regards,
ali

Quoted 3 lines
Signature · 2 lines
-- 
Markus

--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html