On Fri, Jul 20, 2018 at 2:17 PM, Olga Kornievskaia <aglo@xxxxxxxxx> wrote: > On Fri, Jul 20, 2018 at 2:05 PM, Trond Myklebust > <trondmy@xxxxxxxxxxxxxxx> wrote: >> On Fri, 2018-07-20 at 13:26 -0400, Olga Kornievskaia wrote: >>> Hi Trond, >>> >>> I would some help understanding attributes management. >>> >>> Right now, any time a directory inode that was marked with >>> INVALID_ACCESS (say to a change_attribute changed) ends up triggering >>> sending a duplicate GETATTR. I don't think that's correct. >>> >>> In nfs_execute_ok() we check if (nfs_check_cache_invalid(inode, >>> NFS_INO_INVALID_ACCESS)) and the call __nfs_revalidate_inode() which >>> will trigger GETATTR. I don't understand why after calling this >>> function the INVALID_ACCESS doesn't get cleared? Because that's what >>> causes the double GETATTR to be sent. >>> >>> On the open path, the first time the getattr is sent is in >>> >>> Jul 19 15:39:52 ipa18 kernel: dump_stack+0x5a/0x73 >>> Jul 19 15:39:52 ipa18 kernel: nfs4_proc_getattr+0x65/0x110 [nfsv4] >>> Jul 19 15:39:52 ipa18 kernel: __nfs_revalidate_inode+0xe1/0x370 [nfs] >>> Jul 19 15:39:52 ipa18 kernel: nfs_permission+0x16b/0x1f0 [nfs] >>> Jul 19 15:39:52 ipa18 kernel: inode_permission+0xab/0x130 >>> Jul 19 15:39:52 ipa18 kernel: link_path_walk+0x29d/0x520 >>> Jul 19 15:39:52 ipa18 kernel: path_openat+0xf6/0x1230 >>> Jul 19 15:39:52 ipa18 kernel: do_filp_open+0x91/0x100 >>> Jul 19 15:39:52 ipa18 kernel: do_sys_open+0x126/0x210 >>> Jul 19 15:39:52 ipa18 kernel: do_syscall_64+0x55/0x180 >>> Jul 19 15:39:52 ipa18 kernel: >>> entry_SYSCALL_64_after_hwframe+0x44/0xa9 >>> >>> And then again during the open >>> >>> Jul 19 15:39:52 ipa18 kernel: dump_stack+0x5a/0x73 >>> Jul 19 15:39:52 ipa18 kernel: nfs4_proc_getattr+0x65/0x110 [nfsv4] >>> Jul 19 15:39:52 ipa18 kernel: __nfs_revalidate_inode+0xe1/0x370 [nfs] >>> Jul 19 15:39:52 ipa18 kernel: nfs_permission+0x16b/0x1f0 [nfs] >>> Jul 19 15:39:52 ipa18 kernel: inode_permission+0xab/0x130 >>> Jul 19 15:39:52 ipa18 kernel: path_openat+0x942/0x1230 >>> Jul 19 15:39:52 ipa18 kernel: do_filp_open+0x91/0x100 >>> Jul 19 15:39:52 ipa18 kernel: do_sys_open+0x126/0x210 >>> Jul 19 15:39:52 ipa18 kernel: do_syscall_64+0x55/0x180 >>> Jul 19 15:39:52 ipa18 kernel: >>> entry_SYSCALL_64_after_hwframe+0x44/0xa9 >>> >>> Can't we remove INVALID_ACCESS after we revalidated the inode? This >>> removes the duplicated GETATTRs. Is this a valid way of fixing this >>> issue? >>> >>> diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c >>> index 8f8e9e9..2b55a45 100644 >>> --- a/fs/nfs/dir.c >>> +++ b/fs/nfs/dir.c >>> @@ -2504,6 +2504,8 @@ static int nfs_execute_ok(struct inode *inode, >>> int mask) >>> if (mask & MAY_NOT_BLOCK) >>> return -ECHILD; >>> ret = __nfs_revalidate_inode(server, inode); >>> + if (!ret) >>> + NFS_I(inode)->cache_validity &= >>> ~NFS_INO_INVALID_ACCESS; >>> } >>> if (ret == 0 && !execute_ok(inode)) >>> ret = -EACCES; >> >> >> I don't see how the above makes sense. >> >> Either the attribute revalidation confirmed that the change attr, mode >> + uid + gid are unchanged, in which case the call to >> nfs_refresh_inode() during revalidation will fail to set >> NFS_INO_INVALID_ACCESS, or it confirmed that at least one of them has >> changed, in which case we do want to revalidate the access cache. > > I'm confused as to against what are we checking then? > > We flagged a directory inode to have invalid attributes. So we sent a > GETATTR. I would think that after getting the result all our > attributes should be valid. Why would a client as the next operation > send another GETATTR for the same inode? > I didn't answer your question but rather explained what I see client do. Let's me see if I can try again: in nfs_execute_ok() the check for the INVALID_CACHE is true so the code calls __nfs_revalidate_inode() which ends up sending a GETATTR. Could it be that what's happening is that GETATTRs reply for the change_attr is different than what we have stored. BUT that's obvious, we knew that to begin with since we are validating the attributes. So we update it and then we send another GETATTR now the GETATTR sends back the "same" change_attr and this time it matches what we have stored. Why is this 2nd GETATTR necessary? The attribute should be marked as valid after a getting a successful GETATTR. >> >> -- >> Trond Myklebust >> Linux NFS client maintainer, Hammerspace >> trond.myklebust@xxxxxxxxxxxxxxx >> -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
