[mdadm PATCH] Introduce a commandline option for setting RebuildNN increment size

[mdadm PATCH] Introduce a commandline option for setting RebuildNN increment size

[Zdenek Behan]

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

Hi folks,


I have been facing a problem of making a simple cgi GUI for working with
md raid, and reporting back the information. I ended up using the
monitor mode event mechanism for various things including updating the
rebuild percentage. Soon after, people started complaining that 20%
increments in progress are rather silly.

I noticed long ago in the list the thought/request that those increments
may be a bit smaller. So, here's a patch (attachment) to allow sending
RebuildNN events at any percent increment, specified on mdadm commandline.

Any comments? Is this a desirable feature? Anything I should change to
make it acceptable?


Regards,

Zdenek Behan

[-- Attachment #2: git-rebuildincrement.patch --]
[-- Type: text/plain, Size: 5040 bytes --]

From d99a9125a0b713c9d0f2390330862d15500204a7 Mon Sep 17 00:00:00 2001
From: Zdenek Behan <rain@matfyz.cz>
Date: Wed, 14 Oct 2009 22:06:39 +0200
Subject: [PATCH] Monitor: add option to specify rebuild increments, ie. the percent
 increments after which RebuildNN event is generated

This is particulary useful when using --program option, rather than
(only) syslog for alerts.

Signed-off-by: Zdenek Behan <rain@matfyz.cz>
---
 Monitor.c |   26 +++++++++++++-------------
 ReadMe.c  |    2 ++
 mdadm.c   |   12 ++++++++++--
 mdadm.h   |    2 +-
 4 files changed, 26 insertions(+), 16 deletions(-)

diff --git a/Monitor.c b/Monitor.c
index af486d7..63493b7 100644
--- a/Monitor.c
+++ b/Monitor.c
@@ -33,14 +33,6 @@
 static void alert(char *event, char *dev, char *disc, char *mailaddr, char *mailfrom,
 		  char *cmd, int dosyslog);
 
-static char *percentalerts[] = {
-	"RebuildStarted",
-	"Rebuild20",
-	"Rebuild40",
-	"Rebuild60",
-	"Rebuild80",
-};
-
 /* The largest number of disks current arrays can manage is 384
  * This really should be dynamically, but that will have to wait
  * At least it isn't MD_SB_DISKS.
@@ -49,7 +41,7 @@ static char *percentalerts[] = {
 int Monitor(mddev_dev_t devlist,
 	    char *mailaddr, char *alert_cmd,
 	    int period, int daemonise, int scan, int oneshot,
-	    int dosyslog, int test, char* pidfile)
+	    int dosyslog, int test, char* pidfile, int increments)
 {
 	/*
 	 * Every few seconds, scan every md device looking for changes
@@ -77,8 +69,8 @@ int Monitor(mddev_dev_t devlist,
 	 *      An active device had a reverse transition
 	 *    RebuildStarted
 	 *      percent went from -1 to +ve
-	 *    Rebuild20 Rebuild40 Rebuild60 Rebuild80
-	 *      percent went from below to not-below that number
+	 *    RebuildNN
+	 *      percent went from below to not-below NN%
 	 *    DeviceDisappeared
 	 *      Couldn't access a device which was previously visible
 	 *
@@ -311,9 +303,17 @@ int Monitor(mddev_dev_t devlist,
 			if (mse &&
 			    st->percent >= 0 &&
 			    mse->percent >= 0 &&
-			    (mse->percent / 20) > (st->percent / 20))
-				alert(percentalerts[mse->percent/20],
+			    (mse->percent / increments) > (st->percent / increments)) {
+				char percentalert[15]; // "RebuildNN" or "RebuildStarted"
+
+				if((mse->percent / increments) == 0)
+					snprintf(percentalert, 15, "RebuildStarted");
+				else
+					snprintf(percentalert, 11, "Rebuild%02d", mse->percent);
+
+				alert(percentalert,
 				      dev, NULL, mailaddr, mailfrom, alert_cmd, dosyslog);
+			}
 
 			if (mse &&
 			    mse->percent == -1 &&
diff --git a/ReadMe.c b/ReadMe.c
index 90b4daf..3e53e57 100644
--- a/ReadMe.c
+++ b/ReadMe.c
@@ -176,6 +176,7 @@ struct option long_options[] = {
     {"mail",      1, 0, 'm'},
     {"program",   1, 0, 'p'},
     {"alert",     1, 0, 'p'},
+    {"increment", 1, 0, 'r'},
     {"delay",     1, 0, 'd'},
     {"daemonise", 0, 0, 'f'},
     {"daemonize", 0, 0, 'f'},
@@ -495,6 +496,7 @@ char Help_monitor[] =
 "  --mail=       -m   : Address to mail alerts of failure to\n"
 "  --program=    -p   : Program to run when an event is detected\n"
 "  --alert=           : same as --program\n"
+"  --increment=  -r   : Report RebuildNN events in the given increment. default=20\n"
 "  --delay=      -d   : seconds of delay between polling state. default=60\n"
 "  --config=     -c   : specify a different config file\n"
 "  --scan        -s   : find mail-address/program in config file\n"
diff --git a/mdadm.c b/mdadm.c
index bb3e5bb..a7c579c 100644
--- a/mdadm.c
+++ b/mdadm.c
@@ -89,6 +89,7 @@ int main(int argc, char *argv[])
 	int require_homehost = 1;
 	char *mailaddr = NULL;
 	char *program = NULL;
+	int increments = 20;
 	int delay = 0;
 	int daemonise = 0;
 	char *pidfile = NULL;
@@ -697,7 +698,14 @@ int main(int argc, char *argv[])
 			else
 				program = optarg;
 			continue;
-
+		case O(MONITOR,'r'): /* rebuild increments */
+			increments = atoi(optarg);
+			if (increments>99 || increments<1)
+			{
+				fprintf(stderr, Name ": please specify positive integer between 1 and 99 as rebuild increments.\n");
+				exit(2);
+			}
+			continue;	
 		case O(MONITOR,'d'): /* delay in seconds */
 		case O(GROW, 'd'):
 		case O(BUILD,'d'): /* delay for bitmap updates */
@@ -1377,7 +1385,7 @@ int main(int argc, char *argv[])
 		}
 		rv= Monitor(devlist, mailaddr, program,
 			    delay?delay:60, daemonise, scan, oneshot,
-			    dosyslog, test, pidfile);
+			    dosyslog, test, pidfile, increments);
 		break;
 
 	case GROW:
diff --git a/mdadm.h b/mdadm.h
index 91ba624..dc7ebe6 100644
--- a/mdadm.h
+++ b/mdadm.h
@@ -744,7 +744,7 @@ extern int Examine(mddev_dev_t devlist, int brief, int export, int scan,
 extern int Monitor(mddev_dev_t devlist,
 		   char *mailaddr, char *alert_cmd,
 		   int period, int daemonise, int scan, int oneshot,
-		   int dosyslog, int test, char *pidfile);
+		   int dosyslog, int test, char *pidfile, int increments);
 
 extern int Kill(char *dev, int force, int quiet, int noexcl);
 extern int Wait(char *dev);
-- 
1.6.0.6

Re: [mdadm PATCH] Introduce a commandline option for setting RebuildNN increment size

[Drew]

As a end user I like the idea. Two questions around the choice of
increments, though.

If we're going to allow values up 99, why not include 100? To my mind,
reporting 0% -> 99% -> 100% offers about the same value as reporting
0% -> 100%.

Also, It seems to me that increments beyond 50 aren't really that
useful. For example, reporting 0% -> 75% -> 100% seems a bit odd to
me, even for extremely fast rebuilds. Units up to 50 (0%, 50%, 100%)
make more sense.


-- 
Drew

"Nothing in life is to be feared. It is only to be understood."
--Marie Curie

Re: [mdadm PATCH] Introduce a commandline option for setting RebuildNN increment size

[Zdenek Behan]

On 10/15/2009 06:00 PM, Drew wrote:
> If we're going to allow values up 99, why not include 100? To my mind,
> reporting 0% -> 99% -> 100% offers about the same value as reporting
> 0% -> 100%.
>   
As it stands, instead of Rebuild100, there is RebuildFinished, which is
generated by a different part of code. Setting this increment to 100
would mean no RebuildNN events at all. Also, since the events are
supposed to be RebuildNN, some scripts may count on NN being exactly two
digits, not more, not less. That's why my code reports 01 instead of 1,
and same invariant cannot be held if you'd like Rebuild100 as well.
> Also, It seems to me that increments beyond 50 aren't really that
> useful. For example, reporting 0% -> 75% -> 100% seems a bit odd to
> me, even for extremely fast rebuilds. Units up to 50 (0%, 50%, 100%)
> make more sense.
I'm pretty sure more options allow you to configure features which are
mutually weird and don't make a lot of sense. mdadm is a low level tool
and at the very least, common sense should be applied when dealing with it.
On the other hand, if someone needed such option (setting increments
>49), for whatever screwed reason, there's no point stopping them from
doing so. Also, it's not dangerous, it does exactly what you would
expect it to: report the value you requested, then RebuildFinished.


Zdenek

Re: [mdadm PATCH] Introduce a commandline option for setting RebuildNN increment size

[NeilBrown]

On Fri, October 16, 2009 1:25 am, Zdenek Behan wrote:
> Hi folks,
>
>
> I have been facing a problem of making a simple cgi GUI for working with
> md raid, and reporting back the information. I ended up using the
> monitor mode event mechanism for various things including updating the
> rebuild percentage. Soon after, people started complaining that 20%
> increments in progress are rather silly.
>
> I noticed long ago in the list the thought/request that those increments
> may be a bit smaller. So, here's a patch (attachment) to allow sending
> RebuildNN events at any percent increment, specified on mdadm commandline.
>
> Any comments? Is this a desirable feature? Anything I should change to
> make it acceptable?

Thanks for the patch.
Yes, I think it is a feature worth having.
Three improvements you can make to make the patch completely acceptable:
1/ Update mdadm.8 to document this feature
2/ The second argument to snprintf is the size of the buffer, not the
   expected string length.  To me it looks odd that you give different
   numbers for the same buffer.
   I would probably use
         snprintf(percentalert, sizeof(percentalert), ....)
   in both cases, but I would except a literal '15'.
3/ In the switch statement in main() you have removed the blank line
   following 'continue', where you should have added a blank line following
   the 'continue' that you added.

If you resubmit with those changes I will apply the patch.

Thanks,
NeilBrown

Re: [mdadm PATCH] Introduce a commandline option for setting RebuildNN increment size

[Zdenek Behan]

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

Updated patch with Neil's comments.

1) I tried my best at making man page entry/corrections.
2) I slightly disagree about second argument of snprintf having to be
buffer size, not expected string length. But it's not a big deal, and I
changed it to sizeof(...).
3) Fixed whitespace I broke.

I also noticed and fixed one opening brace where I originally broke
local formatting convention.


Regards,

Zdenek

[-- Attachment #2: git-rebuildincrement2.patch --]
[-- Type: text/plain, Size: 5801 bytes --]

From d99a9125a0b713c9d0f2390330862d15500204a7 Mon Sep 17 00:00:00 2001
From: Zdenek Behan <rain@matfyz.cz>
Date: Wed, 14 Oct 2009 22:06:39 +0200
Subject: [PATCH] Monitor: add option to specify rebuild increments, ie. the percent
 increments after which RebuildNN event is generated

This is particulary useful when using --program option, rather than
(only) syslog for alerts.

Signed-off-by: Zdenek Behan <rain@matfyz.cz>
---
diff --git a/Monitor.c b/Monitor.c
index af486d7..b0802f8 100644
--- a/Monitor.c
+++ b/Monitor.c
@@ -33,14 +33,6 @@
 static void alert(char *event, char *dev, char *disc, char *mailaddr, char *mailfrom,
 		  char *cmd, int dosyslog);
 
-static char *percentalerts[] = {
-	"RebuildStarted",
-	"Rebuild20",
-	"Rebuild40",
-	"Rebuild60",
-	"Rebuild80",
-};
-
 /* The largest number of disks current arrays can manage is 384
  * This really should be dynamically, but that will have to wait
  * At least it isn't MD_SB_DISKS.
@@ -49,7 +41,7 @@ static char *percentalerts[] = {
 int Monitor(mddev_dev_t devlist,
 	    char *mailaddr, char *alert_cmd,
 	    int period, int daemonise, int scan, int oneshot,
-	    int dosyslog, int test, char* pidfile)
+	    int dosyslog, int test, char* pidfile, int increments)
 {
 	/*
 	 * Every few seconds, scan every md device looking for changes
@@ -77,8 +69,8 @@ int Monitor(mddev_dev_t devlist,
 	 *      An active device had a reverse transition
 	 *    RebuildStarted
 	 *      percent went from -1 to +ve
-	 *    Rebuild20 Rebuild40 Rebuild60 Rebuild80
-	 *      percent went from below to not-below that number
+	 *    RebuildNN
+	 *      percent went from below to not-below NN%
 	 *    DeviceDisappeared
 	 *      Couldn't access a device which was previously visible
 	 *
@@ -311,9 +303,17 @@ int Monitor(mddev_dev_t devlist,
 			if (mse &&
 			    st->percent >= 0 &&
 			    mse->percent >= 0 &&
-			    (mse->percent / 20) > (st->percent / 20))
-				alert(percentalerts[mse->percent/20],
+			    (mse->percent / increments) > (st->percent / increments)) {
+				char percentalert[15]; // "RebuildNN" (10 chars) or "RebuildStarted" (15 chars)
+
+				if((mse->percent / increments) == 0)
+					snprintf(percentalert, sizeof(percentalert), "RebuildStarted");
+				else
+					snprintf(percentalert, sizeof(percentalert), "Rebuild%02d", mse->percent);
+
+				alert(percentalert,
 				      dev, NULL, mailaddr, mailfrom, alert_cmd, dosyslog);
+			}
 
 			if (mse &&
 			    mse->percent == -1 &&
diff --git a/ReadMe.c b/ReadMe.c
index 90b4daf..3e53e57 100644
--- a/ReadMe.c
+++ b/ReadMe.c
@@ -176,6 +176,7 @@ struct option long_options[] = {
     {"mail",      1, 0, 'm'},
     {"program",   1, 0, 'p'},
     {"alert",     1, 0, 'p'},
+    {"increment", 1, 0, 'r'},
     {"delay",     1, 0, 'd'},
     {"daemonise", 0, 0, 'f'},
     {"daemonize", 0, 0, 'f'},
@@ -495,6 +496,7 @@ char Help_monitor[] =
 "  --mail=       -m   : Address to mail alerts of failure to\n"
 "  --program=    -p   : Program to run when an event is detected\n"
 "  --alert=           : same as --program\n"
+"  --increment=  -r   : Report RebuildNN events in the given increment. default=20\n"
 "  --delay=      -d   : seconds of delay between polling state. default=60\n"
 "  --config=     -c   : specify a different config file\n"
 "  --scan        -s   : find mail-address/program in config file\n"
diff --git a/mdadm.8 b/mdadm.8
index 7f19918..55ca3bc 100644
--- a/mdadm.8
+++ b/mdadm.8
@@ -1221,6 +1221,12 @@ reduce this as the kernel alerts
 immediately when there is any change.
 
 .TP
+.BR \-r ", " \-\-increment
+Give a percentage increment.
+.I mdadm
+will generate RebuildNN events with the given percentage increment.
+
+.TP
 .BR \-f ", " \-\-daemonise
 Tell
 .I mdadm
@@ -1818,8 +1824,10 @@ An md array started reconstruction. (syslog priority: Warning)
 .BI Rebuild NN
 Where
 .I NN
-is 20, 40, 60, or 80, this indicates that rebuild has passed that many
-percentage of the total. (syslog priority: Warning)
+is a two-digit number (ie. 05, 48). This indicates that rebuild
+has passed that many percent of the total. The events are generated
+with fixed increment since 0. Increment size may be specified with
+a commandline option. (syslog priority: Warning)
 
 .TP
 .B RebuildFinished
diff --git a/mdadm.c b/mdadm.c
index bb3e5bb..c1f1fd7 100644
--- a/mdadm.c
+++ b/mdadm.c
@@ -89,6 +89,7 @@ int main(int argc, char *argv[])
 	int require_homehost = 1;
 	char *mailaddr = NULL;
 	char *program = NULL;
+	int increments = 20;
 	int delay = 0;
 	int daemonise = 0;
 	char *pidfile = NULL;
@@ -698,6 +699,14 @@ int main(int argc, char *argv[])
 				program = optarg;
 			continue;
 
+		case O(MONITOR,'r'): /* rebuild increments */
+			increments = atoi(optarg);
+			if (increments>99 || increments<1) {
+				fprintf(stderr, Name ": please specify positive integer between 1 and 99 as rebuild increments.\n");
+				exit(2);
+			}
+			continue;
+
 		case O(MONITOR,'d'): /* delay in seconds */
 		case O(GROW, 'd'):
 		case O(BUILD,'d'): /* delay for bitmap updates */
@@ -1377,7 +1386,7 @@ int main(int argc, char *argv[])
 		}
 		rv= Monitor(devlist, mailaddr, program,
 			    delay?delay:60, daemonise, scan, oneshot,
-			    dosyslog, test, pidfile);
+			    dosyslog, test, pidfile, increments);
 		break;
 
 	case GROW:
diff --git a/mdadm.h b/mdadm.h
index 91ba624..dc7ebe6 100644
--- a/mdadm.h
+++ b/mdadm.h
@@ -744,7 +744,7 @@ extern int Examine(mddev_dev_t devlist, int brief, int export, int scan,
 extern int Monitor(mddev_dev_t devlist,
 		   char *mailaddr, char *alert_cmd,
 		   int period, int daemonise, int scan, int oneshot,
-		   int dosyslog, int test, char *pidfile);
+		   int dosyslog, int test, char *pidfile, int increments);
 
 extern int Kill(char *dev, int force, int quiet, int noexcl);
 extern int Wait(char *dev);