On Wed, 2020-01-29 at 10:36 -0500, Benjamin Coddington wrote:
> On 29 Jan 2020, at 10:04, Trond Myklebust wrote:
>
> > On Wed, 2020-01-29 at 09:18 -0500, Benjamin Coddington wrote:
> > > On 28 Jan 2020, at 16:46, Trond Myklebust wrote:
> > >
> > > > On Tue, 2020-01-28 at 21:30 +0000, Trond Myklebust wrote:
> > > > > On Tue, 2020-01-28 at 16:20 -0500, Trond Myklebust wrote:
> > > > > > On Tue, 2020-01-28 at 15:21 -0500, Trond Myklebust wrote:
> > > > > > > On Tue, 2020-01-28 at 10:56 -0500, Trond Myklebust wrote:
> > > > > > > > On Tue, 2020-01-28 at 10:26 -0500, Benjamin Coddington
> > > > > > > > wrote:
> > > ...
> > > > > > > > > + if (NFS_PROTO(dir)->have_delegation(inode,
> > > > > > > > > FMODE_READ))
> > > > > > > > > + verifier = NFS_DELEGATION_VERF;
> > > > > > > > > + else
> > > > > > > > > + verifier =
> > > > > > > > > nfs_save_change_attribute(dir);
> > > > > > > > > +
> > > > > > > > > nfs_setsecurity(inode, fattr, label);
> > > > > > > > > nfs_set_verifier(dentry,
> > > > > > > > > nfs_save_change_attribute(dir));
> > > > > > >
> > > > > > > Oops! When reviewing, I missed this. Shouldn't the above
> > > > > > > be
> > > > > > > changed
> > > > > > > to
> > > > > > > nfs_set_verifier(dentry, verifier) ?
> > >
> > > Ugh, yep.
> > >
> > > > > > ...and on a similar vein: nfs_lookup_revalidate_delegated()
> > > > > > needs
> > > > > > to
> > > > > > change so as to not reset the verifier...
> > > > > >
> > > > > > Sorry for not catching that one either.
> > > > >
> > > > > Not my day...
> > > > >
> > > > > nfs_prime_dcache() will clobber the verifier too in the
> > > > > nfs_same_file()
> > > > > case. That one also needs to set NFS_DELEGATION_VERF if there
> > > > > is
> > > > > a
> > > > > delegation.
> > > > >
> > > > > Perhaps add a helper function for that +
> > > > > nfs_lookup_revalidate_dentry()?
> > > >
> > > > ....and finally, we should remove the call to
> > > > nfs_set_verifier()
> > > > from
> > > > nfs4_file_open(). Aside from being incorrect in the case where
> > > > we
> > > > used
> > > > an open-by-filehandle, that case is taken care of in the
> > > > preceding
> > > > dentry revalidation.
> > >
> > > Ok, I'll get these done. This doesn't make the revalidation code
> > > any simpler. I am impressed that you can spot these problems just
> > > doing
> > > review. I do wish we could use d_fsdata, seems like exactly the
> > > kind
> > > of thing we need it for, but is it worth it to do another
> > > allocation
> > > every
> > > time we need a dentry. I wonder if we're going to end up having
> > > more
> > > cases like this, or want to have more private information per-
> > > dentry.
> > >
> > > Right now d_fsdata holds the devicename for IS_ROOT, and caches
> > > the
> > > appropriate info for a delayed removal of silly-renamed files.
> > >
> > > Problem is that we don't drop the delegation until after caching
> > > the
> > > silly-rename data, and then we're still doing the same sorts of
> > > things
> > > trying to figure out what data is in which dentry.
> > >
> > > Maybe Al would be willing to reserve some of the top of d_flags
> > > for
> > > filesystems to use privately?
> > >
> > > Al, can we have such? NFS already has DCACHE_NFSFS_RENAMED, that
> > > could move
> > > up above the core d_flags?
> >
> > I did look into this a few weeks ago, and it seemed to me that
> > we're
> > already low on free bits in d_flags.
> >
> > An alternative might just be to reserve a whole bit in d_time as
> > being
> > the 'delegated dentry revalidated' bit. e.g. reserve bit
> > 'BITS_PER_LONG
> > - 1' (or just bit '0') for that purpose.
>
> I did look at doing this already for the purposes of making
> nfs_force_dir_revalidate() faster than having an if statement in it,
> and the
> neat thing is that the if statement is always faster than any bit
> shifting
> work I came up with - however I was playing with most-significant
> bits.
>
> Maybe using the least-significant would be faster, then we just do:
>
> nfs_force_lookup_revalidate() {
> nfsi->cache_change_attribute += 2;
> }
Either that, or just do ' << 1' in nfs_set_verifier(). Either way
avoids the conditional.
>
> But, again - what happens when we need another bit? Is it OK to keep
> chopping d_time in half?
It shouldn't be a problem for 64-bit architectures. It's going to take
quite a while to overflow a 2^63 valued field.
Even for 32-bit, I'd say we're unlikely to see 2 billion updates of the
directory without a single revalidation of the dentry. It would have to
be a very peculiar 32-bit client with some serious high-end networking
hardware...
> What about coming up with a struct for d_fsdata that unionizes the
> devicename and the unlink data, and holds our private flags? The
> case I am
> trying to fix is so rare that we could just do the allocation when it
> occurs, and that way we don't have to modify too much of the
> revalidation
> code just to handle it. It also would give us more flags and private
> dentry
> data if we need it later.
I'm not sure this case warrants it. Maybe later if we find we need more
flags and are worried about the 32-bit arch case.
I'd also dispute that revalidating a delegation is a rare event. The
Hammerspace server will usually prefer to hand out a delegation when
possible on file open, so those cases happen very frequently. Having to
allocate a d_fsdata is likely to be a measurable overhead.
> Another option - allow filesystems to do d_alloc, then we could just
> account
> for the space we might need privately. NFS would have larger
> dentries, but
> VFS could probably drop d_time. Maybe too big a burden to grow
> another api
> for VFS.
>
> I think I like the "allocate it if you need it" option for d_fsdata
> best so
> far.
Again, I'd argue that is overkill for this particular problem, but yes,
it would be a more efficient solution than extending d_fsdata.
> Ben
>
> > We would then want to change nfs_set_verifier() to set the verifier
> > in
> > the remaining bits, and have it set the delegated dentry
> > revalidated
> > bit depending on whether there is a delegation for the inode at the
> > time of revalidation.
> > We may also want to do the same setting of the delegation bit in
> > nfs_verify_change_attribute() when there is a successful match of
> > the
> > remaining verifier.
> >
> > Finally, we should have nfs_mark_delegation_revoked() and
> > nfs_start_delegation_return_locked() clear that delegated dentry
> > revalidated bit, perhaps by iterating over the inode->i_dentry
> > list?
> >
> > Note that this has the advantage that since the actual verifier
> > part of
> > dentry->d_time can be kept up to date while we hold the delegation,
> > we
> > don't have to worry about the dentry getting revalidated for no
> > reason
> > after the delegation is returned.
> >
> > --
> > Trond Myklebust
> > Linux NFS client maintainer, Hammerspace
> > trond.myklebust@xxxxxxxxxxxxxxx
--
Trond Myklebust
CTO, Hammerspace Inc
4300 El Camino Real, Suite 105
Los Altos, CA 94022
www.hammer.space
