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 <[email protected]>
"Darrick J. Wong" <[email protected]> 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.
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 <[email protected]>
"Darrick J. Wong" <[email protected]> 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 <[email protected]>
>
We don't seem to have a changelog any more.
<snip>
Just on a side note: I'm currently working on a patch to make disk
geometry information available through sysfs (for all those devices that
provide getgeo) and and extended patch to make it writable through the
same interface by introducing a setgeo function as a companion to getgeo.
Perhaps this'll make some of this easier...
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
--
[email protected]
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
--
[email protected]
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
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
--
[email protected]
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" <[email protected]>
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 <[email protected]>
Signed-off-by: Alasdair G Kergon <[email protected]>
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 */
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
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
--
[email protected]
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