[PATCH] User-configurable HDIO_GETGEO for dm volumes

[PATCH] User-configurable HDIO_GETGEO for dm volumes

[Darrick J. Wong]

[-- Attachment #1: Type: text/plain, Size: 1750 bytes --]

Hi everyone,

Here's a rework of last week's HDIO_GETGEO patch.  Based on all the 
feedback that I received last week, it seems that a better way to 
approach this problem is:

- Store a hd_geometry structure with each dm_table entry.
- Provide a dm getgeo method that returns that geometry (or
   -ENOTTY if there is no geometry).
- Add a dm control ioctl to set the geometry of an arbitrary dm device.
- Modify dmsetup to be able to set geometries.

This way, dmraid can associate geometries with bootable fakeraid 
devices, and dmsetup can be told to assign a geometry to a single-device 
linear/multipath setup as desired.  Furthermore, HDIO_GETGEO callers 
will go away empty-handed if the userspace config tools do not set up a 
geometry, as is the case now.  The decision to assign a geometry (and 
what that should be) is totally deferred to userspace.

So, dm-getgeo_1.patch is a patch to 2.6.16-rc3 that modifies the dm 
driver to store and retrieve geometries.  I chose to attach the 
hd_geometry structure to dm_table because it seemed like a convenient 
place to attach config data.  The only part of this patch that I think 
to be particularly dodgy is the dev_setgeo function, because I'm using 
the dm_target_msg struct to pass the user's hd_geometry into the kernel. 
  I'm not really sure if or how I'm supposed to send binary blobs into 
the dm code, though the piggyback method works adequately.  Obviously, 
this introduces a new dm control ioctl DM_DEV_SETGEO.

The second patch (device-mapper-geometry_1.patch), unsurprisingly, is a 
patch to the userspace libdevmapper/dmsetup code to enable the passing 
of hd_geometry structures to the kernel.

Questions?  Comments?

--D

Signed-off-by: Darrick J. Wong <djwong@us.ibm.com>

[-- Attachment #2: device-mapper-geometry_1.patch --]
[-- Type: text/x-patch, Size: 4163 bytes --]

diff -Naurp -x '*CVS*' device-mapper-cvs/dmsetup/dmsetup.c device-mapper/dmsetup/dmsetup.c
--- device-mapper-cvs/dmsetup/dmsetup.c	2006-02-03 06:23:22.000000000 -0800
+++ device-mapper/dmsetup/dmsetup.c	2006-02-15 10:57:56.000000000 -0800
@@ -35,6 +35,7 @@
 #include <sys/param.h>
 #include <locale.h>
 #include <langinfo.h>
+#include <linux/hdreg.h>
 
 #ifdef HAVE_SYS_IOCTL_H
 #  include <sys/ioctl.h>
@@ -508,6 +509,73 @@ static int _message(int argc, char **arg
 	return r;
 }
 
+static int _setgeo(int argc, char **argv, void *data)
+{
+	int r = 0, i;
+	struct dm_task *dmt;
+	struct hd_geometry *geo = NULL;
+	long long x;
+
+	if (!(dmt = dm_task_create(DM_DEVICE_SETGEO)))
+		return 0;
+
+	if (_switches[UUID_ARG] || _switches[MAJOR_ARG]) {
+		if (!_set_task_device(dmt, NULL, 0))
+			goto out;
+	} else {
+		if (!_set_task_device(dmt, argv[1], 0))
+			goto out;
+		argc--;
+		argv++;
+	}
+
+	for (i = 0; i < argc; i++) {
+		printf("_setgeo[%d] = %s\n", i, argv[i]);
+	}
+
+	geo = calloc(1, sizeof(*geo) + 1);
+	if (!geo)
+		goto out;
+
+	/* Now start filling out the geometry structure. */
+	x = strtoll(argv[1], NULL, 10);
+	if (x > USHRT_MAX || x < 0)
+		goto out;
+	geo->cylinders = (unsigned long)x;
+
+	x = strtoll(argv[2], NULL, 10);
+	if (x > UCHAR_MAX || x < 0)
+		goto out;
+	geo->heads = x;
+
+	x = strtoll(argv[3], NULL, 10);
+	if (x > UCHAR_MAX || x < 0)
+		goto out;
+	geo->sectors = x;
+	
+	x = strtoll(argv[4], NULL, 10);
+	if (x > UINT_MAX || x < 0)
+		goto out;
+	geo->start = x;
+
+	/* FIXME: Is piggybacking a structure as a string too evil? */
+	if (!dm_task_set_message(dmt, (char *)geo))
+		goto out;
+
+	/* run the task */
+	if (!dm_task_run(dmt))
+		goto out;
+
+	r = 1;
+
+      out:
+	if (geo)
+		free(geo);
+	dm_task_destroy(dmt);
+
+	return r;
+}
+
 static int _version(int argc, char **argv, void *data)
 {
 	char version[80];
@@ -1326,6 +1394,7 @@ static struct command _commands[] = {
 	{"mknodes", "[<device>]", 0, 1, _mknodes},
 	{"targets", "", 0, 0, _targets},
 	{"version", "", 0, 0, _version},
+	{"setgeometry", "<device> <cyl> <head> <sect> <start>", 5, 5, _setgeo},
 	{NULL, NULL, 0, 0, NULL}
 };
 
diff -Naurp -x '*CVS*' device-mapper-cvs/kernel/ioctl/dm-ioctl.h device-mapper/kernel/ioctl/dm-ioctl.h
--- device-mapper-cvs/kernel/ioctl/dm-ioctl.h	2005-10-04 13:12:32.000000000 -0700
+++ device-mapper/kernel/ioctl/dm-ioctl.h	2006-02-14 14:24:08.000000000 -0800
@@ -220,6 +220,8 @@ enum {
 	/* Added later */
 	DM_LIST_VERSIONS_CMD,
 	DM_TARGET_MSG_CMD,
+
+	DM_DEV_SETGEO_CMD
 };
 
 /*
@@ -249,6 +251,7 @@ typedef char ioctl_struct[308];
 #define DM_TABLE_STATUS_32  _IOWR(DM_IOCTL, DM_TABLE_STATUS_CMD, ioctl_struct)
 #define DM_LIST_VERSIONS_32 _IOWR(DM_IOCTL, DM_LIST_VERSIONS_CMD, ioctl_struct)
 #define DM_TARGET_MSG_32    _IOWR(DM_IOCTL, DM_TARGET_MSG_CMD, ioctl_struct)
+#define DM_DEV_SETGEO_32    _IOWR(DM_IOCTL, DM_DEV_SETGEO_CMD, ioctl_struct)
 #endif
 
 #define DM_IOCTL 0xfd
@@ -272,6 +275,7 @@ typedef char ioctl_struct[308];
 #define DM_LIST_VERSIONS _IOWR(DM_IOCTL, DM_LIST_VERSIONS_CMD, struct dm_ioctl)
 
 #define DM_TARGET_MSG	 _IOWR(DM_IOCTL, DM_TARGET_MSG_CMD, struct dm_ioctl)
+#define DM_DEV_SETGEO	 _IOWR(DM_IOCTL, DM_DEV_SETGEO_CMD, struct dm_ioctl)
 
 #define DM_VERSION_MAJOR	4
 #define DM_VERSION_MINOR	5
diff -Naurp -x '*CVS*' device-mapper-cvs/lib/ioctl/libdm-iface.c device-mapper/lib/ioctl/libdm-iface.c
--- device-mapper-cvs/lib/ioctl/libdm-iface.c	2006-02-03 06:23:22.000000000 -0800
+++ device-mapper/lib/ioctl/libdm-iface.c	2006-02-14 14:23:16.000000000 -0800
@@ -103,6 +103,9 @@ static struct cmd_data _cmd_data_v4[] = 
 #ifdef DM_TARGET_MSG
 	{"message",	DM_TARGET_MSG,		{4, 2, 0}},
 #endif
+#ifdef DM_DEV_SETGEO
+	{"setgeo",	DM_DEV_SETGEO,		{4, 2, 0}},
+#endif
 };
 /* *INDENT-ON* */
 
diff -Naurp -x '*CVS*' device-mapper-cvs/lib/libdevmapper.h device-mapper/lib/libdevmapper.h
--- device-mapper-cvs/lib/libdevmapper.h	2006-02-03 06:23:22.000000000 -0800
+++ device-mapper/lib/libdevmapper.h	2006-02-14 14:04:55.000000000 -0800
@@ -80,7 +80,9 @@ enum {
 
 	DM_DEVICE_LIST_VERSIONS,
 	
-	DM_DEVICE_TARGET_MSG
+	DM_DEVICE_TARGET_MSG,
+
+	DM_DEVICE_SETGEO
 };
 
 struct dm_task;

[-- Attachment #3: dm-getgeo_1.patch --]
[-- Type: text/x-patch, Size: 5717 bytes --]

diff -Naurp old/drivers/md/dm.c linux-2.6.16-rc3/drivers/md/dm.c
--- old/drivers/md/dm.c	2006-02-15 10:49:46.000000000 -0800
+++ linux-2.6.16-rc3/drivers/md/dm.c	2006-02-15 10:42:14.000000000 -0800
@@ -17,6 +17,7 @@
 #include <linux/mempool.h>
 #include <linux/slab.h>
 #include <linux/idr.h>
+#include <linux/hdreg.h>
 
 static const char *_name = DM_NAME;
 
@@ -225,6 +226,16 @@ static int dm_blk_close(struct inode *in
 	return 0;
 }
 
+static int dm_blk_getgeo(struct block_device *bdev, struct hd_geometry *geo)
+{
+	int ret;
+	struct mapped_device *md = bdev->bd_disk->private_data;
+
+	ret = dm_table_get_geometry(md->map, geo);
+
+	return (ret ? 0 : -ENOTTY);
+}	
+
 static inline struct dm_io *alloc_io(struct mapped_device *md)
 {
 	return mempool_alloc(md->io_pool, GFP_NOIO);
@@ -1242,6 +1253,7 @@ int dm_suspended(struct mapped_device *m
 static struct block_device_operations dm_blk_dops = {
 	.open = dm_blk_open,
 	.release = dm_blk_close,
+	.getgeo = dm_blk_getgeo,
 	.owner = THIS_MODULE
 };
 
diff -Naurp old/drivers/md/dm.h linux-2.6.16-rc3/drivers/md/dm.h
--- old/drivers/md/dm.h	2006-02-15 10:49:46.000000000 -0800
+++ linux-2.6.16-rc3/drivers/md/dm.h	2006-02-13 17:35:24.000000000 -0800
@@ -123,6 +123,8 @@ void dm_table_resume_targets(struct dm_t
 int dm_table_any_congested(struct dm_table *t, int bdi_bits);
 void dm_table_unplug_all(struct dm_table *t);
 int dm_table_flush_all(struct dm_table *t);
+int dm_table_get_geometry(struct dm_table *t, struct hd_geometry *geo);
+int dm_table_set_geometry(struct dm_table *t, struct hd_geometry *geo);
 
 /*-----------------------------------------------------------------
  * A registry of target types.
diff -Naurp old/drivers/md/dm-ioctl.c linux-2.6.16-rc3/drivers/md/dm-ioctl.c
--- old/drivers/md/dm-ioctl.c	2006-02-15 10:49:46.000000000 -0800
+++ linux-2.6.16-rc3/drivers/md/dm-ioctl.c	2006-02-15 10:51:19.000000000 -0800
@@ -690,6 +690,35 @@ static int dev_rename(struct dm_ioctl *p
 	return dm_hash_rename(param->name, new_name);
 }
 
+static int dev_setgeo(struct dm_ioctl *param, size_t param_size)
+{
+	int r = 0;
+	size_t len;
+	struct mapped_device *md;
+	struct dm_table *tbl;
+	struct dm_geometry_msg *dgm;
+	struct dm_target_msg *tmsg;
+
+	md = find_device(param);
+	if (!md)
+		return -ENXIO;
+
+	/*
+	 * Grab our output buffer.
+	 */
+	tmsg = get_result_buffer(param, param_size, &len);
+	dgm = (struct dm_geometry_msg *)tmsg->message;
+
+	tbl = dm_get_table(md);
+
+	r = dm_table_set_geometry(tbl, &dgm->geo);
+
+	dm_table_put(tbl);
+	dm_put(md);
+
+	return (r ? 0 : -EINVAL);
+}
+
 static int do_suspend(struct dm_ioctl *param)
 {
 	int r = 0;
@@ -1214,7 +1243,8 @@ static ioctl_fn lookup_ioctl(unsigned in
 
 		{DM_LIST_VERSIONS_CMD, list_versions},
 
-		{DM_TARGET_MSG_CMD, target_message}
+		{DM_TARGET_MSG_CMD, target_message},
+		{DM_DEV_SETGEO_CMD, dev_setgeo}
 	};
 
 	return (cmd >= ARRAY_SIZE(_ioctls)) ? NULL : _ioctls[cmd].fn;
diff -Naurp old/drivers/md/dm-table.c linux-2.6.16-rc3/drivers/md/dm-table.c
--- old/drivers/md/dm-table.c	2006-02-15 10:49:46.000000000 -0800
+++ linux-2.6.16-rc3/drivers/md/dm-table.c	2006-02-15 10:42:37.000000000 -0800
@@ -15,6 +15,7 @@
 #include <linux/slab.h>
 #include <linux/interrupt.h>
 #include <asm/atomic.h>
+#include <linux/hdreg.h>
 
 #define MAX_DEPTH 16
 #define NODE_SIZE L1_CACHE_BYTES
@@ -53,6 +54,9 @@ struct dm_table {
 	/* events get handed up using this callback */
 	void (*event_fn)(void *);
 	void *event_context;
+
+	/* forced geometry settings */
+	struct hd_geometry forced_geometry;
 };
 
 /*
@@ -945,6 +949,29 @@ int dm_table_flush_all(struct dm_table *
 	return ret;
 }
 
+int dm_table_get_geometry(struct dm_table *t, struct hd_geometry *geo)
+{
+	sector_t sects;
+
+	sects = t->forced_geometry.cylinders * t->forced_geometry.heads
+		* t->forced_geometry.sectors;
+
+	if (!sects) {
+		return 0;
+	}
+
+	memcpy(geo, &t->forced_geometry, sizeof(*geo));
+
+	return 1;
+}
+
+int dm_table_set_geometry(struct dm_table *t, struct hd_geometry *geo)
+{
+	memcpy(&t->forced_geometry, geo, sizeof(*geo));
+
+	return 1;
+}
+
 EXPORT_SYMBOL(dm_vcalloc);
 EXPORT_SYMBOL(dm_get_device);
 EXPORT_SYMBOL(dm_put_device);
diff -Naurp old/include/linux/dm-ioctl.h linux-2.6.16-rc3/include/linux/dm-ioctl.h
--- old/include/linux/dm-ioctl.h	2006-02-15 10:49:47.000000000 -0800
+++ linux-2.6.16-rc3/include/linux/dm-ioctl.h	2006-02-13 18:05:45.000000000 -0800
@@ -9,6 +9,7 @@
 #define _LINUX_DM_IOCTL_V4_H
 
 #include <linux/types.h>
+#include <linux/hdreg.h>
 
 #define DM_DIR "mapper"		/* Slashes not supported */
 #define DM_MAX_TYPE_NAME 16
@@ -191,6 +192,11 @@ struct dm_target_msg {
 	char message[0];
 };
 
+/* Used to force a geometry */
+struct dm_geometry_msg {
+	struct hd_geometry geo;
+};
+
 /*
  * If you change this make sure you make the corresponding change
  * to dm-ioctl.c:lookup_ioctl()
@@ -218,6 +224,7 @@ enum {
 	/* Added later */
 	DM_LIST_VERSIONS_CMD,
 	DM_TARGET_MSG_CMD,
+	DM_DEV_SETGEO_CMD,
 };
 
 /*
@@ -247,6 +254,7 @@ typedef char ioctl_struct[308];
 #define DM_TABLE_STATUS_32  _IOWR(DM_IOCTL, DM_TABLE_STATUS_CMD, ioctl_struct)
 #define DM_LIST_VERSIONS_32 _IOWR(DM_IOCTL, DM_LIST_VERSIONS_CMD, ioctl_struct)
 #define DM_TARGET_MSG_32    _IOWR(DM_IOCTL, DM_TARGET_MSG_CMD, ioctl_struct)
+#define DM_DEV_SETGEO_32    _IOWR(DM_IOCTL, DM_DEV_SETGEO_CMD, ioctl_struct)
 #endif
 
 #define DM_IOCTL 0xfd
@@ -270,6 +278,7 @@ typedef char ioctl_struct[308];
 #define DM_LIST_VERSIONS _IOWR(DM_IOCTL, DM_LIST_VERSIONS_CMD, struct dm_ioctl)
 
 #define DM_TARGET_MSG	 _IOWR(DM_IOCTL, DM_TARGET_MSG_CMD, struct dm_ioctl)
+#define DM_DEV_SETGEO	 _IOWR(DM_IOCTL, DM_DEV_SETGEO_CMD, struct dm_ioctl)
 
 #define DM_VERSION_MAJOR	4
 #define DM_VERSION_MINOR	5

Re: [PATCH] User-configurable HDIO_GETGEO for dm volumes

[Andrew Morton]

"Darrick J. Wong" <djwong@us.ibm.com> wrote:
>
> Here's a rework of last week's HDIO_GETGEO patch.  Based on all the 
>  feedback that I received last week, it seems that a better way to 
>  approach this problem is:
> 
>  - Store a hd_geometry structure with each dm_table entry.
>  - Provide a dm getgeo method that returns that geometry (or
>     -ENOTTY if there is no geometry).
>  - Add a dm control ioctl to set the geometry of an arbitrary dm device.
>  - Modify dmsetup to be able to set geometries.
> 
>  This way, dmraid can associate geometries with bootable fakeraid 
>  devices, and dmsetup can be told to assign a geometry to a single-device 
>  linear/multipath setup as desired.  Furthermore, HDIO_GETGEO callers 
>  will go away empty-handed if the userspace config tools do not set up a 
>  geometry, as is the case now.  The decision to assign a geometry (and 
>  what that should be) is totally deferred to userspace.
> 
>  So, dm-getgeo_1.patch is a patch to 2.6.16-rc3 that modifies the dm 
>  driver to store and retrieve geometries.  I chose to attach the 
>  hd_geometry structure to dm_table because it seemed like a convenient 
>  place to attach config data.  The only part of this patch that I think 
>  to be particularly dodgy is the dev_setgeo function, because I'm using 
>  the dm_target_msg struct to pass the user's hd_geometry into the kernel. 
>    I'm not really sure if or how I'm supposed to send binary blobs into 
>  the dm code, though the piggyback method works adequately.  Obviously, 
>  this introduces a new dm control ioctl DM_DEV_SETGEO.
> 
>  The second patch (device-mapper-geometry_1.patch), unsurprisingly, is a 
>  patch to the userspace libdevmapper/dmsetup code to enable the passing 
>  of hd_geometry structures to the kernel.
>
> ...
>
> diff -Naurp old/drivers/md/dm.c linux-2.6.16-rc3/drivers/md/dm.c
> --- old/drivers/md/dm.c	2006-02-15 10:49:46.000000000 -0800
> +++ linux-2.6.16-rc3/drivers/md/dm.c	2006-02-15 10:42:14.000000000 -0800
> @@ -17,6 +17,7 @@
>  #include <linux/mempool.h>
>  #include <linux/slab.h>
>  #include <linux/idr.h>
> +#include <linux/hdreg.h>
>  
>  static const char *_name = DM_NAME;
>  
> @@ -225,6 +226,16 @@ static int dm_blk_close(struct inode *in
>  	return 0;
>  }
>  
> +static int dm_blk_getgeo(struct block_device *bdev, struct hd_geometry *geo)
> +{
> +	int ret;
> +	struct mapped_device *md = bdev->bd_disk->private_data;
> +
> +	ret = dm_table_get_geometry(md->map, geo);
> +
> +	return (ret ? 0 : -ENOTTY);
> +}	

Normally kernel functions return zero on success.  So that they can return
negative errno on failure.  Is that appropriate here?  Just propagate the
dm_table_get_geometry() return value?

> +static int dev_setgeo(struct dm_ioctl *param, size_t param_size)
> +{
> +	int r = 0;
> +	size_t len;
> +	struct mapped_device *md;
> +	struct dm_table *tbl;
> +	struct dm_geometry_msg *dgm;
> +	struct dm_target_msg *tmsg;
> +
> +	md = find_device(param);
> +	if (!md)
> +		return -ENXIO;
> +
> +	/*
> +	 * Grab our output buffer.
> +	 */
> +	tmsg = get_result_buffer(param, param_size, &len);
> +	dgm = (struct dm_geometry_msg *)tmsg->message;
> +
> +	tbl = dm_get_table(md);
> +
> +	r = dm_table_set_geometry(tbl, &dgm->geo);
> +
> +	dm_table_put(tbl);
> +	dm_put(md);
> +
> +	return (r ? 0 : -EINVAL);
> +}
> +
> ...
> +int dm_table_set_geometry(struct dm_table *t, struct hd_geometry *geo)
> +{
> +	memcpy(&t->forced_geometry, geo, sizeof(*geo));
> +
> +	return 1;
> +}

That's brave - we take the hd_geometry straight from userspace without
checking anything?

Will this code dtrt if userspace is 32-bit and the kernel is 64-bit?

struct hd_geometry looks like something which different compilers could lay
out differently, perhaps even different gcc versions.  We're relying upon
the userspace representation being identical to the kernel's
representation.

It means that struct hd_geometry becomes part of the kernel ABI.  We can
never again change it and neither we (nor the compiler) can ever change its
layout.  That's dangerous.  I'd suggest that you not use hd_geometry in
this way (unless we're already using it that way, which might be the case).

It'd be better to use some carefully laid-out and documented structure
which is private to DM and which is designed for future-compatibility and
for user<->kernel communication.

Re: [PATCH] User-configurable HDIO_GETGEO for dm volumes

[Darrick J. Wong]

[-- Attachment #1: Type: text/plain, Size: 2455 bytes --]

Andrew Morton wrote:

> Normally kernel functions return zero on success.  So that they can return
> negative errno on failure.  Is that appropriate here?  Just propagate the
> dm_table_get_geometry() return value?

Fair enough, I've changed the functions to return 0 on success.  See the 
patches attached to the end.

> That's brave - we take the hd_geometry straight from userspace without
> checking anything?

My original approach didn't work anyway; libdevmapper thinks that a 
target message is a string and would stop copying at the first null it 
saw.  Since you're also concerned about being locked into a particular 
hd_geometry structure layout, I respun the patch so that dmsetup passes 
a string to the dm configuration code; now dm performs some basic 
range/sanity checks.  However, the patch doesn't check that the CHS 
values make sense or are even close to the real disk size; if somebody 
in userspace wants to configure a 150G dm device to have the same 
geometry as a 360K floppy disk, so be it.  Geometries seem to be rather 
inaccurate anyway.

Or, were you worried that I'm dereferencing a userspace pointer in the 
kernel?  The code that calls _setgeo handles that properly.

> Will this code dtrt if userspace is 32-bit and the kernel is 64-bit?

There shouldn't be any 32/64 mis-match issues with passing a string.  If 
one tries to pass too-large values, -EINVAL is returned.

> > struct hd_geometry looks like something which different compilers could lay
> out differently, perhaps even different gcc versions.  We're relying upon
> the userspace representation being identical to the kernel's
> representation.
> 
> It means that struct hd_geometry becomes part of the kernel ABI.  We can
> never again change it and neither we (nor the compiler) can ever change its
> layout.  That's dangerous.  I'd suggest that you not use hd_geometry in
> this way (unless we're already using it that way, which might be the case).

hd_geometry is already part of the kernel ABI because the HDIO_GETGEO 
ioctl takes a pointer to a struct hd_geometry in userspace and fills it 
out.  Though, now that I've changed the setgeo half to pass a string 
around, I suppose the point is partially moot.

> It'd be better to use some carefully laid-out and documented structure
> which is private to DM and which is designed for future-compatibility and
> for user<->kernel communication.

--D

Signed-off-by: Darrick J. Wong <djwong@us.ibm.com>

[-- Attachment #2: device-mapper-geometry_2.patch --]
[-- Type: text/x-patch, Size: 3645 bytes --]

diff -aurp -x '*CVS*' device-mapper-cvs/dmsetup/dmsetup.c device-mapper/dmsetup/dmsetup.c
--- device-mapper-cvs/dmsetup/dmsetup.c	2006-02-03 06:23:22.000000000 -0800
+++ device-mapper/dmsetup/dmsetup.c	2006-02-16 17:10:04.000000000 -0800
@@ -35,6 +35,7 @@
 #include <sys/param.h>
 #include <locale.h>
 #include <langinfo.h>
+#include <linux/hdreg.h>
 
 #ifdef HAVE_SYS_IOCTL_H
 #  include <sys/ioctl.h>
@@ -508,6 +509,49 @@ static int _message(int argc, char **arg
 	return r;
 }
 
+static int _setgeo(int argc, char **argv, void *data)
+{
+	int r = 0, i;
+	struct dm_task *dmt;
+	char *geo = NULL;
+
+	if (!(dmt = dm_task_create(DM_DEVICE_SETGEO)))
+		return 0;
+
+	if (_switches[UUID_ARG] || _switches[MAJOR_ARG]) {
+		if (!_set_task_device(dmt, NULL, 0))
+			goto out;
+	} else {
+		if (!_set_task_device(dmt, argv[1], 0))
+			goto out;
+		argc--;
+		argv++;
+	}
+
+	geo = malloc(strlen(argv[1]) + strlen(argv[2]) +
+		     strlen(argv[3]) + strlen(argv[4]) + 4);
+	if (!geo)
+		goto out;
+
+	r = sprintf(geo, "%s %s %s %s", argv[1], argv[2], argv[3], argv[4]);
+
+	if (!dm_task_set_message(dmt, geo))
+		goto out;
+
+	/* run the task */
+	if (!dm_task_run(dmt))
+		goto out;
+
+	r = 1;
+
+      out:
+	if (geo)
+		free(geo);
+	dm_task_destroy(dmt);
+
+	return r;
+}
+
 static int _version(int argc, char **argv, void *data)
 {
 	char version[80];
@@ -1326,6 +1370,7 @@ static struct command _commands[] = {
 	{"mknodes", "[<device>]", 0, 1, _mknodes},
 	{"targets", "", 0, 0, _targets},
 	{"version", "", 0, 0, _version},
+	{"setgeometry", "<device> <cyl> <head> <sect> <start>", 5, 5, _setgeo},
 	{NULL, NULL, 0, 0, NULL}
 };
 
diff -aurp -x '*CVS*' device-mapper-cvs/kernel/ioctl/dm-ioctl.h device-mapper/kernel/ioctl/dm-ioctl.h
--- device-mapper-cvs/kernel/ioctl/dm-ioctl.h	2005-10-04 13:12:32.000000000 -0700
+++ device-mapper/kernel/ioctl/dm-ioctl.h	2006-02-16 11:46:12.000000000 -0800
@@ -220,6 +220,7 @@ enum {
 	/* Added later */
 	DM_LIST_VERSIONS_CMD,
 	DM_TARGET_MSG_CMD,
+	DM_DEV_SETGEO_CMD
 };
 
 /*
@@ -249,6 +250,7 @@ typedef char ioctl_struct[308];
 #define DM_TABLE_STATUS_32  _IOWR(DM_IOCTL, DM_TABLE_STATUS_CMD, ioctl_struct)
 #define DM_LIST_VERSIONS_32 _IOWR(DM_IOCTL, DM_LIST_VERSIONS_CMD, ioctl_struct)
 #define DM_TARGET_MSG_32    _IOWR(DM_IOCTL, DM_TARGET_MSG_CMD, ioctl_struct)
+#define DM_DEV_SETGEO_32    _IOWR(DM_IOCTL, DM_DEV_SETGEO_CMD, ioctl_struct)
 #endif
 
 #define DM_IOCTL 0xfd
@@ -272,6 +274,7 @@ typedef char ioctl_struct[308];
 #define DM_LIST_VERSIONS _IOWR(DM_IOCTL, DM_LIST_VERSIONS_CMD, struct dm_ioctl)
 
 #define DM_TARGET_MSG	 _IOWR(DM_IOCTL, DM_TARGET_MSG_CMD, struct dm_ioctl)
+#define DM_DEV_SETGEO	 _IOWR(DM_IOCTL, DM_DEV_SETGEO_CMD, struct dm_ioctl)
 
 #define DM_VERSION_MAJOR	4
 #define DM_VERSION_MINOR	5
diff -aurp -x '*CVS*' device-mapper-cvs/lib/ioctl/libdm-iface.c device-mapper/lib/ioctl/libdm-iface.c
--- device-mapper-cvs/lib/ioctl/libdm-iface.c	2006-02-03 06:23:22.000000000 -0800
+++ device-mapper/lib/ioctl/libdm-iface.c	2006-02-14 14:23:16.000000000 -0800
@@ -103,6 +103,9 @@ static struct cmd_data _cmd_data_v4[] = 
 #ifdef DM_TARGET_MSG
 	{"message",	DM_TARGET_MSG,		{4, 2, 0}},
 #endif
+#ifdef DM_DEV_SETGEO
+	{"setgeo",	DM_DEV_SETGEO,		{4, 2, 0}},
+#endif
 };
 /* *INDENT-ON* */
 
diff -aurp -x '*CVS*' device-mapper-cvs/lib/libdevmapper.h device-mapper/lib/libdevmapper.h
--- device-mapper-cvs/lib/libdevmapper.h	2006-02-03 06:23:22.000000000 -0800
+++ device-mapper/lib/libdevmapper.h	2006-02-14 14:04:55.000000000 -0800
@@ -80,7 +80,9 @@ enum {
 
 	DM_DEVICE_LIST_VERSIONS,
 	
-	DM_DEVICE_TARGET_MSG
+	DM_DEVICE_TARGET_MSG,
+
+	DM_DEVICE_SETGEO
 };
 
 struct dm_task;

[-- Attachment #3: dm-getgeo_2.patch --]
[-- Type: text/x-patch, Size: 6343 bytes --]

diff -aurp old/drivers/md/dm.c linux-2.6.16-rc3/drivers/md/dm.c
--- old/drivers/md/dm.c	2006-02-15 10:49:46.000000000 -0800
+++ linux-2.6.16-rc3/drivers/md/dm.c	2006-02-16 10:23:54.000000000 -0800
@@ -17,6 +17,7 @@
 #include <linux/mempool.h>
 #include <linux/slab.h>
 #include <linux/idr.h>
+#include <linux/hdreg.h>
 
 static const char *_name = DM_NAME;
 
@@ -225,6 +226,13 @@ static int dm_blk_close(struct inode *in
 	return 0;
 }
 
+static int dm_blk_getgeo(struct block_device *bdev, struct hd_geometry *geo)
+{
+	struct mapped_device *md = bdev->bd_disk->private_data;
+
+	return dm_table_get_geometry(md->map, geo);
+}	
+
 static inline struct dm_io *alloc_io(struct mapped_device *md)
 {
 	return mempool_alloc(md->io_pool, GFP_NOIO);
@@ -1242,6 +1250,7 @@ int dm_suspended(struct mapped_device *m
 static struct block_device_operations dm_blk_dops = {
 	.open = dm_blk_open,
 	.release = dm_blk_close,
+	.getgeo = dm_blk_getgeo,
 	.owner = THIS_MODULE
 };
 
diff -aurp old/drivers/md/dm.h linux-2.6.16-rc3/drivers/md/dm.h
--- old/drivers/md/dm.h	2006-02-15 10:49:46.000000000 -0800
+++ linux-2.6.16-rc3/drivers/md/dm.h	2006-02-13 17:35:24.000000000 -0800
@@ -123,6 +123,8 @@ void dm_table_resume_targets(struct dm_t
 int dm_table_any_congested(struct dm_table *t, int bdi_bits);
 void dm_table_unplug_all(struct dm_table *t);
 int dm_table_flush_all(struct dm_table *t);
+int dm_table_get_geometry(struct dm_table *t, struct hd_geometry *geo);
+int dm_table_set_geometry(struct dm_table *t, struct hd_geometry *geo);
 
 /*-----------------------------------------------------------------
  * A registry of target types.
diff -aurp old/drivers/md/dm-ioctl.c linux-2.6.16-rc3/drivers/md/dm-ioctl.c
--- old/drivers/md/dm-ioctl.c	2006-02-15 10:49:46.000000000 -0800
+++ linux-2.6.16-rc3/drivers/md/dm-ioctl.c	2006-02-16 11:49:26.000000000 -0800
@@ -15,6 +15,7 @@
 #include <linux/slab.h>
 #include <linux/devfs_fs_kernel.h>
 #include <linux/dm-ioctl.h>
+#include <linux/hdreg.h>
 
 #include <asm/uaccess.h>
 
@@ -690,6 +691,51 @@ static int dev_rename(struct dm_ioctl *p
 	return dm_hash_rename(param->name, new_name);
 }
 
+static int dev_setgeo(struct dm_ioctl *param, size_t param_size)
+{
+	int r = 0, x;
+	size_t len;
+	struct mapped_device *md;
+	struct dm_table *tbl;
+	struct hd_geometry dgm;
+	struct dm_target_msg *tmsg;
+	unsigned long indata[4];
+
+	r = -ENXIO;
+
+	md = find_device(param);
+	if (!md)
+		goto out;
+
+	/*
+	 * Grab our input buffer.
+	 */
+	tmsg = get_result_buffer(param, param_size, &len);
+
+	r = -EINVAL;
+	x = sscanf(tmsg->message, "%lu %lu %lu %lu", indata, indata + 1, indata + 2, indata + 3);
+	if (x != 4)
+		goto out2;
+
+	if (indata[0] > 65535 || indata[1] > 255
+	    || indata[2] > 255 || indata[3] > ULONG_MAX)
+		goto out2;
+
+	dgm.cylinders = indata[0];
+	dgm.heads = indata[1];
+	dgm.sectors = indata[2];
+	dgm.start = indata[3];
+
+	tbl = dm_get_table(md);
+	r = dm_table_set_geometry(tbl, &dgm);
+
+	dm_table_put(tbl);
+out2:
+	dm_put(md);
+out:
+	return r;
+}
+
 static int do_suspend(struct dm_ioctl *param)
 {
 	int r = 0;
@@ -1214,7 +1260,8 @@ static ioctl_fn lookup_ioctl(unsigned in
 
 		{DM_LIST_VERSIONS_CMD, list_versions},
 
-		{DM_TARGET_MSG_CMD, target_message}
+		{DM_TARGET_MSG_CMD, target_message},
+		{DM_DEV_SETGEO_CMD, dev_setgeo}
 	};
 
 	return (cmd >= ARRAY_SIZE(_ioctls)) ? NULL : _ioctls[cmd].fn;
diff -aurp old/drivers/md/dm-table.c linux-2.6.16-rc3/drivers/md/dm-table.c
--- old/drivers/md/dm-table.c	2006-02-15 10:49:46.000000000 -0800
+++ linux-2.6.16-rc3/drivers/md/dm-table.c	2006-02-16 11:43:05.000000000 -0800
@@ -15,6 +15,7 @@
 #include <linux/slab.h>
 #include <linux/interrupt.h>
 #include <asm/atomic.h>
+#include <linux/hdreg.h>
 
 #define MAX_DEPTH 16
 #define NODE_SIZE L1_CACHE_BYTES
@@ -53,6 +54,9 @@ struct dm_table {
 	/* events get handed up using this callback */
 	void (*event_fn)(void *);
 	void *event_context;
+
+	/* forced geometry settings */
+	struct hd_geometry forced_geometry;
 };
 
 /*
@@ -945,6 +949,34 @@ int dm_table_flush_all(struct dm_table *
 	return ret;
 }
 
+int dm_table_get_geometry(struct dm_table *t, struct hd_geometry *geo)
+{
+	sector_t sects;
+
+	sects = t->forced_geometry.cylinders * t->forced_geometry.heads
+		* t->forced_geometry.sectors;
+
+	if (!sects) {
+		return -ENOTTY;
+	}
+
+	memcpy(geo, &t->forced_geometry, sizeof(*geo));
+
+	return 0;
+}
+
+int dm_table_set_geometry(struct dm_table *t, struct hd_geometry *geo)
+{
+	sector_t sz = (sector_t)geo->cylinders * geo->heads * geo->sectors;
+
+	if (geo->start > sz)
+		return -EINVAL;
+
+	memcpy(&t->forced_geometry, geo, sizeof(*geo));
+
+	return 0;
+}
+
 EXPORT_SYMBOL(dm_vcalloc);
 EXPORT_SYMBOL(dm_get_device);
 EXPORT_SYMBOL(dm_put_device);
diff -aurp old/include/linux/compat_ioctl.h linux-2.6.16-rc3/include/linux/compat_ioctl.h
--- old/include/linux/compat_ioctl.h	2006-02-15 10:49:47.000000000 -0800
+++ linux-2.6.16-rc3/include/linux/compat_ioctl.h	2006-02-16 11:48:53.000000000 -0800
@@ -151,6 +151,7 @@ COMPATIBLE_IOCTL(DM_TABLE_DEPS)
 COMPATIBLE_IOCTL(DM_TABLE_STATUS)
 COMPATIBLE_IOCTL(DM_LIST_VERSIONS)
 COMPATIBLE_IOCTL(DM_TARGET_MSG)
+COMPATIBLE_IOCTL(DM_DEV_SETGEO)
 /* Big K */
 COMPATIBLE_IOCTL(PIO_FONT)
 COMPATIBLE_IOCTL(GIO_FONT)
diff -aurp old/include/linux/dm-ioctl.h linux-2.6.16-rc3/include/linux/dm-ioctl.h
--- old/include/linux/dm-ioctl.h	2006-02-15 10:49:47.000000000 -0800
+++ linux-2.6.16-rc3/include/linux/dm-ioctl.h	2006-02-16 11:47:48.000000000 -0800
@@ -218,6 +218,7 @@ enum {
 	/* Added later */
 	DM_LIST_VERSIONS_CMD,
 	DM_TARGET_MSG_CMD,
+	DM_DEV_SETGEO_CMD
 };
 
 /*
@@ -247,6 +248,7 @@ typedef char ioctl_struct[308];
 #define DM_TABLE_STATUS_32  _IOWR(DM_IOCTL, DM_TABLE_STATUS_CMD, ioctl_struct)
 #define DM_LIST_VERSIONS_32 _IOWR(DM_IOCTL, DM_LIST_VERSIONS_CMD, ioctl_struct)
 #define DM_TARGET_MSG_32    _IOWR(DM_IOCTL, DM_TARGET_MSG_CMD, ioctl_struct)
+#define DM_DEV_SETGEO_32    _IOWR(DM_IOCTL, DM_DEV_SETGEO_CMD, ioctl_struct)
 #endif
 
 #define DM_IOCTL 0xfd
@@ -270,6 +272,7 @@ typedef char ioctl_struct[308];
 #define DM_LIST_VERSIONS _IOWR(DM_IOCTL, DM_LIST_VERSIONS_CMD, struct dm_ioctl)
 
 #define DM_TARGET_MSG	 _IOWR(DM_IOCTL, DM_TARGET_MSG_CMD, struct dm_ioctl)
+#define DM_DEV_SETGEO	 _IOWR(DM_IOCTL, DM_DEV_SETGEO_CMD, struct dm_ioctl)
 
 #define DM_VERSION_MAJOR	4
 #define DM_VERSION_MINOR	5

Re: [PATCH] User-configurable HDIO_GETGEO for dm volumes

[Andrew Morton]

"Darrick J. Wong" <djwong@us.ibm.com> wrote:
>
> ...
> > That's brave - we take the hd_geometry straight from userspace without
> > checking anything?
> 
> My original approach didn't work anyway; libdevmapper thinks that a 
> target message is a string and would stop copying at the first null it 
> saw.  Since you're also concerned about being locked into a particular 
> hd_geometry structure layout, I respun the patch so that dmsetup passes 
> a string to the dm configuration code; now dm performs some basic 
> range/sanity checks.  However, the patch doesn't check that the CHS 
> values make sense or are even close to the real disk size; if somebody 
> in userspace wants to configure a 150G dm device to have the same 
> geometry as a 360K floppy disk, so be it.  Geometries seem to be rather 
> inaccurate anyway.
> 
> Or, were you worried that I'm dereferencing a userspace pointer in the 
> kernel?  The code that calls _setgeo handles that properly.

Well, I was just asking whether you'd thought about it...

> > Will this code dtrt if userspace is 32-bit and the kernel is 64-bit?
> 
> There shouldn't be any 32/64 mis-match issues with passing a string.  If 
> one tries to pass too-large values, -EINVAL is returned.

Yup, strings work.

> > > struct hd_geometry looks like something which different compilers could lay
> > out differently, perhaps even different gcc versions.  We're relying upon
> > the userspace representation being identical to the kernel's
> > representation.
> > 
> > It means that struct hd_geometry becomes part of the kernel ABI.  We can
> > never again change it and neither we (nor the compiler) can ever change its
> > layout.  That's dangerous.  I'd suggest that you not use hd_geometry in
> > this way (unless we're already using it that way, which might be the case).
> 
> hd_geometry is already part of the kernel ABI because the HDIO_GETGEO 
> ioctl takes a pointer to a struct hd_geometry in userspace and fills it 
> out.

argh.

> 
> Signed-off-by: Darrick J. Wong <djwong@us.ibm.com>
> 

We don't seem to have a changelog any more.

Re: [PATCH] User-configurable HDIO_GETGEO for dm volumes

[Alasdair G Kergon]

On Wed, Feb 15, 2006 at 12:22:27PM -0800, Darrick J. Wong wrote:
> - Store a hd_geometry structure with each dm_table entry.

> I chose to attach the 
> hd_geometry structure to dm_table because it seemed like a convenient 
> place to attach config data.  

Given the current device-mapper code structure, I don't think
that's a good place to attach it.  Did you consider 'struct
mapped_device' instead?  In your patch, the geometry will
disappear every time the mapped device's table is reloaded.  
Userspace device-mapper applications are free to reload tables
whenever they wish, so this patch is of little value unless every
userspace device-mapper application you use is updated to support
geometries, or you write a userspace daemon to monitor the devices
for reloads and reset your preferred geometry each time...

That said, it might then be an idea for __bind() to clear
the geometry iff a non-zero device size changes.

+static int dm_blk_getgeo(struct block_device *bdev, struct hd_geometry *geo)
+{
+	struct mapped_device *md = bdev->bd_disk->private_data;
+
+	return dm_table_get_geometry(md->map, geo);
+}

And if md->map is NULL?
See above and side-step this issue by avoiding dm_table* completely?


+		{DM_DEV_SETGEO_CMD, dev_setgeo}

Naming inconsistency here:

  Currently you have a DM_TABLE_* implementation - the data
  is stored per-table not per-device.  I'm suggesting you leave
  this as DM_DEV_* but fix the implementation to match the name.

Also, every time a new ioctl is added upstream you need to increment 
the minor number (and date):

  #define DM_VERSION_MAJOR        4
- #define DM_VERSION_MINOR        5
+ #define DM_VERSION_MINOR        6
  #define DM_VERSION_PATCHLEVEL   0
- #define DM_VERSION_EXTRA        "-ioctl (2005-10-04)"
+ #define DM_VERSION_EXTRA        "-ioctl (2006-02-17)"

And please document the new ioctl briefly within dm-ioctl.h like the
existing ones.


As for how to pass the binary data, passing it as a string in the
same manner as DM_DEV_RENAME should be OK if you prefer not to
define a new binary structure for it.

+	/*
+	 * Grab our input buffer.
+	 */
+	tmsg = get_result_buffer(param, param_size, &len);

'result' here means *output* not *input*.  Calling that function here
might change the pointer that says where your input data starts 
before you've processed it!


+	if (x != 4)
+		goto out2;

+	if (indata[0] > 65535 || indata[1] > 255
+	    || indata[2] > 255 || indata[3] > ULONG_MAX)
+		goto out2;

Please issue error messages with DMWARN().

+	tbl = dm_get_table(md);
+	r = dm_table_set_geometry(tbl, &dgm);

Most device-mapper ioctls populate the 'status' fields on
return.  In this case, it doesn't make much difference because
the ioctl isn't changing any of them.  Regardless, *every* function
in _ioctls[] must set param->data_size correctly before returning.
(Set to zero in this particular case.)

[I also find it helpful if variable names are consistent between
functions e.g. 'table' rather than 'tbl']

+#define DM_DEV_SETGEO_32    _IOWR(DM_IOCTL, DM_DEV_SETGEO_CMD, ioctl_struct)

And include/linux/compat_ioctl.h?

Alasdair
-- 
agk@redhat.com

Re: [dm-devel] Re: [PATCH] User-configurable HDIO_GETGEO for dm volumes

[Alasdair G Kergon]

On Fri, Feb 17, 2006 at 03:16:50PM +0000, Alasdair G Kergon wrote:
> +static int dm_blk_getgeo(struct block_device *bdev, struct hd_geometry *geo)
> +{
> +	struct mapped_device *md = bdev->bd_disk->private_data;
> +
> +	return dm_table_get_geometry(md->map, geo);
> +}
> 
> And if md->map is NULL?

See also:

  /*
   * Everyone (including functions in this file), should use this
   * function to access the md->map field, and make sure they call
   * dm_table_put() when finished.
   */
  struct dm_table *dm_get_table(struct mapped_device *md)


Alasdair
-- 
agk@redhat.com

Re: [PATCH] User-configurable HDIO_GETGEO for dm volumes

[Darrick J. Wong]

[-- Attachment #1: Type: text/plain, Size: 624 bytes --]

Alasdair, et. al.,

Actually, I was not aware that a device could exist without a table. 
However, I suppose that geometry is a property of a device, not its 
underlying configuration, so the forced_geometry is better off in struct 
mapped_device.

Here's the third revision, with the geometry pushed into mapped_device 
as well as fixes for the problems that you pointed out wrt string 
passing, lack of warning messages, etc.  Thanks for all the great feedback!

Also, the v2 patch should have had the appropriate entries in 
include/linux/compat_ioctl.h.  Maybe it fell off?  In any case, it is 
present in this v3.

--D

[-- Attachment #2: device-mapper-geometry_3.patch --]
[-- Type: text/x-patch, Size: 4225 bytes --]

diff -aurp device-mapper-cvs/dmsetup/dmsetup.c device-mapper/dmsetup/dmsetup.c
--- device-mapper-cvs/dmsetup/dmsetup.c	2006-02-03 06:23:22.000000000 -0800
+++ device-mapper/dmsetup/dmsetup.c	2006-02-17 16:31:25.000000000 -0800
@@ -508,6 +508,49 @@ static int _message(int argc, char **arg
 	return r;
 }
 
+static int _setgeo(int argc, char **argv, void *data)
+{
+	int r = 0;
+	struct dm_task *dmt;
+	char *geo = NULL;
+
+	if (!(dmt = dm_task_create(DM_DEVICE_SETGEO)))
+		return 0;
+
+	if (_switches[UUID_ARG] || _switches[MAJOR_ARG]) {
+		if (!_set_task_device(dmt, NULL, 0))
+			goto out;
+	} else {
+		if (!_set_task_device(dmt, argv[1], 0))
+			goto out;
+		argc--;
+		argv++;
+	}
+
+	geo = malloc(strlen(argv[1]) + strlen(argv[2]) +
+		     strlen(argv[3]) + strlen(argv[4]) + 4);
+	if (!geo)
+		goto out;
+
+	r = sprintf(geo, "%s %s %s %s", argv[1], argv[2], argv[3], argv[4]);
+
+	if (!dm_task_set_newname(dmt, geo))
+		goto out;
+
+	/* run the task */
+	if (!dm_task_run(dmt))
+		goto out;
+
+	r = 1;
+
+      out:
+	if (geo)
+		free(geo);
+	dm_task_destroy(dmt);
+
+	return r;
+}
+
 static int _version(int argc, char **argv, void *data)
 {
 	char version[80];
@@ -1326,6 +1369,7 @@ static struct command _commands[] = {
 	{"mknodes", "[<device>]", 0, 1, _mknodes},
 	{"targets", "", 0, 0, _targets},
 	{"version", "", 0, 0, _version},
+	{"setgeometry", "<device> <cyl> <head> <sect> <start>", 5, 5, _setgeo},
 	{NULL, NULL, 0, 0, NULL}
 };
 
diff -aurp device-mapper-cvs/kernel/ioctl/dm-ioctl.h device-mapper/kernel/ioctl/dm-ioctl.h
--- device-mapper-cvs/kernel/ioctl/dm-ioctl.h	2005-10-04 13:12:32.000000000 -0700
+++ device-mapper/kernel/ioctl/dm-ioctl.h	2006-02-17 16:57:33.000000000 -0800
@@ -82,6 +82,17 @@
  *
  * DM_TARGET_MSG:
  * Pass a message string to the target at a specific offset of a device.
+ *
+ * DM_DEV_SETGEO:
+ * Set the geometry of a device by passing in a string.  The
+ * string should have this format:
+ *
+ * "cylinders heads sectors_per_track start_sector"
+ * 
+ * Beware that CHS geometry is nearly obsolete and only provided
+ * for compatibility with dm devices that can be booted by a PC
+ * BIOS.  See struct hd_geometry for range limits.  Also note that
+ * the geometry is erased if the device size changes.
  */
 
 /*
@@ -220,6 +231,7 @@ enum {
 	/* Added later */
 	DM_LIST_VERSIONS_CMD,
 	DM_TARGET_MSG_CMD,
+	DM_DEV_SETGEO_CMD
 };
 
 /*
@@ -249,6 +261,7 @@ typedef char ioctl_struct[308];
 #define DM_TABLE_STATUS_32  _IOWR(DM_IOCTL, DM_TABLE_STATUS_CMD, ioctl_struct)
 #define DM_LIST_VERSIONS_32 _IOWR(DM_IOCTL, DM_LIST_VERSIONS_CMD, ioctl_struct)
 #define DM_TARGET_MSG_32    _IOWR(DM_IOCTL, DM_TARGET_MSG_CMD, ioctl_struct)
+#define DM_DEV_SETGEO_32    _IOWR(DM_IOCTL, DM_DEV_SETGEO_CMD, ioctl_struct)
 #endif
 
 #define DM_IOCTL 0xfd
@@ -272,11 +285,12 @@ typedef char ioctl_struct[308];
 #define DM_LIST_VERSIONS _IOWR(DM_IOCTL, DM_LIST_VERSIONS_CMD, struct dm_ioctl)
 
 #define DM_TARGET_MSG	 _IOWR(DM_IOCTL, DM_TARGET_MSG_CMD, struct dm_ioctl)
+#define DM_DEV_SETGEO	 _IOWR(DM_IOCTL, DM_DEV_SETGEO_CMD, struct dm_ioctl)
 
 #define DM_VERSION_MAJOR	4
-#define DM_VERSION_MINOR	5
+#define DM_VERSION_MINOR	6
 #define DM_VERSION_PATCHLEVEL	0
-#define DM_VERSION_EXTRA	"-ioctl (2005-10-04)"
+#define DM_VERSION_EXTRA	"-ioctl (2006-02-17)"
 
 /* Status bits */
 #define DM_READONLY_FLAG	(1 << 0) /* In/Out */
diff -aurp device-mapper-cvs/lib/ioctl/libdm-iface.c device-mapper/lib/ioctl/libdm-iface.c
--- device-mapper-cvs/lib/ioctl/libdm-iface.c	2006-02-03 06:23:22.000000000 -0800
+++ device-mapper/lib/ioctl/libdm-iface.c	2006-02-14 14:23:16.000000000 -0800
@@ -103,6 +103,9 @@ static struct cmd_data _cmd_data_v4[] = 
 #ifdef DM_TARGET_MSG
 	{"message",	DM_TARGET_MSG,		{4, 2, 0}},
 #endif
+#ifdef DM_DEV_SETGEO
+	{"setgeo",	DM_DEV_SETGEO,		{4, 2, 0}},
+#endif
 };
 /* *INDENT-ON* */
 
diff -aurp device-mapper-cvs/lib/libdevmapper.h device-mapper/lib/libdevmapper.h
--- device-mapper-cvs/lib/libdevmapper.h	2006-02-03 06:23:22.000000000 -0800
+++ device-mapper/lib/libdevmapper.h	2006-02-14 14:04:55.000000000 -0800
@@ -80,7 +80,9 @@ enum {
 
 	DM_DEVICE_LIST_VERSIONS,
 	
-	DM_DEVICE_TARGET_MSG
+	DM_DEVICE_TARGET_MSG,
+
+	DM_DEVICE_SETGEO
 };
 
 struct dm_task;

[-- Attachment #3: dm-getgeo_3.patch --]
[-- Type: text/x-patch, Size: 7839 bytes --]

diff -aurp old/drivers/md/dm.c linux-2.6.16-rc4/drivers/md/dm.c
--- old/drivers/md/dm.c	2006-02-15 10:49:46.000000000 -0800
+++ linux-2.6.16-rc4/drivers/md/dm.c	2006-02-17 16:54:43.000000000 -0800
@@ -17,6 +17,7 @@
 #include <linux/mempool.h>
 #include <linux/slab.h>
 #include <linux/idr.h>
+#include <linux/hdreg.h>
 
 static const char *_name = DM_NAME;
 
@@ -100,6 +101,9 @@ struct mapped_device {
 	 */
 	struct super_block *frozen_sb;
 	struct block_device *suspended_bdev;
+
+	/* forced geometry settings */
+	struct hd_geometry forced_geometry;
 };
 
 #define MIN_IOS 256
@@ -225,6 +229,13 @@ static int dm_blk_close(struct inode *in
 	return 0;
 }
 
+static int dm_blk_getgeo(struct block_device *bdev, struct hd_geometry *geo)
+{
+	struct mapped_device *md = bdev->bd_disk->private_data;
+
+	return dm_get_geometry(md, geo);
+}	
+
 static inline struct dm_io *alloc_io(struct mapped_device *md)
 {
 	return mempool_alloc(md->io_pool, GFP_NOIO);
@@ -311,6 +322,41 @@ struct dm_table *dm_get_table(struct map
 	return t;
 }
 
+/*
+ * Get the geometry associated with a dm device, if one has been set.
+ */
+int dm_get_geometry(struct mapped_device *md, struct hd_geometry *geo)
+{
+	sector_t sects;
+
+	sects = (sector_t)md->forced_geometry.cylinders *
+		md->forced_geometry.heads * md->forced_geometry.sectors;
+
+	if (!sects)
+		return -ENOTTY;
+
+	memcpy(geo, &md->forced_geometry, sizeof(*geo));
+
+	return 0;
+}
+
+/*
+ * Set the geometry of a device.
+ */
+int dm_set_geometry(struct mapped_device *md, struct hd_geometry *geo)
+{
+	sector_t sz = (sector_t)geo->cylinders * geo->heads * geo->sectors;
+
+	if (geo->start > sz) {
+		DMWARN("Start sector is beyond the geometry limits!");
+		return -EINVAL;
+	}
+
+	memcpy(&md->forced_geometry, geo, sizeof(*geo));
+
+	return 0;
+}
+
 /*-----------------------------------------------------------------
  * CRUD START:
  *   A more elegant soln is in the works that uses the queue
@@ -881,10 +927,13 @@ static void __set_size(struct mapped_dev
 static int __bind(struct mapped_device *md, struct dm_table *t)
 {
 	request_queue_t *q = md->queue;
-	sector_t size;
+	sector_t size, oldsize;
 
+	oldsize = get_capacity(md->disk);
 	size = dm_table_get_size(t);
 	__set_size(md, size);
+	if (size != oldsize)
+		memset(&md->forced_geometry, 0, sizeof(md->forced_geometry));
 	if (size == 0)
 		return 0;
 
@@ -1242,6 +1291,7 @@ int dm_suspended(struct mapped_device *m
 static struct block_device_operations dm_blk_dops = {
 	.open = dm_blk_open,
 	.release = dm_blk_close,
+	.getgeo = dm_blk_getgeo,
 	.owner = THIS_MODULE
 };
 
diff -aurp old/drivers/md/dm.h linux-2.6.16-rc4/drivers/md/dm.h
--- old/drivers/md/dm.h	2006-02-15 10:49:46.000000000 -0800
+++ linux-2.6.16-rc4/drivers/md/dm.h	2006-02-17 16:23:51.000000000 -0800
@@ -14,6 +14,7 @@
 #include <linux/device-mapper.h>
 #include <linux/list.h>
 #include <linux/blkdev.h>
+#include <linux/hdreg.h>
 
 #define DM_NAME "device-mapper"
 #define DMWARN(f, x...) printk(KERN_WARNING DM_NAME ": " f "\n" , ## x)
@@ -95,6 +96,12 @@ int dm_wait_event(struct mapped_device *
 struct gendisk *dm_disk(struct mapped_device *md);
 int dm_suspended(struct mapped_device *md);
 
+/*
+ * Geometry functions.
+ */
+int dm_get_geometry(struct mapped_device *md, struct hd_geometry *geo);
+int dm_set_geometry(struct mapped_device *md, struct hd_geometry *geo);
+
 /*-----------------------------------------------------------------
  * Functions for manipulating a table.  Tables are also reference
  * counted.
diff -aurp old/drivers/md/dm-ioctl.c linux-2.6.16-rc4/drivers/md/dm-ioctl.c
--- old/drivers/md/dm-ioctl.c	2006-02-15 10:49:46.000000000 -0800
+++ linux-2.6.16-rc4/drivers/md/dm-ioctl.c	2006-02-17 16:14:24.000000000 -0800
@@ -15,6 +15,7 @@
 #include <linux/slab.h>
 #include <linux/devfs_fs_kernel.h>
 #include <linux/dm-ioctl.h>
+#include <linux/hdreg.h>
 
 #include <asm/uaccess.h>
 
@@ -690,6 +691,58 @@ static int dev_rename(struct dm_ioctl *p
 	return dm_hash_rename(param->name, new_name);
 }
 
+static int dev_setgeo(struct dm_ioctl *param, size_t param_size)
+{
+	int r = 0, x;
+	struct mapped_device *md;
+	struct hd_geometry dgm;
+	char *geostr;
+	unsigned long indata[4];
+
+	r = -ENXIO;
+
+	md = find_device(param);
+	if (!md)
+		goto out;
+
+	r = -EINVAL;
+	geostr = (char *) param + param->data_start;
+	if (geostr < (char *) (param + 1) ||
+	    invalid_str(geostr, (void *) param + param_size)) {
+		DMWARN("Invalid geometry supplied.");
+		goto out2;
+	}
+
+	x = sscanf(geostr, "%lu %lu %lu %lu", indata,
+		   indata + 1, indata + 2, indata + 3);
+
+	if (x != 4) {
+		DMWARN("Unable to interpret geometry settings.");
+		goto out2;
+	}
+
+	if (indata[0] > 65535 || indata[1] > 255 ||
+	    indata[2] > 255 || indata[3] > ULONG_MAX) {
+		DMWARN("Geometry exceeds range limits.");
+		goto out2;
+	}
+
+	dgm.cylinders = indata[0];
+	dgm.heads = indata[1];
+	dgm.sectors = indata[2];
+	dgm.start = indata[3];
+
+	r = dm_set_geometry(md, &dgm);
+
+	if (!r)
+		r = __dev_status(md, param);
+out2:
+	param->data_size = 0;
+	dm_put(md);
+out:
+	return r;
+}
+
 static int do_suspend(struct dm_ioctl *param)
 {
 	int r = 0;
@@ -1214,7 +1267,8 @@ static ioctl_fn lookup_ioctl(unsigned in
 
 		{DM_LIST_VERSIONS_CMD, list_versions},
 
-		{DM_TARGET_MSG_CMD, target_message}
+		{DM_TARGET_MSG_CMD, target_message},
+		{DM_DEV_SETGEO_CMD, dev_setgeo}
 	};
 
 	return (cmd >= ARRAY_SIZE(_ioctls)) ? NULL : _ioctls[cmd].fn;
diff -aurp old/include/linux/compat_ioctl.h linux-2.6.16-rc4/include/linux/compat_ioctl.h
--- old/include/linux/compat_ioctl.h	2006-02-15 10:49:47.000000000 -0800
+++ linux-2.6.16-rc4/include/linux/compat_ioctl.h	2006-02-16 11:48:53.000000000 -0800
@@ -151,6 +151,7 @@ COMPATIBLE_IOCTL(DM_TABLE_DEPS)
 COMPATIBLE_IOCTL(DM_TABLE_STATUS)
 COMPATIBLE_IOCTL(DM_LIST_VERSIONS)
 COMPATIBLE_IOCTL(DM_TARGET_MSG)
+COMPATIBLE_IOCTL(DM_DEV_SETGEO)
 /* Big K */
 COMPATIBLE_IOCTL(PIO_FONT)
 COMPATIBLE_IOCTL(GIO_FONT)
diff -aurp old/include/linux/dm-ioctl.h linux-2.6.16-rc4/include/linux/dm-ioctl.h
--- old/include/linux/dm-ioctl.h	2006-02-15 10:49:47.000000000 -0800
+++ linux-2.6.16-rc4/include/linux/dm-ioctl.h	2006-02-17 16:55:44.000000000 -0800
@@ -80,6 +80,17 @@
  *
  * DM_TARGET_MSG:
  * Pass a message string to the target at a specific offset of a device.
+ *
+ * DM_DEV_SETGEO:
+ * Set the geometry of a device by passing in a string.  The
+ * string should have this format:
+ *
+ * "cylinders heads sectors_per_track start_sector"
+ * 
+ * Beware that CHS geometry is nearly obsolete and only provided
+ * for compatibility with dm devices that can be booted by a PC
+ * BIOS.  See struct hd_geometry for range limits.  Also note that
+ * the geometry is erased if the device size changes.
  */
 
 /*
@@ -218,6 +229,7 @@ enum {
 	/* Added later */
 	DM_LIST_VERSIONS_CMD,
 	DM_TARGET_MSG_CMD,
+	DM_DEV_SETGEO_CMD
 };
 
 /*
@@ -247,6 +259,7 @@ typedef char ioctl_struct[308];
 #define DM_TABLE_STATUS_32  _IOWR(DM_IOCTL, DM_TABLE_STATUS_CMD, ioctl_struct)
 #define DM_LIST_VERSIONS_32 _IOWR(DM_IOCTL, DM_LIST_VERSIONS_CMD, ioctl_struct)
 #define DM_TARGET_MSG_32    _IOWR(DM_IOCTL, DM_TARGET_MSG_CMD, ioctl_struct)
+#define DM_DEV_SETGEO_32    _IOWR(DM_IOCTL, DM_DEV_SETGEO_CMD, ioctl_struct)
 #endif
 
 #define DM_IOCTL 0xfd
@@ -270,11 +283,12 @@ typedef char ioctl_struct[308];
 #define DM_LIST_VERSIONS _IOWR(DM_IOCTL, DM_LIST_VERSIONS_CMD, struct dm_ioctl)
 
 #define DM_TARGET_MSG	 _IOWR(DM_IOCTL, DM_TARGET_MSG_CMD, struct dm_ioctl)
+#define DM_DEV_SETGEO	 _IOWR(DM_IOCTL, DM_DEV_SETGEO_CMD, struct dm_ioctl)
 
 #define DM_VERSION_MAJOR	4
-#define DM_VERSION_MINOR	5
+#define DM_VERSION_MINOR	6
 #define DM_VERSION_PATCHLEVEL	0
-#define DM_VERSION_EXTRA	"-ioctl (2005-10-04)"
+#define DM_VERSION_EXTRA	"-ioctl (2006-02-17)"
 
 /* Status bits */
 #define DM_READONLY_FLAG	(1 << 0) /* In/Out */

Re: [dm-devel] Re: [PATCH] User-configurable HDIO_GETGEO for dm volumes

[Alasdair G Kergon]

On Fri, Feb 17, 2006 at 04:59:58PM -0800, Darrick J. Wong wrote:
> Actually, I was not aware that a device could exist without a table. 
> However, I suppose that geometry is a property of a device, not its 
> underlying configuration, so the forced_geometry is better off in struct 
> mapped_device.
 
There are 0, 1 or 2 tables associated with a device.  dm 
restricts which of them you can address through the ioctl
interface according to the state of the device.
Things are simpler if you can use this ioctl regardless of the state
of the tables, so a device-level one is more appropriate.

> Here's the third revision, with the geometry pushed into mapped_device 
> as well as fixes for the problems that you pointed out wrt string 
> passing, lack of warning messages, etc.  

Much better: I'll do a complete review now, fix up remaining minor things 
and get it submitted.

> Also, the v2 patch should have had the appropriate entries in 
> include/linux/compat_ioctl.h.  Maybe it fell off?  In any case, it is 
> present in this v3.
 
*32 isn't but the other *32 ones are?

[In the userspace patch where you add the ioctl to the list you need to 
use the version number which first included it i.e. 4, 6, 0 and you also
have to add it to the libdm-compat file for v1.]

Alasdair
-- 
agk@redhat.com

Re: [dm-devel] Re: [PATCH] User-configurable HDIO_GETGEO for dm volumes

[Alasdair G Kergon]

On Fri, Feb 17, 2006 at 04:59:58PM -0800, Darrick J. Wong wrote:
> Here's the third revision, with the geometry pushed into mapped_device 
> as well as fixes for the problems that you pointed out wrt string 
> passing, lack of warning messages, etc.  Thanks for all the great feedback!
 
Almost there now: how does the version below look?
Corresponding userspace changes are in device-mapper CVS.

Alasdair



From: "Darrick J. Wong" <djwong@us.ibm.com>
 
Allow drive geometry to be stored with a new DM_DEV_SET_GEOMETRY ioctl.
Device-mapper will now respond to HDIO_GETGEO.
If the geometry information is not available, zero will 
be returned for all of the parameters.

Signed-off-by: Darrick J. Wong <djwong@us.ibm.com>
Signed-off-by: Alasdair G Kergon <agk@redhat.com>

Index: linux-2.6.16-rc1/drivers/md/dm.c
===================================================================
--- linux-2.6.16-rc1.orig/drivers/md/dm.c	2006-02-22 20:44:55.000000000 +0000
+++ linux-2.6.16-rc1/drivers/md/dm.c	2006-02-22 20:44:57.000000000 +0000
@@ -17,6 +17,7 @@
 #include <linux/mempool.h>
 #include <linux/slab.h>
 #include <linux/idr.h>
+#include <linux/hdreg.h>
 
 static const char *_name = DM_NAME;
 
@@ -101,6 +102,9 @@ struct mapped_device {
 	 */
 	struct super_block *frozen_sb;
 	struct block_device *suspended_bdev;
+
+	/* forced geometry settings */
+	struct hd_geometry geometry;
 };
 
 #define MIN_IOS 256
@@ -226,6 +230,13 @@ static int dm_blk_close(struct inode *in
 	return 0;
 }
 
+static int dm_blk_getgeo(struct block_device *bdev, struct hd_geometry *geo)
+{
+	struct mapped_device *md = bdev->bd_disk->private_data;
+
+	return dm_get_geometry(md, geo);
+}
+
 static inline struct dm_io *alloc_io(struct mapped_device *md)
 {
 	return mempool_alloc(md->io_pool, GFP_NOIO);
@@ -312,6 +323,33 @@ struct dm_table *dm_get_table(struct map
 	return t;
 }
 
+/*
+ * Get the geometry associated with a dm device
+ */
+int dm_get_geometry(struct mapped_device *md, struct hd_geometry *geo)
+{
+	*geo = md->geometry;
+
+	return 0;
+}
+
+/*
+ * Set the geometry of a device.
+ */
+int dm_set_geometry(struct mapped_device *md, struct hd_geometry *geo)
+{
+	sector_t sz = (sector_t)geo->cylinders * geo->heads * geo->sectors;
+
+	if (geo->start > sz) {
+		DMWARN("Start sector is beyond the geometry limits.");
+		return -EINVAL;
+	}
+
+	md->geometry = *geo;
+
+	return 0;
+}
+
 /*-----------------------------------------------------------------
  * CRUD START:
  *   A more elegant soln is in the works that uses the queue
@@ -886,6 +924,13 @@ static int __bind(struct mapped_device *
 	sector_t size;
 
 	size = dm_table_get_size(t);
+
+	/*
+	 * Wipe any geometry if the size of the table changed.
+	 */
+	if (size != get_capacity(md->disk))
+		memset(&md->geometry, 0, sizeof(md->geometry));
+
 	__set_size(md, size);
 	if (size == 0)
 		return 0;
@@ -1238,6 +1283,7 @@ int dm_suspended(struct mapped_device *m
 static struct block_device_operations dm_blk_dops = {
 	.open = dm_blk_open,
 	.release = dm_blk_close,
+	.getgeo = dm_blk_getgeo,
 	.owner = THIS_MODULE
 };
 
Index: linux-2.6.16-rc1/drivers/md/dm.h
===================================================================
--- linux-2.6.16-rc1.orig/drivers/md/dm.h	2006-02-22 20:44:56.000000000 +0000
+++ linux-2.6.16-rc1/drivers/md/dm.h	2006-02-22 20:44:57.000000000 +0000
@@ -14,6 +14,7 @@
 #include <linux/device-mapper.h>
 #include <linux/list.h>
 #include <linux/blkdev.h>
+#include <linux/hdreg.h>
 
 #define DM_NAME "device-mapper"
 #define DMWARN(f, x...) printk(KERN_WARNING DM_NAME ": " f "\n" , ## x)
@@ -85,6 +86,12 @@ int dm_wait_event(struct mapped_device *
 struct gendisk *dm_disk(struct mapped_device *md);
 int dm_suspended(struct mapped_device *md);
 
+/*
+ * Geometry functions.
+ */
+int dm_get_geometry(struct mapped_device *md, struct hd_geometry *geo);
+int dm_set_geometry(struct mapped_device *md, struct hd_geometry *geo);
+
 /*-----------------------------------------------------------------
  * Functions for manipulating a table.  Tables are also reference
  * counted.
Index: linux-2.6.16-rc1/drivers/md/dm-ioctl.c
===================================================================
--- linux-2.6.16-rc1.orig/drivers/md/dm-ioctl.c	2006-02-22 20:44:56.000000000 +0000
+++ linux-2.6.16-rc1/drivers/md/dm-ioctl.c	2006-02-22 20:44:57.000000000 +0000
@@ -15,6 +15,7 @@
 #include <linux/slab.h>
 #include <linux/devfs_fs_kernel.h>
 #include <linux/dm-ioctl.h>
+#include <linux/hdreg.h>
 
 #include <asm/uaccess.h>
 
@@ -700,6 +701,54 @@ static int dev_rename(struct dm_ioctl *p
 	return dm_hash_rename(param->name, new_name);
 }
 
+static int dev_set_geometry(struct dm_ioctl *param, size_t param_size)
+{
+	int r = -EINVAL, x;
+	struct mapped_device *md;
+	struct hd_geometry geometry;
+	unsigned long indata[4];
+	char *geostr = (char *) param + param->data_start;
+
+	md = find_device(param);
+	if (!md)
+		return -ENXIO;
+
+	if (geostr < (char *) (param + 1) ||
+	    invalid_str(geostr, (void *) param + param_size)) {
+		DMWARN("Invalid geometry supplied.");
+		goto out;
+	}
+
+	x = sscanf(geostr, "%lu %lu %lu %lu", indata,
+		   indata + 1, indata + 2, indata + 3);
+
+	if (x != 4) {
+		DMWARN("Unable to interpret geometry settings.");
+		goto out;
+	}
+
+	if (indata[0] > 65535 || indata[1] > 255 ||
+	    indata[2] > 255 || indata[3] > ULONG_MAX) {
+		DMWARN("Geometry exceeds range limits.");
+		goto out;
+	}
+
+	geometry.cylinders = indata[0];
+	geometry.heads = indata[1];
+	geometry.sectors = indata[2];
+	geometry.start = indata[3];
+
+	r = dm_set_geometry(md, &geometry);
+	if (!r)
+		r = __dev_status(md, param);
+
+	param->data_size = 0;
+
+out:
+	dm_put(md);
+	return r;
+}
+
 static int do_suspend(struct dm_ioctl *param)
 {
 	int r = 0;
@@ -1233,7 +1282,8 @@ static ioctl_fn lookup_ioctl(unsigned in
 
 		{DM_LIST_VERSIONS_CMD, list_versions},
 
-		{DM_TARGET_MSG_CMD, target_message}
+		{DM_TARGET_MSG_CMD, target_message},
+		{DM_DEV_SET_GEOMETRY_CMD, dev_set_geometry}
 	};
 
 	return (cmd >= ARRAY_SIZE(_ioctls)) ? NULL : _ioctls[cmd].fn;
Index: linux-2.6.16-rc1/include/linux/compat_ioctl.h
===================================================================
--- linux-2.6.16-rc1.orig/include/linux/compat_ioctl.h	2006-02-22 20:42:48.000000000 +0000
+++ linux-2.6.16-rc1/include/linux/compat_ioctl.h	2006-02-22 20:44:57.000000000 +0000
@@ -136,6 +136,7 @@ COMPATIBLE_IOCTL(DM_TABLE_DEPS_32)
 COMPATIBLE_IOCTL(DM_TABLE_STATUS_32)
 COMPATIBLE_IOCTL(DM_LIST_VERSIONS_32)
 COMPATIBLE_IOCTL(DM_TARGET_MSG_32)
+COMPATIBLE_IOCTL(DM_DEV_SET_GEOMETRY_32)
 COMPATIBLE_IOCTL(DM_VERSION)
 COMPATIBLE_IOCTL(DM_REMOVE_ALL)
 COMPATIBLE_IOCTL(DM_LIST_DEVICES)
@@ -151,6 +152,7 @@ COMPATIBLE_IOCTL(DM_TABLE_DEPS)
 COMPATIBLE_IOCTL(DM_TABLE_STATUS)
 COMPATIBLE_IOCTL(DM_LIST_VERSIONS)
 COMPATIBLE_IOCTL(DM_TARGET_MSG)
+COMPATIBLE_IOCTL(DM_DEV_SET_GEOMETRY)
 /* Big K */
 COMPATIBLE_IOCTL(PIO_FONT)
 COMPATIBLE_IOCTL(GIO_FONT)
Index: linux-2.6.16-rc1/include/linux/dm-ioctl.h
===================================================================
--- linux-2.6.16-rc1.orig/include/linux/dm-ioctl.h	2006-02-22 20:42:48.000000000 +0000
+++ linux-2.6.16-rc1/include/linux/dm-ioctl.h	2006-02-22 20:44:57.000000000 +0000
@@ -80,6 +80,16 @@
  *
  * DM_TARGET_MSG:
  * Pass a message string to the target at a specific offset of a device.
+ *
+ * DM_DEV_SET_GEOMETRY:
+ * Set the geometry of a device by passing in a string in this format:
+ *
+ * "cylinders heads sectors_per_track start_sector"
+ *
+ * Beware that CHS geometry is nearly obsolete and only provided
+ * for compatibility with dm devices that can be booted by a PC
+ * BIOS.  See struct hd_geometry for range limits.  Also note that
+ * the geometry is erased if the device size changes.
  */
 
 /*
@@ -218,6 +228,7 @@ enum {
 	/* Added later */
 	DM_LIST_VERSIONS_CMD,
 	DM_TARGET_MSG_CMD,
+	DM_DEV_SET_GEOMETRY_CMD
 };
 
 /*
@@ -247,6 +258,7 @@ typedef char ioctl_struct[308];
 #define DM_TABLE_STATUS_32  _IOWR(DM_IOCTL, DM_TABLE_STATUS_CMD, ioctl_struct)
 #define DM_LIST_VERSIONS_32 _IOWR(DM_IOCTL, DM_LIST_VERSIONS_CMD, ioctl_struct)
 #define DM_TARGET_MSG_32    _IOWR(DM_IOCTL, DM_TARGET_MSG_CMD, ioctl_struct)
+#define DM_DEV_SET_GEOMETRY_32	_IOWR(DM_IOCTL, DM_DEV_SET_GEOMETRY_CMD, ioctl_struct)
 #endif
 
 #define DM_IOCTL 0xfd
@@ -270,11 +282,12 @@ typedef char ioctl_struct[308];
 #define DM_LIST_VERSIONS _IOWR(DM_IOCTL, DM_LIST_VERSIONS_CMD, struct dm_ioctl)
 
 #define DM_TARGET_MSG	 _IOWR(DM_IOCTL, DM_TARGET_MSG_CMD, struct dm_ioctl)
+#define DM_DEV_SET_GEOMETRY	_IOWR(DM_IOCTL, DM_DEV_SET_GEOMETRY_CMD, struct dm_ioctl)
 
 #define DM_VERSION_MAJOR	4
-#define DM_VERSION_MINOR	5
+#define DM_VERSION_MINOR	6
 #define DM_VERSION_PATCHLEVEL	0
-#define DM_VERSION_EXTRA	"-ioctl (2005-10-04)"
+#define DM_VERSION_EXTRA	"-ioctl (2006-02-17)"
 
 /* Status bits */
 #define DM_READONLY_FLAG	(1 << 0) /* In/Out */

Re: [dm-devel] Re: [PATCH] User-configurable HDIO_GETGEO for dm volumes

[Darrick J. Wong]

[-- Attachment #1: Type: text/plain, Size: 840 bytes --]

Looks good and tests ok, with one issue: I still have a preference for
returning -ENOTTY over 0/0/0 when dm doesn't know the geometry.  That
said, most programs will see the zero cylinder count and make a guess,
so it probably doesn't matter.

If you're happy with it, I say put it in.  Thanks for the cleanup, by
the way!

--Darrick

On Wed, 2006-02-22 at 22:32 +0000, Alasdair G Kergon wrote:
> On Fri, Feb 17, 2006 at 04:59:58PM -0800, Darrick J. Wong wrote:
> > Here's the third revision, with the geometry pushed into mapped_device 
> > as well as fixes for the problems that you pointed out wrt string 
> > passing, lack of warning messages, etc.  Thanks for all the great feedback!
>  
> Almost there now: how does the version below look?
> Corresponding userspace changes are in device-mapper CVS.
> 
> Alasdair

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 191 bytes --]

Re: [dm-devel] Re: [PATCH] User-configurable HDIO_GETGEO for dm volumes

[Alasdair G Kergon]

On Wed, Feb 22, 2006 at 04:46:57PM -0800, Darrick J. Wong wrote:
> Looks good and tests ok, with one issue: I still have a preference for
> returning -ENOTTY over 0/0/0 when dm doesn't know the geometry.  That
> said, most programs will see the zero cylinder count and make a guess,
> so it probably doesn't matter.
 
My copy of the sd(4) man page says of that ioctl:

    The information returned in the parameter is the disk
    geometry of the drive as understood by DOS!   This geometry is not
    the physical geometry of the drive.  It is used when constructing the
    drive's partition table, however, and is needed for convenient
    operation of fdisk(1), efdisk(1), and lilo(1).  If the geometry
    information is not available, zero will be returned for all of the
    parameters.
 
Is there a preferred alternative specification?

Alasdair
-- 
agk@redhat.com

Re: [dm-devel] Re: [PATCH] User-configurable HDIO_GETGEO for dm volumes

[Darrick J. Wong]

[-- Attachment #1: Type: text/plain, Size: 734 bytes --]

On Thu, 2006-02-23 at 13:56 +0000, Alasdair G Kergon wrote:

> My copy of the sd(4) man page says of that ioctl:
> 
>     The information returned in the parameter is the disk
>     geometry of the drive as understood by DOS!   This geometry is not
>     the physical geometry of the drive.  It is used when constructing the
>     drive's partition table, however, and is needed for convenient
>     operation of fdisk(1), efdisk(1), and lilo(1).  If the geometry
>     information is not available, zero will be returned for all of the
>     parameters.
>  
> Is there a preferred alternative specification?

Hm... in light of that, I'll acquiesce that 0/0/0 is fine by me, and no
other behavior is necessary.

--D

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 191 bytes --]