[dm-devel] RE: [PATCH] Handle multipath paths in a path group properly during pg_init

[Moger, Babu]

Reviewed and Tested-by: Babu Moger <babu.moger at lsi.com>

> -----Original Message-----
> From: Chandra Seetharaman [mailto:sekharan at us.ibm.com]
> Sent: Monday, March 30, 2009 1:08 PM
> To: dm-devel
> Cc: Alasdair G Kergon; Moger, Babu; Hannes Reinecke; Mike Anderson
> Subject: [PATCH] Handle multipath paths in a path group properly during
> pg_init
> 
> Resending the patch to get in patchwork...
> -------
> The problem reported by Moger Babu was caused due to the architectural
> change made when we moved from dm hardware handler to SCSI hardware
> handler.
> 
> Thanks Babu for finding and reporting the bug.
> 
> All of the hardware handlers, do have a state now, and they are set to
> active and (some form of) inactive. All of them have prep_fn, which use
> this "state" to fail the I/O without it ever being sent to the device.
> 
> As Babu has noted in his email, the pg_init/activate is sent on only one
> path and the "state" of that path is changed appropriately to "active"
> while other paths in the same path group are never changed as they never
> got an "activate".
> 
> Attached is a patch (compiled, tested, but not clean yet), which makes
> changes in the dm-multipath layer to send an "activate" on each paths in
> the path groups.
> 
> Mike (Anderson) and I had a discussion about whether to implement this
> in the dm-mulitpath layer or in the SCSI hardware handler layer and we
> came to a conclusion that it is best suited to be in the dm-mulitpath
> layer as it is the one that knows the relationship between different
> paths.
> 
> If it were to be done at the Hardware handler layer, then the hardware
> handler may end up having a different notion of the path relationship
> and hence may not work as expected by the dm-multipath layer.
> 
> This patch has been tested by Hannes in EMC storage. Babu and I tested it
> in LSI storage.
> ----------
> 
> Signed-off-by: Chandra Seetharaman <sekharan at us.ibm.com>
> 
> ---
> 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
> @@ -36,6 +36,7 @@ struct pgpath {
> 
>  	struct dm_path path;
>  	struct work_struct deactivate_path;
> +	struct work_struct activate_path;
>  };
> 
>  #define path_to_pgpath(__pgp) container_of((__pgp), struct pgpath, path)
> @@ -65,8 +66,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? */
> @@ -129,6 +128,7 @@ static struct pgpath *alloc_pgpath(void)
>  	if (pgpath) {
>  		pgpath->is_active = 1;
>  		INIT_WORK(&pgpath->deactivate_path, deactivate_path);
> +		INIT_WORK(&pgpath->activate_path, activate_path);
>  	}
> 
>  	return pgpath;
> @@ -170,10 +170,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);
>  	}
>  }
> @@ -203,7 +199,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);
> @@ -428,8 +423,8 @@ static void process_queued_ios(struct wo
>  {
>  	struct multipath *m =
>  		container_of(work, struct multipath, process_queued_ios);
> -	struct pgpath *pgpath = NULL;
> -	unsigned init_required = 0, must_queue = 1;
> +	struct pgpath *pgpath = NULL, *tmp;
> +	unsigned must_queue = 1;
>  	unsigned long flags;
> 
>  	spin_lock_irqsave(&m->lock, flags);
> @@ -447,19 +442,15 @@ 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;
> -		init_required = 1;
> +		list_for_each_entry(tmp, &pgpath->pg->pgpaths, list) {
> +			queue_work(kmpath_handlerd, &tmp->activate_path);
> +			m->pg_init_in_progress++;
> +		}
>  	}
> -
>  out:
>  	spin_unlock_irqrestore(&m->lock, flags);
> -
> -	if (init_required)
> -		queue_work(kmpath_handlerd, &m->activate_path);
> -
>  	if (!must_queue)
>  		dispatch_queued_ios(m);
>  }
> @@ -1111,27 +1102,20 @@ static void pg_init_done(struct dm_path
>  		pg->bypassed = 0;
>  	}
> 
> -	m->pg_init_in_progress = 0;
> -	queue_work(kmultipathd, &m->process_queued_ios);
> +	m->pg_init_in_progress--;
> +	if (!m->pg_init_in_progress)
> +		queue_work(kmultipathd, &m->process_queued_ios);
>  	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;
> +	struct pgpath *pgpath =
> +		container_of(work, struct pgpath, activate_path);
> 
> -	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, ret);
> +	ret = scsi_dh_activate(bdev_get_queue(pgpath->path.dev->bdev));
> +	pg_init_done(&pgpath->path, ret);
>  }
> 
>  /*
>