On Fri, Jul 20, 2018 at 3:19 PM, Trond Myklebust
<trondmy@xxxxxxxxxxxxxxx> wrote:
> On Fri, 2018-07-20 at 14:58 -0400, Olga Kornievskaia wrote:
>> On Fri, Jul 20, 2018 at 2:46 PM, Trond Myklebust
>> <trondmy@xxxxxxxxxxxxxxx> wrote:
>> > On Fri, 2018-07-20 at 14:40 -0400, Olga Kornievskaia wrote:
>> > > On Fri, Jul 20, 2018 at 2:17 PM, Olga Kornievskaia <aglo@xxxxxxxx
>> > > u>
>> > > 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.
>> > >
>> >
>> > 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?
--
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
