* NFSv4 ACL set and inode attribute cache @ 2010-11-10 18:01 Aneesh Kumar K.V 2010-11-10 18:51 ` Aneesh Kumar K. V 0 siblings, 1 reply; 9+ messages in thread From: Aneesh Kumar K.V @ 2010-11-10 18:01 UTC (permalink / raw) To: Trond Myklebust; +Cc: linux-nfs Hi, I guess we are not marking the inode attribute as invalid when we set the ACL value. For ex: /d# mkdir sub3 /d# ls -dl sub3 drwxr-xr-x 2 root root 4096 Nov 10 17:56 sub3 /d# nfs4_setfacl -s A:fd:EVERYONE@:rwax sub3 /d# ls -dl sub3 drwxr-xr-x 2 root root 4096 Nov 10 17:56 sub3 /d# On the server i have the mode bits as /d# ls -dl sub3 drwxrwxrwx 2 root root 4096 Nov 10 17:56 sub3 /d# -aneesh ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: NFSv4 ACL set and inode attribute cache 2010-11-10 18:01 NFSv4 ACL set and inode attribute cache Aneesh Kumar K.V @ 2010-11-10 18:51 ` Aneesh Kumar K. V 2010-11-10 20:31 ` J. Bruce Fields 2010-11-12 6:23 ` Aneesh Kumar K. V 0 siblings, 2 replies; 9+ messages in thread From: Aneesh Kumar K. V @ 2010-11-10 18:51 UTC (permalink / raw) To: Trond Myklebust; +Cc: linux-nfs On Wed, 10 Nov 2010 23:31:31 +0530, Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com> wrote: > > Hi, > > I guess we are not marking the inode attribute as invalid when we set > the ACL value. For ex: > > /d# mkdir sub3 > /d# ls -dl sub3 > drwxr-xr-x 2 root root 4096 Nov 10 17:56 sub3 > /d# nfs4_setfacl -s A:fd:EVERYONE@:rwax sub3 > /d# ls -dl sub3 > drwxr-xr-x 2 root root 4096 Nov 10 17:56 sub3 > /d# > > > On the server i have the mode bits as > /d# ls -dl sub3 > drwxrwxrwx 2 root root 4096 Nov 10 17:56 sub3 > /d# We also have similar issue other way round. ie setting the mode bits don't result in ACL values being invalidated. But a second request get the right value of ACL as show below. /d# nfs4_getfacl x A::OWNER@:rw A::GROUP@:rw A::EVERYONE@:r /d# chmod 600 x /d# nfs4_getfacl x A::OWNER@:rw A::GROUP@:rw A::EVERYONE@:r /d# Expected value is /d# nfs4_getfacl x A::OWNER@:rw -aneesh ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: NFSv4 ACL set and inode attribute cache 2010-11-10 18:51 ` Aneesh Kumar K. V @ 2010-11-10 20:31 ` J. Bruce Fields 2010-11-12 6:23 ` Aneesh Kumar K. V 1 sibling, 0 replies; 9+ messages in thread From: J. Bruce Fields @ 2010-11-10 20:31 UTC (permalink / raw) To: Aneesh Kumar K. V; +Cc: Trond Myklebust, linux-nfs On Thu, Nov 11, 2010 at 12:21:27AM +0530, Aneesh Kumar K. V wrote: > On Wed, 10 Nov 2010 23:31:31 +0530, Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com> wrote: > > > > Hi, > > > > I guess we are not marking the inode attribute as invalid when we set > > the ACL value. For ex: > > > > /d# mkdir sub3 > > /d# ls -dl sub3 > > drwxr-xr-x 2 root root 4096 Nov 10 17:56 sub3 > > /d# nfs4_setfacl -s A:fd:EVERYONE@:rwax sub3 > > /d# ls -dl sub3 > > drwxr-xr-x 2 root root 4096 Nov 10 17:56 sub3 > > /d# > > > > > > On the server i have the mode bits as > > /d# ls -dl sub3 > > drwxrwxrwx 2 root root 4096 Nov 10 17:56 sub3 > > /d# > > We also have similar issue other way round. ie setting the mode bits > don't result in ACL values being invalidated. But a second request get > the right value of ACL as show below. Both look like bugs to me. (I remember their being pointed out before, though--did we drop the ball, or was there some reason for the current behavior?) --b. > > /d# nfs4_getfacl x > A::OWNER@:rw > A::GROUP@:rw > A::EVERYONE@:r > /d# chmod 600 x > /d# nfs4_getfacl x > A::OWNER@:rw > A::GROUP@:rw > A::EVERYONE@:r > /d# > > Expected value is > > /d# nfs4_getfacl x > A::OWNER@:rw > > -aneesh > -- > To unsubscribe from this list: send the line "unsubscribe linux-nfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: NFSv4 ACL set and inode attribute cache 2010-11-10 18:51 ` Aneesh Kumar K. V 2010-11-10 20:31 ` J. Bruce Fields @ 2010-11-12 6:23 ` Aneesh Kumar K. V 2010-11-29 10:16 ` Aneesh Kumar K. V 1 sibling, 1 reply; 9+ messages in thread From: Aneesh Kumar K. V @ 2010-11-12 6:23 UTC (permalink / raw) To: Trond Myklebust; +Cc: linux-nfs On Thu, 11 Nov 2010 00:21:27 +0530, "Aneesh Kumar K. V" <aneesh.kumar@linux.vnet.ibm.com> wrote: > On Wed, 10 Nov 2010 23:31:31 +0530, Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com> wrote: > > > > Hi, > > > > I guess we are not marking the inode attribute as invalid when we set > > the ACL value. For ex: > > > > /d# mkdir sub3 > > /d# ls -dl sub3 > > drwxr-xr-x 2 root root 4096 Nov 10 17:56 sub3 > > /d# nfs4_setfacl -s A:fd:EVERYONE@:rwax sub3 > > /d# ls -dl sub3 > > drwxr-xr-x 2 root root 4096 Nov 10 17:56 sub3 > > /d# > > > > > > On the server i have the mode bits as > > /d# ls -dl sub3 > > drwxrwxrwx 2 root root 4096 Nov 10 17:56 sub3 > > /d# > > We also have similar issue other way round. ie setting the mode bits > don't result in ACL values being invalidated. But a second request get > the right value of ACL as show below. > > /d# nfs4_getfacl x > A::OWNER@:rw > A::GROUP@:rw > A::EVERYONE@:r > /d# chmod 600 x > /d# nfs4_getfacl x > A::OWNER@:rw > A::GROUP@:rw > A::EVERYONE@:r > /d# > > Expected value is > > /d# nfs4_getfacl x > A::OWNER@:rw > The below patch fix the problem for me. If this is the right way to fix, I can send a proper patch with commit message and s-o-b. diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c index 0f24cdf..666a48b 100644 --- a/fs/nfs/nfs4proc.c +++ b/fs/nfs/nfs4proc.c @@ -3359,6 +3359,8 @@ static ssize_t nfs4_proc_get_acl(struct inode *inode, void *buf, size_t buflen) ret = nfs_revalidate_inode(server, inode); if (ret < 0) return ret; + if (NFS_I(inode)->cache_validity & NFS_INO_INVALID_ACL) + nfs_zap_acl_cache(inode); ret = nfs4_read_cached_acl(inode, buf, buflen); if (ret != -ENOENT) return ret; @@ -3387,6 +3389,11 @@ static int __nfs4_proc_set_acl(struct inode *inode, const void *buf, size_t bufl nfs_inode_return_delegation(inode); buf_to_pages(buf, buflen, arg.acl_pages, &arg.acl_pgbase); ret = nfs4_call_sync(server, &msg, &arg, &res, 1); + /* + * Acl update can result in inode attribute update. + * so mark the attribute cache invalid. + */ + NFS_I(inode)->cache_validity |= NFS_INO_INVALID_ATTR; nfs_access_zap_cache(inode); nfs_zap_acl_cache(inode); return ret; ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: NFSv4 ACL set and inode attribute cache 2010-11-12 6:23 ` Aneesh Kumar K. V @ 2010-11-29 10:16 ` Aneesh Kumar K. V 2010-11-29 20:13 ` Trond Myklebust 0 siblings, 1 reply; 9+ messages in thread From: Aneesh Kumar K. V @ 2010-11-29 10:16 UTC (permalink / raw) To: Trond Myklebust; +Cc: linux-nfs On Fri, 12 Nov 2010 11:53:20 +0530, "Aneesh Kumar K. V" <aneesh.kumar@linux.vnet.ibm.com> wrote: > On Thu, 11 Nov 2010 00:21:27 +0530, "Aneesh Kumar K. V" <aneesh.kumar@linux.vnet.ibm.com> wrote: > > On Wed, 10 Nov 2010 23:31:31 +0530, Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com> wrote: > > > > > > Hi, > > > > > > I guess we are not marking the inode attribute as invalid when we set > > > the ACL value. For ex: > > > > > > /d# mkdir sub3 > > > /d# ls -dl sub3 > > > drwxr-xr-x 2 root root 4096 Nov 10 17:56 sub3 > > > /d# nfs4_setfacl -s A:fd:EVERYONE@:rwax sub3 > > > /d# ls -dl sub3 > > > drwxr-xr-x 2 root root 4096 Nov 10 17:56 sub3 > > > /d# > > > > > > > > > On the server i have the mode bits as > > > /d# ls -dl sub3 > > > drwxrwxrwx 2 root root 4096 Nov 10 17:56 sub3 > > > /d# > > > > We also have similar issue other way round. ie setting the mode bits > > don't result in ACL values being invalidated. But a second request get > > the right value of ACL as show below. > > > > /d# nfs4_getfacl x > > A::OWNER@:rw > > A::GROUP@:rw > > A::EVERYONE@:r > > /d# chmod 600 x > > /d# nfs4_getfacl x > > A::OWNER@:rw > > A::GROUP@:rw > > A::EVERYONE@:r > > /d# > > > > Expected value is > > > > /d# nfs4_getfacl x > > A::OWNER@:rw > > > > The below patch fix the problem for me. If this is the right way > to fix, I can send a proper patch with commit message and s-o-b. > > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c > index 0f24cdf..666a48b 100644 > --- a/fs/nfs/nfs4proc.c > +++ b/fs/nfs/nfs4proc.c > @@ -3359,6 +3359,8 @@ static ssize_t nfs4_proc_get_acl(struct inode *inode, void *buf, size_t buflen) > ret = nfs_revalidate_inode(server, inode); > if (ret < 0) > return ret; > + if (NFS_I(inode)->cache_validity & NFS_INO_INVALID_ACL) > + nfs_zap_acl_cache(inode); > ret = nfs4_read_cached_acl(inode, buf, buflen); > if (ret != -ENOENT) > return ret; > @@ -3387,6 +3389,11 @@ static int __nfs4_proc_set_acl(struct inode *inode, const void *buf, size_t bufl > nfs_inode_return_delegation(inode); > buf_to_pages(buf, buflen, arg.acl_pages, &arg.acl_pgbase); > ret = nfs4_call_sync(server, &msg, &arg, &res, 1); > + /* > + * Acl update can result in inode attribute update. > + * so mark the attribute cache invalid. > + */ > + NFS_I(inode)->cache_validity |= NFS_INO_INVALID_ATTR; > nfs_access_zap_cache(inode); > nfs_zap_acl_cache(inode); > return ret; Any update on this ? Another option i figured out today is to make sure we add FATTR4_WORD0_ACL in nfs4_fattr_bitmap for fetching the modified acl value on mode update. Similarly setfacl can be compounded with the getattr request. -aneesh ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: NFSv4 ACL set and inode attribute cache 2010-11-29 10:16 ` Aneesh Kumar K. V @ 2010-11-29 20:13 ` Trond Myklebust 2010-11-30 18:03 ` Aneesh Kumar K. V 0 siblings, 1 reply; 9+ messages in thread From: Trond Myklebust @ 2010-11-29 20:13 UTC (permalink / raw) To: Aneesh Kumar K. V; +Cc: linux-nfs On Mon, 2010-11-29 at 15:46 +0530, Aneesh Kumar K. V wrote: > On Fri, 12 Nov 2010 11:53:20 +0530, "Aneesh Kumar K. V" <aneesh.kumar@linux.vnet.ibm.com> wrote: > > On Thu, 11 Nov 2010 00:21:27 +0530, "Aneesh Kumar K. V" <aneesh.kumar@linux.vnet.ibm.com> wrote: > > > On Wed, 10 Nov 2010 23:31:31 +0530, Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com> wrote: > > > > > > > > Hi, > > > > > > > > I guess we are not marking the inode attribute as invalid when we set > > > > the ACL value. For ex: > > > > > > > > /d# mkdir sub3 > > > > /d# ls -dl sub3 > > > > drwxr-xr-x 2 root root 4096 Nov 10 17:56 sub3 > > > > /d# nfs4_setfacl -s A:fd:EVERYONE@:rwax sub3 > > > > /d# ls -dl sub3 > > > > drwxr-xr-x 2 root root 4096 Nov 10 17:56 sub3 > > > > /d# > > > > > > > > > > > > On the server i have the mode bits as > > > > /d# ls -dl sub3 > > > > drwxrwxrwx 2 root root 4096 Nov 10 17:56 sub3 > > > > /d# > > > > > > We also have similar issue other way round. ie setting the mode bits > > > don't result in ACL values being invalidated. But a second request get > > > the right value of ACL as show below. > > > > > > /d# nfs4_getfacl x > > > A::OWNER@:rw > > > A::GROUP@:rw > > > A::EVERYONE@:r > > > /d# chmod 600 x > > > /d# nfs4_getfacl x > > > A::OWNER@:rw > > > A::GROUP@:rw > > > A::EVERYONE@:r > > > /d# > > > > > > Expected value is > > > > > > /d# nfs4_getfacl x > > > A::OWNER@:rw > > > > > > > The below patch fix the problem for me. If this is the right way > > to fix, I can send a proper patch with commit message and s-o-b. > > > > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c > > index 0f24cdf..666a48b 100644 > > --- a/fs/nfs/nfs4proc.c > > +++ b/fs/nfs/nfs4proc.c > > @@ -3359,6 +3359,8 @@ static ssize_t nfs4_proc_get_acl(struct inode *inode, void *buf, size_t buflen) > > ret = nfs_revalidate_inode(server, inode); > > if (ret < 0) > > return ret; > > + if (NFS_I(inode)->cache_validity & NFS_INO_INVALID_ACL) > > + nfs_zap_acl_cache(inode); > > ret = nfs4_read_cached_acl(inode, buf, buflen); > > if (ret != -ENOENT) > > return ret; > > @@ -3387,6 +3389,11 @@ static int __nfs4_proc_set_acl(struct inode *inode, const void *buf, size_t bufl > > nfs_inode_return_delegation(inode); > > buf_to_pages(buf, buflen, arg.acl_pages, &arg.acl_pgbase); > > ret = nfs4_call_sync(server, &msg, &arg, &res, 1); > > + /* > > + * Acl update can result in inode attribute update. > > + * so mark the attribute cache invalid. > > + */ > > + NFS_I(inode)->cache_validity |= NFS_INO_INVALID_ATTR; This needs to be done under the correct spin locks, so please use the helper nfs_mark_for_revalidate() instead of attempting to open coding it. > > nfs_access_zap_cache(inode); > > nfs_zap_acl_cache(inode); > > return ret; > > > Any update on this ? Another option i figured out today is to make sure > we add FATTR4_WORD0_ACL in nfs4_fattr_bitmap for fetching the modified > acl value on mode update. Similarly setfacl can be compounded with the > getattr request. We actually used to compound setacl with a GETATTR(FATTR4_WORD0_ACL) in order to ensure that the server sets it correctly. Unfortunately, that caused some servers to return NFS4ERR_RESOURCE due to the burden of caching all that acl information in the duplicate request queue. I'd be fine with adding an ordinary getattr request, though, as long as it is allowed to fail (i.e. the result of __nfs4_proc_set_acl() must only depend on the setacl). Trond -- Trond Myklebust Linux NFS client maintainer NetApp Trond.Myklebust@netapp.com www.netapp.com ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: NFSv4 ACL set and inode attribute cache 2010-11-29 20:13 ` Trond Myklebust @ 2010-11-30 18:03 ` Aneesh Kumar K. V 2010-11-30 18:38 ` J. Bruce Fields 2010-11-30 18:40 ` Trond Myklebust 0 siblings, 2 replies; 9+ messages in thread From: Aneesh Kumar K. V @ 2010-11-30 18:03 UTC (permalink / raw) To: Trond Myklebust; +Cc: linux-nfs On Mon, 29 Nov 2010 15:13:50 -0500, Trond Myklebust <Trond.Myklebust@netapp.com> wrote: > On Mon, 2010-11-29 at 15:46 +0530, Aneesh Kumar K. V wrote: > > On Fri, 12 Nov 2010 11:53:20 +0530, "Aneesh Kumar K. V" <aneesh.kumar@linux.vnet.ibm.com> wrote: > > > On Thu, 11 Nov 2010 00:21:27 +0530, "Aneesh Kumar K. V" <aneesh.kumar@linux.vnet.ibm.com> wrote: > > > > On Wed, 10 Nov 2010 23:31:31 +0530, Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com> wrote: > > > > > > > > > > Hi, > > > > > > > > > > I guess we are not marking the inode attribute as invalid when we set > > > > > the ACL value. For ex: > > > > > > > > > > /d# mkdir sub3 > > > > > /d# ls -dl sub3 > > > > > drwxr-xr-x 2 root root 4096 Nov 10 17:56 sub3 > > > > > /d# nfs4_setfacl -s A:fd:EVERYONE@:rwax sub3 > > > > > /d# ls -dl sub3 > > > > > drwxr-xr-x 2 root root 4096 Nov 10 17:56 sub3 > > > > > /d# > > > > > > > > > > > > > > > On the server i have the mode bits as > > > > > /d# ls -dl sub3 > > > > > drwxrwxrwx 2 root root 4096 Nov 10 17:56 sub3 > > > > > /d# > > > > > > > > We also have similar issue other way round. ie setting the mode bits > > > > don't result in ACL values being invalidated. But a second request get > > > > the right value of ACL as show below. > > > > > > > > /d# nfs4_getfacl x > > > > A::OWNER@:rw > > > > A::GROUP@:rw > > > > A::EVERYONE@:r > > > > /d# chmod 600 x > > > > /d# nfs4_getfacl x > > > > A::OWNER@:rw > > > > A::GROUP@:rw > > > > A::EVERYONE@:r > > > > /d# > > > > > > > > Expected value is > > > > > > > > /d# nfs4_getfacl x > > > > A::OWNER@:rw > > > > > > > > > > The below patch fix the problem for me. If this is the right way > > > to fix, I can send a proper patch with commit message and s-o-b. > > > > > > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c > > > index 0f24cdf..666a48b 100644 > > > --- a/fs/nfs/nfs4proc.c > > > +++ b/fs/nfs/nfs4proc.c > > > @@ -3359,6 +3359,8 @@ static ssize_t nfs4_proc_get_acl(struct inode *inode, void *buf, size_t buflen) > > > ret = nfs_revalidate_inode(server, inode); > > > if (ret < 0) > > > return ret; > > > + if (NFS_I(inode)->cache_validity & NFS_INO_INVALID_ACL) > > > + nfs_zap_acl_cache(inode); > > > ret = nfs4_read_cached_acl(inode, buf, buflen); > > > if (ret != -ENOENT) > > > return ret; > > > @@ -3387,6 +3389,11 @@ static int __nfs4_proc_set_acl(struct inode *inode, const void *buf, size_t bufl > > > nfs_inode_return_delegation(inode); > > > buf_to_pages(buf, buflen, arg.acl_pages, &arg.acl_pgbase); > > > ret = nfs4_call_sync(server, &msg, &arg, &res, 1); > > > + /* > > > + * Acl update can result in inode attribute update. > > > + * so mark the attribute cache invalid. > > > + */ > > > + NFS_I(inode)->cache_validity |= NFS_INO_INVALID_ATTR; > > This needs to be done under the correct spin locks, so please use the > helper nfs_mark_for_revalidate() instead of attempting to open coding > it. nfs_mark_for_revalidate mark other fields as invalid. Do we need to do that when updating ACL ? If not how about spin_lock(&inode->i_lock); NFS_I(inode)->cache_validity |= NFS_INO_INVALID_ATTR; spin_unlock(&inode->i_lock); > > > > nfs_access_zap_cache(inode); > > > nfs_zap_acl_cache(inode); > > > return ret; > > > > > > Any update on this ? Another option i figured out today is to make sure > > we add FATTR4_WORD0_ACL in nfs4_fattr_bitmap for fetching the modified > > acl value on mode update. Similarly setfacl can be compounded with the > > getattr request. > > We actually used to compound setacl with a GETATTR(FATTR4_WORD0_ACL) in > order to ensure that the server sets it correctly. Unfortunately, that > caused some servers to return NFS4ERR_RESOURCE due to the burden of > caching all that acl information in the duplicate request queue. What i was suggesting was to compound setacl with GETATTR(FATTR4_WORD1_MODE) so that we get the update mode bits as a part of response. Also componding setattr request with GETATTR(FATTR4_WORD0_ACL) > > I'd be fine with adding an ordinary getattr request, though, as long as > it is allowed to fail (i.e. the result of __nfs4_proc_set_acl() must > only depend on the setacl). > -aneesh ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: NFSv4 ACL set and inode attribute cache 2010-11-30 18:03 ` Aneesh Kumar K. V @ 2010-11-30 18:38 ` J. Bruce Fields 2010-11-30 18:40 ` Trond Myklebust 1 sibling, 0 replies; 9+ messages in thread From: J. Bruce Fields @ 2010-11-30 18:38 UTC (permalink / raw) To: Aneesh Kumar K. V; +Cc: Trond Myklebust, linux-nfs On Tue, Nov 30, 2010 at 11:33:03PM +0530, Aneesh Kumar K. V wrote: > On Mon, 29 Nov 2010 15:13:50 -0500, Trond Myklebust <Trond.Myklebust@netapp.com> wrote: > > On Mon, 2010-11-29 at 15:46 +0530, Aneesh Kumar K. V wrote: > > > On Fri, 12 Nov 2010 11:53:20 +0530, "Aneesh Kumar K. V" <aneesh.kumar@linux.vnet.ibm.com> wrote: > > > > On Thu, 11 Nov 2010 00:21:27 +0530, "Aneesh Kumar K. V" <aneesh.kumar@linux.vnet.ibm.com> wrote: > > > > > On Wed, 10 Nov 2010 23:31:31 +0530, Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com> wrote: > > > > > > > > > > > > Hi, > > > > > > > > > > > > I guess we are not marking the inode attribute as invalid when we set > > > > > > the ACL value. For ex: > > > > > > > > > > > > /d# mkdir sub3 > > > > > > /d# ls -dl sub3 > > > > > > drwxr-xr-x 2 root root 4096 Nov 10 17:56 sub3 > > > > > > /d# nfs4_setfacl -s A:fd:EVERYONE@:rwax sub3 > > > > > > /d# ls -dl sub3 > > > > > > drwxr-xr-x 2 root root 4096 Nov 10 17:56 sub3 > > > > > > /d# > > > > > > > > > > > > > > > > > > On the server i have the mode bits as > > > > > > /d# ls -dl sub3 > > > > > > drwxrwxrwx 2 root root 4096 Nov 10 17:56 sub3 > > > > > > /d# > > > > > > > > > > We also have similar issue other way round. ie setting the mode bits > > > > > don't result in ACL values being invalidated. But a second request get > > > > > the right value of ACL as show below. > > > > > > > > > > /d# nfs4_getfacl x > > > > > A::OWNER@:rw > > > > > A::GROUP@:rw > > > > > A::EVERYONE@:r > > > > > /d# chmod 600 x > > > > > /d# nfs4_getfacl x > > > > > A::OWNER@:rw > > > > > A::GROUP@:rw > > > > > A::EVERYONE@:r > > > > > /d# > > > > > > > > > > Expected value is > > > > > > > > > > /d# nfs4_getfacl x > > > > > A::OWNER@:rw > > > > > > > > > > > > > The below patch fix the problem for me. If this is the right way > > > > to fix, I can send a proper patch with commit message and s-o-b. > > > > > > > > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c > > > > index 0f24cdf..666a48b 100644 > > > > --- a/fs/nfs/nfs4proc.c > > > > +++ b/fs/nfs/nfs4proc.c > > > > @@ -3359,6 +3359,8 @@ static ssize_t nfs4_proc_get_acl(struct inode *inode, void *buf, size_t buflen) > > > > ret = nfs_revalidate_inode(server, inode); > > > > if (ret < 0) > > > > return ret; > > > > + if (NFS_I(inode)->cache_validity & NFS_INO_INVALID_ACL) > > > > + nfs_zap_acl_cache(inode); > > > > ret = nfs4_read_cached_acl(inode, buf, buflen); > > > > if (ret != -ENOENT) > > > > return ret; > > > > @@ -3387,6 +3389,11 @@ static int __nfs4_proc_set_acl(struct inode *inode, const void *buf, size_t bufl > > > > nfs_inode_return_delegation(inode); > > > > buf_to_pages(buf, buflen, arg.acl_pages, &arg.acl_pgbase); > > > > ret = nfs4_call_sync(server, &msg, &arg, &res, 1); > > > > + /* > > > > + * Acl update can result in inode attribute update. > > > > + * so mark the attribute cache invalid. > > > > + */ > > > > + NFS_I(inode)->cache_validity |= NFS_INO_INVALID_ATTR; > > > > This needs to be done under the correct spin locks, so please use the > > helper nfs_mark_for_revalidate() instead of attempting to open coding > > it. > > nfs_mark_for_revalidate mark other fields as invalid. Do we need to do that > when updating ACL ? If not how about > > spin_lock(&inode->i_lock); > NFS_I(inode)->cache_validity |= NFS_INO_INVALID_ATTR; > spin_unlock(&inode->i_lock); > > > > > > > nfs_access_zap_cache(inode); > > > > nfs_zap_acl_cache(inode); > > > > return ret; > > > > > > > > > Any update on this ? Another option i figured out today is to make sure > > > we add FATTR4_WORD0_ACL in nfs4_fattr_bitmap for fetching the modified > > > acl value on mode update. Similarly setfacl can be compounded with the > > > getattr request. > > > > We actually used to compound setacl with a GETATTR(FATTR4_WORD0_ACL) in > > order to ensure that the server sets it correctly. Unfortunately, that > > caused some servers to return NFS4ERR_RESOURCE due to the burden of > > caching all that acl information in the duplicate request queue. > > What i was suggesting was to compound setacl with > GETATTR(FATTR4_WORD1_MODE) so that we get the update mode bits as a part > of response. Also componding setattr request with GETATTR(FATTR4_WORD0_ACL) The problem was that ACL's are essentially arbitrary length, so some servers didn't like having to a reply with an ACL in it. Mode bits are a small fixed length so don't have the same problem. So that would suggest setting an ACL and getting a mode in one compound would be OK, but if you set a mode and got an ACL in one compound you might need to be prepared to handle NFS4ERR_RESOURCE. --b. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: NFSv4 ACL set and inode attribute cache 2010-11-30 18:03 ` Aneesh Kumar K. V 2010-11-30 18:38 ` J. Bruce Fields @ 2010-11-30 18:40 ` Trond Myklebust 1 sibling, 0 replies; 9+ messages in thread From: Trond Myklebust @ 2010-11-30 18:40 UTC (permalink / raw) To: Aneesh Kumar K. V; +Cc: linux-nfs On Tue, 2010-11-30 at 23:33 +0530, Aneesh Kumar K. V wrote: > On Mon, 29 Nov 2010 15:13:50 -0500, Trond Myklebust <Trond.Myklebust@netapp.com> wrote: > > On Mon, 2010-11-29 at 15:46 +0530, Aneesh Kumar K. V wrote: > > > On Fri, 12 Nov 2010 11:53:20 +0530, "Aneesh Kumar K. V" <aneesh.kumar@linux.vnet.ibm.com> wrote: > > > > On Thu, 11 Nov 2010 00:21:27 +0530, "Aneesh Kumar K. V" <aneesh.kumar@linux.vnet.ibm.com> wrote: > > > > > On Wed, 10 Nov 2010 23:31:31 +0530, Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com> wrote: > > > > > > > > > > > > Hi, > > > > > > > > > > > > I guess we are not marking the inode attribute as invalid when we set > > > > > > the ACL value. For ex: > > > > > > > > > > > > /d# mkdir sub3 > > > > > > /d# ls -dl sub3 > > > > > > drwxr-xr-x 2 root root 4096 Nov 10 17:56 sub3 > > > > > > /d# nfs4_setfacl -s A:fd:EVERYONE@:rwax sub3 > > > > > > /d# ls -dl sub3 > > > > > > drwxr-xr-x 2 root root 4096 Nov 10 17:56 sub3 > > > > > > /d# > > > > > > > > > > > > > > > > > > On the server i have the mode bits as > > > > > > /d# ls -dl sub3 > > > > > > drwxrwxrwx 2 root root 4096 Nov 10 17:56 sub3 > > > > > > /d# > > > > > > > > > > We also have similar issue other way round. ie setting the mode bits > > > > > don't result in ACL values being invalidated. But a second request get > > > > > the right value of ACL as show below. > > > > > > > > > > /d# nfs4_getfacl x > > > > > A::OWNER@:rw > > > > > A::GROUP@:rw > > > > > A::EVERYONE@:r > > > > > /d# chmod 600 x > > > > > /d# nfs4_getfacl x > > > > > A::OWNER@:rw > > > > > A::GROUP@:rw > > > > > A::EVERYONE@:r > > > > > /d# > > > > > > > > > > Expected value is > > > > > > > > > > /d# nfs4_getfacl x > > > > > A::OWNER@:rw > > > > > > > > > > > > > The below patch fix the problem for me. If this is the right way > > > > to fix, I can send a proper patch with commit message and s-o-b. > > > > > > > > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c > > > > index 0f24cdf..666a48b 100644 > > > > --- a/fs/nfs/nfs4proc.c > > > > +++ b/fs/nfs/nfs4proc.c > > > > @@ -3359,6 +3359,8 @@ static ssize_t nfs4_proc_get_acl(struct inode *inode, void *buf, size_t buflen) > > > > ret = nfs_revalidate_inode(server, inode); > > > > if (ret < 0) > > > > return ret; > > > > + if (NFS_I(inode)->cache_validity & NFS_INO_INVALID_ACL) > > > > + nfs_zap_acl_cache(inode); > > > > ret = nfs4_read_cached_acl(inode, buf, buflen); > > > > if (ret != -ENOENT) > > > > return ret; > > > > @@ -3387,6 +3389,11 @@ static int __nfs4_proc_set_acl(struct inode *inode, const void *buf, size_t bufl > > > > nfs_inode_return_delegation(inode); > > > > buf_to_pages(buf, buflen, arg.acl_pages, &arg.acl_pgbase); > > > > ret = nfs4_call_sync(server, &msg, &arg, &res, 1); > > > > + /* > > > > + * Acl update can result in inode attribute update. > > > > + * so mark the attribute cache invalid. > > > > + */ > > > > + NFS_I(inode)->cache_validity |= NFS_INO_INVALID_ATTR; > > > > This needs to be done under the correct spin locks, so please use the > > helper nfs_mark_for_revalidate() instead of attempting to open coding > > it. > > nfs_mark_for_revalidate mark other fields as invalid. Do we need to do that > when updating ACL ? If not how about > spin_lock(&inode->i_lock); > NFS_I(inode)->cache_validity |= NFS_INO_INVALID_ATTR; > spin_unlock(&inode->i_lock); That's fine as a short term fix. For the long term, let's try to add the getattr, as you suggested, so that we can do the inode attribute revalidation here. > > > > nfs_access_zap_cache(inode); > > > > nfs_zap_acl_cache(inode); > > > > return ret; > > > > > > > > > Any update on this ? Another option i figured out today is to make sure > > > we add FATTR4_WORD0_ACL in nfs4_fattr_bitmap for fetching the modified > > > acl value on mode update. Similarly setfacl can be compounded with the > > > getattr request. > > > > We actually used to compound setacl with a GETATTR(FATTR4_WORD0_ACL) in > > order to ensure that the server sets it correctly. Unfortunately, that > > caused some servers to return NFS4ERR_RESOURCE due to the burden of > > caching all that acl information in the duplicate request queue. > > What i was suggesting was to compound setacl with > GETATTR(FATTR4_WORD1_MODE) so that we get the update mode bits as a part > of response. Also componding setattr request with GETATTR(FATTR4_WORD0_ACL) You'd be missing the change attribute and the ctime, both of which are also guaranteed to change when the acl is changed. We might as well just do a full attribute revalidation... -- Trond Myklebust Linux NFS client maintainer NetApp Trond.Myklebust@netapp.com www.netapp.com ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2010-11-30 21:44 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2010-11-10 18:01 NFSv4 ACL set and inode attribute cache Aneesh Kumar K.V 2010-11-10 18:51 ` Aneesh Kumar K. V 2010-11-10 20:31 ` J. Bruce Fields 2010-11-12 6:23 ` Aneesh Kumar K. V 2010-11-29 10:16 ` Aneesh Kumar K. V 2010-11-29 20:13 ` Trond Myklebust 2010-11-30 18:03 ` Aneesh Kumar K. V 2010-11-30 18:38 ` J. Bruce Fields 2010-11-30 18:40 ` Trond Myklebust
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).