Return-Path: Received: from mail-ua0-f193.google.com ([209.85.217.193]:45274 "EHLO mail-ua0-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727571AbeGTUiW (ORCPT ); Fri, 20 Jul 2018 16:38:22 -0400 Received: by mail-ua0-f193.google.com with SMTP id k8-v6so8169540uaq.12 for ; Fri, 20 Jul 2018 12:48:37 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: References: <7cccfa69267c44dbb629b154ee942df8280875bc.camel@hammerspace.com> From: Olga Kornievskaia Date: Fri, 20 Jul 2018 15:48:36 -0400 Message-ID: Subject: Re: sending duplicate GETATTRs To: Trond Myklebust Cc: "linux-nfs@vger.kernel.org" Content-Type: text/plain; charset="UTF-8" Sender: linux-nfs-owner@vger.kernel.org List-ID: On Fri, Jul 20, 2018 at 3:19 PM, Trond Myklebust wrote: > On Fri, 2018-07-20 at 14:58 -0400, Olga Kornievskaia wrote: >> On Fri, Jul 20, 2018 at 2:46 PM, Trond Myklebust >> wrote: >> > On Fri, 2018-07-20 at 14:40 -0400, Olga Kornievskaia wrote: >> > > On Fri, Jul 20, 2018 at 2:17 PM, Olga Kornievskaia > > > u> >> > > wrote: >> > > > On Fri, Jul 20, 2018 at 2:05 PM, Trond Myklebust >> > > > 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. >> > > >> > >> > Oh... I see what you are saying. Yes, that check looks like it >> > should >> > be looking for NFS_INO_INVALID_OTHER. >> >> Whew. ok I'm glad something make sense. However, you lost me on >> "NFS_INO_INVALID_OTHER". What does that flag means? >> Are you talking about checking the check in nfs_execute_ok() to check >> for INVALID_OTHER instead of INVALID_ACCESS? >> > > I'm saying that function should probably read as: > > static int nfs_execute_ok(struct inode *inode, int mask) > { > struct nfs_server *server = NFS_SERVER(inode); > int ret = 0; > > if (nfs_check_cache_invalid(inode, NFS_INO_INVALID_OTHER)) { > if (mask & MAY_NOT_BLOCK) > return -ECHILD; > ret = __nfs_revalidate_inode(server, inode); > } > if (ret == 0 && !execute_ok(inode)) > ret = -EACCES; > return ret; > } > With that change I also don't see a 2nd GETATTR sent. Would you mind creating a patch for it?