| Message ID | 1391201970.6978.1.camel@leira.trondhjem.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jan 31, 2014 at 03:59:30PM -0500, Trond Myklebust wrote: > On Thu, 2014-01-30 at 15:38 +0000, Russell King - ARM Linux wrote: > > On Thu, Jan 30, 2014 at 06:32:08AM -0800, Christoph Hellwig wrote: > > > On Thu, Jan 30, 2014 at 02:27:52PM +0000, Russell King - ARM Linux wrote: > > > > Yes and no. I still end up with an empty /etc/mtab, but the file now > > > > exists. However, I can create and echo data into /etc/mtab, but it seems > > > > that can't happen at boot time. > > > > > > Odd. Can you disable CONFIG_NFSD_V3_ACL for now to isolate the issue? > > > > Unfortunately, that results in some problem at boot time, which > > ultimately ends up with the other three CPUs being stopped, and > > hence the original reason scrolls off the screen before it can be > > read... even at 1920p. > > > Hi Russell, > > The following patch fixes the issue for me. It doesn't entirely fix the issue for me, instead we've got even weirder behaviour: root@cubox-i4:~# ls -al test ls: cannot access test: No such file or directory root@cubox-i4:~# touch test root@cubox-i4:~# ls -al test -rw-r--r-- 1 root root 0 Feb 1 01:01 test root@cubox-i4:~# echo foo > test root@cubox-i4:~# ls -al test -rw-r--r-- 1 root root 4 Feb 1 01:01 test root@cubox-i4:~# cat test foo root@cubox-i4:~# rm test root@cubox-i4:~# echo foo > test -bash: test: Operation not supported root@cubox-i4:~# ls -al test -rw-r--r-- 1 root root 0 Feb 1 01:01 test
On Sat, Feb 01, 2014 at 01:03:28AM +0000, Russell King - ARM Linux wrote: > On Fri, Jan 31, 2014 at 03:59:30PM -0500, Trond Myklebust wrote: > > On Thu, 2014-01-30 at 15:38 +0000, Russell King - ARM Linux wrote: > > > On Thu, Jan 30, 2014 at 06:32:08AM -0800, Christoph Hellwig wrote: > > > > On Thu, Jan 30, 2014 at 02:27:52PM +0000, Russell King - ARM Linux wrote: > > > > > Yes and no. I still end up with an empty /etc/mtab, but the file now > > > > > exists. However, I can create and echo data into /etc/mtab, but it seems > > > > > that can't happen at boot time. > > > > > > > > Odd. Can you disable CONFIG_NFSD_V3_ACL for now to isolate the issue? > > > > > > Unfortunately, that results in some problem at boot time, which > > > ultimately ends up with the other three CPUs being stopped, and > > > hence the original reason scrolls off the screen before it can be > > > read... even at 1920p. > > > > > Hi Russell, > > > > The following patch fixes the issue for me. > > It doesn't entirely fix the issue for me, instead we've got even weirder > behaviour: > > root@cubox-i4:~# ls -al test > ls: cannot access test: No such file or directory > root@cubox-i4:~# touch test > root@cubox-i4:~# ls -al test > -rw-r--r-- 1 root root 0 Feb 1 01:01 test > root@cubox-i4:~# echo foo > test > root@cubox-i4:~# ls -al test > -rw-r--r-- 1 root root 4 Feb 1 01:01 test > root@cubox-i4:~# cat test > foo > root@cubox-i4:~# rm test > root@cubox-i4:~# echo foo > test > -bash: test: Operation not supported > root@cubox-i4:~# ls -al test > -rw-r--r-- 1 root root 0 Feb 1 01:01 test FYI, I just tested Linus' tip, and NFS is still broken.
On Fri, Jan 31, 2014 at 03:59:30PM -0500, Trond Myklebust wrote: > posix_acl_xattr_get requires get_acl() to return EOPNOTSUPP if the > filesystem cannot support acls. This is needed for NFS, which can't > know whether or not the server supports acls until it tries to get/set > one. > This patch converts posix_acl_chmod and posix_acl_create to deal with > EOPNOTSUPP return values from get_acl(). Shouldn't NFS just return a NULL ACL here?
On Feb 3, 2014, at 3:03, Christoph Hellwig <hch@infradead.org> wrote: > On Fri, Jan 31, 2014 at 03:59:30PM -0500, Trond Myklebust wrote: >> posix_acl_xattr_get requires get_acl() to return EOPNOTSUPP if the >> filesystem cannot support acls. This is needed for NFS, which can't >> know whether or not the server supports acls until it tries to get/set >> one. >> This patch converts posix_acl_chmod and posix_acl_create to deal with >> EOPNOTSUPP return values from get_acl(). > > Shouldn't NFS just return a NULL ACL here? As I said above, that causes posix_acl_xattr_get() to return the wrong answer (ENODATA instead of EOPNOTSUPP). -- Trond Myklebust Linux NFS client maintainer
On Mon, Feb 03, 2014 at 09:17:30AM -0500, Trond Myklebust wrote:
> As I said above, that causes posix_acl_xattr_get() to return the wrong answer (ENODATA instead of EOPNOTSUPP).
Is it really the wrong answer? How does userspace care wether this
server doesn't support ACLs at all or none is set? The resulting
behavior is the same.
If there's a good reason to care we might have to go with your patch,
but if we can avoid it I'd prefer to keep things simple.
On Feb 3, 2014, at 9:57, Christoph Hellwig <hch@infradead.org> wrote: > On Mon, Feb 03, 2014 at 09:17:30AM -0500, Trond Myklebust wrote: >> As I said above, that causes posix_acl_xattr_get() to return the wrong answer (ENODATA instead of EOPNOTSUPP). > > Is it really the wrong answer? How does userspace care wether this > server doesn't support ACLs at all or none is set? The resulting > behavior is the same. It will certainly cause acl_get_file() to behave differently than previously. I’ve no idea how that will affect applications, though. > If there's a good reason to care we might have to go with your patch, > but if we can avoid it I'd prefer to keep things simple. One alternative is to simply wrap posix_acl_xattr_get() in fs/nfs/nfs3acl.c, and have it check the value of nfs_server_capable(inode, NFS_CAP_ACLS) before returning ENODATA. That’s rather ugly too... -- Trond Myklebust Linux NFS client maintainer
Looks good as the lesser evil:
Reviewed-by: Christoph Hellwig <hch@lst.de>
diff --git a/fs/posix_acl.c b/fs/posix_acl.c index 38bae5a0ea25..11c54fd51e16 100644 --- a/fs/posix_acl.c +++ b/fs/posix_acl.c @@ -521,8 +521,11 @@ posix_acl_chmod(struct inode *inode, umode_t mode) return -EOPNOTSUPP; acl = get_acl(inode, ACL_TYPE_ACCESS); - if (IS_ERR_OR_NULL(acl)) + if (IS_ERR_OR_NULL(acl)) { + if (acl == ERR_PTR(-EOPNOTSUPP)) + return 0; return PTR_ERR(acl); + } ret = __posix_acl_chmod(&acl, GFP_KERNEL, mode); if (ret) @@ -544,14 +547,15 @@ posix_acl_create(struct inode *dir, umode_t *mode, goto no_acl; p = get_acl(dir, ACL_TYPE_DEFAULT); - if (IS_ERR(p)) + if (IS_ERR(p)) { + if (p == ERR_PTR(-EOPNOTSUPP)) + goto apply_umask; return PTR_ERR(p); - - if (!p) { - *mode &= ~current_umask(); - goto no_acl; } + if (!p) + goto apply_umask; + *acl = posix_acl_clone(p, GFP_NOFS); if (!*acl) return -ENOMEM; @@ -575,6 +579,8 @@ posix_acl_create(struct inode *dir, umode_t *mode, } return 0; +apply_umask: + *mode &= ~current_umask(); no_acl: *default_acl = NULL; *acl = NULL;