* [PATCH] dm mpath: delay retry activate_path on SCSI_DH_RETRY @ 2009-02-17 13:47 Nikanth Karthikesan 2009-02-19 1:55 ` Chandra Seetharaman 2009-02-19 2:11 ` Alasdair G Kergon 0 siblings, 2 replies; 9+ messages in thread From: Nikanth Karthikesan @ 2009-02-17 13:47 UTC (permalink / raw) To: Alasdair G Kergon; +Cc: device-mapper development Delay retry to activate_path if it returns SCSI_DH_RETRY. Signed-off-by: Nikanth Karthikesan <knikanth@suse.de> --- diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c index 095f77b..af54632 100644 --- a/drivers/md/dm-mpath.c +++ b/drivers/md/dm-mpath.c @@ -65,12 +65,14 @@ struct multipath { spinlock_t lock; const char *hw_handler_name; - struct work_struct activate_path; + struct delayed_work activate_path; struct pgpath *pgpath_to_activate; unsigned nr_priority_groups; struct list_head priority_groups; unsigned pg_init_required; /* pg_init needs calling? */ unsigned pg_init_in_progress; /* Only one pg_init allowed at once */ + unsigned long pg_init_jiffy; /* To delay retry if SCSI_DH_RETRY */ +#define SCSI_DH_RETRY_DELAY ((HZ * 2)) unsigned nr_valid_paths; /* Total number of usable paths */ struct pgpath *current_pgpath; @@ -203,7 +205,7 @@ static struct multipath *alloc_multipath(struct dm_target *ti) m->queue_io = 1; INIT_WORK(&m->process_queued_ios, process_queued_ios); INIT_WORK(&m->trigger_event, trigger_event); - INIT_WORK(&m->activate_path, activate_path); + INIT_DELAYED_WORK(&m->activate_path, activate_path); m->mpio_pool = mempool_create_slab_pool(MIN_IOS, _mpio_cache); if (!m->mpio_pool) { kfree(m); @@ -431,6 +433,8 @@ static void process_queued_ios(struct work_struct *work) struct pgpath *pgpath = NULL; unsigned init_required = 0, must_queue = 1; unsigned long flags; + unsigned long delay = 0; + unsigned long now; spin_lock_irqsave(&m->lock, flags); @@ -452,13 +456,20 @@ static void process_queued_ios(struct work_struct *work) m->pg_init_required = 0; m->pg_init_in_progress = 1; init_required = 1; + /* Delay retry due to SCSI_DH_RETRY */ + if (m->pg_init_jiffy) { + now = jiffies; + if (time_after(now, m->pg_init_jiffy)) + delay = now - m->pg_init_jiffy; + m->pg_init_jiffy = 0; + } } out: spin_unlock_irqrestore(&m->lock, flags); if (init_required) - queue_work(kmpath_handlerd, &m->activate_path); + queue_delayed_work(kmpath_handlerd, &m->activate_path, delay); if (!must_queue) dispatch_queued_ios(m); @@ -1060,6 +1071,7 @@ static void pg_init_done(struct dm_path *path, int errors) struct priority_group *pg = pgpath->pg; struct multipath *m = pg->m; unsigned long flags; + bool delay = false; /* device or driver problems */ switch (errors) { @@ -1084,8 +1096,11 @@ static void pg_init_done(struct dm_path *path, int errors) */ bypass_pg(m, pg, 1); break; - /* TODO: For SCSI_DH_RETRY we should wait a couple seconds */ + /* + * For SCSI_DH_RETRY we wait for a couple seconds. + */ case SCSI_DH_RETRY: + delay = true; case SCSI_DH_IMM_RETRY: case SCSI_DH_RES_TEMP_UNAVAIL: if (pg_init_limit_reached(m, pgpath)) @@ -1112,6 +1127,10 @@ static void pg_init_done(struct dm_path *path, int errors) } m->pg_init_in_progress = 0; + if (delay) + m->pg_init_jiffy = jiffies + SCSI_DH_RETRY_DELAY; + else + m->pg_init_jiffy = 0; queue_work(kmultipathd, &m->process_queued_ios); spin_unlock_irqrestore(&m->lock, flags); } @@ -1120,7 +1139,7 @@ static void activate_path(struct work_struct *work) { int ret; struct multipath *m = - container_of(work, struct multipath, activate_path); + container_of(work, struct multipath, activate_path.work); struct dm_path *path; unsigned long flags; ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] dm mpath: delay retry activate_path on SCSI_DH_RETRY 2009-02-17 13:47 [PATCH] dm mpath: delay retry activate_path on SCSI_DH_RETRY Nikanth Karthikesan @ 2009-02-19 1:55 ` Chandra Seetharaman 2009-02-19 2:11 ` Alasdair G Kergon 1 sibling, 0 replies; 9+ messages in thread From: Chandra Seetharaman @ 2009-02-19 1:55 UTC (permalink / raw) To: device-mapper development; +Cc: Alasdair G Kergon Hi Nikanth, Thanks for the patch. Please see my comment below. chandra On Tue, 2009-02-17 at 19:17 +0530, Nikanth Karthikesan wrote: > Delay retry to activate_path if it returns SCSI_DH_RETRY. > > Signed-off-by: Nikanth Karthikesan <knikanth@suse.de> > > --- > > diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c > index 095f77b..af54632 100644 > --- a/drivers/md/dm-mpath.c > +++ b/drivers/md/dm-mpath.c > @@ -65,12 +65,14 @@ struct multipath { > spinlock_t lock; > > const char *hw_handler_name; > - struct work_struct activate_path; > + struct delayed_work activate_path; > struct pgpath *pgpath_to_activate; > unsigned nr_priority_groups; > struct list_head priority_groups; > unsigned pg_init_required; /* pg_init needs calling? */ > unsigned pg_init_in_progress; /* Only one pg_init allowed at once */ > + unsigned long pg_init_jiffy; /* To delay retry if SCSI_DH_RETRY */ > +#define SCSI_DH_RETRY_DELAY ((HZ * 2)) > > unsigned nr_valid_paths; /* Total number of usable paths */ > struct pgpath *current_pgpath; > @@ -203,7 +205,7 @@ static struct multipath *alloc_multipath(struct dm_target *ti) > m->queue_io = 1; > INIT_WORK(&m->process_queued_ios, process_queued_ios); > INIT_WORK(&m->trigger_event, trigger_event); > - INIT_WORK(&m->activate_path, activate_path); > + INIT_DELAYED_WORK(&m->activate_path, activate_path); > m->mpio_pool = mempool_create_slab_pool(MIN_IOS, _mpio_cache); > if (!m->mpio_pool) { > kfree(m); > @@ -431,6 +433,8 @@ static void process_queued_ios(struct work_struct *work) > struct pgpath *pgpath = NULL; > unsigned init_required = 0, must_queue = 1; > unsigned long flags; > + unsigned long delay = 0; > + unsigned long now; > > spin_lock_irqsave(&m->lock, flags); > > @@ -452,13 +456,20 @@ static void process_queued_ios(struct work_struct *work) > m->pg_init_required = 0; > m->pg_init_in_progress = 1; > init_required = 1; > + /* Delay retry due to SCSI_DH_RETRY */ > + if (m->pg_init_jiffy) { > + now = jiffies; > + if (time_after(now, m->pg_init_jiffy)) > + delay = now - m->pg_init_jiffy; I think the logic is reversed. Acc to linux/jiffies.h, "time_after(a,b) returns true if time a is after time b", We want it other way around, don't we ? IMO, we need _not_ be so critical of the time. We could just set a flag in pg_init_done and use 2 seconds in queue_delayed_work(). > + m->pg_init_jiffy = 0; > + } > } > > out: > spin_unlock_irqrestore(&m->lock, flags); > > if (init_required) > - queue_work(kmpath_handlerd, &m->activate_path); > + queue_delayed_work(kmpath_handlerd, &m->activate_path, delay); > > if (!must_queue) > dispatch_queued_ios(m); > @@ -1060,6 +1071,7 @@ static void pg_init_done(struct dm_path *path, int errors) > struct priority_group *pg = pgpath->pg; > struct multipath *m = pg->m; > unsigned long flags; > + bool delay = false; > > /* device or driver problems */ > switch (errors) { > @@ -1084,8 +1096,11 @@ static void pg_init_done(struct dm_path *path, int errors) > */ > bypass_pg(m, pg, 1); > break; > - /* TODO: For SCSI_DH_RETRY we should wait a couple seconds */ > + /* > + * For SCSI_DH_RETRY we wait for a couple seconds. > + */ > case SCSI_DH_RETRY: > + delay = true; > case SCSI_DH_IMM_RETRY: > case SCSI_DH_RES_TEMP_UNAVAIL: > if (pg_init_limit_reached(m, pgpath)) > @@ -1112,6 +1127,10 @@ static void pg_init_done(struct dm_path *path, int errors) > } > > m->pg_init_in_progress = 0; > + if (delay) > + m->pg_init_jiffy = jiffies + SCSI_DH_RETRY_DELAY; > + else > + m->pg_init_jiffy = 0; > queue_work(kmultipathd, &m->process_queued_ios); > spin_unlock_irqrestore(&m->lock, flags); > } > @@ -1120,7 +1139,7 @@ static void activate_path(struct work_struct *work) > { > int ret; > struct multipath *m = > - container_of(work, struct multipath, activate_path); > + container_of(work, struct multipath, activate_path.work); > struct dm_path *path; > unsigned long flags; > > > -- > dm-devel mailing list > dm-devel@redhat.com > https://www.redhat.com/mailman/listinfo/dm-devel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] dm mpath: delay retry activate_path on SCSI_DH_RETRY 2009-02-17 13:47 [PATCH] dm mpath: delay retry activate_path on SCSI_DH_RETRY Nikanth Karthikesan 2009-02-19 1:55 ` Chandra Seetharaman @ 2009-02-19 2:11 ` Alasdair G Kergon 2009-02-19 7:10 ` Nikanth Karthikesan 1 sibling, 1 reply; 9+ messages in thread From: Alasdair G Kergon @ 2009-02-19 2:11 UTC (permalink / raw) To: Nikanth Karthikesan; +Cc: device-mapper development On Tue, Feb 17, 2009 at 07:17:37PM +0530, Nikanth Karthikesan wrote: > Delay retry to activate_path if it returns SCSI_DH_RETRY. Please write a complete patch header if you'd like this queued for upstream! E.g. Why? Any cases where this will make matters worse or make no difference? > + unsigned long pg_init_jiffy; /* To delay retry if SCSI_DH_RETRY */ > +#define SCSI_DH_RETRY_DELAY ((HZ * 2)) Why that particular value? Please move it into a header file with an explanation of what it does. Alasdair -- agk@redhat.com ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] dm mpath: delay retry activate_path on SCSI_DH_RETRY 2009-02-19 2:11 ` Alasdair G Kergon @ 2009-02-19 7:10 ` Nikanth Karthikesan 2009-02-20 0:45 ` Chandra Seetharaman 0 siblings, 1 reply; 9+ messages in thread From: Nikanth Karthikesan @ 2009-02-19 7:10 UTC (permalink / raw) To: Alasdair G Kergon; +Cc: device-mapper development On Thursday 19 February 2009 07:41:18 Alasdair G Kergon wrote: > On Tue, Feb 17, 2009 at 07:17:37PM +0530, Nikanth Karthikesan wrote: > > Delay retry to activate_path if it returns SCSI_DH_RETRY. > > Please write a complete patch header if you'd like this queued for > upstream! > > E.g. Why? Any cases where this will make matters worse or make no > difference? > Sorry for the terse changelog. > > + unsigned long pg_init_jiffy; /* To delay retry if SCSI_DH_RETRY */ > > +#define SCSI_DH_RETRY_DELAY ((HZ * 2)) > > Why that particular value? There is no specific reason. Just picked 2 seconds based on the TODO comment, "For SCSI_DH_RETRY we should wait a couple seconds". > Please move it into a header file with an explanation of what it does. > done. Attached is the patch with comments from you and Chandra incorporated. Thanks Nikanth SCSI Device Handlers return SCSI_DH_IMM_RETRY if we could retry immediately and SCSI_DH_RETRY in cases where it is better to retry after some delay. Currently we retry immediately irrespective of SCSI_DH_IMM_RETRY and SCSI_DH_RETRY. This patch adds a 2 second delay before retrying to activate a device, if it returns SCSI_DH_RETRY. Signed-off-by: Nikanth Karthikesan <knikanth@suse.de> --- diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c index 095f77b..6fd76f1 100644 --- a/drivers/md/dm-mpath.c +++ b/drivers/md/dm-mpath.c @@ -65,12 +65,13 @@ struct multipath { spinlock_t lock; const char *hw_handler_name; - struct work_struct activate_path; + struct delayed_work activate_path; struct pgpath *pgpath_to_activate; unsigned nr_priority_groups; struct list_head priority_groups; unsigned pg_init_required; /* pg_init needs calling? */ unsigned pg_init_in_progress; /* Only one pg_init allowed at once */ + unsigned pg_init_delay; /* delay required before retry? */ unsigned nr_valid_paths; /* Total number of usable paths */ struct pgpath *current_pgpath; @@ -203,7 +204,7 @@ static struct multipath *alloc_multipath(struct dm_target *ti) m->queue_io = 1; INIT_WORK(&m->process_queued_ios, process_queued_ios); INIT_WORK(&m->trigger_event, trigger_event); - INIT_WORK(&m->activate_path, activate_path); + INIT_DELAYED_WORK(&m->activate_path, activate_path); m->mpio_pool = mempool_create_slab_pool(MIN_IOS, _mpio_cache); if (!m->mpio_pool) { kfree(m); @@ -431,6 +432,7 @@ static void process_queued_ios(struct work_struct *work) struct pgpath *pgpath = NULL; unsigned init_required = 0, must_queue = 1; unsigned long flags; + unsigned long delay = 0; spin_lock_irqsave(&m->lock, flags); @@ -452,13 +454,15 @@ static void process_queued_ios(struct work_struct *work) m->pg_init_required = 0; m->pg_init_in_progress = 1; init_required = 1; + if (m->pg_init_delay) + delay = SCSI_DH_RETRY_DELAY; } out: spin_unlock_irqrestore(&m->lock, flags); if (init_required) - queue_work(kmpath_handlerd, &m->activate_path); + queue_delayed_work(kmpath_handlerd, &m->activate_path, delay); if (!must_queue) dispatch_queued_ios(m); @@ -1060,6 +1064,7 @@ static void pg_init_done(struct dm_path *path, int errors) struct priority_group *pg = pgpath->pg; struct multipath *m = pg->m; unsigned long flags; + unsigned delay = 0; /* device or driver problems */ switch (errors) { @@ -1084,8 +1089,11 @@ static void pg_init_done(struct dm_path *path, int errors) */ bypass_pg(m, pg, 1); break; - /* TODO: For SCSI_DH_RETRY we should wait a couple seconds */ + /* + * For SCSI_DH_RETRY we wait before retrying. + */ case SCSI_DH_RETRY: + delay = 1; case SCSI_DH_IMM_RETRY: case SCSI_DH_RES_TEMP_UNAVAIL: if (pg_init_limit_reached(m, pgpath)) @@ -1112,6 +1120,7 @@ static void pg_init_done(struct dm_path *path, int errors) } m->pg_init_in_progress = 0; + m->pg_init_delay = delay; queue_work(kmultipathd, &m->process_queued_ios); spin_unlock_irqrestore(&m->lock, flags); } @@ -1120,7 +1129,7 @@ static void activate_path(struct work_struct *work) { int ret; struct multipath *m = - container_of(work, struct multipath, activate_path); + container_of(work, struct multipath, activate_path.work); struct dm_path *path; unsigned long flags; diff --git a/include/scsi/scsi_dh.h b/include/scsi/scsi_dh.h index 33efce2..f099d86 100644 --- a/include/scsi/scsi_dh.h +++ b/include/scsi/scsi_dh.h @@ -55,6 +55,10 @@ enum { SCSI_DH_NOSYS, SCSI_DH_DRIVER_MAX, }; + +/* Time to wait before retry in case of SCSI_DH_RETRY */ +#define SCSI_DH_RETRY_DELAY ((HZ * 2)) + #if defined(CONFIG_SCSI_DH) || defined(CONFIG_SCSI_DH_MODULE) extern int scsi_dh_activate(struct request_queue *); extern int scsi_dh_handler_exist(const char *); ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] dm mpath: delay retry activate_path on SCSI_DH_RETRY 2009-02-19 7:10 ` Nikanth Karthikesan @ 2009-02-20 0:45 ` Chandra Seetharaman 2009-02-20 5:03 ` Nikanth Karthikesan 0 siblings, 1 reply; 9+ messages in thread From: Chandra Seetharaman @ 2009-02-20 0:45 UTC (permalink / raw) To: Nikanth Karthikesan; +Cc: device-mapper development, Alasdair G Kergon On Thu, 2009-02-19 at 12:40 +0530, Nikanth Karthikesan wrote: <snip> > @@ -431,6 +432,7 @@ static void process_queued_ios(struct work_struct *work) > struct pgpath *pgpath = NULL; > unsigned init_required = 0, must_queue = 1; > unsigned long flags; > + unsigned long delay = 0; I do not see the reason for this variable, you can as well put the delay in pg_init_delay and use it directly (and set it to zero after using it) ? > > spin_lock_irqsave(&m->lock, flags); > > @@ -452,13 +454,15 @@ static void process_queued_ios(struct work_struct *work) > m->pg_init_required = 0; > m->pg_init_in_progress = 1; > init_required = 1; > + if (m->pg_init_delay) > + delay = SCSI_DH_RETRY_DELAY; > } > > out: > spin_unlock_irqrestore(&m->lock, flags); > > if (init_required) > - queue_work(kmpath_handlerd, &m->activate_path); > + queue_delayed_work(kmpath_handlerd, &m->activate_path, delay); > > if (!must_queue) > dispatch_queued_ios(m); > @@ -1060,6 +1064,7 @@ static void pg_init_done(struct dm_path *path, int errors) > struct priority_group *pg = pgpath->pg; > struct multipath *m = pg->m; > unsigned long flags; > + unsigned delay = 0; You can get rid of this variable also and set it directly under SCSI_DH_RETRY. > > /* device or driver problems */ > switch (errors) { > @@ -1084,8 +1089,11 @@ static void pg_init_done(struct dm_path *path, int errors) > */ > bypass_pg(m, pg, 1); > break; > - /* TODO: For SCSI_DH_RETRY we should wait a couple seconds */ > + /* > + * For SCSI_DH_RETRY we wait before retrying. > + */ > case SCSI_DH_RETRY: > + delay = 1; > case SCSI_DH_IMM_RETRY: > case SCSI_DH_RES_TEMP_UNAVAIL: > if (pg_init_limit_reached(m, pgpath)) > @@ -1112,6 +1120,7 @@ static void pg_init_done(struct dm_path *path, int errors) > } > > m->pg_init_in_progress = 0; > + m->pg_init_delay = delay; > queue_work(kmultipathd, &m->process_queued_ios); > spin_unlock_irqrestore(&m->lock, flags); > } > @@ -1120,7 +1129,7 @@ static void activate_path(struct work_struct *work) > { > int ret; > struct multipath *m = > - container_of(work, struct multipath, activate_path); > + container_of(work, struct multipath, activate_path.work); > struct dm_path *path; > unsigned long flags; > > diff --git a/include/scsi/scsi_dh.h b/include/scsi/scsi_dh.h > index 33efce2..f099d86 100644 > --- a/include/scsi/scsi_dh.h > +++ b/include/scsi/scsi_dh.h > @@ -55,6 +55,10 @@ enum { > SCSI_DH_NOSYS, > SCSI_DH_DRIVER_MAX, > }; > + > +/* Time to wait before retry in case of SCSI_DH_RETRY */ > +#define SCSI_DH_RETRY_DELAY ((HZ * 2)) > + > #if defined(CONFIG_SCSI_DH) || defined(CONFIG_SCSI_DH_MODULE) > extern int scsi_dh_activate(struct request_queue *); > extern int scsi_dh_handler_exist(const char *); > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] dm mpath: delay retry activate_path on SCSI_DH_RETRY 2009-02-20 0:45 ` Chandra Seetharaman @ 2009-02-20 5:03 ` Nikanth Karthikesan 2009-02-20 15:08 ` Konrad Rzeszutek 2009-02-20 21:11 ` Chandra Seetharaman 0 siblings, 2 replies; 9+ messages in thread From: Nikanth Karthikesan @ 2009-02-20 5:03 UTC (permalink / raw) To: Alasdair G Kergon; +Cc: device-mapper development, sekharan On Friday 20 February 2009 06:15:29 Chandra Seetharaman wrote: > On Thu, 2009-02-19 at 12:40 +0530, Nikanth Karthikesan wrote: > > <snip> > > > @@ -431,6 +432,7 @@ static void process_queued_ios(struct work_struct > > *work) struct pgpath *pgpath = NULL; > > unsigned init_required = 0, must_queue = 1; > > unsigned long flags; > > + unsigned long delay = 0; > > I do not see the reason for this variable, you can as well put the delay > in pg_init_delay and use it directly (and set it to zero after using > it) ? > I missed resetting pg_init_delay to zero after using it. I have attached the corrected patch with this. This variable keeps the code cleaner(avoids taking m->lock). Also having only a boolean in struct multipath keeps it a bit smaller. off-topic: I think struct multipath can be shrunk even further by making various flags like pg_init_required, pg_init_in_progress, queue_io, queue_if_no_path, saved_queue_if_no_path in to a single variable. Thoughts? <snip> > > @@ -1060,6 +1064,7 @@ static void pg_init_done(struct dm_path *path, int > > errors) struct priority_group *pg = pgpath->pg; > > struct multipath *m = pg->m; > > unsigned long flags; > > + unsigned delay = 0; > > You can get rid of this variable also and set it directly under > SCSI_DH_RETRY. > pg_init_delay is protected by the m->lock. And this variable helps in keeping the code cleaner. <snip> I am attaching the fixed patch(resetting pg_init_delay to zero after using it). Thanks Nikanth SCSI Device Handlers return SCSI_DH_IMM_RETRY if we could retry immediately and SCSI_DH_RETRY in cases where it is better to retry after some delay. Currently we retry immediately irrespective of SCSI_DH_IMM_RETRY and SCSI_DH_RETRY. This patch adds a 2 second delay before retrying to activate a device, if it returns SCSI_DH_RETRY. Signed-off-by: Nikanth Karthikesan <knikanth@suse.de> --- diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c index 095f77b..7ddf775 100644 --- a/drivers/md/dm-mpath.c +++ b/drivers/md/dm-mpath.c @@ -65,12 +65,13 @@ struct multipath { spinlock_t lock; const char *hw_handler_name; - struct work_struct activate_path; + struct delayed_work activate_path; struct pgpath *pgpath_to_activate; unsigned nr_priority_groups; struct list_head priority_groups; unsigned pg_init_required; /* pg_init needs calling? */ unsigned pg_init_in_progress; /* Only one pg_init allowed at once */ + unsigned pg_init_delay; /* delay required before retry? */ unsigned nr_valid_paths; /* Total number of usable paths */ struct pgpath *current_pgpath; @@ -203,7 +204,7 @@ static struct multipath *alloc_multipath(struct dm_target *ti) m->queue_io = 1; INIT_WORK(&m->process_queued_ios, process_queued_ios); INIT_WORK(&m->trigger_event, trigger_event); - INIT_WORK(&m->activate_path, activate_path); + INIT_DELAYED_WORK(&m->activate_path, activate_path); m->mpio_pool = mempool_create_slab_pool(MIN_IOS, _mpio_cache); if (!m->mpio_pool) { kfree(m); @@ -431,6 +432,7 @@ static void process_queued_ios(struct work_struct *work) struct pgpath *pgpath = NULL; unsigned init_required = 0, must_queue = 1; unsigned long flags; + unsigned long delay = 0; spin_lock_irqsave(&m->lock, flags); @@ -452,13 +454,17 @@ static void process_queued_ios(struct work_struct *work) m->pg_init_required = 0; m->pg_init_in_progress = 1; init_required = 1; + if (m->pg_init_delay) { + delay = SCSI_DH_RETRY_DELAY; + m->pg_init_delay = 0; + } } out: spin_unlock_irqrestore(&m->lock, flags); if (init_required) - queue_work(kmpath_handlerd, &m->activate_path); + queue_delayed_work(kmpath_handlerd, &m->activate_path, delay); if (!must_queue) dispatch_queued_ios(m); @@ -1060,6 +1066,7 @@ static void pg_init_done(struct dm_path *path, int errors) struct priority_group *pg = pgpath->pg; struct multipath *m = pg->m; unsigned long flags; + unsigned delay = 0; /* device or driver problems */ switch (errors) { @@ -1084,8 +1091,11 @@ static void pg_init_done(struct dm_path *path, int errors) */ bypass_pg(m, pg, 1); break; - /* TODO: For SCSI_DH_RETRY we should wait a couple seconds */ + /* + * For SCSI_DH_RETRY we wait before retrying. + */ case SCSI_DH_RETRY: + delay = 1; case SCSI_DH_IMM_RETRY: case SCSI_DH_RES_TEMP_UNAVAIL: if (pg_init_limit_reached(m, pgpath)) @@ -1112,6 +1122,7 @@ static void pg_init_done(struct dm_path *path, int errors) } m->pg_init_in_progress = 0; + m->pg_init_delay = delay; queue_work(kmultipathd, &m->process_queued_ios); spin_unlock_irqrestore(&m->lock, flags); } @@ -1120,7 +1131,7 @@ static void activate_path(struct work_struct *work) { int ret; struct multipath *m = - container_of(work, struct multipath, activate_path); + container_of(work, struct multipath, activate_path.work); struct dm_path *path; unsigned long flags; diff --git a/include/scsi/scsi_dh.h b/include/scsi/scsi_dh.h index 33efce2..f099d86 100644 --- a/include/scsi/scsi_dh.h +++ b/include/scsi/scsi_dh.h @@ -55,6 +55,10 @@ enum { SCSI_DH_NOSYS, SCSI_DH_DRIVER_MAX, }; + +/* Time to wait before retry in case of SCSI_DH_RETRY */ +#define SCSI_DH_RETRY_DELAY ((HZ * 2)) + #if defined(CONFIG_SCSI_DH) || defined(CONFIG_SCSI_DH_MODULE) extern int scsi_dh_activate(struct request_queue *); extern int scsi_dh_handler_exist(const char *); \0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: Re: [PATCH] dm mpath: delay retry activate_path on SCSI_DH_RETRY 2009-02-20 5:03 ` Nikanth Karthikesan @ 2009-02-20 15:08 ` Konrad Rzeszutek 2009-02-20 21:11 ` Chandra Seetharaman 1 sibling, 0 replies; 9+ messages in thread From: Konrad Rzeszutek @ 2009-02-20 15:08 UTC (permalink / raw) To: device-mapper development; +Cc: sekharan, Alasdair G Kergon > off-topic: > I think struct multipath can be shrunk even further by making various flags > like pg_init_required, pg_init_in_progress, queue_io, queue_if_no_path, > saved_queue_if_no_path in to a single variable. Thoughts? Do the benefits (cut down on the size of the structure which is allocated once for a map) vs readability worth it? ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] dm mpath: delay retry activate_path on SCSI_DH_RETRY 2009-02-20 5:03 ` Nikanth Karthikesan 2009-02-20 15:08 ` Konrad Rzeszutek @ 2009-02-20 21:11 ` Chandra Seetharaman 2009-03-02 10:48 ` Nikanth Karthikesan 1 sibling, 1 reply; 9+ messages in thread From: Chandra Seetharaman @ 2009-02-20 21:11 UTC (permalink / raw) To: Nikanth Karthikesan; +Cc: device-mapper development, Alasdair G Kergon On Fri, 2009-02-20 at 10:33 +0530, Nikanth Karthikesan wrote: > On Friday 20 February 2009 06:15:29 Chandra Seetharaman wrote: > > On Thu, 2009-02-19 at 12:40 +0530, Nikanth Karthikesan wrote: > > > > <snip> > > > > > @@ -431,6 +432,7 @@ static void process_queued_ios(struct work_struct > > > *work) struct pgpath *pgpath = NULL; > > > unsigned init_required = 0, must_queue = 1; > > > unsigned long flags; > > > + unsigned long delay = 0; > > > > I do not see the reason for this variable, you can as well put the delay > > in pg_init_delay and use it directly (and set it to zero after using > > it) ? > > > > I missed resetting pg_init_delay to zero after using it. I have attached the > corrected patch with this. This variable keeps the code cleaner(avoids taking > m->lock). Also having only a boolean in struct multipath keeps it a bit > smaller. Ok. I am fine with your justification. > > off-topic: > I think struct multipath can be shrunk even further by making various flags > like pg_init_required, pg_init_in_progress, queue_io, queue_if_no_path, > saved_queue_if_no_path in to a single variable. Thoughts? As Konrad mentioned, it is worth the cost of readability ? <snip> > SCSI Device Handlers return SCSI_DH_IMM_RETRY if we could retry > immediately and SCSI_DH_RETRY in cases where it is better to retry > after some delay. > > Currently we retry immediately irrespective of SCSI_DH_IMM_RETRY and > SCSI_DH_RETRY. This patch adds a 2 second delay before retrying to > activate a device, if it returns SCSI_DH_RETRY. > > Signed-off-by: Nikanth Karthikesan <knikanth@suse.de> Acked-by: Chandra Seetharaman <sekharan@us.ibm.com> > > --- > > diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c > index 095f77b..7ddf775 100644 > --- a/drivers/md/dm-mpath.c > +++ b/drivers/md/dm-mpath.c > @@ -65,12 +65,13 @@ struct multipath { > spinlock_t lock; > > const char *hw_handler_name; > - struct work_struct activate_path; > + struct delayed_work activate_path; > struct pgpath *pgpath_to_activate; > unsigned nr_priority_groups; > struct list_head priority_groups; > unsigned pg_init_required; /* pg_init needs calling? */ > unsigned pg_init_in_progress; /* Only one pg_init allowed at once */ > + unsigned pg_init_delay; /* delay required before retry? */ > > unsigned nr_valid_paths; /* Total number of usable paths */ > struct pgpath *current_pgpath; > @@ -203,7 +204,7 @@ static struct multipath *alloc_multipath(struct dm_target *ti) > m->queue_io = 1; > INIT_WORK(&m->process_queued_ios, process_queued_ios); > INIT_WORK(&m->trigger_event, trigger_event); > - INIT_WORK(&m->activate_path, activate_path); > + INIT_DELAYED_WORK(&m->activate_path, activate_path); > m->mpio_pool = mempool_create_slab_pool(MIN_IOS, _mpio_cache); > if (!m->mpio_pool) { > kfree(m); > @@ -431,6 +432,7 @@ static void process_queued_ios(struct work_struct *work) > struct pgpath *pgpath = NULL; > unsigned init_required = 0, must_queue = 1; > unsigned long flags; > + unsigned long delay = 0; > > spin_lock_irqsave(&m->lock, flags); > > @@ -452,13 +454,17 @@ static void process_queued_ios(struct work_struct *work) > m->pg_init_required = 0; > m->pg_init_in_progress = 1; > init_required = 1; > + if (m->pg_init_delay) { > + delay = SCSI_DH_RETRY_DELAY; > + m->pg_init_delay = 0; > + } > } > > out: > spin_unlock_irqrestore(&m->lock, flags); > > if (init_required) > - queue_work(kmpath_handlerd, &m->activate_path); > + queue_delayed_work(kmpath_handlerd, &m->activate_path, delay); > > if (!must_queue) > dispatch_queued_ios(m); > @@ -1060,6 +1066,7 @@ static void pg_init_done(struct dm_path *path, int errors) > struct priority_group *pg = pgpath->pg; > struct multipath *m = pg->m; > unsigned long flags; > + unsigned delay = 0; > > /* device or driver problems */ > switch (errors) { > @@ -1084,8 +1091,11 @@ static void pg_init_done(struct dm_path *path, int errors) > */ > bypass_pg(m, pg, 1); > break; > - /* TODO: For SCSI_DH_RETRY we should wait a couple seconds */ > + /* > + * For SCSI_DH_RETRY we wait before retrying. > + */ > case SCSI_DH_RETRY: > + delay = 1; > case SCSI_DH_IMM_RETRY: > case SCSI_DH_RES_TEMP_UNAVAIL: > if (pg_init_limit_reached(m, pgpath)) > @@ -1112,6 +1122,7 @@ static void pg_init_done(struct dm_path *path, int errors) > } > > m->pg_init_in_progress = 0; > + m->pg_init_delay = delay; > queue_work(kmultipathd, &m->process_queued_ios); > spin_unlock_irqrestore(&m->lock, flags); > } > @@ -1120,7 +1131,7 @@ static void activate_path(struct work_struct *work) > { > int ret; > struct multipath *m = > - container_of(work, struct multipath, activate_path); > + container_of(work, struct multipath, activate_path.work); > struct dm_path *path; > unsigned long flags; > > diff --git a/include/scsi/scsi_dh.h b/include/scsi/scsi_dh.h > index 33efce2..f099d86 100644 > --- a/include/scsi/scsi_dh.h > +++ b/include/scsi/scsi_dh.h > @@ -55,6 +55,10 @@ enum { > SCSI_DH_NOSYS, > SCSI_DH_DRIVER_MAX, > }; > + > +/* Time to wait before retry in case of SCSI_DH_RETRY */ > +#define SCSI_DH_RETRY_DELAY ((HZ * 2)) > + > #if defined(CONFIG_SCSI_DH) || defined(CONFIG_SCSI_DH_MODULE) > extern int scsi_dh_activate(struct request_queue *); > extern int scsi_dh_handler_exist(const char *); > � ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] dm mpath: delay retry activate_path on SCSI_DH_RETRY 2009-02-20 21:11 ` Chandra Seetharaman @ 2009-03-02 10:48 ` Nikanth Karthikesan 0 siblings, 0 replies; 9+ messages in thread From: Nikanth Karthikesan @ 2009-03-02 10:48 UTC (permalink / raw) To: Alasdair G Kergon; +Cc: device-mapper development, sekharan On Saturday 21 February 2009 02:41:33 Chandra Seetharaman wrote: > On Fri, 2009-02-20 at 10:33 +0530, Nikanth Karthikesan wrote: > > On Friday 20 February 2009 06:15:29 Chandra Seetharaman wrote: > > > On Thu, 2009-02-19 at 12:40 +0530, Nikanth Karthikesan wrote: > > > > > > <snip> > > > > > > > @@ -431,6 +432,7 @@ static void process_queued_ios(struct work_struct > > > > *work) struct pgpath *pgpath = NULL; > > > > unsigned init_required = 0, must_queue = 1; > > > > unsigned long flags; > > > > + unsigned long delay = 0; > > > > > > I do not see the reason for this variable, you can as well put the > > > delay in pg_init_delay and use it directly (and set it to zero after > > > using it) ? > > > > I missed resetting pg_init_delay to zero after using it. I have attached > > the corrected patch with this. This variable keeps the code > > cleaner(avoids taking m->lock). Also having only a boolean in struct > > multipath keeps it a bit smaller. > > Ok. I am fine with your justification. > > > off-topic: > > I think struct multipath can be shrunk even further by making various > > flags like pg_init_required, pg_init_in_progress, queue_io, > > queue_if_no_path, saved_queue_if_no_path in to a single variable. > > Thoughts? > > As Konrad mentioned, it is worth the cost of readability ? > > > <snip> > > > SCSI Device Handlers return SCSI_DH_IMM_RETRY if we could retry > > immediately and SCSI_DH_RETRY in cases where it is better to retry > > after some delay. > > > > Currently we retry immediately irrespective of SCSI_DH_IMM_RETRY and > > SCSI_DH_RETRY. This patch adds a 2 second delay before retrying to > > activate a device, if it returns SCSI_DH_RETRY. > > > > Signed-off-by: Nikanth Karthikesan <knikanth@suse.de> > > Acked-by: Chandra Seetharaman <sekharan@us.ibm.com> > Hi Alasdair Can you merge this? Thanks Nikanth > > --- > > > > diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c > > index 095f77b..7ddf775 100644 > > --- a/drivers/md/dm-mpath.c > > +++ b/drivers/md/dm-mpath.c > > @@ -65,12 +65,13 @@ struct multipath { > > spinlock_t lock; > > > > const char *hw_handler_name; > > - struct work_struct activate_path; > > + struct delayed_work activate_path; > > struct pgpath *pgpath_to_activate; > > unsigned nr_priority_groups; > > struct list_head priority_groups; > > unsigned pg_init_required; /* pg_init needs calling? */ > > unsigned pg_init_in_progress; /* Only one pg_init allowed at once */ > > + unsigned pg_init_delay; /* delay required before retry? */ > > > > unsigned nr_valid_paths; /* Total number of usable paths */ > > struct pgpath *current_pgpath; > > @@ -203,7 +204,7 @@ static struct multipath *alloc_multipath(struct > > dm_target *ti) m->queue_io = 1; > > INIT_WORK(&m->process_queued_ios, process_queued_ios); > > INIT_WORK(&m->trigger_event, trigger_event); > > - INIT_WORK(&m->activate_path, activate_path); > > + INIT_DELAYED_WORK(&m->activate_path, activate_path); > > m->mpio_pool = mempool_create_slab_pool(MIN_IOS, _mpio_cache); > > if (!m->mpio_pool) { > > kfree(m); > > @@ -431,6 +432,7 @@ static void process_queued_ios(struct work_struct > > *work) struct pgpath *pgpath = NULL; > > unsigned init_required = 0, must_queue = 1; > > unsigned long flags; > > + unsigned long delay = 0; > > > > spin_lock_irqsave(&m->lock, flags); > > > > @@ -452,13 +454,17 @@ static void process_queued_ios(struct work_struct > > *work) m->pg_init_required = 0; > > m->pg_init_in_progress = 1; > > init_required = 1; > > + if (m->pg_init_delay) { > > + delay = SCSI_DH_RETRY_DELAY; > > + m->pg_init_delay = 0; > > + } > > } > > > > out: > > spin_unlock_irqrestore(&m->lock, flags); > > > > if (init_required) > > - queue_work(kmpath_handlerd, &m->activate_path); > > + queue_delayed_work(kmpath_handlerd, &m->activate_path, delay); > > > > if (!must_queue) > > dispatch_queued_ios(m); > > @@ -1060,6 +1066,7 @@ static void pg_init_done(struct dm_path *path, int > > errors) struct priority_group *pg = pgpath->pg; > > struct multipath *m = pg->m; > > unsigned long flags; > > + unsigned delay = 0; > > > > /* device or driver problems */ > > switch (errors) { > > @@ -1084,8 +1091,11 @@ static void pg_init_done(struct dm_path *path, int > > errors) */ > > bypass_pg(m, pg, 1); > > break; > > - /* TODO: For SCSI_DH_RETRY we should wait a couple seconds */ > > + /* > > + * For SCSI_DH_RETRY we wait before retrying. > > + */ > > case SCSI_DH_RETRY: > > + delay = 1; > > case SCSI_DH_IMM_RETRY: > > case SCSI_DH_RES_TEMP_UNAVAIL: > > if (pg_init_limit_reached(m, pgpath)) > > @@ -1112,6 +1122,7 @@ static void pg_init_done(struct dm_path *path, int > > errors) } > > > > m->pg_init_in_progress = 0; > > + m->pg_init_delay = delay; > > queue_work(kmultipathd, &m->process_queued_ios); > > spin_unlock_irqrestore(&m->lock, flags); > > } > > @@ -1120,7 +1131,7 @@ static void activate_path(struct work_struct *work) > > { > > int ret; > > struct multipath *m = > > - container_of(work, struct multipath, activate_path); > > + container_of(work, struct multipath, activate_path.work); > > struct dm_path *path; > > unsigned long flags; > > > > diff --git a/include/scsi/scsi_dh.h b/include/scsi/scsi_dh.h > > index 33efce2..f099d86 100644 > > --- a/include/scsi/scsi_dh.h > > +++ b/include/scsi/scsi_dh.h > > @@ -55,6 +55,10 @@ enum { > > SCSI_DH_NOSYS, > > SCSI_DH_DRIVER_MAX, > > }; > > + > > +/* Time to wait before retry in case of SCSI_DH_RETRY */ > > +#define SCSI_DH_RETRY_DELAY ((HZ * 2)) > > + > > #if defined(CONFIG_SCSI_DH) || defined(CONFIG_SCSI_DH_MODULE) > > extern int scsi_dh_activate(struct request_queue *); > > extern int scsi_dh_handler_exist(const char *); > > � ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2009-03-02 10:48 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2009-02-17 13:47 [PATCH] dm mpath: delay retry activate_path on SCSI_DH_RETRY Nikanth Karthikesan 2009-02-19 1:55 ` Chandra Seetharaman 2009-02-19 2:11 ` Alasdair G Kergon 2009-02-19 7:10 ` Nikanth Karthikesan 2009-02-20 0:45 ` Chandra Seetharaman 2009-02-20 5:03 ` Nikanth Karthikesan 2009-02-20 15:08 ` Konrad Rzeszutek 2009-02-20 21:11 ` Chandra Seetharaman 2009-03-02 10:48 ` Nikanth Karthikesan
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).