Re: [PATCH v4 10/10] nfsd: give block_delegation and delegation_blocked its own spinlock

[NeilBrown <neilb@suse.de>]

[-- Attachment #1: Type: text/plain, Size: 2855 bytes --]

On Mon, 21 Jul 2014 17:17:57 -0400 "J. Bruce Fields" <bfields@fieldses.org>
wrote:

> On Tue, Jul 22, 2014 at 06:40:49AM +1000, NeilBrown wrote:
> > On Mon, 21 Jul 2014 07:44:12 -0400 Jeff Layton <jeff.layton@primarydata.com>
> > wrote:
> > 
> > > On Mon, 21 Jul 2014 17:02:54 +1000
> > > NeilBrown <neilb@suse.de> wrote:
> > 
> > > > >  	hash = arch_fast_hash(&fh->fh_base, fh->fh_size, 0);
> > > > >  
> > > > >  	__set_bit(hash&255, bd->set[bd->new]);
> > > > >  	__set_bit((hash>>8)&255, bd->set[bd->new]);
> > > > >  	__set_bit((hash>>16)&255, bd->set[bd->new]);
> > > > > +	spin_lock(&blocked_delegations_lock);
> > > > 
> > > > __set_bit isn't atomic.  The spin_lock should be taken *before* these
> > > > __set_bit() calls.
> > > > 
> > > > Otherwise, looks fine.
> > > > 
> > > > Thanks,
> > > > NeilBrown
> > > > 
> > > > 
> > > 
> > > Ok. I guess the worry is that we could end up setting bits in the
> > > middle of swapping the two fields? Makes sense -- fixed in my repo.
> > 
> > It is more subtle than that.
> > __set_bit() will:
> >   read a value from memory to a register
> >   set a bit in the register
> >   write the register back out to memory
> > 
> > If two threads both run __set_bit on the same word of memory at the same
> > time, one of the updates can get lost.
> > set_bit() (no underscore) performs an atomic RMW to avoid this, but is more
> > expensive.
> > spin_lock() obviously ensures the required exclusion and as we are going to
> > take the lock anyway we may as well take it before setting bits so we can use
> > the non-atomic (cheaper) __set_bit function.
> > 
> > > I'll send out the updated set later today (it also includes a few nits
> > > that HCH pointed out last week).
> > > 
> > > As a side note...I wonder how much we'll get in the way of false
> > > positives with this scheme?
> > 
> > If a future version of NFSv4 could allow delegations to be granted while a
> > file is open (oh, it seems you are the only client using this file at the
> > moment, you can treat this "open" as a delegation if you like) a few false
> > positives would be a complete non-issue.
> 
> For what it's worth, I think 4.1 provides what you're asking for here;
> see
> 
> 	http://tools.ietf.org/html/rfc5661#section-20.7
> 
> and the discussion of the various WANT_ flags in
> 
> 	http://tools.ietf.org/html/rfc5661#section-18.16.3
> 
> As far as I know none of that is implemented yet.
> 
> --b.

I guess I should really read the 4.1 (and 4.2) spec some day....
Though the 20.7 section seems to be about saying "resources in general are
available" rather than "this  specific file that you wanted a delegation for
but didn't get one is how up for delegation"....
But I only had a quick read so I might have missed something.

Thanks,
NeilBrown


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]