[PATCH 01/15] dm: support ioctls on mapped devices

[PATCH 01/15] dm: support ioctls on mapped devices

[Alasdair G Kergon]

From: Milan Broz <mbroz@redhat.com>
 
Extend the core device-mapper infrastructure to accept arbitrary ioctls
on a mapped device provided that it has exactly one target and it is 
capable of supporting ioctls.

[We can't use unlocked_ioctl because we need 'inode': 'file' might be NULL.
Is it worth changing this?]

Signed-off-by: Milan Broz <mbroz@redhat.com>
Signed-off-by: Alasdair G Kergon <agk@redhat.com>

Index: linux-2.6.17/drivers/md/dm.c
===================================================================
--- linux-2.6.17.orig/drivers/md/dm.c	2006-06-21 14:45:28.000000000 +0100
+++ linux-2.6.17/drivers/md/dm.c	2006-06-21 18:32:01.000000000 +0100
@@ -20,6 +20,7 @@
 #include <linux/idr.h>
 #include <linux/hdreg.h>
 #include <linux/blktrace_api.h>
+#include <linux/smp_lock.h>
 
 static const char *_name = DM_NAME;
 
@@ -257,6 +258,45 @@ static int dm_blk_getgeo(struct block_de
 	return dm_get_geometry(md, geo);
 }
 
+static int dm_blk_ioctl(struct inode *inode, struct file *file,
+			unsigned int cmd, unsigned long arg)
+{
+	struct mapped_device *md;
+	struct dm_table *map;
+	struct dm_target *tgt;
+	int r = -ENOTTY;
+
+	/* We don't really need this lock, but we do need 'inode'. */
+	unlock_kernel();
+
+	md = inode->i_bdev->bd_disk->private_data;
+
+	map = dm_get_table(md);
+
+	if (!map || !dm_table_get_size(map))
+		goto out;
+
+	/* We only support devices that have a single target */
+	if (dm_table_get_num_targets(map) != 1)
+		goto out;
+
+	tgt = dm_table_get_target(map, 0);
+
+	if (dm_suspended(md)) {
+		r = -EAGAIN;
+		goto out;
+	}
+
+	if (tgt->type->ioctl)
+		r = tgt->type->ioctl(tgt, inode, file, cmd, arg);
+
+out:
+	dm_table_put(map);
+
+	lock_kernel();
+	return r;
+}
+
 static inline struct dm_io *alloc_io(struct mapped_device *md)
 {
 	return mempool_alloc(md->io_pool, GFP_NOIO);
@@ -1348,6 +1388,7 @@ int dm_suspended(struct mapped_device *m
 static struct block_device_operations dm_blk_dops = {
 	.open = dm_blk_open,
 	.release = dm_blk_close,
+	.ioctl = dm_blk_ioctl,
 	.getgeo = dm_blk_getgeo,
 	.owner = THIS_MODULE
 };
Index: linux-2.6.17/include/linux/device-mapper.h
===================================================================
--- linux-2.6.17.orig/include/linux/device-mapper.h	2006-06-21 14:45:28.000000000 +0100
+++ linux-2.6.17/include/linux/device-mapper.h	2006-06-21 17:45:05.000000000 +0100
@@ -61,6 +61,10 @@ typedef int (*dm_status_fn) (struct dm_t
 
 typedef int (*dm_message_fn) (struct dm_target *ti, unsigned argc, char **argv);
 
+typedef int (*dm_ioctl_fn) (struct dm_target *ti, struct inode *inode,
+			    struct file *filp, unsigned int cmd,
+			    unsigned long arg);
+
 void dm_error(const char *message);
 
 /*
@@ -88,6 +92,7 @@ struct target_type {
 	dm_resume_fn resume;
 	dm_status_fn status;
 	dm_message_fn message;
+	dm_ioctl_fn ioctl;
 };
 
 struct io_restrictions {
Index: linux-2.6.17/include/linux/dm-ioctl.h
===================================================================
--- linux-2.6.17.orig/include/linux/dm-ioctl.h	2006-06-21 14:45:28.000000000 +0100
+++ linux-2.6.17/include/linux/dm-ioctl.h	2006-06-21 17:44:41.000000000 +0100
@@ -285,9 +285,9 @@ typedef char ioctl_struct[308];
 #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	6
+#define DM_VERSION_MINOR	7
 #define DM_VERSION_PATCHLEVEL	0
-#define DM_VERSION_EXTRA	"-ioctl (2006-02-17)"
+#define DM_VERSION_EXTRA	"-ioctl (2006-05-30)"
 
 /* Status bits */
 #define DM_READONLY_FLAG	(1 << 0) /* In/Out */

Re: [PATCH 01/15] dm: support ioctls on mapped devices

[Andrew Morton]

On Wed, 21 Jun 2006 20:31:21 +0100
Alasdair G Kergon <agk@redhat.com> wrote:

> From: Milan Broz <mbroz@redhat.com>
>  
> Extend the core device-mapper infrastructure to accept arbitrary ioctls
> on a mapped device provided that it has exactly one target and it is 
> capable of supporting ioctls.

I don't understand that.  We're taking an ioctl against a dm device and
we're passing it through to an underlying device?  Or something else?

Care to flesh this out a bit?

> [We can't use unlocked_ioctl because we need 'inode': 'file' might be NULL.
> Is it worth changing this?]

It _should_ be possible to use unlocked_ioctl() - unlocked_ioctl() would be
pretty useless if someone was passing it a NULL file*.  More details?

Re: [PATCH 01/15] dm: support ioctls on mapped devices

[Milan Broz]

Andrew Morton wrote:
> On Wed, 21 Jun 2006 20:31:21 +0100
> Alasdair G Kergon <agk@redhat.com> wrote:
> 
>> From: Milan Broz <mbroz@redhat.com>
>>  
>> Extend the core device-mapper infrastructure to accept arbitrary ioctls
>> on a mapped device provided that it has exactly one target and it is 
>> capable of supporting ioctls.
> 
> I don't understand that.  We're taking an ioctl against a dm device and
> we're passing it through to an underlying device?  Or something else?

Solving this situation: logical volume (say /dev/mapper/lv1) mapped in dm 
to single device (/dev/sda):

If there is need to send ioctl you must know that /dev/mapper/lv1
is mapped to /dev/sda (and use /dev/sda for ioctl).
This is dm work -  so send ioctl to /dev/mapper/lv1 directly
and let dm decide what to do.

This is supported only for single mapping. If there are more than one target
it will return -ENOTTY.

>> [We can't use unlocked_ioctl because we need 'inode': 'file' might be NULL.
>> Is it worth changing this?]
> 
> It _should_ be possible to use unlocked_ioctl() - unlocked_ioctl() would be
> pretty useless if someone was passing it a NULL file*.  More details?

yes, 
(I prefer change block code to not pass NULL and use unlocked_ioctl,
- Alasdair ?)

see

drivers/char/raw.c:
126: return blkdev_ioctl(bdev->bd_inode, *NULL*, command, arg);

and block/ioctl.c: [file = NULL here]
206: if (disk->fops->unlocked_ioctl)
207:	return disk->fops->unlocked_ioctl(*file*, cmd, arg);


-- 
Milan Broz
mbroz@redhat.com

Re: [PATCH 01/15] dm: support ioctls on mapped devices

[Andrew Morton]

On Thu, 22 Jun 2006 10:15:30 +0200
Milan Broz <mbroz@redhat.com> wrote:

> Andrew Morton wrote:
> > On Wed, 21 Jun 2006 20:31:21 +0100
> > Alasdair G Kergon <agk@redhat.com> wrote:
> > 
> >> From: Milan Broz <mbroz@redhat.com>
> >>  
> >> Extend the core device-mapper infrastructure to accept arbitrary ioctls
> >> on a mapped device provided that it has exactly one target and it is 
> >> capable of supporting ioctls.
> > 
> > I don't understand that.  We're taking an ioctl against a dm device and
> > we're passing it through to an underlying device?  Or something else?
> 
> Solving this situation: logical volume (say /dev/mapper/lv1) mapped in dm 
> to single device (/dev/sda):
> 
> If there is need to send ioctl you must know that /dev/mapper/lv1
> is mapped to /dev/sda (and use /dev/sda for ioctl).
> This is dm work -  so send ioctl to /dev/mapper/lv1 directly
> and let dm decide what to do.

OK.  I do think dm needs to remember /dev/sda's file* to get this right
though.  That's where the ->ioctl methods are.

> This is supported only for single mapping. If there are more than one target
> it will return -ENOTTY.
> 
> >> [We can't use unlocked_ioctl because we need 'inode': 'file' might be NULL.
> >> Is it worth changing this?]
> > 
> > It _should_ be possible to use unlocked_ioctl() - unlocked_ioctl() would be
> > pretty useless if someone was passing it a NULL file*.  More details?
> 
> yes, 
> (I prefer change block code to not pass NULL and use unlocked_ioctl,
> - Alasdair ?)
> 
> see
> 
> drivers/char/raw.c:
> 126: return blkdev_ioctl(bdev->bd_inode, *NULL*, command, arg);

Oh dear.  raw_open() doesn't have a file* for the device.

> and block/ioctl.c: [file = NULL here]
> 206: if (disk->fops->unlocked_ioctl)
> 207:	return disk->fops->unlocked_ioctl(*file*, cmd, arg);

Re: [PATCH 01/15] dm: support ioctls on mapped devices

[Alasdair G Kergon]

On Wed, Jun 21, 2006 at 08:52:06PM -0700, Andrew Morton wrote:
> I don't understand that.  We're taking an ioctl against a dm device and
> we're passing it through to an underlying device?  Or something else?
 
Indeed.  The motivation behind this came from multipath: people want to
issue certain types of ioctls directly against a dm multipath device and
have them pass through one of the paths to the underlying device.
(Otherwise they'd need a knowledge of dm internals to poke around the tree
of dm devices and probe path statuses to determine the correct path(s) to
use - effectively implementing 'multipath' ioctls themselves from userspace
with unavoidable races.)

Alasdair
-- 
agk@redhat.com

Re: [PATCH 01/15] dm: support ioctls on mapped devices

[Alasdair G Kergon]

On Thu, Jun 22, 2006 at 01:29:57AM -0700, Andrew Morton wrote:
> OK.  I do think dm needs to remember /dev/sda's file* to get this right
> though.  That's where the ->ioctl methods are.

> Oh dear.  raw_open() doesn't have a file* for the device.
 
Similar with device-mapper: in normal usage dm only sees major:minor.

Yes, the filp dm passes along is incorrect:

- return blkdev_driver_ioctl(bdev->bd_inode, filp, bdev->bd_disk, cmd, arg);
+ return blkdev_driver_ioctl(bdev->bd_inode, NULL, bdev->bd_disk, cmd, arg);

But should unlocked_ioctl become ?

- long (*unlocked_ioctl) (struct file *, unsigned, unsigned long);
+ long (*unlocked_ioctl) (struct inode *, struct file *, unsigned, unsigned long);

so it can be used for block devices?

See also block/scsi_ioctl.c:201 verify_command()  [scsi_cmd_ioctl]
         * file can be NULL from ioctl_by_bdev()...

Or should we be working towards eliminating interfaces that use device numbers?

Alasdair
-- 
agk@redhat.com

Re: [PATCH 01/15] dm: support ioctls on mapped devices

[Andrew Morton]

On Thu, 22 Jun 2006 16:17:21 +0100
Alasdair G Kergon <agk@redhat.com> wrote:

> On Thu, Jun 22, 2006 at 01:29:57AM -0700, Andrew Morton wrote:
> > OK.  I do think dm needs to remember /dev/sda's file* to get this right
> > though.  That's where the ->ioctl methods are.
> 
> > Oh dear.  raw_open() doesn't have a file* for the device.
>  
> Similar with device-mapper: in normal usage dm only sees major:minor.
> 
> Yes, the filp dm passes along is incorrect:
> 
> - return blkdev_driver_ioctl(bdev->bd_inode, filp, bdev->bd_disk, cmd, arg);
> + return blkdev_driver_ioctl(bdev->bd_inode, NULL, bdev->bd_disk, cmd, arg);
> 
> But should unlocked_ioctl become ?
> 
> - long (*unlocked_ioctl) (struct file *, unsigned, unsigned long);
> + long (*unlocked_ioctl) (struct inode *, struct file *, unsigned, unsigned long);
> 
> so it can be used for block devices?

Perhaps it should (have).  It's a bit nasty, but we do have at least two
internal callers who don't have a file*.

The alternative would be to cook up a fake file* like blkdev_get() does,
but we don't want to propagate that practice.

Oh well.  I suppose a lock_kernel() won't kill us.

> See also block/scsi_ioctl.c:201 verify_command()  [scsi_cmd_ioctl]
>          * file can be NULL from ioctl_by_bdev()...
> 
> Or should we be working towards eliminating interfaces that use device numbers?

If possible.  I guess that would require DM to track the devices with
file*'s or inode*'s or bdev*'s.  Which, I assume, would be non-trivial.

Re: [PATCH 01/15] dm: support ioctls on mapped devices

[Kevin Corry]

On Thu June 22 2006 11:55 am, Andrew Morton wrote:
> On Thu, 22 Jun 2006 16:17:21 +0100 Alasdair G Kergon <agk@redhat.com> wrote:
> > On Thu, Jun 22, 2006 at 01:29:57AM -0700, Andrew Morton wrote:
> > See also block/scsi_ioctl.c:201 verify_command()  [scsi_cmd_ioctl]
> >          * file can be NULL from ioctl_by_bdev()...
> >
> > Or should we be working towards eliminating interfaces that use device
> > numbers?
>
> If possible.  I guess that would require DM to track the devices with
> file*'s or inode*'s or bdev*'s.  Which, I assume, would be non-trivial.

There already is a bdev pointer available. Each "consumed" device get a struct 
dm_dev, which has a *bdev field. From the bdev, it looks like we should be 
able to get to the gendisk, then the block_device_operations, and then the 
ioctl routine (if it exists). Correct?

-- 
Kevin Corry
kevcorry@us.ibm.com
http://www.ibm.com/linux/
http://evms.sourceforge.net/

Re: [PATCH 01/15] dm: support ioctls on mapped devices

[Andrew Morton]

On Thu, 22 Jun 2006 22:31:16 -0500
Kevin Corry <kevcorry@us.ibm.com> wrote:

> On Thu June 22 2006 11:55 am, Andrew Morton wrote:
> > On Thu, 22 Jun 2006 16:17:21 +0100 Alasdair G Kergon <agk@redhat.com> wrote:
> > > On Thu, Jun 22, 2006 at 01:29:57AM -0700, Andrew Morton wrote:
> > > See also block/scsi_ioctl.c:201 verify_command()  [scsi_cmd_ioctl]
> > >          * file can be NULL from ioctl_by_bdev()...
> > >
> > > Or should we be working towards eliminating interfaces that use device
> > > numbers?
> >
> > If possible.  I guess that would require DM to track the devices with
> > file*'s or inode*'s or bdev*'s.  Which, I assume, would be non-trivial.
> 
> There already is a bdev pointer available. Each "consumed" device get a struct 
> dm_dev, which has a *bdev field. From the bdev, it looks like we should be 
> able to get to the gendisk, then the block_device_operations, and then the 
> ioctl routine (if it exists). Correct?
> 

My head is spinning in a twisty maze of ioctls.  What _should_ we call? 
file_operations.foo_ioctl() or block_device_operations.foo_ioctl() or
blkdev_ioctl()?

I think as far as the user is concerned, file_operations.foo_ioctl(),
because that's what the user would end up calling against /dev/sda. 
Whether that's always, reliably, equivalent to
block_device_operations.foo_ioctl() I am presently disinclined to spare
time to discover.

Re: [PATCH 01/15] dm: support ioctls on mapped devices

[Christoph Hellwig]

On Thu, Jun 22, 2006 at 09:55:51AM -0700, Andrew Morton wrote:
> > - long (*unlocked_ioctl) (struct file *, unsigned, unsigned long);
> > + long (*unlocked_ioctl) (struct inode *, struct file *, unsigned, unsigned long);
> > 
> > so it can be used for block devices?
> 
> Perhaps it should (have).  It's a bit nasty, but we do have at least two
> internal callers who don't have a file*.
> 
> The alternative would be to cook up a fake file* like blkdev_get() does,
> but we don't want to propagate that practice.

Faking up the file struct is the only viable short-term option.  It
should be done in ioctl_by_bdev which every kernel blockdevice ioctl
user should use.  Long-term we should not pass a struct file but
a struct block_device *, but braindamage in floppy.c prevents that.

Re: [PATCH 01/15] dm: support ioctls on mapped devices

[Christoph Hellwig]

On Thu, Jun 22, 2006 at 08:49:07PM -0700, Andrew Morton wrote:
> My head is spinning in a twisty maze of ioctls.  What _should_ we call? 
> file_operations.foo_ioctl() or block_device_operations.foo_ioctl() or
> blkdev_ioctl()?

Neither.  The only valid way to perform ioctls on blockdevices from
kernelspace is ioctl_by_bdev.

Re: [PATCH 01/15] dm: support ioctls on mapped devices

[Milan Broz]

Andrew Morton wrote:
> On Fri, 23 Jun 2006 12:00:18 +0200
> Christoph Hellwig <hch@lst.de> wrote:
> 
>> On Thu, Jun 22, 2006 at 09:55:51AM -0700, Andrew Morton wrote:
>>>> - long (*unlocked_ioctl) (struct file *, unsigned, unsigned long);
>>>> + long (*unlocked_ioctl) (struct inode *, struct file *, unsigned, unsigned long);
>>>>
>>>> so it can be used for block devices?
>>> Perhaps it should (have).  It's a bit nasty, but we do have at least two
>>> internal callers who don't have a file*.
>>>
>>> The alternative would be to cook up a fake file* like blkdev_get() does,
>>> but we don't want to propagate that practice.
>> Faking up the file struct is the only viable short-term option.  It
>> should be done in ioctl_by_bdev which every kernel blockdevice ioctl
>> user should use.  Long-term we should not pass a struct file but
>> a struct block_device *, but braindamage in floppy.c prevents that.
> 
> Ho hum.  The short-term will continue to be long-term.
> 
> Milan, I've lost the plot on where we stand on these patches.  I think some
> rework is needed, yes?

yes, 
we need at least create fake file.

Milan

Re: [PATCH 01/15] dm: support ioctls on mapped devices

[Arnd Bergmann]

Am Wednesday 21 June 2006 21:31 schrieb Alasdair G Kergon:
>  static struct block_device_operations dm_blk_dops = {
>         .open = dm_blk_open,
>         .release = dm_blk_close,
> +       .ioctl = dm_blk_ioctl,
>         .getgeo = dm_blk_getgeo,
>         .owner = THIS_MODULE

I guess this also needs a ->compat_ioctl method, otherwise it won't
work for ioctl numbers that have a compat_ioctl implementation in the
low-level device driver.

	Arnd <><