Re: [PATCH] VFS: br_write_lock locks on possible CPUs other than online CPUs

[Dave Chinner <david@fromorbit.com>]

On Tue, Dec 20, 2011 at 07:58:06PM +0000, Al Viro wrote:
> On Wed, Dec 21, 2011 at 12:42:04AM +0530, Srivatsa S. Bhat wrote:
> > register_hotcpu_notifier(...);
> > grab spinlock
> > for_each_online_cpu(N)
> >   add N to bitmap
> > release spinlock
> > 
> > because the latter code is not fully race-free (because we don't handle
> > CPU_DOWN_PREPARE event in the callback and hence cpu_online_mask can get
> > updated in-between). But it would still work since cpus going down don't
> > really pose problems for us.
> 
> Um?  Sure, that loop can end up adding CPUs on their way down into the set.
> And as soon as they get their CPU_DEAD, notifier will prune them out...  Or
> is there something I'm missing here?  Anyway, the variant I have here
> (untested) follows:

Only thing that concerns me about this patch is the bitmap changing
between lock and unlock operations. i.e.

CPU 1: lock all cpus in mask
CPU 2: brings up new cpu, notifier adds CPU to bitmask
CPU 1: unlock all cpus in mask

And in this case the unlock tries to unlock a cpu that wasn't locked
to start with. It really seems to me that while a global lock is in
progress, the online bitmask cannot be allowed to change.

Perhaps something can be passed between the lock and unlock
operations to be able to detect a changed mask between lock/unlock
operations (e.g. a generation number) and then handle that via a
slow path that unlocks only locks that are active in the online
bitmask?  i.e. all the notifier does is bump the generation count,
and the slow path on the unlock handles everything else?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com