On Mon, 21 Jul 2014 07:44:12 -0400 Jeff Layton <jeff.layton@xxxxxxxxxxxxxxx> wrote: > On Mon, 21 Jul 2014 17:02:54 +1000 > NeilBrown <neilb@xxxxxxx> 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. As it is, I think we just have to hope. Thanks, NeilBrown
Attachment:
signature.asc
Description: PGP signature
