[LVM2 PATCH] libdm: Fail to add tree node when requested major/minor is used.

[LVM2 PATCH] libdm: Fail to add tree node when requested major/minor is used.

[Milan Broz]

Fail to add tree node when requested major/minor is used.

(_info_by_dev() is just moved for definition before use here)

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=204992

Signed-off-by: Milan Broz <mbroz@redhat.com>
---
 libdm/libdm-deptree.c |   64 ++++++++++++++++++++++++++++----------------------
 1 file changed, 36 insertions(+), 28 deletions(-)

Index: LVM2/libdm/libdm-deptree.c
===================================================================
--- LVM2.orig/libdm/libdm-deptree.c	2008-12-17 16:28:33.000000000 +0100
+++ LVM2/libdm/libdm-deptree.c	2008-12-17 17:39:51.000000000 +0100
@@ -544,6 +544,35 @@ static int _node_clear_table(struct dm_t
 	return r;
 }
 
+static int _info_by_dev(uint32_t major, uint32_t minor, int with_open_count,
+			struct dm_info *info)
+{
+	struct dm_task *dmt;
+	int r;
+
+	if (!(dmt = dm_task_create(DM_DEVICE_INFO))) {
+		log_error("_info_by_dev: dm_task creation failed");
+		return 0;
+	}
+
+	if (!dm_task_set_major(dmt, major) || !dm_task_set_minor(dmt, minor)) {
+		log_error("_info_by_dev: Failed to set device number");
+		dm_task_destroy(dmt);
+		return 0;
+	}
+
+	if (!with_open_count && !dm_task_no_open_count(dmt))
+		log_error("Failed to disable open_count");
+
+	if ((r = dm_task_run(dmt)))
+		r = dm_task_get_info(dmt, info);
+
+	dm_task_destroy(dmt);
+
+	return r;
+}
+
+
 struct dm_tree_node *dm_tree_add_new_dev(struct dm_tree *dtree,
 					    const char *name,
 					    const char *uuid,
@@ -559,6 +588,13 @@ struct dm_tree_node *dm_tree_add_new_dev
 
 	/* Do we need to add node to tree? */
 	if (!(dnode = dm_tree_find_node_by_uuid(dtree, uuid))) {
+
+		if (major && minor && _info_by_dev(major, minor, 0, &info) && info.exists) {
+			log_error("Requested major:minor %i:%i number is already used.",
+				  major, minor);
+			return NULL;
+		}
+
 		if (!(name2 = dm_pool_strdup(dtree->mem, name))) {
 			log_error("name pool_strdup failed");
 			return NULL;
@@ -778,34 +814,6 @@ struct dm_tree_node *dm_tree_next_child(
 /*
  * Deactivate a device with its dependencies if the uuid prefix matches.
  */
-static int _info_by_dev(uint32_t major, uint32_t minor, int with_open_count,
-			struct dm_info *info)
-{
-	struct dm_task *dmt;
-	int r;
-
-	if (!(dmt = dm_task_create(DM_DEVICE_INFO))) {
-		log_error("_info_by_dev: dm_task creation failed");
-		return 0;
-	}
-
-	if (!dm_task_set_major(dmt, major) || !dm_task_set_minor(dmt, minor)) {
-		log_error("_info_by_dev: Failed to set device number");
-		dm_task_destroy(dmt);
-		return 0;
-	}
-
-	if (!with_open_count && !dm_task_no_open_count(dmt))
-		log_error("Failed to disable open_count");
-
-	if ((r = dm_task_run(dmt)))
-		r = dm_task_get_info(dmt, info);
-
-	dm_task_destroy(dmt);
-
-	return r;
-}
-
 static int _deactivate_node(const char *name, uint32_t major, uint32_t minor)
 {
 	struct dm_task *dmt;

[LVM2 PATCH] libdm: Fail to add tree node when requested major/minor is used.

[Dave Wysochanski]

On Wed, 2008-12-17 at 18:03 +0100, Milan Broz wrote:
> Fail to add tree node when requested major/minor is used.
> 
> (_info_by_dev() is just moved for definition before use here)
> 
> Fixes https://bugzilla.redhat.com/show_bug.cgi?id=204992
> 
> Signed-off-by: Milan Broz <mbroz@redhat.com>
> ---
>  libdm/libdm-deptree.c |   64 ++++++++++++++++++++++++++++----------------------
>  1 file changed, 36 insertions(+), 28 deletions(-)
> 
> Index: LVM2/libdm/libdm-deptree.c
> ===================================================================
> --- LVM2.orig/libdm/libdm-deptree.c	2008-12-17 16:28:33.000000000 +0100
> +++ LVM2/libdm/libdm-deptree.c	2008-12-17 17:39:51.000000000 +0100
> @@ -544,6 +544,35 @@ static int _node_clear_table(struct dm_t
>  	return r;
>  }
>  
> +static int _info_by_dev(uint32_t major, uint32_t minor, int with_open_count,
> +			struct dm_info *info)
> +{
> +	struct dm_task *dmt;
> +	int r;
> +
> +	if (!(dmt = dm_task_create(DM_DEVICE_INFO))) {
> +		log_error("_info_by_dev: dm_task creation failed");
> +		return 0;
> +	}
> +
> +	if (!dm_task_set_major(dmt, major) || !dm_task_set_minor(dmt, minor)) {
> +		log_error("_info_by_dev: Failed to set device number");
> +		dm_task_destroy(dmt);
> +		return 0;
> +	}
> +
> +	if (!with_open_count && !dm_task_no_open_count(dmt))
> +		log_error("Failed to disable open_count");
> +
> +	if ((r = dm_task_run(dmt)))
> +		r = dm_task_get_info(dmt, info);
> +
> +	dm_task_destroy(dmt);
> +
> +	return r;
> +}
> +
> +
>  struct dm_tree_node *dm_tree_add_new_dev(struct dm_tree *dtree,
>  					    const char *name,
>  					    const char *uuid,
> @@ -559,6 +588,13 @@ struct dm_tree_node *dm_tree_add_new_dev
>  
>  	/* Do we need to add node to tree? */
>  	if (!(dnode = dm_tree_find_node_by_uuid(dtree, uuid))) {
> +
> +		if (major && minor && _info_by_dev(major, minor, 0, &info) && info.exists) {
> +			log_error("Requested major:minor %i:%i number is already used.",
> +				  major, minor);
> +			return NULL;
> +		}
> +


Looks to me like this condition rarely/never is true because major is
always 253 for the devices created but the _info_by_dev() call uses the
major value on the cmdline.  When we create the device the creation
often still fails without explaining the real reason:
# tools/lvm lvcreate -n linear -My --major 200 --minor 75 -L 50M vgtest
  Rounding up size to full physical extent 52.00 MB
  device-mapper: create ioctl failed: No such device or address
  Aborting. Failed to activate new LV to wipe the start of it.
# tools/lvm lvcreate -n linear -My --major 253 --minor 75 -L 50M vgtest
  Rounding up size to full physical extent 52.00 MB
  Requested major:minor 253:75 number is already used.
  Aborting. Failed to activate new LV to wipe the start of it.


Should we be ignoring --major on the lvcreate cmdline, overriding it, or
is there another bug?

[LVM2 PATCH] libdm: Fail to add tree node when requested major/minor is used.

[Milan Broz]

Dave Wysochanski wrote:
> Should we be ignoring --major on the lvcreate cmdline, overriding it, or
> is there another bug?

That's another bug - in 2.6 kernel is setting of major number not supported yet.
(It worked in lvm1 & 2.4)

See https://bugzilla.redhat.com/show_bug.cgi?id=204997

But for metadata we need store both...

Milan

[LVM2 PATCH] libdm: Fail to add tree node when requested major/minor is used.

[Dave Wysochanski]

On Wed, 2008-12-17 at 20:12 +0100, Milan Broz wrote:
> Dave Wysochanski wrote:
> > Should we be ignoring --major on the lvcreate cmdline, overriding it, or
> > is there another bug?
> 
> That's another bug - in 2.6 kernel is setting of major number not supported yet.
> (It worked in lvm1 & 2.4)
> 
> See https://bugzilla.redhat.com/show_bug.cgi?id=204997
> 
> But for metadata we need store both...

Ok - your patch seems fine for the bz you reported.

Ack.

[LVM2 PATCH v2] libdm: Fail to add tree node when requested major/minor is used.

[Milan Broz]

Fail add tree node when requested major/minor is used.

After discussion with Alasdair I tried another approach,
check is added in _add_dev_to_dtree() where we already read info by uuid,
so in the case of requesting major/minor it queries device-mapper
by major/minor for device availability.

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=204992

Signed-off-by: Milan Broz <mbroz@redhat.com>

---
 lib/activate/dev_manager.c |   51 +++++++++++++++++++++++++++++++++++++--------
 1 file changed, 42 insertions(+), 9 deletions(-)

Index: LVM2/lib/activate/dev_manager.c
===================================================================
--- LVM2.orig/lib/activate/dev_manager.c	2008-12-18 15:42:53.000000000 +0100
+++ LVM2/lib/activate/dev_manager.c	2008-12-18 19:03:45.000000000 +0100
@@ -93,7 +93,8 @@ static int _read_only_lv(struct logical_
  * Low level device-layer operations.
  */
 static struct dm_task *_setup_task(const char *name, const char *uuid,
-				   uint32_t *event_nr, int task)
+				   uint32_t *event_nr, int task,
+				   uint32_t major, uint32_t minor)
 {
 	struct dm_task *dmt;
 
@@ -109,12 +110,17 @@ static struct dm_task *_setup_task(const
 	if (event_nr)
 		dm_task_set_event_nr(dmt, *event_nr);
 
+	if (major) {
+		dm_task_set_major(dmt, major);
+		dm_task_set_minor(dmt, minor);
+	}
+
 	return dmt;
 }
 
 static int _info_run(const char *name, const char *dlid, struct dm_info *info,
 		     uint32_t *read_ahead, int mknodes, int with_open_count,
-		     int with_read_ahead)
+		     int with_read_ahead, uint32_t major, uint32_t minor)
 {
 	int r = 0;
 	struct dm_task *dmt;
@@ -122,7 +128,7 @@ static int _info_run(const char *name, c
 
 	dmtask = mknodes ? DM_DEVICE_MKNODES : DM_DEVICE_INFO;
 
-	if (!(dmt = _setup_task(name, dlid, 0, dmtask)))
+	if (!(dmt = _setup_task(name, dlid, 0, dmtask, major, minor)))
 		return_0;
 
 	if (!with_open_count)
@@ -206,23 +212,29 @@ static int _info(const char *name, const
 {
 	if (!mknodes && dlid && *dlid) {
 		if (_info_run(NULL, dlid, info, read_ahead, 0, with_open_count,
-			      with_read_ahead) &&
+			      with_read_ahead, 0, 0) &&
 	    	    info->exists)
 			return 1;
 		else if (_info_run(NULL, dlid + sizeof(UUID_PREFIX) - 1, info,
 				   read_ahead, 0, with_open_count,
-				   with_read_ahead) &&
+				   with_read_ahead, 0, 0) &&
 			 info->exists)
 			return 1;
 	}
 
 	if (name)
 		return _info_run(name, NULL, info, read_ahead, mknodes,
-				 with_open_count, with_read_ahead);
+				 with_open_count, with_read_ahead, 0, 0);
 
 	return 0;
 }
 
+static int _info_by_dev(const char *name, uint32_t major, uint32_t minor,
+			struct dm_info *info)
+{
+	return _info_run(name, NULL, info, NULL, 0, 0, 0, major, minor);
+}
+
 int dev_manager_info(struct dm_pool *mem, const char *name,
 		     const struct logical_volume *lv, int with_mknodes,
 		     int with_open_count, int with_read_ahead,
@@ -252,7 +264,7 @@ static int _status_run(const char *name,
 	char *type = NULL;
 	char *params = NULL;
 
-	if (!(dmt = _setup_task(name, uuid, 0, DM_DEVICE_STATUS)))
+	if (!(dmt = _setup_task(name, uuid, 0, DM_DEVICE_STATUS, 0, 0)))
 		return_0;
 
 	if (!dm_task_no_open_count(dmt))
@@ -339,7 +351,7 @@ static int _percent_run(struct dev_manag
 	*percent = -1;
 
 	if (!(dmt = _setup_task(name, dlid, event_nr,
-				wait ? DM_DEVICE_WAITEVENT : DM_DEVICE_STATUS)))
+				wait ? DM_DEVICE_WAITEVENT : DM_DEVICE_STATUS, 0, 0)))
 		return_0;
 
 	if (!dm_task_no_open_count(dmt))
@@ -610,7 +622,7 @@ static int _add_dev_to_dtree(struct dev_
 			       struct logical_volume *lv, const char *layer)
 {
 	char *dlid, *name;
-	struct dm_info info;
+	struct dm_info info, info2;
 
 	if (!(name = build_dm_name(dm->mem, lv->vg->name, lv->name, layer)))
 		return_0;
@@ -624,6 +636,27 @@ static int _add_dev_to_dtree(struct dev_
 		return 0;
 	}
 
+	/*
+	 * For top level volumes verify that existing device match
+	 * requested major/minor and that major/minor pair is available for use
+	 */
+	if (!layer && lv->major != -1 && lv->minor != -1) {
+		if (info.exists && (info.major != lv->major || info.minor != lv->minor)) {
+			log_error("Volume %s (%" PRIu32 ":%" PRIu32")"
+				  " differs from already active device "
+				  "(%" PRIu32 ":%" PRIu32")",
+				  lv->name, lv->major, lv->minor, info.major, info.minor);
+			return 0;
+		}
+		if (!info.exists && _info_by_dev(NULL, lv->major, lv->minor, &info2) &&
+		    info2.exists) {
+			log_error("The requested major:minor pair "
+				  "(%" PRIu32 ":%" PRIu32") is already used",
+				  lv->major, lv->minor);
+			return 0;
+		}
+	}
+
 	if (info.exists && !dm_tree_add_dev(dtree, info.major, info.minor)) {
 		log_error("Failed to add device (%" PRIu32 ":%" PRIu32") to dtree",
 			  info.major, info.minor);

[LVM2 PATCH v2] libdm: Fail to add tree node when requested major/minor is used.

[Alasdair G Kergon]

On Thu, Dec 18, 2008 at 08:59:47PM +0100, Milan Broz wrote:
> Fail add tree node when requested major/minor is used.
 
Ack.

Alasdair
-- 
agk at redhat.com