mirroring: [patch 5 of 6] device failure tolerance

mirroring: [patch 5 of 6] device failure tolerance

[Jonathan E Brassow]

This patch changes the status output.  It will break pvmove and other 
things that may have been parsing the status output of a mirror.  It is 
necessary, however, if we are going to have a way to determine which 
device in a mirror failed - or distinguish an failure event from one 
that may simply signal the device is in-sync.  Right now, different 
logs may output different arguments (device and failure, region size, 
sync/nosync, etc), which may make it difficult to parse.  I wondered 
about just having the log type and device with liveness status.  
However, this would still have variable arguments between core and disk 
- or some future scheme that may use multiple log devices for 
redundancy.

  brassow

diff -urN linux-2.6.12-00004/drivers/md/dm-log.c 
linux-2.6.12-00005/drivers/md/dm-log.c
--- linux-2.6.12-00004/drivers/md/dm-log.c	2005-06-30 
01:44:58.277954335 -0500
+++ linux-2.6.12-00005/drivers/md/dm-log.c	2005-06-30 
02:06:43.632391301 -0500
@@ -717,6 +717,9 @@

  	switch(status) {
  	case STATUSTYPE_INFO:
+		DMEMIT("%s %u %u ", log->type->name,
+		       lc->sync == DEFAULTSYNC ? 1 : 2, lc->region_size);
+		DMEMIT_SYNC;
  		break;

  	case STATUSTYPE_TABLE:
@@ -737,6 +740,14 @@

  	switch(status) {
  	case STATUSTYPE_INFO:
+		format_dev_t(buffer, lc->log_dev->bdev->bd_dev);
+		DMEMIT("%s %u %s%s %u ",
+		       log->type->name,
+		       lc->sync == DEFAULTSYNC ? 2 : 3,
+		       buffer,
+		       lc->log_dev_failed ? "/D" : "/A",
+		       lc->region_size);
+		DMEMIT_SYNC;
  		break;

  	case STATUSTYPE_TABLE:
diff -urN linux-2.6.12-00004/drivers/md/dm-raid1.c 
linux-2.6.12-00005/drivers/md/dm-raid1.c
--- linux-2.6.12-00004/drivers/md/dm-raid1.c	2005-06-30 
01:56:19.457727576 -0500
+++ linux-2.6.12-00005/drivers/md/dm-raid1.c	2005-06-30 
01:56:27.164784984 -0500
@@ -1546,8 +1546,11 @@
  	switch (type) {
  	case STATUSTYPE_INFO:
  		DMEMIT("%d ", ms->nr_mirrors);
-		for (m = 0; m < ms->nr_mirrors; m++)
-			DMEMIT("%s ", ms->mirror[m].dev->name);
+		for (m = 0; m < ms->nr_mirrors; m++) {
+			DMEMIT("%s/%s ", ms->mirror[m].dev->name,
+			       atomic_read(&(ms->mirror[m].error_count)) ?
+			       "D" : "A");
+		}

  		DMEMIT(SECTOR_FORMAT "/" SECTOR_FORMAT,
  		       ms->rh.log->type->get_sync_count(ms->rh.log),

Re: mirroring: [patch 5 of 6] device failure tolerance

[Kevin Corry]

Hi Jonathan,

On Thu June 30 2005 3:00 am, Jonathan E Brassow wrote:
> This patch changes the status output.  It will break pvmove and other
> things that may have been parsing the status output of a mirror.

Any time a target's table-format or status-format changes, we also need to 
increment that target's version number so user-space can figure out which 
format to expect.

The version for dm-mirror is currently 1.0.1, so we should at least bump it to 
1.0.2 or 1.1.0.

Also, please remember to turn off line-wrap when inserting patches into 
emails.  :)

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


--- dm-raid1.c.old 2005-06-30 11:33:24.000000000 -0500
+++ dm-raid1.c 2005-06-30 11:33:34.000000000 -0500
@@ -1210,7 +1210,7 @@
 
 static struct target_type mirror_target = {
  .name  = "mirror",
- .version = {1, 0, 1},
+ .version = {1, 1, 0},
  .module  = THIS_MODULE,
  .ctr  = mirror_ctr,
  .dtr  = mirror_dtr,

Re: mirroring: [patch 5 of 6] device failure tolerance

[Alasdair G Kergon]

On Thu, Jun 30, 2005 at 11:38:28AM -0500, Kevin Corry wrote:
> Any time a target's table-format or status-format changes, we also need to 
> increment that target's version number so user-space can figure out which 
> format to expect.
 
Indeed, but I don't want version numbers updating till we know what
sequence things will go upstream.

BTW I've not seen a justification for breaking the existing format here
yet as opposed to simply appending new data to the end of the status line.

Alasdair
-- 
agk@redhat.com

Re: mirroring: [patch 5 of 6] device failure tolerance

[Kevin Corry]

On Thu June 30 2005 12:56 pm, Alasdair G Kergon wrote:
> On Thu, Jun 30, 2005 at 11:38:28AM -0500, Kevin Corry wrote:
> > Any time a target's table-format or status-format changes, we also need
> > to increment that target's version number so user-space can figure out
> > which format to expect.
>
> Indeed, but I don't want version numbers updating till we know what
> sequence things will go upstream.

Of course. As long as it gets updated before it hits mainline. :)

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

Re: mirroring: [patch 5 of 6] device failure tolerance

[Jonathan E Brassow]

On Jun 30, 2005, at 12:56 PM, Alasdair G Kergon wrote:

> On Thu, Jun 30, 2005 at 11:38:28AM -0500, Kevin Corry wrote:
>> Any time a target's table-format or status-format changes, we also 
>> need to
>> increment that target's version number so user-space can figure out 
>> which
>> format to expect.
>
> Indeed, but I don't want version numbers updating till we know what
> sequence things will go upstream.
>
> BTW I've not seen a justification for breaking the existing format here
> yet as opposed to simply appending new data to the end of the status 
> line.
>

mirror_status first calls the log status function and then adds on to 
that.  If we want to do what you are proposing, we would move the log 
status function _after_ the DMEMIT's for mirror and it would only print 
out a char for each log device indicating status.  We would have to 
remember that STATUSTYPE_INFO and STATUSTYPE_TABLE would call the log 
status function at different place (one after it's own DMEMITs, the 
other before).

  brassow

Re: mirroring: [patch 5 of 6] device failure tolerance

[Alasdair G Kergon]

On Thu, Jun 30, 2005 at 01:10:04PM -0500, Jon Brassow wrote:
> mirror_status first calls the log status function and then adds on to 
> that.  If we want to do what you are proposing, we would move the log 
> status function _after_ the DMEMIT's for mirror and it would only print 
> out a char for each log device indicating status.  We would have to 
> remember that STATUSTYPE_INFO and STATUSTYPE_TABLE would call the log 
> status function at different place (one after it's own DMEMITs, the 
> other before).
 
Backwards compatibility has to take precedence over tidiness here if you
want these patches including quickly.  If you don't mind waiting several
months while the necessary userspace support gets widely propagated,
then we can indeed tidy things up now.  But I'm not prepared to see
device-mapper kernel incompatibilities go in ahead of the userspace 
preparation.

Alasdair
-- 
agk@redhat.com

Re: mirroring: [patch 5 of 6] device failure tolerance

[Jonathan E Brassow]

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


On Jun 30, 2005, at 1:10 PM, Jonathan E Brassow wrote:

>
> On Jun 30, 2005, at 12:56 PM, Alasdair G Kergon wrote:
>
>> On Thu, Jun 30, 2005 at 11:38:28AM -0500, Kevin Corry wrote:
>>> Any time a target's table-format or status-format changes, we also 
>>> need to
>>> increment that target's version number so user-space can figure out 
>>> which
>>> format to expect.
>>
>> Indeed, but I don't want version numbers updating till we know what
>> sequence things will go upstream.
>>
>> BTW I've not seen a justification for breaking the existing format 
>> here
>> yet as opposed to simply appending new data to the end of the status 
>> line.
>>
>
> mirror_status first calls the log status function and then adds on to 
> that.  If we want to do what you are proposing, we would move the log 
> status function _after_ the DMEMIT's for mirror and it would only 
> print out a char for each log device indicating status.  We would have 
> to remember that STATUSTYPE_INFO and STATUSTYPE_TABLE would call the 
> log status function at different place (one after it's own DMEMITs, 
> the other before).
>

Perhaps like the attached patch.

I think the pvmove stuff is already busted.  How does it handle mirrors 
with more than 2 sides?  (ok, so pvmove isn't broken, but code that 
uses the same function to determine a mirrors status would be - lvs?)  
If we are going to add code that must correctly handle the number of 
mirror args, I think we could add code to properly handle the log args 
at the same time.  Then we have some (limited) flexibility in how 
things are handled.

  brassow


[-- Attachment #2: 00005-alt.patch --]
[-- Type: application/octet-stream, Size: 1882 bytes --]

diff -urN linux-2.6.12-00004/drivers/md/dm-log.c linux-2.6.12-00005-alt/drivers/md/dm-log.c
--- linux-2.6.12-00004/drivers/md/dm-log.c	2005-06-30 01:44:58.000000000 -0500
+++ linux-2.6.12-00005-alt/drivers/md/dm-log.c	2005-06-30 13:46:43.260023309 -0500
@@ -737,6 +737,8 @@
 
 	switch(status) {
 	case STATUSTYPE_INFO:
+		format_dev_t(buffer, lc->log_dev->bdev->bd_dev);
+		DMEMIT("%s %c", buffer, lc->log_dev_failed ? 'D' : 'A');
 		break;
 
 	case STATUSTYPE_TABLE:
diff -urN linux-2.6.12-00004/drivers/md/dm-raid1.c linux-2.6.12-00005-alt/drivers/md/dm-raid1.c
--- linux-2.6.12-00004/drivers/md/dm-raid1.c	2005-06-30 01:56:19.000000000 -0500
+++ linux-2.6.12-00005-alt/drivers/md/dm-raid1.c	2005-06-30 13:55:18.644187058 -0500
@@ -1538,23 +1538,28 @@
 static int mirror_status(struct dm_target *ti, status_type_t type,
 			 char *result, unsigned int maxlen)
 {
-	unsigned int m, sz;
+	unsigned int m, sz = 0;
 	struct mirror_set *ms = (struct mirror_set *) ti->private;
-
-	sz = ms->rh.log->type->status(ms->rh.log, type, result, maxlen);
+	char buffer[ms->nr_mirrors + 1];
 
 	switch (type) {
 	case STATUSTYPE_INFO:
 		DMEMIT("%d ", ms->nr_mirrors);
-		for (m = 0; m < ms->nr_mirrors; m++)
+		for (m = 0; m < ms->nr_mirrors; m++) {
 			DMEMIT("%s ", ms->mirror[m].dev->name);
+			buffer[m] = atomic_read(&(ms->mirror[m].error_count)) ? 
+				'D' : 'A';
+		}
+		buffer[m] = '\0';
 
-		DMEMIT(SECTOR_FORMAT "/" SECTOR_FORMAT,
+		DMEMIT(SECTOR_FORMAT "/" SECTOR_FORMAT " %s ",
 		       ms->rh.log->type->get_sync_count(ms->rh.log),
-		       ms->nr_regions);
+		       ms->nr_regions, buffer);
+		ms->rh.log->type->status(ms->rh.log, type, result+sz, maxlen-sz);
 		break;
 
 	case STATUSTYPE_TABLE:
+		sz = ms->rh.log->type->status(ms->rh.log, type, result, maxlen);
 		DMEMIT("%d ", ms->nr_mirrors);
 		for (m = 0; m < ms->nr_mirrors; m++)
 			DMEMIT("%s " SECTOR_FORMAT " ",

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



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

Re: mirroring: [patch 5 of 6] device failure tolerance

[Alasdair G Kergon]

On Thu, Jun 30, 2005 at 02:05:29PM -0500, Jon Brassow wrote:
> I think the pvmove stuff is already busted.  How does it handle mirrors 
> with more than 2 sides?  

It only has to handle 2 devices.

> (ok, so pvmove isn't broken, 

Correct.  No compatibility problems there.

> code that 
> uses the same function to determine a mirrors status would be - lvs?)  

But that can easily be enhanced when needed.

Alasdair
-- 
agk@redhat.com