[PATCH 002 of 2] md: Improve the is_mddev_idle test
During a 'resync' or similar activity, md checks if the devices in the
array are otherwise active and winds back resync activity when they
are. This test in done in is_mddev_idle, and it is somewhat fragile -
it sometimes thinks there is non-sync io when there isn't.
The test compares the total sectors of io (disk_stat_read) with the sectors
of resync io (disk-sync_io).
This has problems because total sectors gets updated when a request completes,
while resync io gets updated when the request is submitted. The time difference
can cause large differenced between the two which do not actually imply
non-resync activity. The test currently allows for some fuzz (+/- 4096)
but there are some cases when it is not enough.
The test currently looks for any (non-fuzz) difference, either
positive or negative. This clearly is not needed. Any non-sync
activity will cause the total sectors to grow faster than the sync_io
count (never slower) so we only need to look for a positive differences.
If we do this then the amount of in-flight sync io will never cause
the appearance of non-sync IO. Once enough non-sync IO to worry about
starts happening, resync will be slowed down and the measurements will
thus be more precise (as there is less in-flight) and control of resync
will still be suitably responsive.
Signed-off-by: Neil Brown [EMAIL PROTECTED]
### Diffstat output
./drivers/md/md.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff .prev/drivers/md/md.c ./drivers/md/md.c
--- .prev/drivers/md/md.c 2007-05-10 15:51:54.0 +1000
+++ ./drivers/md/md.c 2007-05-10 16:05:10.0 +1000
@@ -5095,7 +5095,7 @@ static int is_mddev_idle(mddev_t *mddev)
*
* Note: the following is an unsigned comparison.
*/
- if ((curr_events - rdev-last_events + 4096) 8192) {
+ if ((long)curr_events - (long)rdev-last_events 4096) {
rdev-last_events = curr_events;
idle = 0;
}
-
To unsubscribe from this list: send the line unsubscribe linux-raid in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 002 of 2] md: Improve the is_mddev_idle test
On Thu, 10 May 2007 16:22:31 +1000 NeilBrown [EMAIL PROTECTED] wrote:
The test currently looks for any (non-fuzz) difference, either
positive or negative. This clearly is not needed. Any non-sync
activity will cause the total sectors to grow faster than the sync_io
count (never slower) so we only need to look for a positive differences.
...
--- .prev/drivers/md/md.c 2007-05-10 15:51:54.0 +1000
+++ ./drivers/md/md.c 2007-05-10 16:05:10.0 +1000
@@ -5095,7 +5095,7 @@ static int is_mddev_idle(mddev_t *mddev)
*
* Note: the following is an unsigned comparison.
*/
- if ((curr_events - rdev-last_events + 4096) 8192) {
+ if ((long)curr_events - (long)rdev-last_events 4096) {
rdev-last_events = curr_events;
idle = 0;
In which case would unsigned counters be more appropriate?
-
To unsubscribe from this list: send the line unsubscribe linux-raid in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 002 of 2] md: Improve the is_mddev_idle test
On Thursday May 10, [EMAIL PROTECTED] wrote:
On Thu, 10 May 2007 16:22:31 +1000 NeilBrown [EMAIL PROTECTED] wrote:
The test currently looks for any (non-fuzz) difference, either
positive or negative. This clearly is not needed. Any non-sync
activity will cause the total sectors to grow faster than the sync_io
count (never slower) so we only need to look for a positive differences.
...
--- .prev/drivers/md/md.c 2007-05-10 15:51:54.0 +1000
+++ ./drivers/md/md.c 2007-05-10 16:05:10.0 +1000
@@ -5095,7 +5095,7 @@ static int is_mddev_idle(mddev_t *mddev)
*
* Note: the following is an unsigned comparison.
*/
- if ((curr_events - rdev-last_events + 4096) 8192) {
+ if ((long)curr_events - (long)rdev-last_events 4096) {
rdev-last_events = curr_events;
idle = 0;
In which case would unsigned counters be more appropriate?
I guess.
It is really the comparison that I want to be signed, I don't much
care about the counted - they are expected to wrap (though they might
not).
So maybe I really want
if ((signed long)(curr_events - rdev-last_events) 4096) {
to make it clear...
But people expect number to be signed by default, so that probably
isn't necessary.
Yeah, I'll make them signed one day.
Thanks,
NeilBrown
-
To unsubscribe from this list: send the line unsubscribe linux-raid in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 002 of 2] md: Improve the is_mddev_idle test
On May 10 2007 16:22, NeilBrown wrote:
diff .prev/drivers/md/md.c ./drivers/md/md.c
--- .prev/drivers/md/md.c 2007-05-10 15:51:54.0 +1000
+++ ./drivers/md/md.c 2007-05-10 16:05:10.0 +1000
@@ -5095,7 +5095,7 @@ static int is_mddev_idle(mddev_t *mddev)
*
* Note: the following is an unsigned comparison.
*/
- if ((curr_events - rdev-last_events + 4096) 8192) {
+ if ((long)curr_events - (long)rdev-last_events 4096) {
rdev-last_events = curr_events;
idle = 0;
}
What did really change? Unless I am seriously mistaken,
curr_events - last_evens + 4096 8192
is mathematically equivalent to
curr_events - last_evens 4096
The casting to (long) may however force a signed comparison which turns
things quite upside down, and the comment does not apply anymore.
Jan
--
-
To unsubscribe from this list: send the line unsubscribe linux-raid in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 002 of 2] md: Improve the is_mddev_idle test
On Thursday May 10, [EMAIL PROTECTED] wrote:
On May 10 2007 16:22, NeilBrown wrote:
diff .prev/drivers/md/md.c ./drivers/md/md.c
--- .prev/drivers/md/md.c2007-05-10 15:51:54.0 +1000
+++ ./drivers/md/md.c2007-05-10 16:05:10.0 +1000
@@ -5095,7 +5095,7 @@ static int is_mddev_idle(mddev_t *mddev)
*
* Note: the following is an unsigned comparison.
*/
-if ((curr_events - rdev-last_events + 4096) 8192) {
+if ((long)curr_events - (long)rdev-last_events 4096) {
rdev-last_events = curr_events;
idle = 0;
}
What did really change? Unless I am seriously mistaken,
curr_events - last_evens + 4096 8192
is mathematically equivalent to
curr_events - last_evens 4096
The casting to (long) may however force a signed comparison which turns
things quite upside down, and the comment does not apply anymore.
Yes, the use of a signed comparison is the significant difference.
And yes, the comment becomes wrong. I'm in the process of redrafting
that. It currently stands at:
/* sync IO will cause sync_io to increase before the disk_stats
* as sync_io is counted when a request starts, and
* disk_stats is counted when it completes.
* So resync activity will cause curr_events to be smaller than
* when there was no such activity.
* non-sync IO will cause disk_stat to increase without
* increasing sync_io so curr_events will (eventually)
* be larger than it was before. Once it becomes
* substantially larger, the test below will cause
* the array to appear non-idle, and resync will slow
* down.
* If there is a lot of outstanding resync activity when
* we set last_event to curr_events, then all that activity
* completing might cause the array to appear non-idle
* and resync will be slowed down even though there might
* not have been non-resync activity. This will only
* happen once though. 'last_events' will soon reflect
* the state where there is little or no outstanding
* resync requests, and further resync activity will
* always make curr_events less than last_events.
*
*/
Does that read at all well?
NeilBrown
-
To unsubscribe from this list: send the line unsubscribe linux-raid in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 002 of 2] md: Improve the is_mddev_idle test
On May 10 2007 20:04, Neil Brown wrote:
- if ((curr_events - rdev-last_events + 4096) 8192) {
+ if ((long)curr_events - (long)rdev-last_events 4096) {
rdev-last_events = curr_events;
idle = 0;
}
/* sync IO will cause sync_io to increase before the disk_stats
* as sync_io is counted when a request starts, and
* disk_stats is counted when it completes.
* So resync activity will cause curr_events to be smaller than
* when there was no such activity.
* non-sync IO will cause disk_stat to increase without
* increasing sync_io so curr_events will (eventually)
* be larger than it was before. Once it becomes
* substantially larger, the test below will cause
* the array to appear non-idle, and resync will slow
* down.
* If there is a lot of outstanding resync activity when
* we set last_event to curr_events, then all that activity
* completing might cause the array to appear non-idle
* and resync will be slowed down even though there might
* not have been non-resync activity. This will only
* happen once though. 'last_events' will soon reflect
* the state where there is little or no outstanding
* resync requests, and further resync activity will
* always make curr_events less than last_events.
*
*/
Does that read at all well?
It is a more verbose explanation of your patch description, yes.
Jan
--
-
To unsubscribe from this list: send the line unsubscribe linux-raid in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
