Re: [patch 7/8] raid5: raid5d handle stripe in batch way

[NeilBrown <neilb@suse.de>]

[-- Attachment #1: Type: text/plain, Size: 1693 bytes --]

On Thu, 7 Jun 2012 14:35:47 +0800 Shaohua Li <shli@kernel.org> 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@kernel.org> wrote:
> > 
> > > Let raid5d handle stripe in batch way to reduce conf->device_lock locking.
> > > 
> > > Signed-off-by: Shaohua Li <shli@fusionio.com>
> > 
> > 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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]