scsi_dh_rdac: Add empty set_params function to scsi_dh_rdac

Commit Message

babu moger Aug. 8, 2012, 4:10 p.m. UTC

This patch adds empty set_params function to scsi_dh_rdac.

This patch is required for the following features to work properly.
1. add  retain_attached_hw_handler feature

http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commit;h=a58a935d5a1b2ad267017a68c3a1bca26226cc76

2. add scsi_dh_attached_handler_name

http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commit;h=7e8a74b177f17d100916b6ad415450f7c9508691


DM layer detaches the handler if the set_params is not implemented or fails.

For example if we pass following parameters from multipath:

	features "1 retain_attached_hw_handler" (consider rdac as attached_hw_handler)
	hardware_handler "2 alua 1"

If the attached_hw_handler is rdac, then DM anyway tries to call set_params on rdac. Because rdac does not 
implement set_params, scsi_dh_set_params fails and DM detaches the handler.

This patch fixes this problem. 

Signed-off-by: Babu Moger <[e-mail]>
---



--
dm-devel mailing list
[e-mail]
https://www.redhat.com/mailman/listinfo/dm-devel

Comments

Mike Snitzer Aug. 8, 2012, 7:31 p.m. UTC | #1

On Wed, Aug 08 2012 at 12:10pm -0400,
Moger, Babu <[e-mail]> wrote:

> This patch adds empty set_params function to scsi_dh_rdac.
> 
> This patch is required for the following features to work properly.
> 1. add  retain_attached_hw_handler feature
> 
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commit;h=a58a935d5a1b2ad267017a68c3a1bca26226cc76
> 
> 2. add scsi_dh_attached_handler_name
> 
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commit;h=7e8a74b177f17d100916b6ad415450f7c9508691
> 
> 
> DM layer detaches the handler if the set_params is not implemented or fails.
> 
> For example if we pass following parameters from multipath:
> 
> 	features "1 retain_attached_hw_handler" (consider rdac as attached_hw_handler)
> 	hardware_handler "2 alua 1"
> 
> If the attached_hw_handler is rdac, then DM anyway tries to call set_params on rdac. Because rdac does not 
> implement set_params, scsi_dh_set_params fails and DM detaches the handler.
> 
> This patch fixes this problem. 
> 
> Signed-off-by: Babu Moger <[e-mail]>

I think it'd be best to fix scsi_dh_set_params() so that it returns 0 if
scsi_dh->set_params is NULL.  That'd avoid all scsi_dh from having to
stub out a similar set_params to return 0.

But taking a step back, have you actually seen the problem you're saying
this patch fixes?

This problem shouldn't exist:
drivers/md/dm-mpath.c:parse_path will free m->hw_handler_params (and
reset it to NULL) if a scsi_dh is already attached.  Have a look at the
if (attached_handler_name) {} case.

Mike

--
dm-devel mailing list
[e-mail]
https://www.redhat.com/mailman/listinfo/dm-devel

babu moger Aug. 8, 2012, 9:34 p.m. UTC | #2

> -----Original Message-----
> From: Mike Snitzer [mailto:[e-mail]]
> Sent: Wednesday, August 08, 2012 2:31 PM
> To: Moger, Babu
> Cc: linux-scsi; device-mapper development ([e-mail])
> Subject: Re: scsi_dh_rdac: Add empty set_params function to scsi_dh_rdac
> 
> On Wed, Aug 08 2012 at 12:10pm -0400,
> Moger, Babu <[e-mail]> wrote:
> 
> > This patch adds empty set_params function to scsi_dh_rdac.
> >
> > This patch is required for the following features to work properly.
> > 1. add  retain_attached_hw_handler feature
> >
> >
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commit;h=a58a
> 935d5a1b2ad267017a68c3a1bca26226cc76
> >
> > 2. add scsi_dh_attached_handler_name
> >
> >
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commit;h=7e8
> a74b177f17d100916b6ad415450f7c9508691
> >
> >
> > DM layer detaches the handler if the set_params is not implemented or
> fails.
> >
> > For example if we pass following parameters from multipath:
> >
> > 	features "1 retain_attached_hw_handler" (consider rdac as
> attached_hw_handler)
> > 	hardware_handler "2 alua 1"
> >
> > If the attached_hw_handler is rdac, then DM anyway tries to call
> set_params on rdac. Because rdac does not
> > implement set_params, scsi_dh_set_params fails and DM detaches the
> handler.
> >
> > This patch fixes this problem.
> >
> > Signed-off-by: Babu Moger <[e-mail]>
> 
> I think it'd be best to fix scsi_dh_set_params() so that it returns 0 if
> scsi_dh->set_params is NULL.  That'd avoid all scsi_dh from having to
> stub out a similar set_params to return 0.
> 
> But taking a step back, have you actually seen the problem you're saying
> this patch fixes?
> 
> This problem shouldn't exist:
> drivers/md/dm-mpath.c:parse_path will free m->hw_handler_params (and
> reset it to NULL) if a scsi_dh is already attached.  Have a look at the
> if (attached_handler_name) {} case.

My bad. I was still using one of your old patches.
http://www.redhat.com/archives/dm-devel/2012-June/msg00174.html

Things have changed after this version.  We don't have this problem with the new set of patches. 


--
dm-devel mailing list
[e-mail]
https://www.redhat.com/mailman/listinfo/dm-devel

Patch

--- linux-3.5-rc7/drivers/scsi/device_handler/scsi_dh_rdac.c.orig	2012-08-08 10:38:54.000000000 -0500
+++ linux-3.5-rc7/drivers/scsi/device_handler/scsi_dh_rdac.c	2012-08-08 11:02:10.000000000 -0500
@@ -816,6 +816,17 @@  static const struct scsi_dh_devlist rdac
 	{NULL, NULL},
 };
 
+/*
+ * params - parameters in the following format
+ *      "no_of_params\0param1\0param2\0param3\0...\0"
+ *      for example, string for 2 parameters with value 10 and 21
+ *      is specified as "2\010\021\0".
+ */
+static int rdac_set_params(struct scsi_device *sdev, const char *params)
+{
+	return 0;
+}
+
 static bool rdac_match(struct scsi_device *sdev)
 {
 	int i;
@@ -846,6 +857,7 @@  static struct scsi_device_handler rdac_d
 	.attach = rdac_bus_attach,
 	.detach = rdac_bus_detach,
 	.activate = rdac_activate,
+	.set_params = rdac_set_params,
 	.match = rdac_match,
 };