Re: [PATCH 4/4] raid1: Rewrite the implementation of iobarrier.

[NeilBrown <neilb@suse.de>]

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

On Fri, 15 Nov 2013 10:29:56 +0800 majianpeng <majianpeng@gmail.com> wrote:

> >> Yes.How about those code:
> >> spin_lock_irq(&conf->resync_lock);
> >> retry_check:
> >>         if (need_to_wait_for_sync(conf, bio)) {
> >>                 conf->nr_waiting++;
> >>                 /* Wait for the barrier to drop.
> >>                  * However if there are already pending
> >>                  * requests (preventing the barrier from
> >>                  * rising completely), and the
> >>                  * pre-process bio queue isn't empty,
> >>                  * then don't wait, as we need to empty
> >>                  * that queue to get the nr_pending
> >>                  * count down.
> >>                  */
> >>                 wait_event_lock_irq(conf->wait_barrier,
> >>                                     !conf->array_frozen &&
> >>                                     (!conf->barrier ||
> >>                                     (conf->nr_pending &&
> >>                                      current->bio_list &&
> >>                                      !bio_list_empty(current->bio_list))),
> >>                                     conf->resync_lock);
> >>                 conf->nr_waiting--;
> >> 				goto retry_check;
> >> >
> >
> >It would look a lot better if you made it a while loop:
> >
> > while (need_to_wait....) {
> >     nr_waiting++
> >     wait_event.....
> >     nr_waiting--
> > if (bio_data_dir.....
> >
> For this, i think it don;t need while() or recheck.
> The code should:
> 
> if (nedd_to_wait...) {
> 	nr_waiting++
> 	wait_event.....
> 	nr_waiting--
> }
> if (bio && bio_data_dir(bio) == WRITE) {
> 	 do;
> }
> 
> I think again need_to_wait...(),it must return false. So we don't recheck this.
> Or am I missing something?
> 

An 'if' is fine.  I just didn't like the goto.
If you can send me the updated patch in a day or 2 I should be able to get it
into the current merge window.

thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]