Skip to contentSkip to conversation

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

PATCH New Milan Broz 3 participants 6 messages Dec 17, 2008 – Dec 19, 2008

lore
Share & tools
lore

Milan Broz

author
#0 lore raw

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

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

---
 libdm/libdm-deptree.c |   64 ++++++++++++++++++++++++++++----------------------
 1 file changed, 36 insertions(+), 28 deletions(-)
libdm/libdm-deptree.c +36 -28
Index: LVM2/libdm/libdm-deptree.c
===================================================================
@@ -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;
#1 lore raw

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

Quoted 52 lines · 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

---
 libdm/libdm-deptree.c |   64 ++++++++++++++++++++++++++++----------------------
 1 file changed, 36 insertions(+), 28 deletions(-)

Index: LVM2/libdm/libdm-deptree.c
===================================================================

libdm/libdm-deptree.c +29 -0
@@ -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,

libdm/libdm-deptree.c struct dm_tree_node *dm_tree_add_new_dev · line 597

Quoted 10 lines
libdm/libdm-deptree.c +7 -0
@@ -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?

author
#2 lore raw

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

Quoted 2 lines · Dave Wysochanski

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

#3 lore raw

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

Quoted 10 lines · Milan Broz
Quoted 2 lines · Dave Wysochanski

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.

author
#4 lore raw

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

In reply to 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

---
 lib/activate/dev_manager.c |   51 +++++++++++++++++++++++++++++++++++++--------
 1 file changed, 42 insertions(+), 9 deletions(-)
lib/activate/dev_manager.c +42 -9
Index: LVM2/lib/activate/dev_manager.c
===================================================================
@@ -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);
#5 lore raw

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

Quoted 1 line · Milan Broz

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

Ack.

Alasdair

Signature · 1 line
agk at redhat.com