switch DM tables to readonly automatically

Hi all,

this patch switches the device-mapper table to read-only status automatically if one underlying device returns -EROFS.

Rationale:
Whenever a SCSI device is switched to read-only a table reload from multipath-tools fails, without any indication about the reason. And it's actually quite tricky to detect the read-only status from userland. And quite pointless, too, as the kernel already knows about it.

And we now can create tables for CD-ROMs, too, without having to use the '-r' flag to dmsetup ...

Christophe, this might also fix your problem.

As usual, comments etc are welcome.

Cheers,

Hannes

Dr. Hannes Reinecke		      zSeries & Storage
[email protected]			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Markus Rex, HRB 16746 (AG Nürnberg)

index e75b143..f615e85 100644
@@ -501,11 +501,19 @@ static int __table_get_device(struct dm_table *t, struct dm_target *ti,
 		dd->mode = mode;
 		dd->bdev = NULL;
 
-		if ((r = open_dev(dd, dev, t->md))) {
+		r = open_dev(dd, dev, t->md);
+		if (r == -EROFS) {
+			dd->mode &= ~FMODE_WRITE;
+			r = open_dev(dd, dev, t->md);
+		}
+		if (r) {
 			kfree(dd);
 			return r;
 		}
 
+		if (dd->mode != mode)
+			t->mode = dd->mode;
+
 		format_dev_t(dd->name, dev);
 
 		atomic_set(&dd->count, 0);

Hi,
Though the patch creates read-only device mappings, the READONLY status is not reflected on device mapper multipath devices.
Example: open with O_RDWR flag succeeds for read-only mpath devices, and write fails with "Operation not permitted" error.

Attached patch would fix this.

Thanks,
Vijay

-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Hannes Reinecke
Sent: Friday, April 25, 2008 8:03 PM
To: device-mapper development
Subject: [dm-devel] [PATCH] [RFC] switch DM tables to readonly automatically

Hi all,

this patch switches the device-mapper table to read-only status automatically if one underlying device returns -EROFS.

Rationale:
Whenever a SCSI device is switched to read-only a table reload from multipath-tools fails, without any indication about the reason. And it's actually quite tricky to detect the read-only status from userland. And quite pointless, too, as the kernel already knows about it.

And we now can create tables for CD-ROMs, too, without having to use the '-r' flag to dmsetup ...

Christophe, this might also fix your problem.

As usual, comments etc are welcome.

Cheers,

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

diff -pNaur md-org/dm-table.c md/dm-table.c
@@ -461,11 +461,19 @@ static int __table_get_device(struct dm_
 		dd->mode = mode;
 		dd->bdev = NULL;
 
-		if ((r = open_dev(dd, dev))) {
+		r = open_dev(dd, dev);
+		if (r == -EROFS) {
+			dd->mode &= ~FMODE_WRITE;
+			r = open_dev(dd, dev);
+		}
+		if (r) {
 			kfree(dd);
 			return r;
 		}
 
+		if (dd->mode != mode)
+			t->mode = dd->mode;
+
 		format_dev_t(dd->name, dev);
 
 		atomic_set(&dd->count, 0);

diff -pNaur md-org/dm.c md/dm.c
@@ -221,16 +221,26 @@ static void __exit dm_exit(void)
 static int dm_blk_open(struct inode *inode, struct file *file)
 {
 	struct mapped_device *md;
+	struct gendisk *disk = inode->i_bdev->bd_disk;
+	int retval = 0;
 
 	spin_lock(&_minor_lock);
 
 	md = inode->i_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 (disk->policy && (file->f_mode & FMODE_WRITE)) {
+		md = NULL;
+		retval = -EROFS;
 		goto out;
 	}
 

@@ -240,7 +250,7 @@ static int dm_blk_open(struct inode *ino
 out:
 	spin_unlock(&_minor_lock);
 
-	return md ? 0 : -ENXIO;
+	return retval;
 }
 
 static int dm_blk_close(struct inode *inode, struct file *file)

@@ -1089,6 +1099,10 @@ static int __bind(struct mapped_device *
 	write_lock(&md->map_lock);
 	md->map = t;
 	dm_table_set_restrictions(t, q);
+	if (!(dm_table_get_mode(t) & FMODE_WRITE))
+	{
+		set_disk_ro(md->disk, 1);
+	}
 	write_unlock(&md->map_lock);
 
 	return 0;

@@ -1104,6 +1118,10 @@ static void __unbind(struct mapped_devic
 	dm_table_event_callback(map, NULL, NULL);
 	write_lock(&md->map_lock);
 	md->map = NULL;
+	if (!(dm_table_get_mode(map) & FMODE_WRITE))
+	{
+		set_disk_ro(md->disk, 0);
+	}
 	write_unlock(&md->map_lock);
 	dm_table_put(map);
 }

I'm going to need some persuading here...

An analogy. If I call open() with O_RDWR and but that can't be done because the device is read-only - what happens? Does the open() silently give me a read-only file descriptor instead? Or does it give me -EROFS?

If I request DM_TABLE_LOAD without the DM_READONLY_FLAG, I am asking for a device I can write to and if that's not possible I expect an error. If I'd wanted a read-only device I'd have set the DM_READONLY_FLAG on my request.

Is the real problem that -EROFS/-ENXIO errors are not propagating back up through target _ctr functions as perhaps they should?

Alasdair

[email protected]

No, the real error is that you currently cannot create multipath targets on devices exported as read-only from the storage. Having device-mapper setting the 'read-only' flag automatically on those devices instead of just bailing out will fix this.

The main problem we're facing here (from the point of multipathd) is the lack of error reporting from device-mapper. We're just getting the very helpful error 'ioctl failed', with no indication about _why_.
So we have no clue at all that we might have to check for the read-only setting on this device.
And checking this setting on _any_ ioctl error is just plain silly.

The rest of the patch (calling _set_disk_ro and refusing to open the device with O_RDWR) is just whitewhash to get a better user experience.

Cheers,

Hannes

Dr. Hannes Reinecke		      zSeries & Storage
[email protected]			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Markus Rex, HRB 16746 (AG Nürnberg)