[dm-devel] Re: [PATCH] dm mpath: delay retry activate_path on SCSI_DH_RETRY

[Nikanth Karthikesan]

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 at 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 *);