On Wed, 3 Aug 2011 16:45:35 -0700 "Williams, Dan J" <dan.j.williams@xxxxxxxxx> wrote: > On Wed, Aug 3, 2011 at 4:35 PM, NeilBrown <neilb@xxxxxxx> wrote: > > On Wed, 3 Aug 2011 15:47:47 -0700 Dan Williams <dan.j.williams@xxxxxxxxx> > > wrote: > > > >> On Wed, Jul 20, 2011 at 7:32 PM, NeilBrown <neilb@xxxxxxx> wrote: > >> > 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. > >> > > >> > >> This removal reminds me of one thing I have wondered about, but to > >> date have not found the time to measure (maybe someone might beat me > >> to it if the idea is out there), is what is the overhead of all the > >> atomic operations that raid5.c generates? If we can guarantee that > >> certain updates only happen under sh->lock (now STRIPE_ACTIVE) can we > >> downgrade set_bit and clear_bit to their non-atomic __set_bit and > >> __clear_bit versions and recover some cpu cycles? > >> > > > > You can only used the unlocked version if you know that no other CPU will > > change any of the bits in the 'unsigned long'. As STRIPE_HANDLE can be set > > at any time, I think all accesses to sh->state must be atomic. > > > > However 'struct stripe_head_state' is thread-local so setting/clearing flags > > in ops_request can probably benefit from non-atomic ops. > > I was particularly eyeing 'flags' in struct r5dev... good point.... yes, they would probably be safe. Async updates only happen when the refcount on the stripe head is != 0, and handle_stripe only updates things when it own the sole reference. So the updates in handle_stripe and functions directly call from there should be safe to use unlocked versions... 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
