Re: [md PATCH 08/34] md/raid5: replace sh->lock with an 'active' flag.

[NeilBrown <neilb@suse.de>]

On Fri, 22 Jul 2011 13:27:36 +0900 Namhyung Kim <namhyung@gmail.com> wrote:

> NeilBrown <neilb@suse.de> writes:
> 
> > sh->lock is now mainly used to ensure that two threads aren't running
> > in the locked part of handle_stripe[56] at the same time.
> >
> > That can more neatly be achieved with an 'active' flag which we set
> > while running handle_stripe.  If we find the flag is set, we simply
> > requeue the stripe for later by setting STRIPE_HANDLE.
> >
> > For safety we take ->device_lock while examining the state of the
> > stripe and creating a summary in 'stripe_head_state / r6_state'.
> > This possibly isn't needed but as shared fields like ->toread,
> > ->towrite are checked it is safer for now at least.
> >
> > We leave the label after the old 'unlock' called "unlock" because it
> > will disappear in a few patches, so renaming seems pointless.
> >
> > This leaves the stripe 'locked' for longer as we clear STRIPE_ACTIVE
> > later, but that is not a problem.
> >
> > Signed-off-by: NeilBrown <neilb@suse.de>
> 
> Reviewed-by: Namhyung Kim <namhyung@gmail.com>

Thanks.

> 
> But I have a question, please see below.

I like questions...

> > @@ -3037,6 +3028,7 @@ static void handle_stripe5(struct stripe_head *sh)
> >  
> >  	/* Now to look around and see what can be done */
> >  	rcu_read_lock();
> > +	spin_lock_irq(&conf->device_lock);
> 
> Do we still need rcu_read_lock()? AFAIK rcu_read_lock() only prevents
> task from preemption but spin_lock does same thing as well.
> 
> I know it's been already there under sh->lock before this patch, and
> it doesn't hurt anything, but I'm not sure it is really needed.

I see your point, but I think there are two reasons why it really is needed.

1/ I think rcu_read_lock does more than prevent preemption.  What it does
   exactly I don't know and I think it depends on which RCU scheme was chosen
   when configuring the kernel.  But what it conceptually does is stop
   a subsequent synchronize_rcu() from completing, and that it what I need.
   Disabling preemption might stop that as well, but I really don't know and
   so cannot trust it.

2/ It is useful documentation.  A reader seeing rcu_read_lock() knows that
   there must be some rcu-protected data here which needs to be handled with
   a little care and so they can be on the lookout for it.

So I think it is really needed.

Thanks,

NeilBrown