On Thu, 7 Jun 2012 14:35:47 +0800 Shaohua Li <shli@xxxxxxxxxx> wrote:
> On Thu, Jun 07, 2012 at 11:32:39AM +1000, NeilBrown wrote:
> > On Mon, 04 Jun 2012 16:01:59 +0800 Shaohua Li <shli@xxxxxxxxxx> wrote:
> >
> > > Let raid5d handle stripe in batch way to reduce conf->device_lock locking.
> > >
> > > Signed-off-by: Shaohua Li <shli@xxxxxxxxxxxx>
> >
> > I like this.
> > I don't think it justifies a separate function.
> >
> > #define MAX_STRIPE_BATCH 8
> > struct stripe_head *batch[MAX_STRIPE_BATCH]
> > int batch_size = 0;
> >
> > ...
> >
> > while (batch_size < MAX_STRPE_BATCH &&
> > (sh = __get_priority_stripe(conf)) != NULL)
> > batch[batch_size++] = sh;
> >
> > spin_unlock(&conf->device_lock);
> > if (batch_size == 0)
> > break;
> >
> > handled += batch_size;
> >
> > for (i = 0; i < batch_size; i++)
> > handle_stripe(batch[i]);
> > cond_resched();
> > if (....) md_check_recovery(mddev);
> >
> > spin_lock_irq(&conf->lock);
> > for (i = 0; i < batch_size; i++)
> > __release_stripe(batch[i]);
> >
> >
> > something like that?
>
> the 8th patch does the same thing, so I moved the code to a separate function.
The 8th patch should instead move all of the above into a separate function,
then call it both from raid5d and raid5auxd.
Maybe keep the md_check_recovery bit separate, in raid5d it would look like
if (mddev->flags & ~(1<<MD_CHANGE_PENDING)) {
spin_unlock();
make_check_recovert();
spin_lock();
}
Having the
fill the batch
handle the batch
release the batch
all open coded in the one place significantly aids readability.
NeilBrown
Attachment:
signature.asc
Description: PGP signature
