RE: [PATCH] [RFC] switch DM tables to readonly automatically

RE: [PATCH] [RFC] switch DM tables to readonly automatically

[Christophe Varoqui]

> 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.
> 
It seems it will solve the map creation error on read-only LU, but what about these LU becoming writable ? ...
as is the case with the Symmetrix R2 upon spliting the synchronisation link.
Will the devmap become writable too automagically or is the multipathd daemon expected to take action the promote the map RW ?

Thanks for caring anyway.

> As usual, comments etc are welcome.
> 
> 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)
> 

Re: [PATCH] [RFC] switch DM tables to readonly automatically

[Hannes Reinecke]

Hi Christophe,

Christophe Varoqui wrote:
>> 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.
>>
> It seems it will solve the map creation error on read-only LU, but what about these LU becoming writable ? ...
> as is the case with the Symmetrix R2 upon spliting the synchronisation link.
> Will the devmap become writable too automagically or is the multipathd daemon expected to take action the promote the map RW ?
> 
Tricky business. We'll first have to be notified that the LU is becoming writeable. The current SCSI stack is not
very good at providing that sort of information.

I would think the best way here would be to modify the 'tur' checker to check for the READ-ONLY state, too.
That would mean we'll have to implement a new path state 'READONLY', but that should be okay methinks.
But this would have to be done at the SCSI level as the current 'sd' driver is not capable of switching
between read-only and read-write on the fly, either.

Maybe I'll give it a go if I find some time.

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)

Re: [PATCH] [RFC] switch DM tables to readonly automatically

[Christophe Varoqui]

Le mercredi 30 avril 2008 à 10:57 +0200, Hannes Reinecke a écrit :
> Hi Christophe,
> 
> Christophe Varoqui wrote:
> >> 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.
> >>
> > It seems it will solve the map creation error on read-only LU, but what about these LU becoming writable ? ...
> > as is the case with the Symmetrix R2 upon spliting the synchronisation link.
> > Will the devmap become writable too automagically or is the multipathd daemon expected to take action the promote the map RW ?
> > 
> Tricky business. We'll first have to be notified that the LU is becoming writeable. The current SCSI stack is not
> very good at providing that sort of information.
> 
> I would think the best way here would be to modify the 'tur' checker to check for the READ-ONLY state, too.
> That would mean we'll have to implement a new path state 'READONLY', but that should be okay methinks.
> But this would have to be done at the SCSI level as the current 'sd' driver is not capable of switching
> between read-only and read-write on the fly, either.
> 
> Maybe I'll give it a go if I find some time.
> 
For your information, I checked the rhel4 behaviour.

That stuff worked out-of-the-box with rhel4 kernel : paths to R2 were
not flagged RO, so maps over R2 LU paths were RW; the Symmetrix failed
ios when paths were actually not writable and, upon spliting, multipaths
were usable without any userspace action.

Is all this precaution about not submitting ios to the R2 LU really
worth the pain ? Can't we just let the device mapper moun RW maps over
RO paths ?

Regards,
cvaroqui

[PATCH] [RFC] switch DM tables to readonly automatically

[Hannes Reinecke]

[-- Attachment #1: Type: text/plain, Size: 824 bytes --]

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
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Markus Rex, HRB 16746 (AG Nürnberg)

[-- Attachment #2: dm-table-switch-to-readonly --]
[-- Type: text/plain, Size: 609 bytes --]

diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
index e75b143..f615e85 100644
--- a/drivers/md/dm-table.c
+++ b/drivers/md/dm-table.c
@@ -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);

[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

RE: [PATCH] [RFC] switch DM tables to readonly automatically

[Balasubramanian, Vijayakumar (STSD)]

[-- Attachment #1: Type: text/plain, Size: 1432 bytes --]

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: dm-devel-bounces@redhat.com [mailto:dm-devel-bounces@redhat.com] 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
hare@suse.de                          +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Markus Rex, HRB 16746 (AG Nürnberg)

[-- Attachment #2: md-patch.diff --]
[-- Type: application/octet-stream, Size: 2076 bytes --]

diff -pNaur md-org/dm-table.c md/dm-table.c
--- md-org/dm-table.c	2008-03-17 23:23:31.000000000 +0530
+++ md/dm-table.c	2008-04-30 09:58:09.000000000 +0530
@@ -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
--- md-org/dm.c	2008-03-17 23:23:31.000000000 +0530
+++ md/dm.c	2008-04-30 09:58:46.000000000 +0530
@@ -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);
 }

[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] [RFC] switch DM tables to readonly automatically

[Alasdair G Kergon]

On Fri, Apr 25, 2008 at 04:32:37PM +0200, Hannes Reinecke wrote:
> this patch switches the device-mapper table to read-only
> status automatically if one underlying device returns -EROFS.
 
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
-- 
agk@redhat.com

Re: [PATCH] [RFC] switch DM tables to readonly automatically

[Hannes Reinecke]

Alasdair G Kergon wrote:
> On Fri, Apr 25, 2008 at 04:32:37PM +0200, Hannes Reinecke wrote:
>> this patch switches the device-mapper table to read-only
>> status automatically if one underlying device returns -EROFS.
>  
> 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?
> 
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
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Markus Rex, HRB 16746 (AG Nürnberg)