| Message ID | 1391459326-21569-8-git-send-email-snitzer@redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On 02/04/14 05:28, Mike Snitzer wrote: > Return early for case when no path exists, this eliminates the need for > extra nesting for the case when a path exists (the common case). ... > - if (pgpath) { > - if (__pgpath_busy(pgpath)) > - goto out_unlock; > + if (!pgpath) { This should be "unlikely(!pgpath)". > + if (__pgpath_busy(pgpath)) > + goto out_unlock; The above 2 lines should be removed. (Actually it is a comment for PATCH 1/7 but just in case.) Other than that: Reviewed-by: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>
On 02/03/2014 09:28 PM, Mike Snitzer wrote: > Return early for case when no path exists, this eliminates the need for > extra nesting for the case when a path exists (the common case). > > Signed-off-by: Mike Snitzer <snitzer@redhat.com> > --- > drivers/md/dm-mpath.c | 51 +++++++++++++++++++++++++++------------------------ > 1 file changed, 27 insertions(+), 24 deletions(-) > > diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c > index 971fbec..272e7d4 100644 > --- a/drivers/md/dm-mpath.c > +++ b/drivers/md/dm-mpath.c > @@ -392,32 +392,35 @@ static int multipath_map(struct dm_target *ti, struct request *clone, > > pgpath = m->current_pgpath; > > - if (pgpath) { > - if (__pgpath_busy(pgpath)) > - goto out_unlock; > + if (!pgpath) { > + if (!__must_push_back(m)) > + r = -EIO; /* Failed */ > + goto out_unlock; > + } > + > + if (__pgpath_busy(pgpath)) > + goto out_unlock; > > - if (pg_ready(m)) { > - if (set_mapinfo(m, map_context) < 0) > - /* ENOMEM, requeue */ > - goto out_unlock; > - > - bdev = pgpath->path.dev->bdev; > - clone->q = bdev_get_queue(bdev); > - clone->rq_disk = bdev->bd_disk; > - clone->cmd_flags |= REQ_FAILFAST_TRANSPORT; > - mpio = map_context->ptr; > - mpio->pgpath = pgpath; > - mpio->nr_bytes = nr_bytes; > - if (pgpath->pg->ps.type->start_io) > - pgpath->pg->ps.type->start_io(&pgpath->pg->ps, > - &pgpath->path, > - nr_bytes); > - r = DM_MAPIO_REMAPPED; > + if (pg_ready(m)) { > + if (set_mapinfo(m, map_context) < 0) > + /* ENOMEM, requeue */ > goto out_unlock; > - } > - __pg_init_all_paths(m, 0); > - } else if (!__must_push_back(m)) > - r = -EIO; /* Failed */ > + > + bdev = pgpath->path.dev->bdev; > + clone->q = bdev_get_queue(bdev); > + clone->rq_disk = bdev->bd_disk; > + clone->cmd_flags |= REQ_FAILFAST_TRANSPORT; > + mpio = map_context->ptr; > + mpio->pgpath = pgpath; > + mpio->nr_bytes = nr_bytes; > + if (pgpath->pg->ps.type->start_io) > + pgpath->pg->ps.type->start_io(&pgpath->pg->ps, > + &pgpath->path, > + nr_bytes); > + r = DM_MAPIO_REMAPPED; > + goto out_unlock; > + } > + __pg_init_all_paths(m, 0); > > out_unlock: > spin_unlock_irqrestore(&m->lock, flags); > But then you should be going full circle and move the now somewhat lonely statement '__pg_init_all_paths' to the top, inverting the if (pg_ready(m)) statement. I'll be redoing the patchset (yet again) Cheers, Hannes
diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c index 971fbec..272e7d4 100644 --- a/drivers/md/dm-mpath.c +++ b/drivers/md/dm-mpath.c @@ -392,32 +392,35 @@ static int multipath_map(struct dm_target *ti, struct request *clone, pgpath = m->current_pgpath; - if (pgpath) { - if (__pgpath_busy(pgpath)) - goto out_unlock; + if (!pgpath) { + if (!__must_push_back(m)) + r = -EIO; /* Failed */ + goto out_unlock; + } + + if (__pgpath_busy(pgpath)) + goto out_unlock; - if (pg_ready(m)) { - if (set_mapinfo(m, map_context) < 0) - /* ENOMEM, requeue */ - goto out_unlock; - - bdev = pgpath->path.dev->bdev; - clone->q = bdev_get_queue(bdev); - clone->rq_disk = bdev->bd_disk; - clone->cmd_flags |= REQ_FAILFAST_TRANSPORT; - mpio = map_context->ptr; - mpio->pgpath = pgpath; - mpio->nr_bytes = nr_bytes; - if (pgpath->pg->ps.type->start_io) - pgpath->pg->ps.type->start_io(&pgpath->pg->ps, - &pgpath->path, - nr_bytes); - r = DM_MAPIO_REMAPPED; + if (pg_ready(m)) { + if (set_mapinfo(m, map_context) < 0) + /* ENOMEM, requeue */ goto out_unlock; - } - __pg_init_all_paths(m, 0); - } else if (!__must_push_back(m)) - r = -EIO; /* Failed */ + + bdev = pgpath->path.dev->bdev; + clone->q = bdev_get_queue(bdev); + clone->rq_disk = bdev->bd_disk; + clone->cmd_flags |= REQ_FAILFAST_TRANSPORT; + mpio = map_context->ptr; + mpio->pgpath = pgpath; + mpio->nr_bytes = nr_bytes; + if (pgpath->pg->ps.type->start_io) + pgpath->pg->ps.type->start_io(&pgpath->pg->ps, + &pgpath->path, + nr_bytes); + r = DM_MAPIO_REMAPPED; + goto out_unlock; + } + __pg_init_all_paths(m, 0); out_unlock: spin_unlock_irqrestore(&m->lock, flags);
Return early for case when no path exists, this eliminates the need for extra nesting for the case when a path exists (the common case). Signed-off-by: Mike Snitzer <snitzer@redhat.com> --- drivers/md/dm-mpath.c | 51 +++++++++++++++++++++++++++------------------------ 1 file changed, 27 insertions(+), 24 deletions(-)