From: Alasdair G Kergon <agk@redhat.com>
To: "Darrick J. Wong" <djwong@us.ibm.com>
Cc: dm-devel@redhat.com, Chris McDermott <lcm@us.ibm.com>,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH] User-configurable HDIO_GETGEO for dm volumes
Date: Fri, 17 Feb 2006 15:16:50 +0000 [thread overview]
Message-ID: <20060217151650.GC12173@agk.surrey.redhat.com> (raw)
In-Reply-To: <43F38D83.3040702@us.ibm.com>
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
next prev parent reply other threads:[~2006-02-17 15:17 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2006-02-15 20:22 [PATCH] User-configurable HDIO_GETGEO for dm volumes Darrick J. Wong
2006-02-16 3:05 ` Andrew Morton
2006-02-17 1:31 ` Darrick J. Wong
2006-02-17 1:49 ` Andrew Morton
2006-02-17 15:16 ` Alasdair G Kergon [this message]
2006-02-17 15:21 ` [dm-devel] " Alasdair G Kergon
2006-02-18 0:59 ` Darrick J. Wong
2006-02-19 1:25 ` [dm-devel] " Alasdair G Kergon
2006-02-22 22:32 ` Alasdair G Kergon
2006-02-23 0:46 ` Darrick J. Wong
2006-02-23 13:56 ` Alasdair G Kergon
2006-02-23 18:16 ` Darrick J. Wong
2006-02-17 8:14 Seewer Philippe
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20060217151650.GC12173@agk.surrey.redhat.com \
--to=agk@redhat.com \
--cc=djwong@us.ibm.com \
--cc=dm-devel@redhat.com \
--cc=lcm@us.ibm.com \
--cc=linux-kernel@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).