[PATCH 0/3] scsi_dh: Make scsi_dh_activate asynchronous

[PATCH 0/3] scsi_dh: Make scsi_dh_activate asynchronous

[Chandra Seetharaman]

Hello All,

Currently, device handlers process path activation in series. This leads
to a lot of time delay when more than 100 luns are involved. For example,
with lsi rdac 100+ luns take about 12-15 minutes. This was found by Moger
Babu of LSI.

This time delay can be avoided if we can do the activations asynchronously.
By making scsi_dh_activate() async, we can give the device handlers an
oppurtunity to decide on how to send the device activation down the wire
to make the turn around time faster. They can send the commands
asynchronously or send them in batches.

I brought up this issue on th emailing list few days back
(http://marc.info/?l=linux-scsi&m=123888063818755&w=2).

This set of patches applies cleanly on 2.6.30-rc3 and is tested on the same.

Please review and provide comments.

This set of patched adds asynchronous support only to rdac handler. WIll be
sending patches to support other handlers too.

Thanks & Regards,

chandra

[PATCH 1/3] scsi_dh: Change the scsidh_activate interface to be asynchronous

[Chandra Seetharaman]

Make scsi_dh_activate() function asynchronous, by taking in two additional 
parameters, one is the callback function and the other is the data to call
the callback function with.

--------------
Signed-off-by: Chandra Seetharaman <sekharan@us.ibm.com>

---
 drivers/md/dm-mpath.c                       |    7 ++++---
 drivers/scsi/device_handler/scsi_dh.c       |    6 +++---
 drivers/scsi/device_handler/scsi_dh_alua.c  |    6 ++++--
 drivers/scsi/device_handler/scsi_dh_emc.c   |    6 ++++--
 drivers/scsi/device_handler/scsi_dh_hp_sw.c |    6 ++++--
 drivers/scsi/device_handler/scsi_dh_rdac.c  |    6 ++++--
 include/scsi/scsi_device.h                  |    3 ++-
 include/scsi/scsi_dh.h                      |    7 +++++--
 8 files changed, 30 insertions(+), 17 deletions(-)

Index: linux-2.6.29/include/scsi/scsi_dh.h
===================================================================
--- linux-2.6.29.orig/include/scsi/scsi_dh.h
+++ linux-2.6.29/include/scsi/scsi_dh.h
@@ -55,14 +55,17 @@ enum {
 	SCSI_DH_NOSYS,
 	SCSI_DH_DRIVER_MAX,
 };
+
 #if defined(CONFIG_SCSI_DH) || defined(CONFIG_SCSI_DH_MODULE)
-extern int scsi_dh_activate(struct request_queue *);
+extern int scsi_dh_activate(struct request_queue *, activate_complete, void *);
 extern int scsi_dh_handler_exist(const char *);
 extern int scsi_dh_attach(struct request_queue *, const char *);
 extern void scsi_dh_detach(struct request_queue *);
 #else
-static inline int scsi_dh_activate(struct request_queue *req)
+static inline int scsi_dh_activate(struct request_queue *req,
+					activate_complete fn, void *data)
 {
+	fn(data, 0);
 	return 0;
 }
 static inline int scsi_dh_handler_exist(const char *name)
Index: linux-2.6.29/drivers/scsi/device_handler/scsi_dh.c
===================================================================
--- linux-2.6.29.orig/drivers/scsi/device_handler/scsi_dh.c
+++ linux-2.6.29/drivers/scsi/device_handler/scsi_dh.c
@@ -214,7 +214,7 @@ store_dh_state(struct device *dev, struc
 			 * Activate a device handler
 			 */
 			if (scsi_dh->activate)
-				err = scsi_dh->activate(sdev);
+				err = scsi_dh->activate(sdev, NULL, NULL);
 			else
 				err = 0;
 		}
@@ -414,7 +414,7 @@ EXPORT_SYMBOL_GPL(scsi_unregister_device
  * @q - Request queue that is associated with the scsi_device to be
  *      activated.
  */
-int scsi_dh_activate(struct request_queue *q)
+int scsi_dh_activate(struct request_queue *q, activate_complete fn, void *data)
 {
 	int err = 0;
 	unsigned long flags;
@@ -433,7 +433,7 @@ int scsi_dh_activate(struct request_queu
 		return err;
 
 	if (scsi_dh->activate)
-		err = scsi_dh->activate(sdev);
+		err = scsi_dh->activate(sdev, fn, data);
 	put_device(&sdev->sdev_gendev);
 	return err;
 }
Index: linux-2.6.29/drivers/scsi/device_handler/scsi_dh_rdac.c
===================================================================
--- linux-2.6.29.orig/drivers/scsi/device_handler/scsi_dh_rdac.c
+++ linux-2.6.29/drivers/scsi/device_handler/scsi_dh_rdac.c
@@ -516,7 +516,7 @@ done:
 	return err;
 }
 
-static int rdac_activate(struct scsi_device *sdev)
+static int rdac_activate(struct scsi_device *sdev, activate_complete fn, void *data)
 {
 	struct rdac_dh_data *h = get_rdac_data(sdev);
 	int err = SCSI_DH_OK;
@@ -539,7 +539,9 @@ static int rdac_activate(struct scsi_dev
 	if (h->lun_state == RDAC_LUN_UNOWNED)
 		err = send_mode_select(sdev, h);
 done:
-	return err;
+	if (fn)
+		fn(data, err);
+	return 0;
 }
 
 static int rdac_prep_fn(struct scsi_device *sdev, struct request *req)
Index: linux-2.6.29/drivers/scsi/device_handler/scsi_dh_alua.c
===================================================================
--- linux-2.6.29.orig/drivers/scsi/device_handler/scsi_dh_alua.c
+++ linux-2.6.29/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -652,7 +652,7 @@ out:
  * based on a certain policy. But until we actually encounter them it
  * should be okay.
  */
-static int alua_activate(struct scsi_device *sdev)
+static int alua_activate(struct scsi_device *sdev, activate_complete fn, void *data)
 {
 	struct alua_dh_data *h = get_alua_data(sdev);
 	int err = SCSI_DH_OK;
@@ -667,7 +667,9 @@ static int alua_activate(struct scsi_dev
 		err = alua_stpg(sdev, TPGS_STATE_OPTIMIZED, h);
 
 out:
-	return err;
+	if (fn)
+		fn(data, err);
+	return 0;
 }
 
 /*
Index: linux-2.6.29/drivers/scsi/device_handler/scsi_dh_emc.c
===================================================================
--- linux-2.6.29.orig/drivers/scsi/device_handler/scsi_dh_emc.c
+++ linux-2.6.29/drivers/scsi/device_handler/scsi_dh_emc.c
@@ -528,7 +528,7 @@ retry:
 	return err;
 }
 
-static int clariion_activate(struct scsi_device *sdev)
+static int clariion_activate(struct scsi_device *sdev, activate_complete fn, void *data)
 {
 	struct clariion_dh_data *csdev = get_clariion_data(sdev);
 	int result;
@@ -559,7 +559,9 @@ done:
 		    csdev->port, lun_state[csdev->lun_state],
 		    csdev->default_sp + 'A');
 
-	return result;
+	if (fn)
+		fn(data, result);
+	return 0;
 }
 
 static const struct scsi_dh_devlist clariion_dev_list[] = {
Index: linux-2.6.29/drivers/scsi/device_handler/scsi_dh_hp_sw.c
===================================================================
--- linux-2.6.29.orig/drivers/scsi/device_handler/scsi_dh_hp_sw.c
+++ linux-2.6.29/drivers/scsi/device_handler/scsi_dh_hp_sw.c
@@ -268,7 +268,7 @@ static int hp_sw_prep_fn(struct scsi_dev
  * activate the passive path (and deactivate the
  * previously active one).
  */
-static int hp_sw_activate(struct scsi_device *sdev)
+static int hp_sw_activate(struct scsi_device *sdev, activate_complete fn, void *data)
 {
 	int ret = SCSI_DH_OK;
 	struct hp_sw_dh_data *h = get_hp_sw_data(sdev);
@@ -283,7 +283,9 @@ static int hp_sw_activate(struct scsi_de
 				    HP_SW_NAME);
 	}
 
-	return ret;
+	if (fn)
+		fn(data, ret);
+	return 0;
 }
 
 static const struct scsi_dh_devlist hp_sw_dh_data_list[] = {
Index: linux-2.6.29/include/scsi/scsi_device.h
===================================================================
--- linux-2.6.29.orig/include/scsi/scsi_device.h
+++ linux-2.6.29/include/scsi/scsi_device.h
@@ -174,6 +174,7 @@ struct scsi_dh_devlist {
 	char *model;
 };
 
+typedef void (*activate_complete)(void *, int);
 struct scsi_device_handler {
 	/* Used by the infrastructure */
 	struct list_head list; /* list of scsi_device_handlers */
@@ -185,7 +186,7 @@ struct scsi_device_handler {
 	int (*check_sense)(struct scsi_device *, struct scsi_sense_hdr *);
 	int (*attach)(struct scsi_device *);
 	void (*detach)(struct scsi_device *);
-	int (*activate)(struct scsi_device *);
+	int (*activate)(struct scsi_device *, activate_complete, void *);
 	int (*prep_fn)(struct scsi_device *, struct request *);
 };
 
Index: linux-2.6.29/drivers/md/dm-mpath.c
===================================================================
--- linux-2.6.29.orig/drivers/md/dm-mpath.c
+++ linux-2.6.29/drivers/md/dm-mpath.c
@@ -1053,8 +1053,9 @@ static int pg_init_limit_reached(struct
 	return limit_reached;
 }
 
-static void pg_init_done(struct dm_path *path, int errors)
+static void pg_init_done(void *data, int errors)
 {
+	struct dm_path *path = data;
 	struct pgpath *pgpath = path_to_pgpath(path);
 	struct priority_group *pg = pgpath->pg;
 	struct multipath *m = pg->m;
@@ -1129,8 +1130,8 @@ static void activate_path(struct work_st
 	spin_unlock_irqrestore(&m->lock, flags);
 	if (!path)
 		return;
-	ret = scsi_dh_activate(bdev_get_queue(path->dev->bdev));
-	pg_init_done(path, ret);
+	ret = scsi_dh_activate(bdev_get_queue(path->dev->bdev),
+				pg_init_done, path);
 }
 
 /*

RE: [PATCH 1/3] scsi_dh: Change the scsidh_activate interface to be asynchronous

[Moger, Babu]

Tested by: Babu Moger <babu.moger@lsi.com>

> -----Original Message-----
> From: linux-scsi-owner@vger.kernel.org [mailto:linux-scsi-
> owner@vger.kernel.org] On Behalf Of Chandra Seetharaman
> Sent: Monday, April 27, 2009 9:53 PM
> To: linux-scsi@vger.kernel.org
> Cc: dm-devel@redhat.com; Moger, Babu; michaelc@cs.wisc.edu; Chandra
> Seetharaman
> Subject: [PATCH 1/3] scsi_dh: Change the scsidh_activate interface to be
> asynchronous
> 
> Make scsi_dh_activate() function asynchronous, by taking in two additional
> parameters, one is the callback function and the other is the data to call
> the callback function with.
> 
> --------------
> Signed-off-by: Chandra Seetharaman <sekharan@us.ibm.com>
> 
> ---
>  drivers/md/dm-mpath.c                       |    7 ++++---
>  drivers/scsi/device_handler/scsi_dh.c       |    6 +++---
>  drivers/scsi/device_handler/scsi_dh_alua.c  |    6 ++++--
>  drivers/scsi/device_handler/scsi_dh_emc.c   |    6 ++++--
>  drivers/scsi/device_handler/scsi_dh_hp_sw.c |    6 ++++--
>  drivers/scsi/device_handler/scsi_dh_rdac.c  |    6 ++++--
>  include/scsi/scsi_device.h                  |    3 ++-
>  include/scsi/scsi_dh.h                      |    7 +++++--
>  8 files changed, 30 insertions(+), 17 deletions(-)
> 
> Index: linux-2.6.29/include/scsi/scsi_dh.h
> ===================================================================
> --- linux-2.6.29.orig/include/scsi/scsi_dh.h
> +++ linux-2.6.29/include/scsi/scsi_dh.h
> @@ -55,14 +55,17 @@ enum {
>  	SCSI_DH_NOSYS,
>  	SCSI_DH_DRIVER_MAX,
>  };
> +
>  #if defined(CONFIG_SCSI_DH) || defined(CONFIG_SCSI_DH_MODULE)
> -extern int scsi_dh_activate(struct request_queue *);
> +extern int scsi_dh_activate(struct request_queue *, activate_complete,
> void *);
>  extern int scsi_dh_handler_exist(const char *);
>  extern int scsi_dh_attach(struct request_queue *, const char *);
>  extern void scsi_dh_detach(struct request_queue *);
>  #else
> -static inline int scsi_dh_activate(struct request_queue *req)
> +static inline int scsi_dh_activate(struct request_queue *req,
> +					activate_complete fn, void *data)
>  {
> +	fn(data, 0);
>  	return 0;
>  }
>  static inline int scsi_dh_handler_exist(const char *name)
> Index: linux-2.6.29/drivers/scsi/device_handler/scsi_dh.c
> ===================================================================
> --- linux-2.6.29.orig/drivers/scsi/device_handler/scsi_dh.c
> +++ linux-2.6.29/drivers/scsi/device_handler/scsi_dh.c
> @@ -214,7 +214,7 @@ store_dh_state(struct device *dev, struc
>  			 * Activate a device handler
>  			 */
>  			if (scsi_dh->activate)
> -				err = scsi_dh->activate(sdev);
> +				err = scsi_dh->activate(sdev, NULL, NULL);
>  			else
>  				err = 0;
>  		}
> @@ -414,7 +414,7 @@ EXPORT_SYMBOL_GPL(scsi_unregister_device
>   * @q - Request queue that is associated with the scsi_device to be
>   *      activated.
>   */
> -int scsi_dh_activate(struct request_queue *q)
> +int scsi_dh_activate(struct request_queue *q, activate_complete fn, void
> *data)
>  {
>  	int err = 0;
>  	unsigned long flags;
> @@ -433,7 +433,7 @@ int scsi_dh_activate(struct request_queu
>  		return err;
> 
>  	if (scsi_dh->activate)
> -		err = scsi_dh->activate(sdev);
> +		err = scsi_dh->activate(sdev, fn, data);
>  	put_device(&sdev->sdev_gendev);
>  	return err;
>  }
> Index: linux-2.6.29/drivers/scsi/device_handler/scsi_dh_rdac.c
> ===================================================================
> --- linux-2.6.29.orig/drivers/scsi/device_handler/scsi_dh_rdac.c
> +++ linux-2.6.29/drivers/scsi/device_handler/scsi_dh_rdac.c
> @@ -516,7 +516,7 @@ done:
>  	return err;
>  }
> 
> -static int rdac_activate(struct scsi_device *sdev)
> +static int rdac_activate(struct scsi_device *sdev, activate_complete fn,
> void *data)
>  {
>  	struct rdac_dh_data *h = get_rdac_data(sdev);
>  	int err = SCSI_DH_OK;
> @@ -539,7 +539,9 @@ static int rdac_activate(struct scsi_dev
>  	if (h->lun_state == RDAC_LUN_UNOWNED)
>  		err = send_mode_select(sdev, h);
>  done:
> -	return err;
> +	if (fn)
> +		fn(data, err);
> +	return 0;
>  }
> 
>  static int rdac_prep_fn(struct scsi_device *sdev, struct request *req)
> Index: linux-2.6.29/drivers/scsi/device_handler/scsi_dh_alua.c
> ===================================================================
> --- linux-2.6.29.orig/drivers/scsi/device_handler/scsi_dh_alua.c
> +++ linux-2.6.29/drivers/scsi/device_handler/scsi_dh_alua.c
> @@ -652,7 +652,7 @@ out:
>   * based on a certain policy. But until we actually encounter them it
>   * should be okay.
>   */
> -static int alua_activate(struct scsi_device *sdev)
> +static int alua_activate(struct scsi_device *sdev, activate_complete fn,
> void *data)
>  {
>  	struct alua_dh_data *h = get_alua_data(sdev);
>  	int err = SCSI_DH_OK;
> @@ -667,7 +667,9 @@ static int alua_activate(struct scsi_dev
>  		err = alua_stpg(sdev, TPGS_STATE_OPTIMIZED, h);
> 
>  out:
> -	return err;
> +	if (fn)
> +		fn(data, err);
> +	return 0;
>  }
> 
>  /*
> Index: linux-2.6.29/drivers/scsi/device_handler/scsi_dh_emc.c
> ===================================================================
> --- linux-2.6.29.orig/drivers/scsi/device_handler/scsi_dh_emc.c
> +++ linux-2.6.29/drivers/scsi/device_handler/scsi_dh_emc.c
> @@ -528,7 +528,7 @@ retry:
>  	return err;
>  }
> 
> -static int clariion_activate(struct scsi_device *sdev)
> +static int clariion_activate(struct scsi_device *sdev, activate_complete
> fn, void *data)
>  {
>  	struct clariion_dh_data *csdev = get_clariion_data(sdev);
>  	int result;
> @@ -559,7 +559,9 @@ done:
>  		    csdev->port, lun_state[csdev->lun_state],
>  		    csdev->default_sp + 'A');
> 
> -	return result;
> +	if (fn)
> +		fn(data, result);
> +	return 0;
>  }
> 
>  static const struct scsi_dh_devlist clariion_dev_list[] = {
> Index: linux-2.6.29/drivers/scsi/device_handler/scsi_dh_hp_sw.c
> ===================================================================
> --- linux-2.6.29.orig/drivers/scsi/device_handler/scsi_dh_hp_sw.c
> +++ linux-2.6.29/drivers/scsi/device_handler/scsi_dh_hp_sw.c
> @@ -268,7 +268,7 @@ static int hp_sw_prep_fn(struct scsi_dev
>   * activate the passive path (and deactivate the
>   * previously active one).
>   */
> -static int hp_sw_activate(struct scsi_device *sdev)
> +static int hp_sw_activate(struct scsi_device *sdev, activate_complete fn,
> void *data)
>  {
>  	int ret = SCSI_DH_OK;
>  	struct hp_sw_dh_data *h = get_hp_sw_data(sdev);
> @@ -283,7 +283,9 @@ static int hp_sw_activate(struct scsi_de
>  				    HP_SW_NAME);
>  	}
> 
> -	return ret;
> +	if (fn)
> +		fn(data, ret);
> +	return 0;
>  }
> 
>  static const struct scsi_dh_devlist hp_sw_dh_data_list[] = {
> Index: linux-2.6.29/include/scsi/scsi_device.h
> ===================================================================
> --- linux-2.6.29.orig/include/scsi/scsi_device.h
> +++ linux-2.6.29/include/scsi/scsi_device.h
> @@ -174,6 +174,7 @@ struct scsi_dh_devlist {
>  	char *model;
>  };
> 
> +typedef void (*activate_complete)(void *, int);
>  struct scsi_device_handler {
>  	/* Used by the infrastructure */
>  	struct list_head list; /* list of scsi_device_handlers */
> @@ -185,7 +186,7 @@ struct scsi_device_handler {
>  	int (*check_sense)(struct scsi_device *, struct scsi_sense_hdr *);
>  	int (*attach)(struct scsi_device *);
>  	void (*detach)(struct scsi_device *);
> -	int (*activate)(struct scsi_device *);
> +	int (*activate)(struct scsi_device *, activate_complete, void *);
>  	int (*prep_fn)(struct scsi_device *, struct request *);
>  };
> 
> Index: linux-2.6.29/drivers/md/dm-mpath.c
> ===================================================================
> --- linux-2.6.29.orig/drivers/md/dm-mpath.c
> +++ linux-2.6.29/drivers/md/dm-mpath.c
> @@ -1053,8 +1053,9 @@ static int pg_init_limit_reached(struct
>  	return limit_reached;
>  }
> 
> -static void pg_init_done(struct dm_path *path, int errors)
> +static void pg_init_done(void *data, int errors)
>  {
> +	struct dm_path *path = data;
>  	struct pgpath *pgpath = path_to_pgpath(path);
>  	struct priority_group *pg = pgpath->pg;
>  	struct multipath *m = pg->m;
> @@ -1129,8 +1130,8 @@ static void activate_path(struct work_st
>  	spin_unlock_irqrestore(&m->lock, flags);
>  	if (!path)
>  		return;
> -	ret = scsi_dh_activate(bdev_get_queue(path->dev->bdev));
> -	pg_init_done(path, ret);
> +	ret = scsi_dh_activate(bdev_get_queue(path->dev->bdev),
> +				pg_init_done, path);
>  }
> 
>  /*
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/3] scsi_dh: Remove the workqueue used for activate

[Chandra Seetharaman]

Since scsi_dh_activate() has become an asynchronous function,
we do not need a workqueue for submitting scsi_dh_activate().

This patch just removes the workqueue that submits scsi_dh_activate().

---------
Signed-off-by: Chandra Seetharaman <sekharan@us.ibm.com>
---
 drivers/md/dm-mpath.c |   51 ++++----------------------------------------------
 1 file changed, 4 insertions(+), 47 deletions(-)

Index: linux-2.6.29/drivers/md/dm-mpath.c
===================================================================
--- linux-2.6.29.orig/drivers/md/dm-mpath.c
+++ linux-2.6.29/drivers/md/dm-mpath.c
@@ -64,8 +64,6 @@ struct multipath {
 	spinlock_t lock;
 
 	const char *hw_handler_name;
-	struct work_struct activate_path;
-	struct pgpath *pgpath_to_activate;
 	unsigned nr_priority_groups;
 	struct list_head priority_groups;
 	unsigned pg_init_required;	/* pg_init needs calling? */
@@ -110,11 +108,11 @@ typedef int (*action_fn) (struct pgpath
 
 static struct kmem_cache *_mpio_cache;
 
-static struct workqueue_struct *kmultipathd, *kmpath_handlerd;
+static struct workqueue_struct *kmultipathd;
 static void process_queued_ios(struct work_struct *work);
 static void trigger_event(struct work_struct *work);
-static void activate_path(struct work_struct *work);
 static void deactivate_path(struct work_struct *work);
+static void pg_init_done(void *path, int errors);
 
 
 /*-----------------------------------------------
@@ -160,7 +158,6 @@ static struct priority_group *alloc_prio
 
 static void free_pgpaths(struct list_head *pgpaths, struct dm_target *ti)
 {
-	unsigned long flags;
 	struct pgpath *pgpath, *tmp;
 	struct multipath *m = ti->private;
 
@@ -169,10 +166,6 @@ static void free_pgpaths(struct list_hea
 		if (m->hw_handler_name)
 			scsi_dh_detach(bdev_get_queue(pgpath->path.dev->bdev));
 		dm_put_device(ti, pgpath->path.dev);
-		spin_lock_irqsave(&m->lock, flags);
-		if (m->pgpath_to_activate == pgpath)
-			m->pgpath_to_activate = NULL;
-		spin_unlock_irqrestore(&m->lock, flags);
 		free_pgpath(pgpath);
 	}
 }
@@ -202,7 +195,6 @@ static struct multipath *alloc_multipath
 		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);
 		m->mpio_pool = mempool_create_slab_pool(MIN_IOS, _mpio_cache);
 		if (!m->mpio_pool) {
 			kfree(m);
@@ -446,7 +438,6 @@ static void process_queued_ios(struct wo
 		must_queue = 0;
 
 	if (m->pg_init_required && !m->pg_init_in_progress && pgpath) {
-		m->pgpath_to_activate = pgpath;
 		m->pg_init_count++;
 		m->pg_init_required = 0;
 		m->pg_init_in_progress = 1;
@@ -457,7 +448,8 @@ out:
 	spin_unlock_irqrestore(&m->lock, flags);
 
 	if (init_required)
-		queue_work(kmpath_handlerd, &m->activate_path);
+		scsi_dh_activate(bdev_get_queue(pgpath->path.dev->bdev),
+						 pg_init_done, &pgpath->path);
 
 	if (!must_queue)
 		dispatch_queued_ios(m);
@@ -834,7 +826,6 @@ static void multipath_dtr(struct dm_targ
 {
 	struct multipath *m = (struct multipath *) ti->private;
 
-	flush_workqueue(kmpath_handlerd);
 	flush_workqueue(kmultipathd);
 	free_multipath(m);
 }
@@ -1116,24 +1107,6 @@ static void pg_init_done(void *data, int
 	spin_unlock_irqrestore(&m->lock, flags);
 }
 
-static void activate_path(struct work_struct *work)
-{
-	int ret;
-	struct multipath *m =
-		container_of(work, struct multipath, activate_path);
-	struct dm_path *path;
-	unsigned long flags;
-
-	spin_lock_irqsave(&m->lock, flags);
-	path = &m->pgpath_to_activate->path;
-	m->pgpath_to_activate = NULL;
-	spin_unlock_irqrestore(&m->lock, flags);
-	if (!path)
-		return;
-	ret = scsi_dh_activate(bdev_get_queue(path->dev->bdev),
-				pg_init_done, path);
-}
-
 /*
  * end_io handling
  */
@@ -1471,21 +1444,6 @@ static int __init dm_multipath_init(void
 		return -ENOMEM;
 	}
 
-	/*
-	 * A separate workqueue is used to handle the device handlers
-	 * to avoid overloading existing workqueue. Overloading the
-	 * old workqueue would also create a bottleneck in the
-	 * path of the storage hardware device activation.
-	 */
-	kmpath_handlerd = create_singlethread_workqueue("kmpath_handlerd");
-	if (!kmpath_handlerd) {
-		DMERR("failed to create workqueue kmpath_handlerd");
-		destroy_workqueue(kmultipathd);
-		dm_unregister_target(&multipath_target);
-		kmem_cache_destroy(_mpio_cache);
-		return -ENOMEM;
-	}
-
 	DMINFO("version %u.%u.%u loaded",
 	       multipath_target.version[0], multipath_target.version[1],
 	       multipath_target.version[2]);
@@ -1495,7 +1453,6 @@ static int __init dm_multipath_init(void
 
 static void __exit dm_multipath_exit(void)
 {
-	destroy_workqueue(kmpath_handlerd);
 	destroy_workqueue(kmultipathd);
 
 	dm_unregister_target(&multipath_target);

RE: [PATCH 2/3] scsi_dh: Remove the workqueue used for activate

[Moger, Babu]

Tested by: Babu Moger <babu.moger@lsi.com>

> -----Original Message-----
> From: linux-scsi-owner@vger.kernel.org [mailto:linux-scsi-
> owner@vger.kernel.org] On Behalf Of Chandra Seetharaman
> Sent: Monday, April 27, 2009 9:53 PM
> To: linux-scsi@vger.kernel.org
> Cc: dm-devel@redhat.com; Moger, Babu; michaelc@cs.wisc.edu; Chandra
> Seetharaman
> Subject: [PATCH 2/3] scsi_dh: Remove the workqueue used for activate
> 
> Since scsi_dh_activate() has become an asynchronous function,
> we do not need a workqueue for submitting scsi_dh_activate().
> 
> This patch just removes the workqueue that submits scsi_dh_activate().
> 
> ---------
> Signed-off-by: Chandra Seetharaman <sekharan@us.ibm.com>
> ---
>  drivers/md/dm-mpath.c |   51 ++++----------------------------------------
> ------
>  1 file changed, 4 insertions(+), 47 deletions(-)
> 
> Index: linux-2.6.29/drivers/md/dm-mpath.c
> ===================================================================
> --- linux-2.6.29.orig/drivers/md/dm-mpath.c
> +++ linux-2.6.29/drivers/md/dm-mpath.c
> @@ -64,8 +64,6 @@ struct multipath {
>  	spinlock_t lock;
> 
>  	const char *hw_handler_name;
> -	struct work_struct activate_path;
> -	struct pgpath *pgpath_to_activate;
>  	unsigned nr_priority_groups;
>  	struct list_head priority_groups;
>  	unsigned pg_init_required;	/* pg_init needs calling? */
> @@ -110,11 +108,11 @@ typedef int (*action_fn) (struct pgpath
> 
>  static struct kmem_cache *_mpio_cache;
> 
> -static struct workqueue_struct *kmultipathd, *kmpath_handlerd;
> +static struct workqueue_struct *kmultipathd;
>  static void process_queued_ios(struct work_struct *work);
>  static void trigger_event(struct work_struct *work);
> -static void activate_path(struct work_struct *work);
>  static void deactivate_path(struct work_struct *work);
> +static void pg_init_done(void *path, int errors);
> 
> 
>  /*-----------------------------------------------
> @@ -160,7 +158,6 @@ static struct priority_group *alloc_prio
> 
>  static void free_pgpaths(struct list_head *pgpaths, struct dm_target *ti)
>  {
> -	unsigned long flags;
>  	struct pgpath *pgpath, *tmp;
>  	struct multipath *m = ti->private;
> 
> @@ -169,10 +166,6 @@ static void free_pgpaths(struct list_hea
>  		if (m->hw_handler_name)
>  			scsi_dh_detach(bdev_get_queue(pgpath->path.dev->bdev));
>  		dm_put_device(ti, pgpath->path.dev);
> -		spin_lock_irqsave(&m->lock, flags);
> -		if (m->pgpath_to_activate == pgpath)
> -			m->pgpath_to_activate = NULL;
> -		spin_unlock_irqrestore(&m->lock, flags);
>  		free_pgpath(pgpath);
>  	}
>  }
> @@ -202,7 +195,6 @@ static struct multipath *alloc_multipath
>  		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);
>  		m->mpio_pool = mempool_create_slab_pool(MIN_IOS, _mpio_cache);
>  		if (!m->mpio_pool) {
>  			kfree(m);
> @@ -446,7 +438,6 @@ static void process_queued_ios(struct wo
>  		must_queue = 0;
> 
>  	if (m->pg_init_required && !m->pg_init_in_progress && pgpath) {
> -		m->pgpath_to_activate = pgpath;
>  		m->pg_init_count++;
>  		m->pg_init_required = 0;
>  		m->pg_init_in_progress = 1;
> @@ -457,7 +448,8 @@ out:
>  	spin_unlock_irqrestore(&m->lock, flags);
> 
>  	if (init_required)
> -		queue_work(kmpath_handlerd, &m->activate_path);
> +		scsi_dh_activate(bdev_get_queue(pgpath->path.dev->bdev),
> +						 pg_init_done, &pgpath->path);
> 
>  	if (!must_queue)
>  		dispatch_queued_ios(m);
> @@ -834,7 +826,6 @@ static void multipath_dtr(struct dm_targ
>  {
>  	struct multipath *m = (struct multipath *) ti->private;
> 
> -	flush_workqueue(kmpath_handlerd);
>  	flush_workqueue(kmultipathd);
>  	free_multipath(m);
>  }
> @@ -1116,24 +1107,6 @@ static void pg_init_done(void *data, int
>  	spin_unlock_irqrestore(&m->lock, flags);
>  }
> 
> -static void activate_path(struct work_struct *work)
> -{
> -	int ret;
> -	struct multipath *m =
> -		container_of(work, struct multipath, activate_path);
> -	struct dm_path *path;
> -	unsigned long flags;
> -
> -	spin_lock_irqsave(&m->lock, flags);
> -	path = &m->pgpath_to_activate->path;
> -	m->pgpath_to_activate = NULL;
> -	spin_unlock_irqrestore(&m->lock, flags);
> -	if (!path)
> -		return;
> -	ret = scsi_dh_activate(bdev_get_queue(path->dev->bdev),
> -				pg_init_done, path);
> -}
> -
>  /*
>   * end_io handling
>   */
> @@ -1471,21 +1444,6 @@ static int __init dm_multipath_init(void
>  		return -ENOMEM;
>  	}
> 
> -	/*
> -	 * A separate workqueue is used to handle the device handlers
> -	 * to avoid overloading existing workqueue. Overloading the
> -	 * old workqueue would also create a bottleneck in the
> -	 * path of the storage hardware device activation.
> -	 */
> -	kmpath_handlerd = create_singlethread_workqueue("kmpath_handlerd");
> -	if (!kmpath_handlerd) {
> -		DMERR("failed to create workqueue kmpath_handlerd");
> -		destroy_workqueue(kmultipathd);
> -		dm_unregister_target(&multipath_target);
> -		kmem_cache_destroy(_mpio_cache);
> -		return -ENOMEM;
> -	}
> -
>  	DMINFO("version %u.%u.%u loaded",
>  	       multipath_target.version[0], multipath_target.version[1],
>  	       multipath_target.version[2]);
> @@ -1495,7 +1453,6 @@ static int __init dm_multipath_init(void
> 
>  static void __exit dm_multipath_exit(void)
>  {
> -	destroy_workqueue(kmpath_handlerd);
>  	destroy_workqueue(kmultipathd);
> 
>  	dm_unregister_target(&multipath_target);
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] scsi_dh: Remove the workqueue used for activate

[Mike Christie]

Chandra Seetharaman wrote:
> Since scsi_dh_activate() has become an asynchronous function,
> we do not need a workqueue for submitting scsi_dh_activate().
> 
> This patch just removes the workqueue that submits scsi_dh_activate().
> 

Before this is merged, we need to convert the other device handlers to 
use blk_execute_rq_nowait or use their own workqueue_struct right?

Re: [PATCH 2/3] scsi_dh: Remove the workqueue used foractivate

[Chandra Seetharaman]

On Wed, 2009-05-06 at 22:32 -0500, Mike Christie wrote:
> Chandra Seetharaman wrote:
> > Since scsi_dh_activate() has become an asynchronous function,
> > we do not need a workqueue for submitting scsi_dh_activate().
> > 
> > This patch just removes the workqueue that submits scsi_dh_activate().
> > 
> 
> Before this is merged, we need to convert the other device handlers to 
> use blk_execute_rq_nowait or use their own workqueue_struct right?

If we want those modules also behave async, then yes.

But, this set of 3 patches would work fine as I made the relevant
changes to those modules so that they work properly.
 
> 
> --
> dm-devel mailing list
> dm-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/dm-devel

Re: [PATCH 2/3] scsi_dh: Remove the workqueue used foractivate

[Mike Christie]

Chandra Seetharaman wrote:
> On Wed, 2009-05-06 at 22:32 -0500, Mike Christie wrote:
>> Chandra Seetharaman wrote:
>>> Since scsi_dh_activate() has become an asynchronous function,
>>> we do not need a workqueue for submitting scsi_dh_activate().
>>>
>>> This patch just removes the workqueue that submits scsi_dh_activate().
>>>
>> Before this is merged, we need to convert the other device handlers to 
>> use blk_execute_rq_nowait or use their own workqueue_struct right?
> 
> If we want those modules also behave async, then yes.
> 
> But, this set of 3 patches would work fine as I made the relevant
> changes to those modules so that they work properly.
>  

What was the point of the workqueue?

I think if you merged the patch and left the other modules as is you 
would hit a problem where the multipathd work queue struct thread is 
busy waiting on activate commands when it should be routing IO to paths 
that do not need a activate.

Re: [dm-devel] [PATCH 2/3] scsi_dh: Remove the workqueue usedforactivate

[Chandra Seetharaman]

On Thu, 2009-05-07 at 22:09 -0500, Mike Christie wrote:
> Chandra Seetharaman wrote:
> > On Wed, 2009-05-06 at 22:32 -0500, Mike Christie wrote:
> >> Chandra Seetharaman wrote:
> >>> Since scsi_dh_activate() has become an asynchronous function,
> >>> we do not need a workqueue for submitting scsi_dh_activate().
> >>>
> >>> This patch just removes the workqueue that submits scsi_dh_activate().
> >>>
> >> Before this is merged, we need to convert the other device handlers to 
> >> use blk_execute_rq_nowait or use their own workqueue_struct right?
> > 
> > If we want those modules also behave async, then yes.
> > 
> > But, this set of 3 patches would work fine as I made the relevant
> > changes to those modules so that they work properly.
> >  
> 
> What was the point of the workqueue?
> 
> I think if you merged the patch and left the other modules as is you 
> would hit a problem where the multipathd work queue struct thread is 
> busy waiting on activate commands when it should be routing IO to paths 
> that do not need a activate.

I am sorry for not being clear. As I mentioned in my original posting, I
am working on making the other modules also async. WIll be posting those
patches in a day or two. May be I will resubmit this set (with your
suggestion for GFP_NOIO) along with those.

In my reply (above), I was just trying to say that these set of patches
work (with the caveat you noted).

> 
> --
> dm-devel mailing list
> dm-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/dm-devel

[PATCH 3/3] scsi_dh: rdac handler: Batch up MODE SELECTs and send few of them

[Chandra Seetharaman]

Batch up MODE_SELECT in rdac device handler.

LSI RDAC storage has the capability of handling mode selects for
multiple luns in a same command. Make use of that ability to send
as few MODE SELECTs as possible to the storage controller as possible.

This patch creates a work queue and queues up activate requests
when a MODE SELECT is sent down the wire. When that MODE SELECT 
completes, it compiles queued up activate requests for multiple
luns into a single MODE SELECT.

This reduces the time to do failover/failback of large number of LUNS.

Signed-off-by: Babu Moger <babu.moger@lsi.com>
Signed-off-by: Chandra Seetharaman <sekharan@us.ibm.com>

---
 drivers/scsi/device_handler/scsi_dh_rdac.c |  117 ++++++++++++++++++++++++++---
 1 file changed, 104 insertions(+), 13 deletions(-)

Index: linux-2.6.29/drivers/scsi/device_handler/scsi_dh_rdac.c
===================================================================
--- linux-2.6.29.orig/drivers/scsi/device_handler/scsi_dh_rdac.c
+++ linux-2.6.29/drivers/scsi/device_handler/scsi_dh_rdac.c
@@ -22,6 +22,7 @@
 #include <scsi/scsi.h>
 #include <scsi/scsi_eh.h>
 #include <scsi/scsi_dh.h>
+#include <linux/workqueue.h>
 
 #define RDAC_NAME "rdac"
 #define RDAC_RETRY_COUNT 5
@@ -135,6 +136,11 @@ struct rdac_controller {
 		struct rdac_pg_legacy legacy;
 		struct rdac_pg_expanded expanded;
 	} mode_select;
+	spinlock_t		ms_lock;
+	int			ms_queued;
+	struct work_struct	ms_work;
+	struct scsi_device	*ms_sdev;
+	struct list_head	ms_head;
 };
 struct c8_inquiry {
 	u8	peripheral_info;
@@ -195,8 +201,17 @@ static const char *lun_state[] =
 	"owned (AVT mode)",
 };
 
+struct rdac_queue_data {
+	struct list_head	entry;
+	struct rdac_dh_data	*h;
+	activate_complete	callback_fn;
+	void			*callback_data;
+};
+
 static LIST_HEAD(ctlr_list);
 static DEFINE_SPINLOCK(list_lock);
+static struct workqueue_struct *kmpath_rdacd;
+static void send_mode_select(struct work_struct *work);
 
 static inline struct rdac_dh_data *get_rdac_data(struct scsi_device *sdev)
 {
@@ -253,7 +268,6 @@ static struct request *rdac_failover_get
 		rdac_pg->subpage_code = 0x1;
 		rdac_pg->page_len[0] = 0x01;
 		rdac_pg->page_len[1] = 0x28;
-		rdac_pg->lun_table[h->lun] = 0x81;
 	} else {
 		struct rdac_pg_legacy *rdac_pg;
 
@@ -263,7 +277,6 @@ static struct request *rdac_failover_get
 		common = &rdac_pg->common;
 		rdac_pg->page_code = RDAC_PAGE_CODE_REDUNDANT_CONTROLLER;
 		rdac_pg->page_len = 0x68;
-		rdac_pg->lun_table[h->lun] = 0x81;
 	}
 	common->rdac_mode[1] = RDAC_MODE_TRANSFER_SPECIFIED_LUNS;
 	common->quiescence_timeout = RDAC_QUIESCENCE_TIME;
@@ -297,6 +310,7 @@ static void release_controller(struct kr
 	struct rdac_controller *ctlr;
 	ctlr = container_of(kref, struct rdac_controller, kref);
 
+	flush_workqueue(kmpath_rdacd);
 	spin_lock(&list_lock);
 	list_del(&ctlr->node);
 	spin_unlock(&list_lock);
@@ -326,6 +340,11 @@ static struct rdac_controller *get_contr
 	memcpy(ctlr->slot_id, slot_id, SLOT_ID_LEN);
 	kref_init(&ctlr->kref);
 	ctlr->use_ms10 = -1;
+	ctlr->ms_queued = 0;
+	ctlr->ms_sdev = NULL;
+	spin_lock_init(&ctlr->ms_lock);
+	INIT_WORK(&ctlr->ms_work, send_mode_select);
+	INIT_LIST_HEAD(&ctlr->ms_head);
 	list_add(&ctlr->node, &ctlr_list);
 done:
 	spin_unlock(&list_lock);
@@ -445,8 +464,7 @@ static int set_mode_select(struct scsi_d
 	return err;
 }
 
-static int mode_select_handle_sense(struct scsi_device *sdev,
-				    unsigned char *sensebuf)
+static int mode_select_handle_sense(unsigned char *sensebuf)
 {
 	struct scsi_sense_hdr sense_hdr;
 	int err = SCSI_DH_IO, ret;
@@ -478,7 +496,7 @@ static int mode_select_handle_sense(stru
 			err = SCSI_DH_RETRY;
 		break;
 	default:
-		sdev_printk(KERN_INFO, sdev,
+		printk(KERN_INFO
 			    "MODE_SELECT failed with sense %02x/%02x/%02x.\n",
 			    sense_hdr.sense_key, sense_hdr.asc, sense_hdr.ascq);
 	}
@@ -487,11 +505,29 @@ done:
 	return err;
 }
 
-static int send_mode_select(struct scsi_device *sdev, struct rdac_dh_data *h)
+static void send_mode_select(struct work_struct *work)
 {
+	struct rdac_controller *ctlr =
+		container_of(work, struct rdac_controller, ms_work);
 	struct request *rq;
+	struct scsi_device *sdev = ctlr->ms_sdev;
+	struct rdac_dh_data *h = get_rdac_data(sdev);
 	struct request_queue *q = sdev->request_queue;
 	int err, retry_cnt = RDAC_RETRY_COUNT;
+	struct rdac_queue_data *tmp, *qdata;
+	LIST_HEAD(list);
+	u8 *lun_table;
+
+	spin_lock(&ctlr->ms_lock);
+	list_splice_init(&ctlr->ms_head, &list);
+	ctlr->ms_queued = 0;
+	ctlr->ms_sdev = NULL;
+	spin_unlock(&ctlr->ms_lock);
+
+	if (ctlr->use_ms10)
+		lun_table = ctlr->mode_select.expanded.lun_table;
+	else
+		lun_table = ctlr->mode_select.legacy.lun_table;
 
 retry:
 	err = SCSI_DH_RES_TEMP_UNAVAIL;
@@ -499,13 +535,17 @@ retry:
 	if (!rq)
 		goto done;
 
-	sdev_printk(KERN_INFO, sdev, "%s MODE_SELECT command.\n",
+ 	list_for_each_entry(qdata, &list, entry) {
+		lun_table[qdata->h->lun] = 0x81;
+	}
+
+	printk(KERN_INFO "%s MODE_SELECT command.\n",
 		(retry_cnt == RDAC_RETRY_COUNT) ? "queueing" : "retrying");
 
 	err = blk_execute_rq(q, NULL, rq, 1);
 	blk_put_request(rq);
 	if (err != SCSI_DH_OK) {
-		err = mode_select_handle_sense(sdev, h->sense);
+		err = mode_select_handle_sense(h->sense);
 		if (err == SCSI_DH_RETRY && retry_cnt--)
 			goto retry;
 	}
@@ -513,10 +553,45 @@ retry:
 		h->state = RDAC_STATE_ACTIVE;
 
 done:
-	return err;
+	list_for_each_entry_safe(qdata, tmp, &list, entry) {
+		list_del(&qdata->entry);
+		if (err == SCSI_DH_OK)
+			qdata->h->state = RDAC_STATE_ACTIVE;
+		if (qdata->callback_fn)
+			qdata->callback_fn(qdata->callback_data, err);
+		kfree(qdata);
+	}
+	return;
 }
 
-static int rdac_activate(struct scsi_device *sdev, activate_complete fn, void *data)
+static int queue_mode_select(struct scsi_device *sdev,
+				activate_complete fn, void *data)
+{
+	struct rdac_queue_data *qdata;
+	struct rdac_controller *ctlr;
+
+	qdata = kzalloc(sizeof(*qdata), GFP_KERNEL);
+	if (!qdata)
+		return SCSI_DH_RETRY;
+
+ 	qdata->h = get_rdac_data(sdev);
+	qdata->callback_fn = fn;
+	qdata->callback_data = data;
+
+	ctlr = qdata->h->ctlr;
+	spin_lock(&ctlr->ms_lock);
+	list_add_tail(&qdata->entry, &ctlr->ms_head);
+	if (!ctlr->ms_queued) {
+		ctlr->ms_queued = 1;
+		ctlr->ms_sdev = sdev;
+		queue_work(kmpath_rdacd, &ctlr->ms_work);
+	}
+	spin_unlock(&ctlr->ms_lock);
+	return SCSI_DH_OK;
+}
+
+static int rdac_activate(struct scsi_device *sdev,
+				activate_complete fn, void *data)
 {
 	struct rdac_dh_data *h = get_rdac_data(sdev);
 	int err = SCSI_DH_OK;
@@ -536,8 +611,11 @@ static int rdac_activate(struct scsi_dev
 		if (err != SCSI_DH_OK)
 			goto done;
 	}
-	if (h->lun_state == RDAC_LUN_UNOWNED)
-		err = send_mode_select(sdev, h);
+	if (h->lun_state == RDAC_LUN_UNOWNED) {
+		err = queue_mode_select(sdev, fn, data);
+		if (err == SCSI_DH_OK)
+			return 0;
+	}
 done:
 	if (fn)
 		fn(data, err);
@@ -720,13 +798,26 @@ static int __init rdac_init(void)
 	int r;
 
 	r = scsi_register_device_handler(&rdac_dh);
-	if (r != 0)
+	if (r != 0) {
 		printk(KERN_ERR "Failed to register scsi device handler.");
+		goto done;
+	}
+
+	/*
+	 * Create workqueue to handle mode selects for rdac
+	 */
+	kmpath_rdacd = create_singlethread_workqueue("kmpath_rdacd");
+	if (!kmpath_rdacd) {
+		scsi_unregister_device_handler(&rdac_dh);
+		printk(KERN_ERR "kmpath_rdacd creation failed.\n");
+	}
+done:
 	return r;
 }
 
 static void __exit rdac_exit(void)
 {
+	destroy_workqueue(kmpath_rdacd);
 	scsi_unregister_device_handler(&rdac_dh);
 }
 

RE: [PATCH 3/3] scsi_dh: rdac handler: Batch up MODE SELECTs and send few of them

[Moger, Babu]

Tested by: Babu Moger <babu.moger@lsi.com>

> -----Original Message-----
> From: linux-scsi-owner@vger.kernel.org [mailto:linux-scsi-
> owner@vger.kernel.org] On Behalf Of Chandra Seetharaman
> Sent: Monday, April 27, 2009 9:53 PM
> To: linux-scsi@vger.kernel.org
> Cc: dm-devel@redhat.com; Moger, Babu; michaelc@cs.wisc.edu; Chandra
> Seetharaman
> Subject: [PATCH 3/3] scsi_dh: rdac handler: Batch up MODE SELECTs and send
> few of them
> 
> Batch up MODE_SELECT in rdac device handler.
> 
> LSI RDAC storage has the capability of handling mode selects for
> multiple luns in a same command. Make use of that ability to send
> as few MODE SELECTs as possible to the storage controller as possible.
> 
> This patch creates a work queue and queues up activate requests
> when a MODE SELECT is sent down the wire. When that MODE SELECT
> completes, it compiles queued up activate requests for multiple
> luns into a single MODE SELECT.
> 
> This reduces the time to do failover/failback of large number of LUNS.
> 
> Signed-off-by: Babu Moger <babu.moger@lsi.com>
> Signed-off-by: Chandra Seetharaman <sekharan@us.ibm.com>
> 
> ---
>  drivers/scsi/device_handler/scsi_dh_rdac.c |  117
> ++++++++++++++++++++++++++---
>  1 file changed, 104 insertions(+), 13 deletions(-)
> 
> Index: linux-2.6.29/drivers/scsi/device_handler/scsi_dh_rdac.c
> ===================================================================
> --- linux-2.6.29.orig/drivers/scsi/device_handler/scsi_dh_rdac.c
> +++ linux-2.6.29/drivers/scsi/device_handler/scsi_dh_rdac.c
> @@ -22,6 +22,7 @@
>  #include <scsi/scsi.h>
>  #include <scsi/scsi_eh.h>
>  #include <scsi/scsi_dh.h>
> +#include <linux/workqueue.h>
> 
>  #define RDAC_NAME "rdac"
>  #define RDAC_RETRY_COUNT 5
> @@ -135,6 +136,11 @@ struct rdac_controller {
>  		struct rdac_pg_legacy legacy;
>  		struct rdac_pg_expanded expanded;
>  	} mode_select;
> +	spinlock_t		ms_lock;
> +	int			ms_queued;
> +	struct work_struct	ms_work;
> +	struct scsi_device	*ms_sdev;
> +	struct list_head	ms_head;
>  };
>  struct c8_inquiry {
>  	u8	peripheral_info;
> @@ -195,8 +201,17 @@ static const char *lun_state[] =
>  	"owned (AVT mode)",
>  };
> 
> +struct rdac_queue_data {
> +	struct list_head	entry;
> +	struct rdac_dh_data	*h;
> +	activate_complete	callback_fn;
> +	void			*callback_data;
> +};
> +
>  static LIST_HEAD(ctlr_list);
>  static DEFINE_SPINLOCK(list_lock);
> +static struct workqueue_struct *kmpath_rdacd;
> +static void send_mode_select(struct work_struct *work);
> 
>  static inline struct rdac_dh_data *get_rdac_data(struct scsi_device
> *sdev)
>  {
> @@ -253,7 +268,6 @@ static struct request *rdac_failover_get
>  		rdac_pg->subpage_code = 0x1;
>  		rdac_pg->page_len[0] = 0x01;
>  		rdac_pg->page_len[1] = 0x28;
> -		rdac_pg->lun_table[h->lun] = 0x81;
>  	} else {
>  		struct rdac_pg_legacy *rdac_pg;
> 
> @@ -263,7 +277,6 @@ static struct request *rdac_failover_get
>  		common = &rdac_pg->common;
>  		rdac_pg->page_code = RDAC_PAGE_CODE_REDUNDANT_CONTROLLER;
>  		rdac_pg->page_len = 0x68;
> -		rdac_pg->lun_table[h->lun] = 0x81;
>  	}
>  	common->rdac_mode[1] = RDAC_MODE_TRANSFER_SPECIFIED_LUNS;
>  	common->quiescence_timeout = RDAC_QUIESCENCE_TIME;
> @@ -297,6 +310,7 @@ static void release_controller(struct kr
>  	struct rdac_controller *ctlr;
>  	ctlr = container_of(kref, struct rdac_controller, kref);
> 
> +	flush_workqueue(kmpath_rdacd);
>  	spin_lock(&list_lock);
>  	list_del(&ctlr->node);
>  	spin_unlock(&list_lock);
> @@ -326,6 +340,11 @@ static struct rdac_controller *get_contr
>  	memcpy(ctlr->slot_id, slot_id, SLOT_ID_LEN);
>  	kref_init(&ctlr->kref);
>  	ctlr->use_ms10 = -1;
> +	ctlr->ms_queued = 0;
> +	ctlr->ms_sdev = NULL;
> +	spin_lock_init(&ctlr->ms_lock);
> +	INIT_WORK(&ctlr->ms_work, send_mode_select);
> +	INIT_LIST_HEAD(&ctlr->ms_head);
>  	list_add(&ctlr->node, &ctlr_list);
>  done:
>  	spin_unlock(&list_lock);
> @@ -445,8 +464,7 @@ static int set_mode_select(struct scsi_d
>  	return err;
>  }
> 
> -static int mode_select_handle_sense(struct scsi_device *sdev,
> -				    unsigned char *sensebuf)
> +static int mode_select_handle_sense(unsigned char *sensebuf)
>  {
>  	struct scsi_sense_hdr sense_hdr;
>  	int err = SCSI_DH_IO, ret;
> @@ -478,7 +496,7 @@ static int mode_select_handle_sense(stru
>  			err = SCSI_DH_RETRY;
>  		break;
>  	default:
> -		sdev_printk(KERN_INFO, sdev,
> +		printk(KERN_INFO
>  			    "MODE_SELECT failed with sense %02x/%02x/%02x.\n",
>  			    sense_hdr.sense_key, sense_hdr.asc, sense_hdr.ascq);
>  	}
> @@ -487,11 +505,29 @@ done:
>  	return err;
>  }
> 
> -static int send_mode_select(struct scsi_device *sdev, struct rdac_dh_data
> *h)
> +static void send_mode_select(struct work_struct *work)
>  {
> +	struct rdac_controller *ctlr =
> +		container_of(work, struct rdac_controller, ms_work);
>  	struct request *rq;
> +	struct scsi_device *sdev = ctlr->ms_sdev;
> +	struct rdac_dh_data *h = get_rdac_data(sdev);
>  	struct request_queue *q = sdev->request_queue;
>  	int err, retry_cnt = RDAC_RETRY_COUNT;
> +	struct rdac_queue_data *tmp, *qdata;
> +	LIST_HEAD(list);
> +	u8 *lun_table;
> +
> +	spin_lock(&ctlr->ms_lock);
> +	list_splice_init(&ctlr->ms_head, &list);
> +	ctlr->ms_queued = 0;
> +	ctlr->ms_sdev = NULL;
> +	spin_unlock(&ctlr->ms_lock);
> +
> +	if (ctlr->use_ms10)
> +		lun_table = ctlr->mode_select.expanded.lun_table;
> +	else
> +		lun_table = ctlr->mode_select.legacy.lun_table;
> 
>  retry:
>  	err = SCSI_DH_RES_TEMP_UNAVAIL;
> @@ -499,13 +535,17 @@ retry:
>  	if (!rq)
>  		goto done;
> 
> -	sdev_printk(KERN_INFO, sdev, "%s MODE_SELECT command.\n",
> + 	list_for_each_entry(qdata, &list, entry) {
> +		lun_table[qdata->h->lun] = 0x81;
> +	}
> +
> +	printk(KERN_INFO "%s MODE_SELECT command.\n",
>  		(retry_cnt == RDAC_RETRY_COUNT) ? "queueing" : "retrying");
> 
>  	err = blk_execute_rq(q, NULL, rq, 1);
>  	blk_put_request(rq);
>  	if (err != SCSI_DH_OK) {
> -		err = mode_select_handle_sense(sdev, h->sense);
> +		err = mode_select_handle_sense(h->sense);
>  		if (err == SCSI_DH_RETRY && retry_cnt--)
>  			goto retry;
>  	}
> @@ -513,10 +553,45 @@ retry:
>  		h->state = RDAC_STATE_ACTIVE;
> 
>  done:
> -	return err;
> +	list_for_each_entry_safe(qdata, tmp, &list, entry) {
> +		list_del(&qdata->entry);
> +		if (err == SCSI_DH_OK)
> +			qdata->h->state = RDAC_STATE_ACTIVE;
> +		if (qdata->callback_fn)
> +			qdata->callback_fn(qdata->callback_data, err);
> +		kfree(qdata);
> +	}
> +	return;
>  }
> 
> -static int rdac_activate(struct scsi_device *sdev, activate_complete fn,
> void *data)
> +static int queue_mode_select(struct scsi_device *sdev,
> +				activate_complete fn, void *data)
> +{
> +	struct rdac_queue_data *qdata;
> +	struct rdac_controller *ctlr;
> +
> +	qdata = kzalloc(sizeof(*qdata), GFP_KERNEL);
> +	if (!qdata)
> +		return SCSI_DH_RETRY;
> +
> + 	qdata->h = get_rdac_data(sdev);
> +	qdata->callback_fn = fn;
> +	qdata->callback_data = data;
> +
> +	ctlr = qdata->h->ctlr;
> +	spin_lock(&ctlr->ms_lock);
> +	list_add_tail(&qdata->entry, &ctlr->ms_head);
> +	if (!ctlr->ms_queued) {
> +		ctlr->ms_queued = 1;
> +		ctlr->ms_sdev = sdev;
> +		queue_work(kmpath_rdacd, &ctlr->ms_work);
> +	}
> +	spin_unlock(&ctlr->ms_lock);
> +	return SCSI_DH_OK;
> +}
> +
> +static int rdac_activate(struct scsi_device *sdev,
> +				activate_complete fn, void *data)
>  {
>  	struct rdac_dh_data *h = get_rdac_data(sdev);
>  	int err = SCSI_DH_OK;
> @@ -536,8 +611,11 @@ static int rdac_activate(struct scsi_dev
>  		if (err != SCSI_DH_OK)
>  			goto done;
>  	}
> -	if (h->lun_state == RDAC_LUN_UNOWNED)
> -		err = send_mode_select(sdev, h);
> +	if (h->lun_state == RDAC_LUN_UNOWNED) {
> +		err = queue_mode_select(sdev, fn, data);
> +		if (err == SCSI_DH_OK)
> +			return 0;
> +	}
>  done:
>  	if (fn)
>  		fn(data, err);
> @@ -720,13 +798,26 @@ static int __init rdac_init(void)
>  	int r;
> 
>  	r = scsi_register_device_handler(&rdac_dh);
> -	if (r != 0)
> +	if (r != 0) {
>  		printk(KERN_ERR "Failed to register scsi device handler.");
> +		goto done;
> +	}
> +
> +	/*
> +	 * Create workqueue to handle mode selects for rdac
> +	 */
> +	kmpath_rdacd = create_singlethread_workqueue("kmpath_rdacd");
> +	if (!kmpath_rdacd) {
> +		scsi_unregister_device_handler(&rdac_dh);
> +		printk(KERN_ERR "kmpath_rdacd creation failed.\n");
> +	}
> +done:
>  	return r;
>  }
> 
>  static void __exit rdac_exit(void)
>  {
> +	destroy_workqueue(kmpath_rdacd);
>  	scsi_unregister_device_handler(&rdac_dh);
>  }
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] scsi_dh: rdac handler: Batch up MODE SELECTs and send few of them

[Mike Christie]

Chandra Seetharaman wrote:
>  }
>  
> -static int rdac_activate(struct scsi_device *sdev, activate_complete fn, void *data)
> +static int queue_mode_select(struct scsi_device *sdev,
> +				activate_complete fn, void *data)
> +{
> +	struct rdac_queue_data *qdata;
> +	struct rdac_controller *ctlr;
> +
> +	qdata = kzalloc(sizeof(*qdata), GFP_KERNEL);


I think you want to use GFP_NOIO or GFP_ATOMIC here. If GFP_NOIO can 
block and screw up other devices using that multipathd workqueue struct 
maybe GFP_ATOMIC would be best since we can have dm-mpath retry later.

Re: [PATCH 3/3] scsi_dh: rdac handler: Batch up MODE SELECTsand send few of them

[Chandra Seetharaman]

On Wed, 2009-05-06 at 22:39 -0500, Mike Christie wrote:
> Chandra Seetharaman wrote:
> >  }
> >  
> > -static int rdac_activate(struct scsi_device *sdev, activate_complete fn, void *data)
> > +static int queue_mode_select(struct scsi_device *sdev,
> > +				activate_complete fn, void *data)
> > +{
> > +	struct rdac_queue_data *qdata;
> > +	struct rdac_controller *ctlr;
> > +
> > +	qdata = kzalloc(sizeof(*qdata), GFP_KERNEL);
> 
> 
> I think you want to use GFP_NOIO or GFP_ATOMIC here. If GFP_NOIO can 
> block and screw up other devices using that multipathd workqueue struct 
> maybe GFP_ATOMIC would be best since we can have dm-mpath retry later.

Will do.

Thanks
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [dm-devel] [PATCH 3/3] scsi_dh: rdac handler: Batch up MODE SELECTsand send few of them

[Shyam_Iyer]

> -----Original Message-----
> From: dm-devel-bounces@redhat.com [mailto:dm-devel-bounces@redhat.com]
> On Behalf Of Chandra Seetharaman
> Sent: Friday, May 08, 2009 6:06 AM
> To: Mike Christie
> Cc: device-mapper development; linux-scsi@vger.kernel.org
> Subject: Re: [dm-devel] [PATCH 3/3] scsi_dh: rdac handler: Batch up
> MODE SELECTsand send few of them
> 
> 
> On Wed, 2009-05-06 at 22:39 -0500, Mike Christie wrote:
> > Chandra Seetharaman wrote:
> > >  }
> > >
> > > -static int rdac_activate(struct scsi_device *sdev,
> activate_complete fn, void *data)
> > > +static int queue_mode_select(struct scsi_device *sdev,
> > > +				activate_complete fn, void *data)
> > > +{
> > > +	struct rdac_queue_data *qdata;
> > > +	struct rdac_controller *ctlr;
> > > +
> > > +	qdata = kzalloc(sizeof(*qdata), GFP_KERNEL);
> >
> >
> > I think you want to use GFP_NOIO or GFP_ATOMIC here. If GFP_NOIO can
> > block and screw up other devices using that multipathd workqueue
> struct
> > maybe GFP_ATOMIC would be best since we can have dm-mpath retry
> later.
> 
> Will do.
> 
> Thanks
> >

Chandra - Thanks for doing this. How can we help here to get this
upstream?

RE: [dm-devel] [PATCH 3/3] scsi_dh: rdac handler: Batch up MODE SELECTsand send few of them

[Chandra Seetharaman]

Hi Shyam,

As per Mike (Christie)'s suggestion I am working on getting all the
hardware handlers to do async pg_init(), EMC handler is getting little
cumbersome. Working on it. Will try to post the set of patches by end of
week.

Testing and reporting the same (for as many hardware handler as
possible) would certainly help these get accepted (of course after I
post them :)....

chandra
On Sat, 2009-09-12 at 09:39 +0530, Shyam_Iyer@Dell.com wrote:
> > -----Original Message-----
> > From: dm-devel-bounces@redhat.com [mailto:dm-devel-bounces@redhat.com]
> > On Behalf Of Chandra Seetharaman
> > Sent: Friday, May 08, 2009 6:06 AM
> > To: Mike Christie
> > Cc: device-mapper development; linux-scsi@vger.kernel.org
> > Subject: Re: [dm-devel] [PATCH 3/3] scsi_dh: rdac handler: Batch up
> > MODE SELECTsand send few of them
> > 
> > 
> > On Wed, 2009-05-06 at 22:39 -0500, Mike Christie wrote:
> > > Chandra Seetharaman wrote:
> > > >  }
> > > >
> > > > -static int rdac_activate(struct scsi_device *sdev,
> > activate_complete fn, void *data)
> > > > +static int queue_mode_select(struct scsi_device *sdev,
> > > > +				activate_complete fn, void *data)
> > > > +{
> > > > +	struct rdac_queue_data *qdata;
> > > > +	struct rdac_controller *ctlr;
> > > > +
> > > > +	qdata = kzalloc(sizeof(*qdata), GFP_KERNEL);
> > >
> > >
> > > I think you want to use GFP_NOIO or GFP_ATOMIC here. If GFP_NOIO can
> > > block and screw up other devices using that multipathd workqueue
> > struct
> > > maybe GFP_ATOMIC would be best since we can have dm-mpath retry
> > later.
> > 
> > Will do.
> > 
> > Thanks
> > >
> 
> Chandra - Thanks for doing this. How can we help here to get this
> upstream?
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html