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? > > -- > 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
