| Message ID | 1389955328-107148-3-git-send-email-hare@suse.de (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On 01/17/14 19:42, Hannes Reinecke wrote: > @@ -908,19 +910,9 @@ static void multipath_dtr(struct dm_target *ti) > static int multipath_map(struct dm_target *ti, struct request *clone, > union map_info *map_context) > { > - int r; > struct multipath *m = (struct multipath *) ti->private; > > - if (set_mapinfo(m, map_context) < 0) > - /* ENOMEM, requeue */ > - return DM_MAPIO_REQUEUE; > - > - clone->cmd_flags |= REQ_FAILFAST_TRANSPORT; > - r = map_io(m, clone, map_context); > - if (r < 0 || r == DM_MAPIO_REQUEUE) > - clear_mapinfo(m, map_context); > - > - return r; > + return map_io(m, clone, map_context); > } Now multipath_map() is the only caller of map_io() and most part of multipath_map() is moved to map_io(), there is no reason to separate those functions. You could fold map_io() into multipath_map().
On 01/20/2014 12:59 PM, Junichi Nomura wrote: > On 01/17/14 19:42, Hannes Reinecke wrote: >> @@ -908,19 +910,9 @@ static void multipath_dtr(struct dm_target *ti) >> static int multipath_map(struct dm_target *ti, struct request *clone, >> union map_info *map_context) >> { >> - int r; >> struct multipath *m = (struct multipath *) ti->private; >> >> - if (set_mapinfo(m, map_context) < 0) >> - /* ENOMEM, requeue */ >> - return DM_MAPIO_REQUEUE; >> - >> - clone->cmd_flags |= REQ_FAILFAST_TRANSPORT; >> - r = map_io(m, clone, map_context); >> - if (r < 0 || r == DM_MAPIO_REQUEUE) >> - clear_mapinfo(m, map_context); >> - >> - return r; >> + return map_io(m, clone, map_context); >> } > > Now multipath_map() is the only caller of map_io() and > most part of multipath_map() is moved to map_io(), > there is no reason to separate those functions. > You could fold map_io() into multipath_map(). > Yes, I could. However, I didn't do so (for this patchset) as this would make reviewing harder. But yeah, it should be merged. Cheers, Hannes
On Mon, Jan 20 2014 at 7:15am -0500, Hannes Reinecke <hare@suse.de> wrote: > On 01/20/2014 12:59 PM, Junichi Nomura wrote: > > On 01/17/14 19:42, Hannes Reinecke wrote: > >> @@ -908,19 +910,9 @@ static void multipath_dtr(struct dm_target *ti) > >> static int multipath_map(struct dm_target *ti, struct request *clone, > >> union map_info *map_context) > >> { > >> - int r; > >> struct multipath *m = (struct multipath *) ti->private; > >> > >> - if (set_mapinfo(m, map_context) < 0) > >> - /* ENOMEM, requeue */ > >> - return DM_MAPIO_REQUEUE; > >> - > >> - clone->cmd_flags |= REQ_FAILFAST_TRANSPORT; > >> - r = map_io(m, clone, map_context); > >> - if (r < 0 || r == DM_MAPIO_REQUEUE) > >> - clear_mapinfo(m, map_context); > >> - > >> - return r; > >> + return map_io(m, clone, map_context); > >> } > > > > Now multipath_map() is the only caller of map_io() and > > most part of multipath_map() is moved to map_io(), > > there is no reason to separate those functions. > > You could fold map_io() into multipath_map(). > > > Yes, I could. > > However, I didn't do so (for this patchset) > as this would make reviewing harder. > > But yeah, it should be merged. Really not that hard to review. I'd be in favor of folding it in a revised patch. -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c index 71c6f2c..46f8a04 100644 --- a/drivers/md/dm-mpath.c +++ b/drivers/md/dm-mpath.c @@ -372,12 +372,12 @@ static int __must_push_back(struct multipath *m) static int map_io(struct multipath *m, struct request *clone, union map_info *map_context) { - int r = DM_MAPIO_REMAPPED; + int r = DM_MAPIO_REQUEUE; size_t nr_bytes = blk_rq_bytes(clone); unsigned long flags; struct pgpath *pgpath; struct block_device *bdev; - struct dm_mpath_io *mpio = map_context->ptr; + struct dm_mpath_io *mpio; spin_lock_irqsave(&m->lock, flags); @@ -390,29 +390,31 @@ static int map_io(struct multipath *m, struct request *clone, if (pgpath) { if (__pgpath_busy(pgpath)) - r = DM_MAPIO_REQUEUE; - else if (pg_ready(m)) { + goto out_unlock; + + if (pg_ready(m)) { + if (set_mapinfo(m, map_context) < 0) + 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); - } else { - __pg_init_all_paths(m); - r = DM_MAPIO_REQUEUE; + r = DM_MAPIO_REMAPPED; + goto out_unlock; } - } else { - /* No path */ - if (__must_push_back(m)) - r = DM_MAPIO_REQUEUE; - else + __pg_init_all_paths(m); + } else if (!__must_push_back(m)) r = -EIO; /* Failed */ - } +out_unlock: spin_unlock_irqrestore(&m->lock, flags); return r; @@ -908,19 +910,9 @@ static void multipath_dtr(struct dm_target *ti) static int multipath_map(struct dm_target *ti, struct request *clone, union map_info *map_context) { - int r; struct multipath *m = (struct multipath *) ti->private; - if (set_mapinfo(m, map_context) < 0) - /* ENOMEM, requeue */ - return DM_MAPIO_REQUEUE; - - clone->cmd_flags |= REQ_FAILFAST_TRANSPORT; - r = map_io(m, clone, map_context); - if (r < 0 || r == DM_MAPIO_REQUEUE) - clear_mapinfo(m, map_context); - - return r; + return map_io(m, clone, map_context); } /*
When multipath needs to requeue I/O in the block layer the per-request context shouldn't be allocated, as it will be freed immediately afterwards anyway. Avoiding this memory allocation will reduce memory pressure during requeuing. Cc: Mike Snitzer <snitzer@redhat.com> Cc: Jun'ichi Nomura <j-nomura@ce.jp.nec.com> Signed-off-by: Hannes Reinecke <hare@suse.de> --- drivers/md/dm-mpath.c | 40 ++++++++++++++++------------------------ 1 file changed, 16 insertions(+), 24 deletions(-)