[PATCH] dm-mpath: do not change SCSI device handler

[PATCH] dm-mpath: do not change SCSI device handler

[Mikulas Patocka]

Hi

This fixes BZ 912245 and 902595.

Mikulas

---

dm-mpath: do not change SCSI device handler

This patch prevents the multipath target from changing the device handler.
This fixes a kernel crash that can happen when changing the device
handler.

When we reload a multipath device, there are two instances of the
multipath target - the first instance that is active and the second
instance that is being constructed with "ctr" method.

If the multipath constructor finds out that the device is using a
different device handler, it detaches the existing handler and attaches a
new handler. However, the first instance of the multipath target still
exists and processes requests. If the first instance sends some
path-management request with scsi_dh_activate and the second instance
detaches the device handler while the path-management request is in
flight, a crash happens. The reason for the crash is that the endio
routine for the path-management request is working with structures that
were freed when the handler was detached.

There is no practical need to change device handlers on an active device,
so this patch disables it.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>

---
 drivers/md/dm-mpath.c |   13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

Index: linux-3.9-rc5-fast/drivers/md/dm-mpath.c
===================================================================
--- linux-3.9-rc5-fast.orig/drivers/md/dm-mpath.c	2013-04-02 21:54:25.000000000 +0200
+++ linux-3.9-rc5-fast/drivers/md/dm-mpath.c	2013-04-03 01:15:04.000000000 +0200
@@ -618,12 +618,13 @@ static struct pgpath *parse_path(struct 
 		 */
 		r = scsi_dh_attach(q, m->hw_handler_name);
 		if (r == -EBUSY) {
-			/*
-			 * Already attached to different hw_handler:
-			 * try to reattach with correct one.
-			 */
-			scsi_dh_detach(q);
-			r = scsi_dh_attach(q, m->hw_handler_name);
+			ti->error = "a different device handler is already "
+				"attached";
+			DMERR("A different device handler for device %s is "
+				"attached. You need to deactivate "
+				"the device to change device handler.",
+				p->path.dev->name);
+			goto bad;
 		}
 
 		if (r < 0) {

Re: dm-mpath: do not change SCSI device handler

[Mike Snitzer]

On Tue, Apr 02 2013 at  8:04pm -0400,
Mikulas Patocka <mpatocka@redhat.com> wrote:

> Hi
> 
> This fixes BZ 912245 and 902595.
> 
> Mikulas
> 
> ---
> 
> dm-mpath: do not change SCSI device handler
> 
> This patch prevents the multipath target from changing the device handler.
> This fixes a kernel crash that can happen when changing the device
> handler.
> 
> When we reload a multipath device, there are two instances of the
> multipath target - the first instance that is active and the second
> instance that is being constructed with "ctr" method.
> 
> If the multipath constructor finds out that the device is using a
> different device handler, it detaches the existing handler and attaches a
> new handler. However, the first instance of the multipath target still
> exists and processes requests. If the first instance sends some
> path-management request with scsi_dh_activate and the second instance
> detaches the device handler while the path-management request is in
> flight, a crash happens. The reason for the crash is that the endio
> routine for the path-management request is working with structures that
> were freed when the handler was detached.
> 
> There is no practical need to change device handlers on an active device,
> so this patch disables it.
> 
> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> 
> ---
>  drivers/md/dm-mpath.c |   13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> Index: linux-3.9-rc5-fast/drivers/md/dm-mpath.c
> ===================================================================
> --- linux-3.9-rc5-fast.orig/drivers/md/dm-mpath.c	2013-04-02 21:54:25.000000000 +0200
> +++ linux-3.9-rc5-fast/drivers/md/dm-mpath.c	2013-04-03 01:15:04.000000000 +0200
> @@ -618,12 +618,13 @@ static struct pgpath *parse_path(struct 
>  		 */
>  		r = scsi_dh_attach(q, m->hw_handler_name);
>  		if (r == -EBUSY) {
> -			/*
> -			 * Already attached to different hw_handler:
> -			 * try to reattach with correct one.
> -			 */
> -			scsi_dh_detach(q);
> -			r = scsi_dh_attach(q, m->hw_handler_name);
> +			ti->error = "a different device handler is already "
> +				"attached";

Don't really need to wrap this to multiple lines (helps searching).

> +			DMERR("A different device handler for device %s is "
> +				"attached. You need to deactivate "
> +				"the device to change device handler.",
> +				p->path.dev->name);
> +			goto bad;
>  		}
>  
>  		if (r < 0) {

We should include the currently attached scsi_dh in the DMERR message,
e.g.:
	attached_handler_name = scsi_dh_attached_handler_name(q, GFP_KERNEL);
	DMERR("The %s device handler for devices %s is already attached.  ...",
	      attached_handler_name, p->path.dev->name);
	kfree(attached_handler_name);
	goto bad;

Otherwise, looks good.

Acked-by: Mike Snitzer <snitzer@redhat.com>

Re: dm-mpath: do not change SCSI device handler

[Mikulas Patocka]

On Wed, 3 Apr 2013, Mike Snitzer wrote:

> On Tue, Apr 02 2013 at  8:04pm -0400,
> Mikulas Patocka <mpatocka@redhat.com> wrote:
> 
> > Hi
> > 
> > This fixes BZ 912245 and 902595.
> > 
> > Mikulas
> > 
> > ---
>
> We should include the currently attached scsi_dh in the DMERR message,
> e.g.:
> 	attached_handler_name = scsi_dh_attached_handler_name(q, GFP_KERNEL);
> 	DMERR("The %s device handler for devices %s is already attached.  ...",
> 	      attached_handler_name, p->path.dev->name);
> 	kfree(attached_handler_name);
> 	goto bad;
> 
> Otherwise, looks good.
> 
> Acked-by: Mike Snitzer <snitzer@redhat.com>
> 

OK. This is the patch with these changes.

---

dm-mpath: do not change SCSI device handler

This patch prevents the multipath target from changing the device handler.
This fixes a kernel crash that can happen when changing the device
handler.

When we reload a multipath device, there are two instances of the
multipath target - the first instance that is active and the second
instance that is being constructed with "ctr" method.

If the multipath constructor finds out that the device is using a
different device handler, it detaches the existing handler and attaches a
new handler. However, the first instance of the multipath target still
exists and processes requests. If the first instance sends some
path-management request with scsi_dh_activate and the second instance
detaches the device handler while the path-management request is in
flight, a crash happens. The reason for the crash is that the endio
routine for the path-management request is working with structures that
were freed when the handler was detached.

There is no practical need to change device handlers on an active device,
so this patch disables it.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>

---
 drivers/md/dm-mpath.c |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

Index: linux-3.9-rc5-fast/drivers/md/dm-mpath.c
===================================================================
--- linux-3.9-rc5-fast.orig/drivers/md/dm-mpath.c	2013-04-02 21:54:25.000000000 +0200
+++ linux-3.9-rc5-fast/drivers/md/dm-mpath.c	2013-04-03 22:51:11.000000000 +0200
@@ -618,12 +618,12 @@ static struct pgpath *parse_path(struct 
 		 */
 		r = scsi_dh_attach(q, m->hw_handler_name);
 		if (r == -EBUSY) {
-			/*
-			 * Already attached to different hw_handler:
-			 * try to reattach with correct one.
-			 */
-			scsi_dh_detach(q);
-			r = scsi_dh_attach(q, m->hw_handler_name);
+			const char *a = scsi_dh_attached_handler_name(q, GFP_KERNEL);
+			ti->error = "a different device handler is already attached";
+			DMERR("The %s device handler for device %s is already attached. Can't attach new handler %s",
+				a ? a : "none", p->path.dev->name, m->hw_handler_name);
+			kfree(a);
+			goto bad;
 		}
 
 		if (r < 0) {

Re: [PATCH] dm-mpath: do not change SCSI device handler

[Hannes Reinecke]

On 04/03/2013 02:04 AM, Mikulas Patocka wrote:
> Hi
> 
> This fixes BZ 912245 and 902595.
> 
> Mikulas
> 
> ---
> 
> dm-mpath: do not change SCSI device handler
> 
> This patch prevents the multipath target from changing the device handler.
> This fixes a kernel crash that can happen when changing the device
> handler.
> 
> When we reload a multipath device, there are two instances of the
> multipath target - the first instance that is active and the second
> instance that is being constructed with "ctr" method.
> 
> If the multipath constructor finds out that the device is using a
> different device handler, it detaches the existing handler and attaches a
> new handler. However, the first instance of the multipath target still
> exists and processes requests. If the first instance sends some
> path-management request with scsi_dh_activate and the second instance
> detaches the device handler while the path-management request is in
> flight, a crash happens. The reason for the crash is that the endio
> routine for the path-management request is working with structures that
> were freed when the handler was detached.
> 
> There is no practical need to change device handlers on an active device,
> so this patch disables it.
> 
> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> 

NACK.

This kills multipath startup when scsi_dh_* modules are already loaded.
After boot scsi_dh_* modules might already be loaded without
multipath running.
Multipath might have defined other hardware handlers in the
configuration file, and will re-attach them on startup.

With this patch multipath cannot configure the devices properly and
will abort.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)

Re: dm-mpath: do not change SCSI device handler

[Mike Snitzer]

On Thu, Apr 04 2013 at  2:47am -0400,
Hannes Reinecke <hare@suse.de> wrote:

> NACK.
> 
> This kills multipath startup when scsi_dh_* modules are already loaded.

You mean an incorrect scsi_dh is already attached.

> After boot scsi_dh_* modules might already be loaded without
> multipath running.
> Multipath might have defined other hardware handlers in the
> configuration file, and will re-attach them on startup.
> 
> With this patch multipath cannot configure the devices properly and
> will abort.

In practice the scenario you describe is configuration error and can
easily be avoided in a properly configured system.

But that aside, Alasdair pointed out that it appears the heart of the
problem which Mikulas described in his patch header is due the fact that
the scsi_dh is switched, while IO may still be in-flight, during table
load (multipath_ctr -> parse_priority_group -> parse_path) rather than
deferring the switch until multipath_resume.  

I'll take a look at fixing this by deferring the scsi_dh switch until
resume.  This fix would assume multipath-tools is _not_ doing a noflush
suspend/resume when it is switching the scsi_dh.

Mike

Re: dm-mpath: do not change SCSI device handler

[Mikulas Patocka]

On Thu, 4 Apr 2013, Mike Snitzer wrote:

> I'll take a look at fixing this by deferring the scsi_dh switch until
> resume.  This fix would assume multipath-tools is _not_ doing a noflush
> suspend/resume when it is switching the scsi_dh.
> 
> Mike

This won't work because scsi_dh_attach allocates memory and you can't 
allocate memory when something is suspended.

If we want to make it possible to change device handler, we need to add a 
counter to "struct scsi_dh_data", increment the counter when the device 
handler issues some request, decrement the counter when it finishes the 
request (it may be tricky because you can't decrement it from the device 
handler module because the module may be unloaded as soon as you decrement 
it). Then, you wait until the counter is zero and detach the handler.

Mikulas

Re: dm-mpath: do not change SCSI device handler

[Mike Snitzer]

On Thu, Apr 04 2013 at  8:55am -0400,
Mikulas Patocka <mpatocka@redhat.com> wrote:

> 
> 
> On Thu, 4 Apr 2013, Mike Snitzer wrote:
> 
> > I'll take a look at fixing this by deferring the scsi_dh switch until
> > resume.  This fix would assume multipath-tools is _not_ doing a noflush
> > suspend/resume when it is switching the scsi_dh.
> > 
> > Mike
> 
> This won't work because scsi_dh_attach allocates memory and you can't 
> allocate memory when something is suspended.

Ah yeah, scsi_dh->attach allocates memory for scsi_dh_data.  But
couldn't those scsi_dh_* attach allocations be switched from GFP_KERNEL
to GFP_NOIO?

> If we want to make it possible to change device handler, we need to add a 
> counter to "struct scsi_dh_data", increment the counter when the device 
> handler issues some request, decrement the counter when it finishes the 
> request (it may be tricky because you can't decrement it from the device 
> handler module because the module may be unloaded as soon as you decrement 
> it). Then, you wait until the counter is zero and detach the handler.

Noted, but I haven't given up hope on just deferring the scsi_dh switch
quite yet.

Mike

Re: dm-mpath: do not change SCSI device handler

[Mikulas Patocka]

On Thu, 4 Apr 2013, Mike Snitzer wrote:

> On Thu, Apr 04 2013 at  8:55am -0400,
> Mikulas Patocka <mpatocka@redhat.com> wrote:
> 
> > 
> > 
> > On Thu, 4 Apr 2013, Mike Snitzer wrote:
> > 
> > > I'll take a look at fixing this by deferring the scsi_dh switch until
> > > resume.  This fix would assume multipath-tools is _not_ doing a noflush
> > > suspend/resume when it is switching the scsi_dh.
> > > 
> > > Mike
> > 
> > This won't work because scsi_dh_attach allocates memory and you can't 
> > allocate memory when something is suspended.
> 
> Ah yeah, scsi_dh->attach allocates memory for scsi_dh_data.  But
> couldn't those scsi_dh_* attach allocations be switched from GFP_KERNEL
> to GFP_NOIO?

Yes and no.

GFP_NOIO allocations don't issue any IO, so they have higher possibility 
of failure - if the memory is full of user space pages or dirty file pages 
and there are no clean cache pages, then GFP_NOIO allocation can't make 
any progress and fails. GFP_KERNEL allocation could swap out some pages 
and succeed.

On the other hand, kernel developers use GFP_NOIO allocations and assume 
that they don't fail. And they don't fail most of the time. Although I 
have seen cases where it failed and caused trouble - when allocating 
inodes with GFP_NOIO under high memory stress. (the correct solution would 
be to allocate the inode with GFP_KERNEL earlier, when we are not holding 
any filesystem lock).

From the point of formal correctness, relying on GFP_NOIO is wrong, but if 
the allocated space is small and it happens infrequently (only on 
activation), you can likely get away with it without being caught.

Mikulas

Re: dm-mpath: do not change SCSI device handler

[Mike Snitzer]

On Thu, Apr 04 2013 at  9:36am -0400,
Mikulas Patocka <mpatocka@redhat.com> wrote:

> 
> 
> On Thu, 4 Apr 2013, Mike Snitzer wrote:
> 
> > On Thu, Apr 04 2013 at  8:55am -0400,
> > Mikulas Patocka <mpatocka@redhat.com> wrote:
> > 
> > > 
> > > 
> > > On Thu, 4 Apr 2013, Mike Snitzer wrote:
> > > 
> > > > I'll take a look at fixing this by deferring the scsi_dh switch until
> > > > resume.  This fix would assume multipath-tools is _not_ doing a noflush
> > > > suspend/resume when it is switching the scsi_dh.
> > > > 
> > > > Mike
> > > 
> > > This won't work because scsi_dh_attach allocates memory and you can't 
> > > allocate memory when something is suspended.
> > 
> > Ah yeah, scsi_dh->attach allocates memory for scsi_dh_data.  But
> > couldn't those scsi_dh_* attach allocations be switched from GFP_KERNEL
> > to GFP_NOIO?
> 
> Yes and no.
> 
> GFP_NOIO allocations don't issue any IO, so they have higher possibility 
> of failure - if the memory is full of user space pages or dirty file pages 
> and there are no clean cache pages, then GFP_NOIO allocation can't make 
> any progress and fails. GFP_KERNEL allocation could swap out some pages 
> and succeed.
> 
> On the other hand, kernel developers use GFP_NOIO allocations and assume 
> that they don't fail. And they don't fail most of the time. Although I 
> have seen cases where it failed and caused trouble - when allocating 
> inodes with GFP_NOIO under high memory stress. (the correct solution would 
> be to allocate the inode with GFP_KERNEL earlier, when we are not holding 
> any filesystem lock).
> 
> >From the point of formal correctness, relying on GFP_NOIO is wrong, but if 
> the allocated space is small and it happens infrequently (only on 
> activation), you can likely get away with it without being caught.

I'm suggesting that switching the scsi_dh is not something that will be
done on a system that is suffering from serious memory contention.

But I think we need to get back to analyzing the scsi_dh change you
mentioned before with tracking counts, etc.

Re: dm-mpath: do not change SCSI device handler

[Mikulas Patocka]

On Thu, 4 Apr 2013, Mike Snitzer wrote:

> I'm suggesting that switching the scsi_dh is not something that will be
> done on a system that is suffering from serious memory contention.
> 
> But I think we need to get back to analyzing the scsi_dh change you
> mentioned before with tracking counts, etc.

When I look at the code, I see module_put(THIS_MODULE) in all scsi device 
handlers. Moreover, there are many 'module_put(THIS_MODULE)' over the 
whole kernel. It seems buggy, because when the function module_put 
returns, reschedule happens, module is unloaded, the process that called 
module_put(THIS_MODULE) is scheduled back again, crash happens because we 
are running a code that no longer exists. Is it a bug or is there some 
trick that prevents the kernel from crashing in this scenario?

Mikulas

Re: dm-mpath: do not change SCSI device handler

[Mikulas Patocka]

On Thu, 4 Apr 2013, Mikulas Patocka wrote:

> On Thu, 4 Apr 2013, Mike Snitzer wrote:
> 
> > I'm suggesting that switching the scsi_dh is not something that will be
> > done on a system that is suffering from serious memory contention.
> > 
> > But I think we need to get back to analyzing the scsi_dh change you
> > mentioned before with tracking counts, etc.
> 
> When I look at the code, I see module_put(THIS_MODULE) in all scsi device 
> handlers. Moreover, there are many 'module_put(THIS_MODULE)' over the 
> whole kernel. It seems buggy, because when the function module_put 
> returns, reschedule happens, module is unloaded, the process that called 
> module_put(THIS_MODULE) is scheduled back again, crash happens because we 
> are running a code that no longer exists. Is it a bug or is there some 
> trick that prevents the kernel from crashing in this scenario?
> 
> Mikulas

I found this 
http://lkml.indiana.edu/hypermail/linux/kernel/1111.0/00218.html saying 
that they don't care about module unloading races ...

Mikulas

[PATCH 1/2] [SCSI] scsi_dh: add scsi_dh_alloc_data

[Mike Snitzer]

Factor scsi_dh_data allocation out from scsi_dh_attach to the
optional scsi_dh_alloc_data interface.  scsi_dh_attach will still
allocate the the appropriate scsi_dh_data if a NULL scsi_dh_data struct
is passed to it.  In that case, all scsi_dh will now use GFP_NOIO to
allocate scsi_dh_data.

This change will allow DM multipath to preallocate the scsi_dh_data
during the multipath table load but then defer the scsi_dh_attach until
the multipath table resume (memory allocation is not allowed during DM
table resume).

Signed-off-by: Mike Snitzer <snitzer@redhat.com>
---
 drivers/md/dm-mpath.c                       |    4 +-
 drivers/scsi/device_handler/scsi_dh.c       |   42 +++++++++++++++++++++-----
 drivers/scsi/device_handler/scsi_dh_alua.c  |   18 ++++++++---
 drivers/scsi/device_handler/scsi_dh_emc.c   |   18 ++++++++---
 drivers/scsi/device_handler/scsi_dh_hp_sw.c |   18 ++++++++---
 drivers/scsi/device_handler/scsi_dh_rdac.c  |   18 ++++++++---
 include/scsi/scsi_device.h                  |    3 +-
 include/scsi/scsi_dh.h                      |    4 ++-
 8 files changed, 93 insertions(+), 32 deletions(-)

diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
index 51bb816..37d9ead 100644
--- a/drivers/md/dm-mpath.c
+++ b/drivers/md/dm-mpath.c
@@ -616,14 +616,14 @@ static struct pgpath *parse_path(struct dm_arg_set *as, struct path_selector *ps
 		 * Increments scsi_dh reference, even when using an
 		 * already-attached handler.
 		 */
-		r = scsi_dh_attach(q, m->hw_handler_name);
+		r = scsi_dh_attach(q, m->hw_handler_name, NULL);
 		if (r == -EBUSY) {
 			/*
 			 * Already attached to different hw_handler:
 			 * try to reattach with correct one.
 			 */
 			scsi_dh_detach(q);
-			r = scsi_dh_attach(q, m->hw_handler_name);
+			r = scsi_dh_attach(q, m->hw_handler_name, NULL);
 		}
 
 		if (r < 0) {
diff --git a/drivers/scsi/device_handler/scsi_dh.c b/drivers/scsi/device_handler/scsi_dh.c
index 33e422e..034b394 100644
--- a/drivers/scsi/device_handler/scsi_dh.c
+++ b/drivers/scsi/device_handler/scsi_dh.c
@@ -94,19 +94,24 @@ device_handler_match(struct scsi_device_handler *scsi_dh,
  * scsi_dh_handler_attach - Attach a device handler to a device
  * @sdev - SCSI device the device handler should attach to
  * @scsi_dh - The device handler to attach
+ * @scsi_dh_data - if not NULL it is either assigned to sdev->scsi_dh_data
+ *                 on attach or free'd if the scsi_dh is already attached.
  */
 static int scsi_dh_handler_attach(struct scsi_device *sdev,
-				  struct scsi_device_handler *scsi_dh)
+				  struct scsi_device_handler *scsi_dh,
+				  struct scsi_dh_data *scsi_dh_data)
 {
 	int err = 0;
 
 	if (sdev->scsi_dh_data) {
 		if (sdev->scsi_dh_data->scsi_dh != scsi_dh)
 			err = -EBUSY;
-		else
+		else {
 			kref_get(&sdev->scsi_dh_data->kref);
+			kfree(scsi_dh_data);
+		}
 	} else if (scsi_dh->attach) {
-		err = scsi_dh->attach(sdev);
+		err = scsi_dh->attach(sdev, scsi_dh_data);
 		if (!err) {
 			kref_init(&sdev->scsi_dh_data->kref);
 			sdev->scsi_dh_data->sdev = sdev;
@@ -166,7 +171,7 @@ store_dh_state(struct device *dev, struct device_attribute *attr,
 		 */
 		if (!(scsi_dh = get_device_handler(buf)))
 			return err;
-		err = scsi_dh_handler_attach(sdev, scsi_dh);
+		err = scsi_dh_handler_attach(sdev, scsi_dh, NULL);
 	} else {
 		scsi_dh = sdev->scsi_dh_data->scsi_dh;
 		if (!strncmp(buf, "detach", 6)) {
@@ -262,7 +267,7 @@ static int scsi_dh_notifier(struct notifier_block *nb,
 		/* don't care about err */
 		devinfo = device_handler_match(NULL, sdev);
 		if (devinfo)
-			err = scsi_dh_handler_attach(sdev, devinfo);
+			err = scsi_dh_handler_attach(sdev, devinfo, NULL);
 	} else if (action == BUS_NOTIFY_DEL_DEVICE) {
 		device_remove_file(dev, &scsi_dh_state_attr);
 		scsi_dh_handler_detach(sdev, NULL);
@@ -287,7 +292,7 @@ static int scsi_dh_notifier_add(struct device *dev, void *data)
 	sdev = to_scsi_device(dev);
 
 	if (device_handler_match(scsi_dh, sdev))
-		scsi_dh_handler_attach(sdev, scsi_dh);
+		scsi_dh_handler_attach(sdev, scsi_dh, NULL);
 
 	put_device(dev);
 
@@ -466,13 +471,34 @@ int scsi_dh_handler_exist(const char *name)
 }
 EXPORT_SYMBOL_GPL(scsi_dh_handler_exist);
 
+struct scsi_dh_data *scsi_dh_alloc_data(const char *name, gfp_t flags)
+{
+	struct scsi_device_handler *scsi_dh;
+	struct scsi_dh_data *scsi_dh_data;
+	size_t scsi_dh_data_size = sizeof(sizeof(*scsi_dh_data));
+
+	scsi_dh = get_device_handler(name);
+	if (!scsi_dh)
+		return ERR_PTR(-EINVAL);
+
+	if (scsi_dh->get_dh_data_size)
+		scsi_dh_data_size += scsi_dh->get_dh_data_size();
+	scsi_dh_data = kzalloc(scsi_dh_data_size, flags);
+	if (!scsi_dh_data)
+		return ERR_PTR(-ENOMEM);
+
+	return scsi_dh_data;
+}
+EXPORT_SYMBOL_GPL(scsi_dh_alloc_data);
+
 /*
  * scsi_dh_attach - Attach device handler
  * @q - Request queue that is associated with the scsi_device
  *      the handler should be attached to
  * @name - name of the handler to attach
  */
-int scsi_dh_attach(struct request_queue *q, const char *name)
+int scsi_dh_attach(struct request_queue *q, const char *name,
+		   struct scsi_dh_data *scsi_dh_data)
 {
 	unsigned long flags;
 	struct scsi_device *sdev;
@@ -490,7 +516,7 @@ int scsi_dh_attach(struct request_queue *q, const char *name)
 	spin_unlock_irqrestore(q->queue_lock, flags);
 
 	if (!err) {
-		err = scsi_dh_handler_attach(sdev, scsi_dh);
+		err = scsi_dh_handler_attach(sdev, scsi_dh, scsi_dh_data);
 		put_device(&sdev->sdev_gendev);
 	}
 	return err;
diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
index 6f4d8e6..2334db1 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -798,7 +798,13 @@ static bool alua_match(struct scsi_device *sdev)
 	return (scsi_device_tpgs(sdev) != 0);
 }
 
-static int alua_bus_attach(struct scsi_device *sdev);
+static size_t alua_dh_data_size(void)
+{
+	return sizeof(struct alua_dh_data);
+}
+
+static int alua_bus_attach(struct scsi_device *sdev,
+			   struct scsi_dh_data *scsi_dh_data);
 static void alua_bus_detach(struct scsi_device *sdev);
 
 static struct scsi_device_handler alua_dh = {
@@ -811,21 +817,23 @@ static struct scsi_device_handler alua_dh = {
 	.activate = alua_activate,
 	.set_params = alua_set_params,
 	.match = alua_match,
+	.get_dh_data_size = alua_dh_data_size,
 };
 
 /*
  * alua_bus_attach - Attach device handler
  * @sdev: device to be attached to
  */
-static int alua_bus_attach(struct scsi_device *sdev)
+static int alua_bus_attach(struct scsi_device *sdev,
+			   struct scsi_dh_data *scsi_dh_data)
 {
-	struct scsi_dh_data *scsi_dh_data;
 	struct alua_dh_data *h;
 	unsigned long flags;
 	int err = SCSI_DH_OK;
 
-	scsi_dh_data = kzalloc(sizeof(*scsi_dh_data)
-			       + sizeof(*h) , GFP_KERNEL);
+	if (!scsi_dh_data)
+		scsi_dh_data = kzalloc(sizeof(*scsi_dh_data)
+				       + sizeof(*h) , GFP_NOIO);
 	if (!scsi_dh_data) {
 		sdev_printk(KERN_ERR, sdev, "%s: Attach failed\n",
 			    ALUA_DH_NAME);
diff --git a/drivers/scsi/device_handler/scsi_dh_emc.c b/drivers/scsi/device_handler/scsi_dh_emc.c
index e1c8be0..fa0553a 100644
--- a/drivers/scsi/device_handler/scsi_dh_emc.c
+++ b/drivers/scsi/device_handler/scsi_dh_emc.c
@@ -647,7 +647,13 @@ static bool clariion_match(struct scsi_device *sdev)
 	return false;
 }
 
-static int clariion_bus_attach(struct scsi_device *sdev);
+static size_t clariion_dh_data_size(void)
+{
+	return sizeof(struct clariion_dh_data);
+}
+
+static int clariion_bus_attach(struct scsi_device *sdev,
+			       struct scsi_dh_data *scsi_dh_data);
 static void clariion_bus_detach(struct scsi_device *sdev);
 
 static struct scsi_device_handler clariion_dh = {
@@ -661,17 +667,19 @@ static struct scsi_device_handler clariion_dh = {
 	.prep_fn	= clariion_prep_fn,
 	.set_params	= clariion_set_params,
 	.match		= clariion_match,
+	.get_dh_data_size = clariion_dh_data_size,
 };
 
-static int clariion_bus_attach(struct scsi_device *sdev)
+static int clariion_bus_attach(struct scsi_device *sdev,
+			       struct scsi_dh_data *scsi_dh_data)
 {
-	struct scsi_dh_data *scsi_dh_data;
 	struct clariion_dh_data *h;
 	unsigned long flags;
 	int err;
 
-	scsi_dh_data = kzalloc(sizeof(*scsi_dh_data)
-			       + sizeof(*h) , GFP_KERNEL);
+	if (!scsi_dh_data)
+		scsi_dh_data = kzalloc(sizeof(*scsi_dh_data)
+				       + sizeof(*h) , GFP_NOIO);
 	if (!scsi_dh_data) {
 		sdev_printk(KERN_ERR, sdev, "%s: Attach failed\n",
 			    CLARIION_NAME);
diff --git a/drivers/scsi/device_handler/scsi_dh_hp_sw.c b/drivers/scsi/device_handler/scsi_dh_hp_sw.c
index 084062b..c0a13e6 100644
--- a/drivers/scsi/device_handler/scsi_dh_hp_sw.c
+++ b/drivers/scsi/device_handler/scsi_dh_hp_sw.c
@@ -338,7 +338,13 @@ static bool hp_sw_match(struct scsi_device *sdev)
 	return false;
 }
 
-static int hp_sw_bus_attach(struct scsi_device *sdev);
+static size_t hp_sw_dh_data_size(void)
+{
+	return sizeof(struct hp_sw_dh_data);
+}
+
+static int hp_sw_bus_attach(struct scsi_device *sdev,
+			    struct scsi_dh_data *scsi_dh_data);
 static void hp_sw_bus_detach(struct scsi_device *sdev);
 
 static struct scsi_device_handler hp_sw_dh = {
@@ -350,17 +356,19 @@ static struct scsi_device_handler hp_sw_dh = {
 	.activate	= hp_sw_activate,
 	.prep_fn	= hp_sw_prep_fn,
 	.match		= hp_sw_match,
+	.get_dh_data_size = hp_sw_dh_data_size,
 };
 
-static int hp_sw_bus_attach(struct scsi_device *sdev)
+static int hp_sw_bus_attach(struct scsi_device *sdev,
+			    struct scsi_dh_data *scsi_dh_data)
 {
-	struct scsi_dh_data *scsi_dh_data;
 	struct hp_sw_dh_data *h;
 	unsigned long flags;
 	int ret;
 
-	scsi_dh_data = kzalloc(sizeof(*scsi_dh_data)
-			       + sizeof(*h) , GFP_KERNEL);
+	if (!scsi_dh_data)
+		scsi_dh_data = kzalloc(sizeof(*scsi_dh_data)
+				       + sizeof(*h) , GFP_NOIO);
 	if (!scsi_dh_data) {
 		sdev_printk(KERN_ERR, sdev, "%s: Attach Failed\n",
 			    HP_SW_NAME);
diff --git a/drivers/scsi/device_handler/scsi_dh_rdac.c b/drivers/scsi/device_handler/scsi_dh_rdac.c
index 69c915a..77ebc2d 100644
--- a/drivers/scsi/device_handler/scsi_dh_rdac.c
+++ b/drivers/scsi/device_handler/scsi_dh_rdac.c
@@ -824,7 +824,13 @@ static bool rdac_match(struct scsi_device *sdev)
 	return false;
 }
 
-static int rdac_bus_attach(struct scsi_device *sdev);
+static size_t rdac_dh_data_size(void)
+{
+	return sizeof(struct rdac_dh_data);
+}
+
+static int rdac_bus_attach(struct scsi_device *sdev,
+			   struct scsi_dh_data *scsi_dh_data);
 static void rdac_bus_detach(struct scsi_device *sdev);
 
 static struct scsi_device_handler rdac_dh = {
@@ -837,19 +843,21 @@ static struct scsi_device_handler rdac_dh = {
 	.detach = rdac_bus_detach,
 	.activate = rdac_activate,
 	.match = rdac_match,
+	.get_dh_data_size = rdac_dh_data_size,
 };
 
-static int rdac_bus_attach(struct scsi_device *sdev)
+static int rdac_bus_attach(struct scsi_device *sdev,
+			   struct scsi_dh_data *scsi_dh_data)
 {
-	struct scsi_dh_data *scsi_dh_data;
 	struct rdac_dh_data *h;
 	unsigned long flags;
 	int err;
 	char array_name[ARRAY_LABEL_LEN];
 	char array_id[UNIQUE_ID_LEN];
 
-	scsi_dh_data = kzalloc(sizeof(*scsi_dh_data)
-			       + sizeof(*h) , GFP_KERNEL);
+	if (!scsi_dh_data)
+		scsi_dh_data = kzalloc(sizeof(*scsi_dh_data)
+				       + sizeof(*h) , GFP_NOIO);
 	if (!scsi_dh_data) {
 		sdev_printk(KERN_ERR, sdev, "%s: Attach failed\n",
 			    RDAC_NAME);
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index a7f9cba..4f4feb4 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -201,12 +201,13 @@ struct scsi_device_handler {
 	const char *name;
 	const struct scsi_dh_devlist *devlist;
 	int (*check_sense)(struct scsi_device *, struct scsi_sense_hdr *);
-	int (*attach)(struct scsi_device *);
+	int (*attach)(struct scsi_device *, struct scsi_dh_data *);
 	void (*detach)(struct scsi_device *);
 	int (*activate)(struct scsi_device *, activate_complete, void *);
 	int (*prep_fn)(struct scsi_device *, struct request *);
 	int (*set_params)(struct scsi_device *, const char *);
 	bool (*match)(struct scsi_device *);
+	size_t (*get_dh_data_size)(void);
 };
 
 struct scsi_dh_data {
diff --git a/include/scsi/scsi_dh.h b/include/scsi/scsi_dh.h
index 620c723..26b7d8b 100644
--- a/include/scsi/scsi_dh.h
+++ b/include/scsi/scsi_dh.h
@@ -58,7 +58,9 @@ enum {
 #if defined(CONFIG_SCSI_DH) || defined(CONFIG_SCSI_DH_MODULE)
 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 struct scsi_dh_data *scsi_dh_alloc_data(const char *, gfp_t);
+extern int scsi_dh_attach(struct request_queue *, const char *,
+			  struct scsi_dh_data *);
 extern void scsi_dh_detach(struct request_queue *);
 extern const char *scsi_dh_attached_handler_name(struct request_queue *, gfp_t);
 extern int scsi_dh_set_params(struct request_queue *, const char *);
-- 
1.7.1

[PATCH 2/2] dm mpath: attach scsi_dh during table resume

[Mike Snitzer]

Preallocate scsi_dh_data using scsi_dh_alloc_data() during table load
but attach the scsi_dh for each path during table resume.  This avoids a
kernel crash that can happen when changing the scsi_dh during table
load.

When we reload a multipath device, there are two instances of the
multipath target - the first instance that is active and the second
instance that is being constructed during table load with "ctr" method.

If the multipath constructor finds out that the device is using a
different device handler, it detaches the existing handler and attaches
a new handler. However, the first instance of the multipath target still
exists and processes requests. If the first instance sends some
path-management request with scsi_dh_activate and the second instance
detaches the device handler while the path-management request is in
flight, a crash happens. The reason for the crash is that the endio
routine for the path-management request is working with structures that
were freed when the handler was detached.

References:
  http://bugzilla.redhat.com/912245
  http://bugzilla.redhat.com/902595

Signed-off-by: Mike Snitzer <snitzer@redhat.com>
---
 drivers/md/dm-mpath.c |  160 ++++++++++++++++++++++++++++++++++---------------
 1 files changed, 111 insertions(+), 49 deletions(-)

diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
index 37d9ead..fd16be1 100644
--- a/drivers/md/dm-mpath.c
+++ b/drivers/md/dm-mpath.c
@@ -36,6 +36,7 @@ struct pgpath {
 
 	struct dm_path path;
 	struct delayed_work activate_path;
+	struct scsi_dh_data *hw_handler_data;
 };
 
 #define path_to_pgpath(__pgp) container_of((__pgp), struct pgpath, path)
@@ -144,6 +145,7 @@ static struct pgpath *alloc_pgpath(void)
 
 static void free_pgpath(struct pgpath *pgpath)
 {
+	kfree(pgpath->hw_handler_data);
 	kfree(pgpath);
 }
 
@@ -563,14 +565,53 @@ static int parse_path_selector(struct dm_arg_set *as, struct priority_group *pg,
 	return 0;
 }
 
+/*
+ * Return: @true if handler is already attached.
+ */
+static bool validate_attached_hardware_handler(struct multipath *m, struct pgpath *p)
+{
+	struct request_queue *q;
+	const char *attached_handler_name;
+	bool handler_already_attached = false;
+
+	q = bdev_get_queue(p->path.dev->bdev);
+	attached_handler_name = scsi_dh_attached_handler_name(q, GFP_KERNEL);
+	if (attached_handler_name) {
+		if (!strncmp(attached_handler_name, m->hw_handler_name,
+			     strlen(attached_handler_name)))
+			handler_already_attached = true;
+
+		if (!m->retain_attached_hw_handler) {
+			kfree(attached_handler_name);
+			return handler_already_attached;
+		}
+
+		handler_already_attached = true;
+		/*
+		 * Reset hw_handler_name to match the attached handler
+		 * and clear any hw_handler_params associated with the
+		 * ignored handler.
+		 *
+		 * NB. This modifies the table line to show the actual
+		 * handler instead of the original table passed in.
+		 */
+		kfree(m->hw_handler_name);
+		m->hw_handler_name = attached_handler_name;
+
+		kfree(m->hw_handler_params);
+		m->hw_handler_params = NULL;
+	}
+
+	return handler_already_attached;
+}
+
 static struct pgpath *parse_path(struct dm_arg_set *as, struct path_selector *ps,
-			       struct dm_target *ti)
+				 struct dm_target *ti)
 {
 	int r;
 	struct pgpath *p;
 	struct multipath *m = ti->private;
-	struct request_queue *q = NULL;
-	const char *attached_handler_name;
+	bool skip_scsi_dh_alloc_data = false;
 
 	/* we need at least a path arg */
 	if (as->argc < 1) {
@@ -589,59 +630,25 @@ static struct pgpath *parse_path(struct dm_arg_set *as, struct path_selector *ps
 		goto bad;
 	}
 
+	/*
+	 * Check if scsi_dh_data allocation can be avoided (as an optimization)
+	 * based on which, if any, device handler is already attached.
+	 */
 	if (m->retain_attached_hw_handler || m->hw_handler_name)
-		q = bdev_get_queue(p->path.dev->bdev);
-
-	if (m->retain_attached_hw_handler) {
-		attached_handler_name = scsi_dh_attached_handler_name(q, GFP_KERNEL);
-		if (attached_handler_name) {
-			/*
-			 * Reset hw_handler_name to match the attached handler
-			 * and clear any hw_handler_params associated with the
-			 * ignored handler.
-			 *
-			 * NB. This modifies the table line to show the actual
-			 * handler instead of the original table passed in.
-			 */
-			kfree(m->hw_handler_name);
-			m->hw_handler_name = attached_handler_name;
-
-			kfree(m->hw_handler_params);
-			m->hw_handler_params = NULL;
-		}
-	}
+		skip_scsi_dh_alloc_data = validate_attached_hardware_handler(m, p);
 
-	if (m->hw_handler_name) {
+	if (m->hw_handler_name && !skip_scsi_dh_alloc_data) {
 		/*
-		 * Increments scsi_dh reference, even when using an
-		 * already-attached handler.
+		 * Pre-allocate the scsi_dh_data for this path.  Either the scsi_dh
+		 * will take ownership of the memory or free_pgpath() will clean it up.
 		 */
-		r = scsi_dh_attach(q, m->hw_handler_name, NULL);
-		if (r == -EBUSY) {
-			/*
-			 * Already attached to different hw_handler:
-			 * try to reattach with correct one.
-			 */
-			scsi_dh_detach(q);
-			r = scsi_dh_attach(q, m->hw_handler_name, NULL);
-		}
-
-		if (r < 0) {
-			ti->error = "error attaching hardware handler";
+		p->hw_handler_data = scsi_dh_alloc_data(m->hw_handler_name, GFP_KERNEL);
+		if (IS_ERR(p->hw_handler_data)) {
+			ti->error = "error allocating hardware handler data";
 			dm_put_device(ti, p->path.dev);
+			r = PTR_ERR(p->hw_handler_data);
 			goto bad;
 		}
-
-		if (m->hw_handler_params) {
-			r = scsi_dh_set_params(q, m->hw_handler_params);
-			if (r < 0) {
-				ti->error = "unable to set hardware "
-							"handler parameters";
-				scsi_dh_detach(q);
-				dm_put_device(ti, p->path.dev);
-				goto bad;
-			}
-		}
 	}
 
 	r = ps->type->add_path(ps, &p->path, as->argc, as->argv, &ti->error);
@@ -1349,16 +1356,71 @@ static void multipath_postsuspend(struct dm_target *ti)
 	mutex_unlock(&m->work_mutex);
 }
 
+static void __attach_scsi_dh(struct multipath *m, struct pgpath *p)
+{
+	int r;
+	char b[BDEVNAME_SIZE];
+	struct request_queue *q = bdev_get_queue(p->path.dev->bdev);
+
+	/*
+	 * Increments scsi_dh reference, even when using an
+	 * already-attached handler.  On success, scsi_dh_attach() will
+	 * take ownership of p->hw_handler_data.
+	 */
+	r = scsi_dh_attach(q, m->hw_handler_name, p->hw_handler_data);
+	if (r == -EBUSY) {
+		/*
+		 * Already attached to different hw_handler:
+		 * try to reattach with correct one.
+		 */
+		scsi_dh_detach(q);
+		r = scsi_dh_attach(q, m->hw_handler_name, p->hw_handler_data);
+	}
+
+	if (r < 0) {
+		DMERR("error attaching hardware handler to: %s",
+		      bdevname(p->path.dev->bdev, b));
+		return;
+	}
+	/*
+	 * scsi_dh_attach() took ownership of p->hw_handler_data.
+	 */
+	p->hw_handler_data = NULL;
+
+	if (m->hw_handler_params) {
+		r = scsi_dh_set_params(q, m->hw_handler_params);
+		if (r < 0) {
+			DMERR("unable to set hardware handler parameters, "
+			      "detaching hardware handler from: %s",
+			      bdevname(p->path.dev->bdev, b));
+			scsi_dh_detach(q);
+			return;
+		}
+	}
+}
+
 /*
  * Restore the queue_if_no_path setting.
  */
 static void multipath_resume(struct dm_target *ti)
 {
 	struct multipath *m = (struct multipath *) ti->private;
+	struct priority_group *pg;
+	struct pgpath *p;
 	unsigned long flags;
 
 	spin_lock_irqsave(&m->lock, flags);
 	m->queue_if_no_path = m->saved_queue_if_no_path;
+
+	if (!m->hw_handler_name)
+		goto out;
+
+	/* Attach the named scsi_dh to all paths in the priority groups */
+	list_for_each_entry(pg, &m->priority_groups, list) {
+		list_for_each_entry(p, &pg->pgpaths, list)
+			__attach_scsi_dh(m, p);
+	}
+out:
 	spin_unlock_irqrestore(&m->lock, flags);
 }
 
-- 
1.7.1

Re: [PATCH 2/2] dm mpath: attach scsi_dh during table resume

[Mike Snitzer]

On Mon, Apr 08 2013 at  5:50pm -0400,
Mike Snitzer <snitzer@redhat.com> wrote:

> Preallocate scsi_dh_data using scsi_dh_alloc_data() during table load
> but attach the scsi_dh for each path during table resume.  This avoids a
> kernel crash that can happen when changing the scsi_dh during table
> load.
> 
> When we reload a multipath device, there are two instances of the
> multipath target - the first instance that is active and the second
> instance that is being constructed during table load with "ctr" method.
> 
> If the multipath constructor finds out that the device is using a
> different device handler, it detaches the existing handler and attaches
> a new handler. However, the first instance of the multipath target still
> exists and processes requests. If the first instance sends some
> path-management request with scsi_dh_activate and the second instance
> detaches the device handler while the path-management request is in
> flight, a crash happens. The reason for the crash is that the endio
> routine for the path-management request is working with structures that
> were freed when the handler was detached.
> 
> References:
>   http://bugzilla.redhat.com/912245
>   http://bugzilla.redhat.com/902595

While this patch addresses the problem of switching the SCSI device
handler prematurely (during load rather than resume) it doesn't do
anything to defend against the use after free NULL pointers that are
possible with the scenario explained above (and as detailed in the
referenced BZs).

I spoke with Hannes at LSF, to address the potential crashes in the
endio path (e.g. stpg_endio) we'd have to bump the scsi_dh_data kref
where appropriate (e.g. for ALUA kref_get in submit_stpg and kref_put in
stpg_endio).

But that is just the tip of the iceberg relative to scsi_dh lifetime.
Seems we've been playing it pretty fast and loose with scsi_dh issued
requests vs detach for quite some time.

I'm now inclined to not care about this issue.  Take away is: don't
switch the device handler (attach the correct one from the start).

Re: [PATCH 2/2] dm mpath: attach scsi_dh during table resume

[Mikulas Patocka]

On Mon, 22 Apr 2013, Mike Snitzer wrote:

> I spoke with Hannes at LSF, to address the potential crashes in the
> endio path (e.g. stpg_endio) we'd have to bump the scsi_dh_data kref
> where appropriate (e.g. for ALUA kref_get in submit_stpg and kref_put in
> stpg_endio).
> 
> But that is just the tip of the iceberg relative to scsi_dh lifetime.
> Seems we've been playing it pretty fast and loose with scsi_dh issued
> requests vs detach for quite some time.
> 
> I'm now inclined to not care about this issue.  Take away is: don't
> switch the device handler (attach the correct one from the start).

I did a patch that disables device handler switching and it was NACKed by 
Hannes. The problem that he pointed out was - when we load SCSI device 
handler modules, they attach automatically to SCSI devices they think they 
belong to. The user then can't set the desired device handler in multipath 
configuration because a different handler is already attached.

So we need a functionality to change device handlers.

(or maybe stop the scsi device handlers from attaching automatically, but 
it would surely generate a lot of other regressions)

Mikulas

Re: [PATCH 2/2] dm mpath: attach scsi_dh during table resume

[Mike Snitzer]

On Thu, Apr 25 2013 at  9:48am -0400,
Mikulas Patocka <mpatocka@redhat.com> wrote:

> 
> 
> On Mon, 22 Apr 2013, Mike Snitzer wrote:
> 
> > I spoke with Hannes at LSF, to address the potential crashes in the
> > endio path (e.g. stpg_endio) we'd have to bump the scsi_dh_data kref
> > where appropriate (e.g. for ALUA kref_get in submit_stpg and kref_put in
> > stpg_endio).
> > 
> > But that is just the tip of the iceberg relative to scsi_dh lifetime.
> > Seems we've been playing it pretty fast and loose with scsi_dh issued
> > requests vs detach for quite some time.
> > 
> > I'm now inclined to not care about this issue.  Take away is: don't
> > switch the device handler (attach the correct one from the start).
> 
> I did a patch that disables device handler switching and it was NACKed by 
> Hannes. The problem that he pointed out was - when we load SCSI device 
> handler modules, they attach automatically to SCSI devices they think they 
> belong to. The user then can't set the desired device handler in multipath 
> configuration because a different handler is already attached.

The handler that is automatically attached _should_ be the correct
handler.  We now have the .match() hook for scsi_dh and it has made for
reliable scsi_dh attachment of the correct handler.
 
> So we need a functionality to change device handlers.

I really cannot think of a sequence where the scsi_dh .match() will
attach the incorrect handler.  This is why I added the
"retain_attached_hw_handler" feature to mpath (commit a58a935d5).

> (or maybe stop the scsi device handlers from attaching automatically, but 
> it would surely generate a lot of other regressions)

The need to support changing device handlers (via multipath table load)
is overblown/historic.

Re: [PATCH 2/2] dm mpath: attach scsi_dh during table resume

[Mikulas Patocka]

On Thu, 25 Apr 2013, Mike Snitzer wrote:

> On Thu, Apr 25 2013 at  9:48am -0400,
> Mikulas Patocka <mpatocka@redhat.com> wrote:
> 
> > 
> > 
> > On Mon, 22 Apr 2013, Mike Snitzer wrote:
> > 
> > > I spoke with Hannes at LSF, to address the potential crashes in the
> > > endio path (e.g. stpg_endio) we'd have to bump the scsi_dh_data kref
> > > where appropriate (e.g. for ALUA kref_get in submit_stpg and kref_put in
> > > stpg_endio).
> > > 
> > > But that is just the tip of the iceberg relative to scsi_dh lifetime.
> > > Seems we've been playing it pretty fast and loose with scsi_dh issued
> > > requests vs detach for quite some time.
> > > 
> > > I'm now inclined to not care about this issue.  Take away is: don't
> > > switch the device handler (attach the correct one from the start).
> > 
> > I did a patch that disables device handler switching and it was NACKed by 
> > Hannes. The problem that he pointed out was - when we load SCSI device 
> > handler modules, they attach automatically to SCSI devices they think they 
> > belong to. The user then can't set the desired device handler in multipath 
> > configuration because a different handler is already attached.
> 
> The handler that is automatically attached _should_ be the correct
> handler.  We now have the .match() hook for scsi_dh and it has made for
> reliable scsi_dh attachment of the correct handler.

The EMC devices work with both ALUA and EMC handlers - so there is no one 
"correct" handler, the correct handler is the one that the user specified 
in multipath configuration.

> > So we need a functionality to change device handlers.
> 
> I really cannot think of a sequence where the scsi_dh .match() will
> attach the incorrect handler.  This is why I added the
> "retain_attached_hw_handler" feature to mpath (commit a58a935d5).

The automatic handler assigment can't change existing handler.

But if one handler was automatically selected and the user selects a 
different handler in multipath configuration, the handler is changed.

> > (or maybe stop the scsi device handlers from attaching automatically, but 
> > it would surely generate a lot of other regressions)
> 
> The need to support changing device handlers (via multipath table load)
> is overblown/historic.

So - do you mean that we make "retain_attached_hw_handler" the default 
option and don't allow the user to change existing device handler in 
multipath configuration?

That's what my patch did and it was NACKed by Hannes. The problem there is 
that behavior depends on module loading order - if you activate multipath 
with "EMC" option, it activates the EMC handler. If you load the ALUA 
module and activate multipath with "EMC" option, it stays with the ALUA 
handler.

Mikulas

Re: [PATCH 2/2] dm mpath: attach scsi_dh during table resume

[Bryn M. Reeves]

On 04/25/2013 03:50 PM, Mikulas Patocka wrote:
> On Thu, 25 Apr 2013, Mike Snitzer wrote:
>> The handler that is automatically attached _should_ be the correct
>> handler.  We now have the .match() hook for scsi_dh and it has made for
>> reliable scsi_dh attachment of the correct handler.
>
> The EMC devices work with both ALUA and EMC handlers - so there is no one
> "correct" handler, the correct handler is the one that the user specified
> in multipath configuration.

I think it's more absolute than that; if a Clariion array is in failover 
mode 4 (ALUA) then it's incorrect to use scsi_dh_emc and vice-versa.

The user can configure this in multipath.conf but it does not make it 
correct. The correct handler is the one that matches the configured 
failover mode of the array.

The ALUA handler scsi_device_tgps() in its match function but since the 
scsi_dh_emc match function only looks at the vendor/product it's 
impossible for it to make the correct decision.

The array can tell us what mode it's running in - teaching scsi_dh_emc 
to do this would seem to be an improvement.

Regards,
Bryn.

Re: [PATCH 2/2] dm mpath: attach scsi_dh during table resume

[Mike Snitzer]

On Thu, Apr 25 2013 at 10:50am -0400,
Mikulas Patocka <mpatocka@redhat.com> wrote:

> 
> 
> On Thu, 25 Apr 2013, Mike Snitzer wrote:
> 
> > On Thu, Apr 25 2013 at  9:48am -0400,
> > Mikulas Patocka <mpatocka@redhat.com> wrote:
> > 
> > > 
> > > 
> > > On Mon, 22 Apr 2013, Mike Snitzer wrote:
> > > 
> > > > I spoke with Hannes at LSF, to address the potential crashes in the
> > > > endio path (e.g. stpg_endio) we'd have to bump the scsi_dh_data kref
> > > > where appropriate (e.g. for ALUA kref_get in submit_stpg and kref_put in
> > > > stpg_endio).
> > > > 
> > > > But that is just the tip of the iceberg relative to scsi_dh lifetime.
> > > > Seems we've been playing it pretty fast and loose with scsi_dh issued
> > > > requests vs detach for quite some time.
> > > > 
> > > > I'm now inclined to not care about this issue.  Take away is: don't
> > > > switch the device handler (attach the correct one from the start).
> > > 
> > > I did a patch that disables device handler switching and it was NACKed by 
> > > Hannes. The problem that he pointed out was - when we load SCSI device 
> > > handler modules, they attach automatically to SCSI devices they think they 
> > > belong to. The user then can't set the desired device handler in multipath 
> > > configuration because a different handler is already attached.
> > 
> > The handler that is automatically attached _should_ be the correct
> > handler.  We now have the .match() hook for scsi_dh and it has made for
> > reliable scsi_dh attachment of the correct handler.
> 
> The EMC devices work with both ALUA and EMC handlers - so there is no one 
> "correct" handler, the correct handler is the one that the user specified 
> in multipath configuration.
> 
> > > So we need a functionality to change device handlers.
> > 
> > I really cannot think of a sequence where the scsi_dh .match() will
> > attach the incorrect handler.  This is why I added the
> > "retain_attached_hw_handler" feature to mpath (commit a58a935d5).
> 
> The automatic handler assigment can't change existing handler.
> 
> But if one handler was automatically selected and the user selects a 
> different handler in multipath configuration, the handler is changed.
> 
> > > (or maybe stop the scsi device handlers from attaching automatically, but 
> > > it would surely generate a lot of other regressions)
> > 
> > The need to support changing device handlers (via multipath table load)
> > is overblown/historic.
> 
> So - do you mean that we make "retain_attached_hw_handler" the default 
> option and don't allow the user to change existing device handler in 
> multipath configuration?
> 
> That's what my patch did and it was NACKed by Hannes. The problem there is 
> that behavior depends on module loading order - if you activate multipath 
> with "EMC" option, it activates the EMC handler. If you load the ALUA 
> module and activate multipath with "EMC" option, it stays with the ALUA 
> handler.

.match allows for correct scsi_dh selection in the decision of alua vs
emc (alua has the tpgs bit set) -- but both scsi_dh modules must be
loaded.

If the incorrect handler is getting attached then it is either a bug in
the .match method (for the handler that should've been attached) or the
storage isn't configured how the user thought and they need to
adjust/reconfigure to have it be like they expected.

Either way we really _could_ impose not allowing the scsi_dh handler to
be changed (by multipath) -- which is why I Acked your patch.  There is
always the scsi_dh sysfs interface to allow the user to change the
scsi_dh (and possibly shoot themselves in the foot).

Re: [PATCH 2/2] dm mpath: attach scsi_dh during table resume

[Mike Snitzer]

On Thu, Apr 25 2013 at 11:27am -0400,
Bryn M. Reeves <bmr@redhat.com> wrote:

> On 04/25/2013 03:50 PM, Mikulas Patocka wrote:
> >On Thu, 25 Apr 2013, Mike Snitzer wrote:
> >>The handler that is automatically attached _should_ be the correct
> >>handler.  We now have the .match() hook for scsi_dh and it has made for
> >>reliable scsi_dh attachment of the correct handler.
> >
> >The EMC devices work with both ALUA and EMC handlers - so there is no one
> >"correct" handler, the correct handler is the one that the user specified
> >in multipath configuration.
> 
> I think it's more absolute than that; if a Clariion array is in
> failover mode 4 (ALUA) then it's incorrect to use scsi_dh_emc and
> vice-versa.
> 
> The user can configure this in multipath.conf but it does not make
> it correct. The correct handler is the one that matches the
> configured failover mode of the array.
> 
> The ALUA handler scsi_device_tgps() in its match function but since
> the scsi_dh_emc match function only looks at the vendor/product it's
> impossible for it to make the correct decision.
> 
> The array can tell us what mode it's running in - teaching
> scsi_dh_emc to do this would seem to be an improvement.

clariion_match does more than check the vendor and product; if tpgs is
set (ALUA mode) it returns false.

So yes, while there is room for improvement in clariion_match the
current code should work just fine with reasoning between emc and alua.

Re: [PATCH 2/2] dm mpath: attach scsi_dh during table resume

[Bryn M. Reeves]

On 04/25/2013 04:37 PM, Mike Snitzer wrote:> clariion_match does more 
than check the vendor and product; if tpgs is
 > set (ALUA mode) it returns false.
 >
 > So yes, while there is room for improvement in clariion_match the
 > current code should work just fine with reasoning between emc and alua.

Duh, sorry you're right. Completely missed the test for tpgs in emc.

Re: [PATCH 2/2] dm mpath: attach scsi_dh during table resume

[Hannes Reinecke]

On 04/25/2013 05:31 PM, Mike Snitzer wrote:
> On Thu, Apr 25 2013 at 10:50am -0400,
> Mikulas Patocka <mpatocka@redhat.com> wrote:
> 
>>
>>
>> On Thu, 25 Apr 2013, Mike Snitzer wrote:
>>
>>> On Thu, Apr 25 2013 at  9:48am -0400,
>>> Mikulas Patocka <mpatocka@redhat.com> wrote:
>>>
>>>>
>>>>
>>>> On Mon, 22 Apr 2013, Mike Snitzer wrote:
>>>>
>>>>> I spoke with Hannes at LSF, to address the potential crashes in the
>>>>> endio path (e.g. stpg_endio) we'd have to bump the scsi_dh_data kref
>>>>> where appropriate (e.g. for ALUA kref_get in submit_stpg and kref_put in
>>>>> stpg_endio).
>>>>>
>>>>> But that is just the tip of the iceberg relative to scsi_dh lifetime.
>>>>> Seems we've been playing it pretty fast and loose with scsi_dh issued
>>>>> requests vs detach for quite some time.
>>>>>
>>>>> I'm now inclined to not care about this issue.  Take away is: don't
>>>>> switch the device handler (attach the correct one from the start).
>>>>
>>>> I did a patch that disables device handler switching and it was NACKed by 
>>>> Hannes. The problem that he pointed out was - when we load SCSI device 
>>>> handler modules, they attach automatically to SCSI devices they think they 
>>>> belong to. The user then can't set the desired device handler in multipath 
>>>> configuration because a different handler is already attached.
>>>
>>> The handler that is automatically attached _should_ be the correct
>>> handler.  We now have the .match() hook for scsi_dh and it has made for
>>> reliable scsi_dh attachment of the correct handler.
>>
>> The EMC devices work with both ALUA and EMC handlers - so there is no one 
>> "correct" handler, the correct handler is the one that the user specified 
>> in multipath configuration.
>>
>>>> So we need a functionality to change device handlers.
>>>
>>> I really cannot think of a sequence where the scsi_dh .match() will
>>> attach the incorrect handler.  This is why I added the
>>> "retain_attached_hw_handler" feature to mpath (commit a58a935d5).
>>
>> The automatic handler assigment can't change existing handler.
>>
>> But if one handler was automatically selected and the user selects a 
>> different handler in multipath configuration, the handler is changed.
>>
>>>> (or maybe stop the scsi device handlers from attaching automatically, but 
>>>> it would surely generate a lot of other regressions)
>>>
>>> The need to support changing device handlers (via multipath table load)
>>> is overblown/historic.
>>
>> So - do you mean that we make "retain_attached_hw_handler" the default 
>> option and don't allow the user to change existing device handler in 
>> multipath configuration?
>>
>> That's what my patch did and it was NACKed by Hannes. The problem there is 
>> that behavior depends on module loading order - if you activate multipath 
>> with "EMC" option, it activates the EMC handler. If you load the ALUA 
>> module and activate multipath with "EMC" option, it stays with the ALUA 
>> handler.
> 
> .match allows for correct scsi_dh selection in the decision of alua vs
> emc (alua has the tpgs bit set) -- but both scsi_dh modules must be
> loaded.
> 
> If the incorrect handler is getting attached then it is either a bug in
> the .match method (for the handler that should've been attached) or the
> storage isn't configured how the user thought and they need to
> adjust/reconfigure to have it be like they expected.
> 
> Either way we really _could_ impose not allowing the scsi_dh handler to
> be changed (by multipath) -- which is why I Acked your patch.  There is
> always the scsi_dh sysfs interface to allow the user to change the
> scsi_dh (and possibly shoot themselves in the foot).
> 
Always providing there _is_ a correct way.
For eg RDAC might run in ALUA mode, but this is by no means
exclusively; the original 'rdac' mode will work there, too.

Plus some vendors / admins might prefer for whatever reasons
to continue to use the original mode.

So I don't think there is a 'correct' hardware handler.
Only a preferred one. And the preference is set by the user,
not the installation. Hence it would be a bad idea to
disallow scsi_dh changes.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
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/2] dm mpath: attach scsi_dh during table resume

[Mike Snitzer]

On Fri, Apr 26 2013 at  2:05am -0400,
Hannes Reinecke <hare@suse.de> wrote:

> On 04/25/2013 05:31 PM, Mike Snitzer wrote:
> > On Thu, Apr 25 2013 at 10:50am -0400,
> > Mikulas Patocka <mpatocka@redhat.com> wrote:
> > 
> >>
> >>
> >> On Thu, 25 Apr 2013, Mike Snitzer wrote:
> >>
> >>> On Thu, Apr 25 2013 at  9:48am -0400,
> >>> Mikulas Patocka <mpatocka@redhat.com> wrote:
> >>>
> >>>>
> >>>>
> >>>> On Mon, 22 Apr 2013, Mike Snitzer wrote:
> >>>
> >>> The need to support changing device handlers (via multipath table load)
> >>> is overblown/historic.
> >>
> >> So - do you mean that we make "retain_attached_hw_handler" the default 
> >> option and don't allow the user to change existing device handler in 
> >> multipath configuration?
> >>
> >> That's what my patch did and it was NACKed by Hannes. The problem there is 
> >> that behavior depends on module loading order - if you activate multipath 
> >> with "EMC" option, it activates the EMC handler. If you load the ALUA 
> >> module and activate multipath with "EMC" option, it stays with the ALUA 
> >> handler.
> > 
> > .match allows for correct scsi_dh selection in the decision of alua vs
> > emc (alua has the tpgs bit set) -- but both scsi_dh modules must be
> > loaded.
> > 
> > If the incorrect handler is getting attached then it is either a bug in
> > the .match method (for the handler that should've been attached) or the
> > storage isn't configured how the user thought and they need to
> > adjust/reconfigure to have it be like they expected.
> > 
> > Either way we really _could_ impose not allowing the scsi_dh handler to
> > be changed (by multipath) -- which is why I Acked your patch.  There is
> > always the scsi_dh sysfs interface to allow the user to change the
> > scsi_dh (and possibly shoot themselves in the foot).
> > 
> Always providing there _is_ a correct way.
> For eg RDAC might run in ALUA mode, but this is by no means
> exclusively; the original 'rdac' mode will work there, too.

Just like the emc handler, rdac_match will prefer alua over rdac if tpgs
is set.

> Plus some vendors / admins might prefer for whatever reasons
> to continue to use the original mode.

So you're saying an admin should have the flexibility to use rdac if
they know better?  I don't disagree in principle but in practice
providing that flexibility comes at a cost (e.g. potential for
kernel crashes).

> So I don't think there is a 'correct' hardware handler.
> Only a preferred one. And the preference is set by the user,
> not the installation. Hence it would be a bad idea to
> disallow scsi_dh changes.

Disallowing scsi_dh changes is more about avoiding the potential for
crashes (which have been seen in testing).  As such I think there is
more risk of hitting a crash (very bad) than there is of an admin
_really_ wanting to prefer the proprietary handler (rdac) over the
standards based one (alua).

So I'm in favor of disallowing scsi_dh changes via multipath (until
if/when the potential for scsi_dh crashes is eliminated).  dm-multipath
really doesn't have a role in attaching a scsi_dh these days.  Disallowing
scsi_dh changes acknowledges that the underlying kernel support has
improved and that admins should take notice (and multipath-tools should
now default to 'retain_attached_hw_handler').