On Fri, 22 Jul 2011 13:27:36 +0900 Namhyung Kim <namhyung@xxxxxxxxx> wrote: > NeilBrown <neilb@xxxxxxx> 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@xxxxxxx> > > Reviewed-by: Namhyung Kim <namhyung@xxxxxxxxx> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-raid" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
