[PATCH] Allow multipath devices to be created for readonly devices

[PATCH] Allow multipath devices to be created for readonly devices

[Nikanth Karthikesan]

From: Hannes Reinecke <hare@suse.de>
Subject: Allow multipath devices to be created for readonly devices

Currently we cannot create device-mapper tables for multipath devices
whenever they are read-only.This patch modifies the device-mapper to
set the 'READ-ONLY' flag automatically whenever a read-only is added
to the table.

Signed-off-by: Hannes Reinecke <hare@suse.de>
Signed-off-by: Nikanth Karthikesan <knikanth@suse.de>

---

Index: linux-2.6-dm/drivers/md/dm.c
===================================================================
--- linux-2.6-dm.orig/drivers/md/dm.c
+++ linux-2.6-dm/drivers/md/dm.c
@@ -322,16 +322,26 @@ static void __exit dm_exit(void)
 static int dm_blk_open(struct block_device *bdev, fmode_t mode)
 {
 	struct mapped_device *md;
+	int retval = 0;
 
 	spin_lock(&_minor_lock);
 
 	md = bdev->bd_disk->private_data;
-	if (!md)
+	if (!md) {
+		retval = -ENXIO;
 		goto out;
+	}
 
 	if (test_bit(DMF_FREEING, &md->flags) ||
 	    test_bit(DMF_DELETING, &md->flags)) {
 		md = NULL;
+		retval = -ENXIO;
+		goto out;
+	}
+
+	if (bdev_read_only(bdev) && (mode & FMODE_WRITE)) {
+		md = NULL;
+		retval = -EROFS;
 		goto out;
 	}
 
@@ -341,7 +351,7 @@ static int dm_blk_open(struct block_devi
 out:
 	spin_unlock(&_minor_lock);
 
-	return md ? 0 : -ENXIO;
+	return retval;
 }
 
 static int dm_blk_close(struct gendisk *disk, fmode_t mode)
@@ -1948,6 +1958,14 @@ static int __bind(struct mapped_device *
 
 	__bind_mempools(md, t);
 
+	dm_get_table(md);
+	if (dm_table_get_mode(t) & FMODE_WRITE) {
+		set_disk_ro(md->disk, 0);
+	} else {
+		set_disk_ro(md->disk, 1);
+	}
+	dm_table_put(md->map);
+
 	write_lock_irqsave(&md->map_lock, flags);
 	md->map = t;
 	dm_table_set_restrictions(t, q, limits);
Index: linux-2.6-dm/drivers/md/dm-table.c
===================================================================
--- linux-2.6-dm.orig/drivers/md/dm-table.c
+++ linux-2.6-dm/drivers/md/dm-table.c
@@ -455,11 +455,19 @@ static int __table_get_device(struct dm_
 		dd->dm_dev.mode = mode;
 		dd->dm_dev.bdev = NULL;
 
-		if ((r = open_dev(dd, dev, t->md))) {
+		r = open_dev(dd, dev, t->md);
+		if (r == -EROFS) {
+			dd->dm_dev.mode &= ~FMODE_WRITE;
+			r = open_dev(dd, dev, t->md);
+		}
+		if (r) {
 			kfree(dd);
 			return r;
 		}
 
+		if (dd->dm_dev.mode != mode)
+			t->mode = dd->dm_dev.mode;
+
 		format_dev_t(dd->dm_dev.name, dev);
 
 		atomic_set(&dd->count, 0);

Re: [PATCH] Allow multipath devices to be created for readonly devices

[Alasdair G Kergon]

On Wed, Jul 08, 2009 at 02:17:59PM +0530, Nikanth Karthikesan wrote:
> Currently we cannot create device-mapper tables for multipath devices
> whenever they are read-only.This patch modifies the device-mapper to
> set the 'READ-ONLY' flag automatically whenever a read-only is added
> to the table.
 
I recall discussing this before:

1) Contrary to the assertion above, you can already create such dm
tables, by marking them explicitly as read-only.

2) The distinction between read-only and read-write device-mapper
devices is currently clear and simple.  This proposal replaces the
definitions with far-less-intuitive ones and that is why I ignored it
first time around.

If you believe the existing interface and behaviour is inadequate,
think about some alternatives:
  - What is the heart of the problem here?  Give a specific example
to show why we need improvements: Is the patch really about
optimisation, arguing that to achieve the desired result through the
current interface involves avoidable effort?
  - Consider whether adding another clearly-defined state to the system 
or extending the userspace interface would be a better approach.

Alasdair

Re: Re: [PATCH] Allow multipath devices to be created for readonly devices

[Alasdair G Kergon]

On Mon, Jul 20, 2009 at 09:08:36PM +0100, Alasdair G Kergon wrote:
> I recall discussing this before:

https://www.redhat.com/archives/dm-devel/2008-May/msg00000.html

Alasdair

Re: [PATCH] Allow multipath devices to be created for readonly devices

[Hannes Reinecke]

Alasdair G Kergon wrote:
> On Wed, Jul 08, 2009 at 02:17:59PM +0530, Nikanth Karthikesan wrote:
>> Currently we cannot create device-mapper tables for multipath devices
>> whenever they are read-only.This patch modifies the device-mapper to
>> set the 'READ-ONLY' flag automatically whenever a read-only is added
>> to the table.
>  
> I recall discussing this before:
> 
> 1) Contrary to the assertion above, you can already create such dm
> tables, by marking them explicitly as read-only.
> 
Yes, I know. The whole point of this patch is to make the interface
more userfriendly.
Or, push the implementation details down one layer into device-mapper.

Sure, it is possible to create read-only dm tables, but this requires
some prior knowledge as you have to set additional flags.

This patch actually got kicked off by a difference in behaviour
when try to mount read-only devices as read-write.

With 'normal' block devices you'll get an -EROFS, whereas on device-mapper
you get -ENXIO. The userland tools are normally equipped to handle the
former, not the latter.

The other part of the patch set links the read-only state of the device-mapper
device to the underlying devices, so when one is changed the others will
get updated, too.

> 2) The distinction between read-only and read-write device-mapper
> devices is currently clear and simple.  This proposal replaces the
> definitions with far-less-intuitive ones and that is why I ignored it
> first time around.
> 
> If you believe the existing interface and behaviour is inadequate,
> think about some alternatives:
>   - What is the heart of the problem here?  Give a specific example
> to show why we need improvements: Is the patch really about
> optimisation, arguing that to achieve the desired result through the
> current interface involves avoidable effort?
>   - Consider whether adding another clearly-defined state to the system 
> or extending the userspace interface would be a better approach.
> 
The main point here is that by changing the device-mapper code we don't
have to modify any of the userland tools. Basically what this patches
do is moving the 'special device-mapper read-only handling code' from
userland into the device-mapper core.

Yes, this is debatable. But I don't know how many userspace tools there
are, and changing all of them to handle this case correctly is a daunting
task.
Plus that it'll be 'device-mapper only' code, which I don't think is a
good idea.

To summarise: I don't object to keeping the device-mapper interface fixed,
so that any program using libdevmapper or device-mapper directly has to be
aware of this.
What I do object to is that programs using generic means (ie syscalls)
should be changed to accomodate special device-mapper handling.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Markus Rex, HRB 16746 (AG Nürnberg)