[PATCH 1/2] dm mpath: fail message ioctl if specified path is not valid

[PATCH 1/2] dm mpath: fail message ioctl if specified path is not valid

[Mike Snitzer]

The message ioctl would succeed for the 'reinistate_path' and
'fail_path' messages even if action was not taken because the
specified device was not a valid path of the multipath device.

before:
$ dmsetup message mpathb 0 reinstate_path /dev/vdb
$ echo $?
0

after:
$ dmsetup message mpathb 0 reinstate_path /dev/vdb
device-mapper: message ioctl failed: Invalid argument
Command failed
$ echo $?
1

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

diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
index b82d288..97842c6 100644
--- a/drivers/md/dm-mpath.c
+++ b/drivers/md/dm-mpath.c
@@ -1065,7 +1065,7 @@ out:
 static int action_dev(struct multipath *m, struct dm_dev *dev,
 		      action_fn action)
 {
-	int r = 0;
+	int r = -EINVAL;
 	struct pgpath *pgpath;
 	struct priority_group *pg;
 
-- 
1.7.3.4

[PATCH 2/2] dm mpath: allow table load with 0 priority groups

[Mike Snitzer]

If an mpath device is held open when all paths in the last priority
group have failed userspace multipathd will attempt to reload the
associated DM table to reflect that the device no longer has any
priority groups.  But the reload attempt always failed because the
multipath target did not allow 0 priority groups.

Adjust multipath target to allow a table with both 0 priority groups
and 0 for the initial priority group number.

All multipath target messages related to priority group (enable_group,
disable_group, switch_group) will properly handle a priority group of
0 (will cause error).

When reloading a multipath table with 0 priority groups, userspace
multipathd must be updated to specify an initial priority group number
of 0 (rather than 1).

Signed-off-by: Mike Snitzer <snitzer@redhat.com>
Cc: Babu Moger <babu.moger@lsi.com>
---
 drivers/md/dm-mpath.c |   13 ++++++++++---
 1 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
index 97842c6..bc824bd 100644
--- a/drivers/md/dm-mpath.c
+++ b/drivers/md/dm-mpath.c
@@ -844,8 +844,8 @@ static int multipath_ctr(struct dm_target *ti, unsigned int argc,
 {
 	/* target parameters */
 	static struct param _params[] = {
-		{1, 1024, "invalid number of priority groups"},
-		{1, 1024, "invalid initial priority group number"},
+		{0, 1024, "invalid number of priority groups"},
+		{0, 1024, "invalid initial priority group number"},
 	};
 
 	int r;
@@ -879,6 +879,13 @@ static int multipath_ctr(struct dm_target *ti, unsigned int argc,
 	if (r)
 		goto bad;
 
+	if ((!m->nr_priority_groups && next_pg_num) ||
+	    (m->nr_priority_groups && !next_pg_num)) {
+		ti->error = "invalid initial priority group";
+		r = -EINVAL;
+		goto bad;
+	}
+
 	/* parse the priority groups */
 	while (as.argc) {
 		struct priority_group *pg;
@@ -1417,7 +1424,7 @@ static int multipath_status(struct dm_target *ti, status_type_t type,
 	else if (m->current_pg)
 		pg_num = m->current_pg->pg_num;
 	else
-			pg_num = 1;
+		pg_num = (m->nr_priority_groups ? 1 : 0);
 
 	DMEMIT("%u ", pg_num);
 
-- 
1.7.3.4

Re: [PATCH 2/2] dm mpath: allow table load with 0 priority groups

[Moger, Babu]

Looks good to me. 

> -----Original Message-----
> From: Mike Snitzer [mailto:snitzer@redhat.com]
> Sent: Monday, January 31, 2011 10:14 AM
> To: dm-devel@redhat.com
> Cc: Mike Snitzer; Moger, Babu
> Subject: [PATCH 2/2] dm mpath: allow table load with 0 priority groups
> 
> If an mpath device is held open when all paths in the last priority
> group have failed userspace multipathd will attempt to reload the
> associated DM table to reflect that the device no longer has any
> priority groups.  But the reload attempt always failed because the
> multipath target did not allow 0 priority groups.
> 
> Adjust multipath target to allow a table with both 0 priority groups
> and 0 for the initial priority group number.
> 
> All multipath target messages related to priority group (enable_group,
> disable_group, switch_group) will properly handle a priority group of
> 0 (will cause error).
> 
> When reloading a multipath table with 0 priority groups, userspace
> multipathd must be updated to specify an initial priority group number
> of 0 (rather than 1).

 Looks like we still have some action from multipath tool. CCing Ben..
> 
> Signed-off-by: Mike Snitzer <snitzer@redhat.com>
> Cc: Babu Moger <babu.moger@lsi.com>
  
> ---
>  drivers/md/dm-mpath.c |   13 ++++++++++---
>  1 files changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
> index 97842c6..bc824bd 100644
> --- a/drivers/md/dm-mpath.c
> +++ b/drivers/md/dm-mpath.c
> @@ -844,8 +844,8 @@ static int multipath_ctr(struct dm_target *ti,
> unsigned int argc,
>  {
>  	/* target parameters */
>  	static struct param _params[] = {
> -		{1, 1024, "invalid number of priority groups"},
> -		{1, 1024, "invalid initial priority group number"},
> +		{0, 1024, "invalid number of priority groups"},
> +		{0, 1024, "invalid initial priority group number"},
>  	};
> 
>  	int r;
> @@ -879,6 +879,13 @@ static int multipath_ctr(struct dm_target *ti,
> unsigned int argc,
>  	if (r)
>  		goto bad;
> 
> +	if ((!m->nr_priority_groups && next_pg_num) ||
> +	    (m->nr_priority_groups && !next_pg_num)) {
> +		ti->error = "invalid initial priority group";
> +		r = -EINVAL;
> +		goto bad;
> +	}
> +
>  	/* parse the priority groups */
>  	while (as.argc) {
>  		struct priority_group *pg;
> @@ -1417,7 +1424,7 @@ static int multipath_status(struct dm_target *ti,
> status_type_t type,
>  	else if (m->current_pg)
>  		pg_num = m->current_pg->pg_num;
>  	else
> -			pg_num = 1;
> +		pg_num = (m->nr_priority_groups ? 1 : 0);
> 
>  	DMEMIT("%u ", pg_num);
> 
> --
> 1.7.3.4

[RFC PATCH] multipathd: use 0 for initial pg if nr_priority_groups=0 (was: Re: [PATCH 2/2] dm mpath: allow table load with 0 priority groups)

[Mike Snitzer]

On Mon, Jan 31 2011 at  1:09pm -0500,
Moger, Babu <Babu.Moger@lsi.com> wrote:

> Looks good to me. 
> 
> > -----Original Message-----
> > From: Mike Snitzer [mailto:snitzer@redhat.com]
> > Sent: Monday, January 31, 2011 10:14 AM
> > To: dm-devel@redhat.com
> > Cc: Mike Snitzer; Moger, Babu
> > Subject: [PATCH 2/2] dm mpath: allow table load with 0 priority groups
> > 
> > If an mpath device is held open when all paths in the last priority
> > group have failed userspace multipathd will attempt to reload the
> > associated DM table to reflect that the device no longer has any
> > priority groups.  But the reload attempt always failed because the
> > multipath target did not allow 0 priority groups.
> > 
> > Adjust multipath target to allow a table with both 0 priority groups
> > and 0 for the initial priority group number.
> > 
> > All multipath target messages related to priority group (enable_group,
> > disable_group, switch_group) will properly handle a priority group of
> > 0 (will cause error).
> > 
> > When reloading a multipath table with 0 priority groups, userspace
> > multipathd must be updated to specify an initial priority group number
> > of 0 (rather than 1).
> 
>  Looks like we still have some action from multipath tool. CCing Ben..

Right, I looked in to the multipath-tools change.  Here is an RFC patch.

Inlined "FIXME"s pose the relevant questions.  I held off on having
select_path_group() return 0 when !mpp->pg because I wasn't confident
that I wouldn't break multipathd in some unintuitive way.

I did audit the various callers (indirect through setup_map call):

update_path_groups
ev_add_path
ev_remove_path

(all above first call setup_map)

setup_map()
  - mpp->bestpg = select_path_group()
  - assemble_map() -- establishes mp->params

do_map()

Even though I verified that much I wasn't completely confident in
changing select_path_group()'s defualt return of 1 -- so I localized the
change to where I _knew_ it was safe (assemble_map):

diff --git a/libmultipath/dmparser.c b/libmultipath/dmparser.c
index 1ef3aad..97b6420 100644
--- a/libmultipath/dmparser.c
+++ b/libmultipath/dmparser.c
@@ -52,6 +52,7 @@ assemble_map (struct multipath * mp)
 	int i, j;
 	int shift, freechar;
 	int minio;
+	int nr_priority_groups, initial_pg_nr;
 	char * p;
 	struct pathgroup * pgp;
 	struct path * pp;
@@ -60,9 +61,19 @@ assemble_map (struct multipath * mp)
 	p = mp->params;
 	freechar = sizeof(mp->params);
 
+	nr_priority_groups = VECTOR_SIZE(mp->pg);
+	/*
+	 * FIXME: make this conditional on multipath target version?
+	 * - but no existing code is conditional on multipath target version,
+	 *   nor does dm_drvprereq() store the version for code to do so.
+	 * - thing is older multipath never allowed nr_priority_groups=0 so
+	 *   it doesn't _really_ matter if initial_pg_nr=0 here...
+	 */
+	initial_pg_nr = (nr_priority_groups ? mp->bestpg : 0);
+
 	shift = snprintf(p, freechar, "%s %s %i %i",
 			 mp->features, mp->hwhandler,
-			 VECTOR_SIZE(mp->pg), mp->bestpg);
+			 nr_priority_groups, initial_pg_nr);
 
 	if (shift >= freechar) {
 		fprintf(stderr, "mp->params too small\n");
diff --git a/libmultipath/switchgroup.c b/libmultipath/switchgroup.c
index 025a95d..82c30ad 100644
--- a/libmultipath/switchgroup.c
+++ b/libmultipath/switchgroup.c
@@ -41,7 +41,7 @@ select_path_group (struct multipath * mpp)
 	struct pathgroup * pgp;
 
 	if (!mpp->pg)
-		return 1;
+		return 1; /* FIXME: return 0 here? */
 
 	vector_foreach_slot (mpp->pg, pgp, i) {
 		if (!pgp->paths)

Re: [RFC PATCH] multipathd: use 0 for initial pg if nr_priority_groups=0

[Hannes Reinecke]

On 01/31/2011 07:34 PM, Mike Snitzer wrote:
> On Mon, Jan 31 2011 at  1:09pm -0500,
> Moger, Babu <Babu.Moger@lsi.com> wrote:
> 
>> Looks good to me. 
>>
>>> -----Original Message-----
>>> From: Mike Snitzer [mailto:snitzer@redhat.com]
>>> Sent: Monday, January 31, 2011 10:14 AM
>>> To: dm-devel@redhat.com
>>> Cc: Mike Snitzer; Moger, Babu
>>> Subject: [PATCH 2/2] dm mpath: allow table load with 0 priority groups
>>>
>>> If an mpath device is held open when all paths in the last priority
>>> group have failed userspace multipathd will attempt to reload the
>>> associated DM table to reflect that the device no longer has any
>>> priority groups.  But the reload attempt always failed because the
>>> multipath target did not allow 0 priority groups.
>>>
>>> Adjust multipath target to allow a table with both 0 priority groups
>>> and 0 for the initial priority group number.
>>>
>>> All multipath target messages related to priority group (enable_group,
>>> disable_group, switch_group) will properly handle a priority group of
>>> 0 (will cause error).
>>>
>>> When reloading a multipath table with 0 priority groups, userspace
>>> multipathd must be updated to specify an initial priority group number
>>> of 0 (rather than 1).
>>
>>  Looks like we still have some action from multipath tool. CCing Ben..
> 
> Right, I looked in to the multipath-tools change.  Here is an RFC patch.
> 
> Inlined "FIXME"s pose the relevant questions.  I held off on having
> select_path_group() return 0 when !mpp->pg because I wasn't confident
> that I wouldn't break multipathd in some unintuitive way.
> 
> I did audit the various callers (indirect through setup_map call):
> 
> update_path_groups
> ev_add_path
> ev_remove_path
> 
> (all above first call setup_map)
> 
> setup_map()
>   - mpp->bestpg = select_path_group()
>   - assemble_map() -- establishes mp->params
> 
> do_map()
> 
> Even though I verified that much I wasn't completely confident in
> changing select_path_group()'s defualt return of 1 -- so I localized the
> change to where I _knew_ it was safe (assemble_map):
> 
> diff --git a/libmultipath/dmparser.c b/libmultipath/dmparser.c
> index 1ef3aad..97b6420 100644
> --- a/libmultipath/dmparser.c
> +++ b/libmultipath/dmparser.c
> @@ -52,6 +52,7 @@ assemble_map (struct multipath * mp)
>  	int i, j;
>  	int shift, freechar;
>  	int minio;
> +	int nr_priority_groups, initial_pg_nr;
>  	char * p;
>  	struct pathgroup * pgp;
>  	struct path * pp;
> @@ -60,9 +61,19 @@ assemble_map (struct multipath * mp)
>  	p = mp->params;
>  	freechar = sizeof(mp->params);
>  
> +	nr_priority_groups = VECTOR_SIZE(mp->pg);
> +	/*
> +	 * FIXME: make this conditional on multipath target version?
> +	 * - but no existing code is conditional on multipath target version,
> +	 *   nor does dm_drvprereq() store the version for code to do so.
> +	 * - thing is older multipath never allowed nr_priority_groups=0 so
> +	 *   it doesn't _really_ matter if initial_pg_nr=0 here...
> +	 */
> +	initial_pg_nr = (nr_priority_groups ? mp->bestpg : 0);
> +
>  	shift = snprintf(p, freechar, "%s %s %i %i",
>  			 mp->features, mp->hwhandler,
> -			 VECTOR_SIZE(mp->pg), mp->bestpg);
> +			 nr_priority_groups, initial_pg_nr);
>  
>  	if (shift >= freechar) {
>  		fprintf(stderr, "mp->params too small\n");
> diff --git a/libmultipath/switchgroup.c b/libmultipath/switchgroup.c
> index 025a95d..82c30ad 100644
> --- a/libmultipath/switchgroup.c
> +++ b/libmultipath/switchgroup.c
> @@ -41,7 +41,7 @@ select_path_group (struct multipath * mpp)
>  	struct pathgroup * pgp;
>  
>  	if (!mpp->pg)
> -		return 1;
> +		return 1; /* FIXME: return 0 here? */
>  
>  	vector_foreach_slot (mpp->pg, pgp, i) {
>  		if (!pgp->paths)
> 
I have a similar patchset in my multipath-tools tree, to allow maps
with zero paths, too.
However, there are quite a few places which required checking, as
we're basically _never_ check if a pointer is NULL before accessing
it. With a bit of luck I'll find some time to send out the patchset.

Otherwise you could have a look at

git://git.kernel.org/pub/scm/linux/kernel/git/hare/multipath-tools.git
branch sles11-sp1 will have the most recent stuff.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Markus Rex, HRB 16746 (AG Nürnberg)

Re: [PATCH 2/2] dm mpath: allow table load with 0 priority groups

[Hannes Reinecke]

On 01/31/2011 05:14 PM, Mike Snitzer wrote:
> If an mpath device is held open when all paths in the last priority
> group have failed userspace multipathd will attempt to reload the
> associated DM table to reflect that the device no longer has any
> priority groups.  But the reload attempt always failed because the
> multipath target did not allow 0 priority groups.
> 
> Adjust multipath target to allow a table with both 0 priority groups
> and 0 for the initial priority group number.
> 
> All multipath target messages related to priority group (enable_group,
> disable_group, switch_group) will properly handle a priority group of
> 0 (will cause error).
> 
> When reloading a multipath table with 0 priority groups, userspace
> multipathd must be updated to specify an initial priority group number
> of 0 (rather than 1).
> 
I actually send a similar patch way back, but Alasdair refused to
take it as he still had to mull them over.

And, as mentioned in my other mail, to properly support maps with no
paths there are quite a few changes required in multipath-tools itself.

But other than that: Congrats. One patchset less in my queue.

> Signed-off-by: Mike Snitzer <snitzer@redhat.com>
> Cc: Babu Moger <babu.moger@lsi.com>
Acked-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Markus Rex, HRB 16746 (AG Nürnberg)

[PATCH 2/2 v2] dm mpath: allow table load with 0 priority groups

[Mike Snitzer]

If an mpath device is held open when all paths in the last priority
group have failed userspace multipathd will attempt to reload the
associated DM table to reflect that the device no longer has any
priority groups.  But the reload attempt always failed because the
multipath target did not allow 0 priority groups.

Adjust multipath target to allow a table with both 0 priority groups
and 0 for the initial priority group number.

All multipath target messages related to priority group (enable_group,
disable_group, switch_group) will properly handle a priority group of
0 (will cause error).

When reloading a multipath table with 0 priority groups, userspace
multipathd must be updated to specify an initial priority group number
of 0 (rather than 1).

Signed-off-by: Mike Snitzer <snitzer@redhat.com>
Acked-by: Babu Moger <babu.moger@lsi.com>
---
 drivers/md/dm-mpath.c |   15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

v2: bumped multipath target version to 1.3.0, added babu's acked-by

Index: linux-2.6/drivers/md/dm-mpath.c
===================================================================
--- linux-2.6.orig/drivers/md/dm-mpath.c
+++ linux-2.6/drivers/md/dm-mpath.c
@@ -844,8 +844,8 @@ static int multipath_ctr(struct dm_targe
 {
 	/* target parameters */
 	static struct param _params[] = {
-		{1, 1024, "invalid number of priority groups"},
-		{1, 1024, "invalid initial priority group number"},
+		{0, 1024, "invalid number of priority groups"},
+		{0, 1024, "invalid initial priority group number"},
 	};
 
 	int r;
@@ -879,6 +879,13 @@ static int multipath_ctr(struct dm_targe
 	if (r)
 		goto bad;
 
+	if ((!m->nr_priority_groups && next_pg_num) ||
+	    (m->nr_priority_groups && !next_pg_num)) {
+		ti->error = "invalid initial priority group";
+		r = -EINVAL;
+		goto bad;
+	}
+
 	/* parse the priority groups */
 	while (as.argc) {
 		struct priority_group *pg;
@@ -1417,7 +1424,7 @@ static int multipath_status(struct dm_ta
 	else if (m->current_pg)
 		pg_num = m->current_pg->pg_num;
 	else
-			pg_num = 1;
+		pg_num = (m->nr_priority_groups ? 1 : 0);
 
 	DMEMIT("%u ", pg_num);
 
@@ -1671,7 +1678,7 @@ out:
  *---------------------------------------------------------------*/
 static struct target_type multipath_target = {
 	.name = "multipath",
-	.version = {1, 2, 0},
+	.version = {1, 3, 0},
 	.module = THIS_MODULE,
 	.ctr = multipath_ctr,
 	.dtr = multipath_dtr,

Re: [PATCH 2/2 v2] dm mpath: allow table load with 0 priority groups

[Mike Snitzer]

On Thu, Feb 10 2011 at  4:06pm -0500,
Mike Snitzer <snitzer@redhat.com> wrote:

> If an mpath device is held open when all paths in the last priority
> group have failed userspace multipathd will attempt to reload the
> associated DM table to reflect that the device no longer has any
> priority groups.  But the reload attempt always failed because the
> multipath target did not allow 0 priority groups.
> 
> Adjust multipath target to allow a table with both 0 priority groups
> and 0 for the initial priority group number.
> 
> All multipath target messages related to priority group (enable_group,
> disable_group, switch_group) will properly handle a priority group of
> 0 (will cause error).
> 
> When reloading a multipath table with 0 priority groups, userspace
> multipathd must be updated to specify an initial priority group number
> of 0 (rather than 1).
> 
> Signed-off-by: Mike Snitzer <snitzer@redhat.com>
> Acked-by: Babu Moger <babu.moger@lsi.com>

Forgot Hannes':

Acked-by: Hannes Reinecke <hare@suse.de>

Re: [RFC PATCH] multipathd: use 0 for initial pg if nr_priority_groups=0

[Mike Snitzer]

On Tue, Feb 01 2011 at 10:38am -0500,
Hannes Reinecke <hare@suse.de> wrote:

> On 01/31/2011 07:34 PM, Mike Snitzer wrote:
> > On Mon, Jan 31 2011 at  1:09pm -0500,
> > Moger, Babu <Babu.Moger@lsi.com> wrote:
> > 
> >> Looks good to me. 
> >>
> >>> -----Original Message-----
> >>> From: Mike Snitzer [mailto:snitzer@redhat.com]
> >>> Sent: Monday, January 31, 2011 10:14 AM
> >>> To: dm-devel@redhat.com
> >>> Cc: Mike Snitzer; Moger, Babu
> >>> Subject: [PATCH 2/2] dm mpath: allow table load with 0 priority groups
> >>>
> >>> If an mpath device is held open when all paths in the last priority
> >>> group have failed userspace multipathd will attempt to reload the
> >>> associated DM table to reflect that the device no longer has any
> >>> priority groups.  But the reload attempt always failed because the
> >>> multipath target did not allow 0 priority groups.
> >>>
> >>> Adjust multipath target to allow a table with both 0 priority groups
> >>> and 0 for the initial priority group number.
> >>>
> >>> All multipath target messages related to priority group (enable_group,
> >>> disable_group, switch_group) will properly handle a priority group of
> >>> 0 (will cause error).
> >>>
> >>> When reloading a multipath table with 0 priority groups, userspace
> >>> multipathd must be updated to specify an initial priority group number
> >>> of 0 (rather than 1).
> >>
> >>  Looks like we still have some action from multipath tool. CCing Ben..
> > 
> > Right, I looked in to the multipath-tools change.  Here is an RFC patch.
> > 
> > Inlined "FIXME"s pose the relevant questions.  I held off on having
> > select_path_group() return 0 when !mpp->pg because I wasn't confident
> > that I wouldn't break multipathd in some unintuitive way.
> > 
> > I did audit the various callers (indirect through setup_map call):
> > 
> > update_path_groups
> > ev_add_path
> > ev_remove_path
> > 
> > (all above first call setup_map)
> > 
> > setup_map()
> >   - mpp->bestpg = select_path_group()
> >   - assemble_map() -- establishes mp->params
> > 
> > do_map()
> > 
> > Even though I verified that much I wasn't completely confident in
> > changing select_path_group()'s defualt return of 1 -- so I localized the
> > change to where I _knew_ it was safe (assemble_map):
> > 
> > diff --git a/libmultipath/dmparser.c b/libmultipath/dmparser.c
> > index 1ef3aad..97b6420 100644
> > --- a/libmultipath/dmparser.c
> > +++ b/libmultipath/dmparser.c
> > @@ -52,6 +52,7 @@ assemble_map (struct multipath * mp)
> >  	int i, j;
> >  	int shift, freechar;
> >  	int minio;
> > +	int nr_priority_groups, initial_pg_nr;
> >  	char * p;
> >  	struct pathgroup * pgp;
> >  	struct path * pp;
> > @@ -60,9 +61,19 @@ assemble_map (struct multipath * mp)
> >  	p = mp->params;
> >  	freechar = sizeof(mp->params);
> >  
> > +	nr_priority_groups = VECTOR_SIZE(mp->pg);
> > +	/*
> > +	 * FIXME: make this conditional on multipath target version?
> > +	 * - but no existing code is conditional on multipath target version,
> > +	 *   nor does dm_drvprereq() store the version for code to do so.
> > +	 * - thing is older multipath never allowed nr_priority_groups=0 so
> > +	 *   it doesn't _really_ matter if initial_pg_nr=0 here...
> > +	 */
> > +	initial_pg_nr = (nr_priority_groups ? mp->bestpg : 0);
> > +
> >  	shift = snprintf(p, freechar, "%s %s %i %i",
> >  			 mp->features, mp->hwhandler,
> > -			 VECTOR_SIZE(mp->pg), mp->bestpg);
> > +			 nr_priority_groups, initial_pg_nr);
> >  
> >  	if (shift >= freechar) {
> >  		fprintf(stderr, "mp->params too small\n");
> > diff --git a/libmultipath/switchgroup.c b/libmultipath/switchgroup.c
> > index 025a95d..82c30ad 100644
> > --- a/libmultipath/switchgroup.c
> > +++ b/libmultipath/switchgroup.c
> > @@ -41,7 +41,7 @@ select_path_group (struct multipath * mpp)
> >  	struct pathgroup * pgp;
> >  
> >  	if (!mpp->pg)
> > -		return 1;
> > +		return 1; /* FIXME: return 0 here? */
> >  
> >  	vector_foreach_slot (mpp->pg, pgp, i) {
> >  		if (!pgp->paths)
> > 
> I have a similar patchset in my multipath-tools tree, to allow maps
> with zero paths, too.
> However, there are quite a few places which required checking, as
> we're basically _never_ check if a pointer is NULL before accessing
> it. With a bit of luck I'll find some time to send out the patchset.
> 
> Otherwise you could have a look at
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/hare/multipath-tools.git
> branch sles11-sp1 will have the most recent stuff.

I found the following commit but it doesn't have any meaningful changes:
1e53326 Allow zero paths for device-mapper strings

Could you be more explicit on which commits you're thinking of?

Re: [RFC PATCH] multipathd: use 0 for initial pg if nr_priority_groups=0

[Hannes Reinecke]

On 02/10/2011 11:00 PM, Mike Snitzer wrote:
> On Tue, Feb 01 2011 at 10:38am -0500,
> Hannes Reinecke <hare@suse.de> wrote:
> 
>> On 01/31/2011 07:34 PM, Mike Snitzer wrote:
>>> On Mon, Jan 31 2011 at  1:09pm -0500,
>>> Moger, Babu <Babu.Moger@lsi.com> wrote:
>>>
>>>> Looks good to me. 
>>>>
>>>>> -----Original Message-----
>>>>> From: Mike Snitzer [mailto:snitzer@redhat.com]
>>>>> Sent: Monday, January 31, 2011 10:14 AM
>>>>> To: dm-devel@redhat.com
>>>>> Cc: Mike Snitzer; Moger, Babu
>>>>> Subject: [PATCH 2/2] dm mpath: allow table load with 0 priority groups
>>>>>
>>>>> If an mpath device is held open when all paths in the last priority
>>>>> group have failed userspace multipathd will attempt to reload the
>>>>> associated DM table to reflect that the device no longer has any
>>>>> priority groups.  But the reload attempt always failed because the
>>>>> multipath target did not allow 0 priority groups.
>>>>>
>>>>> Adjust multipath target to allow a table with both 0 priority groups
>>>>> and 0 for the initial priority group number.
>>>>>
>>>>> All multipath target messages related to priority group (enable_group,
>>>>> disable_group, switch_group) will properly handle a priority group of
>>>>> 0 (will cause error).
>>>>>
>>>>> When reloading a multipath table with 0 priority groups, userspace
>>>>> multipathd must be updated to specify an initial priority group number
>>>>> of 0 (rather than 1).
>>>>
>>>>  Looks like we still have some action from multipath tool. CCing Ben..
>>>
>>> Right, I looked in to the multipath-tools change.  Here is an RFC patch.
>>>
>>> Inlined "FIXME"s pose the relevant questions.  I held off on having
>>> select_path_group() return 0 when !mpp->pg because I wasn't confident
>>> that I wouldn't break multipathd in some unintuitive way.
>>>
>>> I did audit the various callers (indirect through setup_map call):
>>>
>>> update_path_groups
>>> ev_add_path
>>> ev_remove_path
>>>
>>> (all above first call setup_map)
>>>
>>> setup_map()
>>>   - mpp->bestpg = select_path_group()
>>>   - assemble_map() -- establishes mp->params
>>>
>>> do_map()
>>>
>>> Even though I verified that much I wasn't completely confident in
>>> changing select_path_group()'s defualt return of 1 -- so I localized the
>>> change to where I _knew_ it was safe (assemble_map):
>>>
>>> diff --git a/libmultipath/dmparser.c b/libmultipath/dmparser.c
>>> index 1ef3aad..97b6420 100644
>>> --- a/libmultipath/dmparser.c
>>> +++ b/libmultipath/dmparser.c
>>> @@ -52,6 +52,7 @@ assemble_map (struct multipath * mp)
>>>  	int i, j;
>>>  	int shift, freechar;
>>>  	int minio;
>>> +	int nr_priority_groups, initial_pg_nr;
>>>  	char * p;
>>>  	struct pathgroup * pgp;
>>>  	struct path * pp;
>>> @@ -60,9 +61,19 @@ assemble_map (struct multipath * mp)
>>>  	p = mp->params;
>>>  	freechar = sizeof(mp->params);
>>>  
>>> +	nr_priority_groups = VECTOR_SIZE(mp->pg);
>>> +	/*
>>> +	 * FIXME: make this conditional on multipath target version?
>>> +	 * - but no existing code is conditional on multipath target version,
>>> +	 *   nor does dm_drvprereq() store the version for code to do so.
>>> +	 * - thing is older multipath never allowed nr_priority_groups=0 so
>>> +	 *   it doesn't _really_ matter if initial_pg_nr=0 here...
>>> +	 */
>>> +	initial_pg_nr = (nr_priority_groups ? mp->bestpg : 0);
>>> +
>>>  	shift = snprintf(p, freechar, "%s %s %i %i",
>>>  			 mp->features, mp->hwhandler,
>>> -			 VECTOR_SIZE(mp->pg), mp->bestpg);
>>> +			 nr_priority_groups, initial_pg_nr);
>>>  
>>>  	if (shift >= freechar) {
>>>  		fprintf(stderr, "mp->params too small\n");
>>> diff --git a/libmultipath/switchgroup.c b/libmultipath/switchgroup.c
>>> index 025a95d..82c30ad 100644
>>> --- a/libmultipath/switchgroup.c
>>> +++ b/libmultipath/switchgroup.c
>>> @@ -41,7 +41,7 @@ select_path_group (struct multipath * mpp)
>>>  	struct pathgroup * pgp;
>>>  
>>>  	if (!mpp->pg)
>>> -		return 1;
>>> +		return 1; /* FIXME: return 0 here? */
>>>  
>>>  	vector_foreach_slot (mpp->pg, pgp, i) {
>>>  		if (!pgp->paths)
>>>
>> I have a similar patchset in my multipath-tools tree, to allow maps
>> with zero paths, too.
>> However, there are quite a few places which required checking, as
>> we're basically _never_ check if a pointer is NULL before accessing
>> it. With a bit of luck I'll find some time to send out the patchset.
>>
>> Otherwise you could have a look at
>>
>> git://git.kernel.org/pub/scm/linux/kernel/git/hare/multipath-tools.git
>> branch sles11-sp1 will have the most recent stuff.
> 
> I found the following commit but it doesn't have any meaningful changes:
> 1e53326 Allow zero paths for device-mapper strings
> 
> Could you be more explicit on which commits you're thinking of?

Ah, I just rechecked the code. There used to be quite a few missing
NULL pointer checks, especially if mpp->pg or mpp->paths are NULL.
But they seem to have been fixed now.

So ignore my objection here.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Markus Rex, HRB 16746 (AG Nürnberg)