Skip to contentSkip to conversation

2.4.20-rmap15a

PATCH Marc-Christian Petersen 7 participants 13 messages Dec 1, 2002 – Dec 3, 2002 Block layer

lore
Share & tools
lore
#0 lore raw

Hi Rik, Hi all,

Quoted 7 lines

This is a merge of rmap15a with marcelo's 2.4 bitkeeper tree, which is identical to 2.4.20-rc4 (he didn't push the makefile update). The only thing left out of the merge for now is Andrew Morton's read_latency patch, both because I'm not sure how needed it is with the elevator updates and because this part of the merge was too tricky to do at merge time; I'll port over Andrew Morton's read_latency patch later...

Well, it is needed. This makes a difference for the I/O pauses noticed in 2.4.19 and 2.4.20. Anyway, readlatency-2 won't make them go away, but those stops/pauses are a bit less than before.

So, here my patch proposal. Ontop of 2.4.20-rmap15a.

ciao, Marc

drivers/block/elevator.c renamed from drivers/block/elevator.c~read-latency2 +76 -9
@@ -80,25 +80,38 @@ int elevator_linus_merge(request_queue_t
 			 struct buffer_head *bh, int rw,
 			 int max_sectors)
 {
-	struct list_head *entry = &q->queue_head;
-	unsigned int count = bh->b_size >> 9, ret = ELEVATOR_NO_MERGE;
+	struct list_head *entry;
+	unsigned int count = bh->b_size >> 9;
+	unsigned int ret = ELEVATOR_NO_MERGE;
+	int merge_only = 0;
+	const int max_bomb_segments = q->elevator.max_bomb_segments;
 	struct request *__rq;
+	int passed_a_read = 0;
+
+	entry = &q->queue_head;
 
 	while ((entry = entry->prev) != head) {
 		__rq = blkdev_entry_to_request(entry);
 
-		/*
-		 * we can't insert beyond a zero sequence point
-		 */
-		if (__rq->elevator_sequence <= 0)
-			break;
+		if (__rq->elevator_sequence-- <= 0) {
+			/*
+			 * OK, we've exceeded someone's latency limit.
+			 * But we still continue to look for merges,
+			 * because they're so much better than seeks.
+			 */
+			merge_only = 1;
+		}
 
 		if (__rq->waiting)
 			continue;
 		if (__rq->rq_dev != bh->b_rdev)
 			continue;
-		if (!*req && bh_rq_in_between(bh, __rq, &q->queue_head))
+		if (!*req && !merge_only &&
+				bh_rq_in_between(bh, __rq, &q->queue_head)) {
 			*req = __rq;
+		}
+		if (__rq->cmd != WRITE)
+			passed_a_read = 1;
 		if (__rq->cmd != rw)
 			continue;
 		if (__rq->nr_sectors + count > max_sectors)
@@ -129,6 +142,57 @@ int elevator_linus_merge(request_queue_t
 		}
 	}
 
+	/*
+	 * If we failed to merge a read anywhere in the request
+	 * queue, we really don't want to place it at the end
+	 * of the list, behind lots of writes.  So place it near
+	 * the front.
+	 *
+	 * We don't want to place it in front of _all_ writes: that
+	 * would create lots of seeking, and isn't tunable.
+	 * We try to avoid promoting this read in front of existing
+	 * reads.
+	 *
+	 * max_bomb_segments becomes the maximum number of write
+	 * requests which we allow to remain in place in front of
+	 * a newly introduced read.  We weight things a little bit,
+	 * so large writes are more expensive than small ones, but it's
+	 * requests which count, not sectors.
+	 */
+	if (max_bomb_segments && rw == READ && !passed_a_read &&
+				ret == ELEVATOR_NO_MERGE) {
+		int cur_latency = 0;
+		struct request * const cur_request = *req;
+
+		entry = head->next;
+		while (entry != &q->queue_head) {
+			struct request *__rq;
+
+			if (entry == &q->queue_head)
+				BUG();
+			if (entry == q->queue_head.next &&
+					q->head_active && !q->plugged)
+				BUG();
+			__rq = blkdev_entry_to_request(entry);
+
+			if (__rq == cur_request) {
+				/*
+				 * This is where the old algorithm placed it.
+				 * There's no point pushing it further back,
+				 * so leave it here, in sorted order.
+				 */
+				break;
+			}
+			if (__rq->cmd == WRITE) {
+				cur_latency += 1 + __rq->nr_sectors / 64;
+				if (cur_latency >= max_bomb_segments) {
+					*req = __rq;
+					break;
+				}
+			}
+			entry = entry->next;
+		}
+	}
 	return ret;
 }
 
@@ -186,7 +250,7 @@ int blkelvget_ioctl(elevator_t * elevato
 	output.queue_ID			= elevator->queue_ID;
 	output.read_latency		= elevator->read_latency;
 	output.write_latency		= elevator->write_latency;
-	output.max_bomb_segments	= 0;
+	output.max_bomb_segments	= elevator->max_bomb_segments;
 
 	if (copy_to_user(arg, &output, sizeof(blkelv_ioctl_arg_t)))
 		return -EFAULT;
@@ -205,9 +269,12 @@ int blkelvset_ioctl(elevator_t * elevato
 		return -EINVAL;
 	if (input.write_latency < 0)
 		return -EINVAL;
+	if (input.max_bomb_segments < 0)
+		return -EINVAL;
 
 	elevator->read_latency		= input.read_latency;
 	elevator->write_latency		= input.write_latency;
+	elevator->max_bomb_segments	= input.max_bomb_segments;
 	return 0;
 }
 
drivers/block/ll_rw_blk.c renamed from drivers/block/ll_rw_blk.c~read-latency2 +5 -3
@@ -432,9 +432,11 @@ static void blk_init_free_list(request_q
 
 	si_meminfo(&si);
 	megs = si.totalram >> (20 - PAGE_SHIFT);
-	nr_requests = 128;
-	if (megs < 32)
-		nr_requests /= 2;
+	nr_requests = (megs * 2) & ~15;	/* One per half-megabyte */
+	if (nr_requests < 32)
+		nr_requests = 32;
+	if (nr_requests > 1024)
+		nr_requests = 1024;
 	blk_grow_request_list(q, nr_requests);
 
 	init_waitqueue_head(&q->wait_for_requests[0]);
#1 lore raw
Quoted 7 lines · Marc-Christian Petersen
Quoted 3 lines

update). The only thing left out of the merge for now is Andrew Morton's read_latency patch, both because I'm not sure how needed it is with the elevator updates

Well, it is needed. This makes a difference for the I/O pauses noticed in 2.4.19 and 2.4.20. Anyway, readlatency-2 won't make them go away, but those stops/pauses are a bit less than before.

That was my gut feeling as well, but I guess it's a good thing to quantify how much of a difference it makes. I wonder if we could convince Con to test a kernel both with and without this patch and look at the difference.

Quoted 1 line

So, here my patch proposal. Ontop of 2.4.20-rmap15a.

Looks good, now lets test it. If the patch is as needed as you say we should push it to marcelo ;)

regards,

Rik

Signature · 3 lines
Bravely reimplemented by the knights who say "NIH".
http://www.surriel.com/		http://guru.conectiva.com/
Current spamtrap:  <a href=mailto:"[email protected]">[email protected]</a>
drivers/block/elevator.c renamed from drivers/block/elevator.c~read-latency2 +76 -9
@@ -80,25 +80,38 @@ int elevator_linus_merge(request_queue_t
 			 struct buffer_head *bh, int rw,
 			 int max_sectors)
 {
-	struct list_head *entry = &q->queue_head;
-	unsigned int count = bh->b_size >> 9, ret = ELEVATOR_NO_MERGE;
+	struct list_head *entry;
+	unsigned int count = bh->b_size >> 9;
+	unsigned int ret = ELEVATOR_NO_MERGE;
+	int merge_only = 0;
+	const int max_bomb_segments = q->elevator.max_bomb_segments;
 	struct request *__rq;
+	int passed_a_read = 0;
+
+	entry = &q->queue_head;
 
 	while ((entry = entry->prev) != head) {
 		__rq = blkdev_entry_to_request(entry);
 
-		/*
-		 * we can't insert beyond a zero sequence point
-		 */
-		if (__rq->elevator_sequence <= 0)
-			break;
+		if (__rq->elevator_sequence-- <= 0) {
+			/*
+			 * OK, we've exceeded someone's latency limit.
+			 * But we still continue to look for merges,
+			 * because they're so much better than seeks.
+			 */
+			merge_only = 1;
+		}
 
 		if (__rq->waiting)
 			continue;
 		if (__rq->rq_dev != bh->b_rdev)
 			continue;
-		if (!*req && bh_rq_in_between(bh, __rq, &q->queue_head))
+		if (!*req && !merge_only &&
+				bh_rq_in_between(bh, __rq, &q->queue_head)) {
 			*req = __rq;
+		}
+		if (__rq->cmd != WRITE)
+			passed_a_read = 1;
 		if (__rq->cmd != rw)
 			continue;
 		if (__rq->nr_sectors + count > max_sectors)
@@ -129,6 +142,57 @@ int elevator_linus_merge(request_queue_t
 		}
 	}
 
+	/*
+	 * If we failed to merge a read anywhere in the request
+	 * queue, we really don't want to place it at the end
+	 * of the list, behind lots of writes.  So place it near
+	 * the front.
+	 *
+	 * We don't want to place it in front of _all_ writes: that
+	 * would create lots of seeking, and isn't tunable.
+	 * We try to avoid promoting this read in front of existing
+	 * reads.
+	 *
+	 * max_bomb_segments becomes the maximum number of write
+	 * requests which we allow to remain in place in front of
+	 * a newly introduced read.  We weight things a little bit,
+	 * so large writes are more expensive than small ones, but it's
+	 * requests which count, not sectors.
+	 */
+	if (max_bomb_segments && rw == READ && !passed_a_read &&
+				ret == ELEVATOR_NO_MERGE) {
+		int cur_latency = 0;
+		struct request * const cur_request = *req;
+
+		entry = head->next;
+		while (entry != &q->queue_head) {
+			struct request *__rq;
+
+			if (entry == &q->queue_head)
+				BUG();
+			if (entry == q->queue_head.next &&
+					q->head_active && !q->plugged)
+				BUG();
+			__rq = blkdev_entry_to_request(entry);
+
+			if (__rq == cur_request) {
+				/*
+				 * This is where the old algorithm placed it.
+				 * There's no point pushing it further back,
+				 * so leave it here, in sorted order.
+				 */
+				break;
+			}
+			if (__rq->cmd == WRITE) {
+				cur_latency += 1 + __rq->nr_sectors / 64;
+				if (cur_latency >= max_bomb_segments) {
+					*req = __rq;
+					break;
+				}
+			}
+			entry = entry->next;
+		}
+	}
 	return ret;
 }
 
@@ -186,7 +250,7 @@ int blkelvget_ioctl(elevator_t * elevato
 	output.queue_ID			= elevator->queue_ID;
 	output.read_latency		= elevator->read_latency;
 	output.write_latency		= elevator->write_latency;
-	output.max_bomb_segments	= 0;
+	output.max_bomb_segments	= elevator->max_bomb_segments;
 
 	if (copy_to_user(arg, &output, sizeof(blkelv_ioctl_arg_t)))
 		return -EFAULT;
@@ -205,9 +269,12 @@ int blkelvset_ioctl(elevator_t * elevato
 		return -EINVAL;
 	if (input.write_latency < 0)
 		return -EINVAL;
+	if (input.max_bomb_segments < 0)
+		return -EINVAL;
 
 	elevator->read_latency		= input.read_latency;
 	elevator->write_latency		= input.write_latency;
+	elevator->max_bomb_segments	= input.max_bomb_segments;
 	return 0;
 }
 
drivers/block/ll_rw_blk.c renamed from drivers/block/ll_rw_blk.c~read-latency2 +5 -3
@@ -432,9 +432,11 @@ static void blk_init_free_list(request_q
 
 	si_meminfo(&si);
 	megs = si.totalram >> (20 - PAGE_SHIFT);
-	nr_requests = 128;
-	if (megs < 32)
-		nr_requests /= 2;
+	nr_requests = (megs * 2) & ~15;	/* One per half-megabyte */
+	if (nr_requests < 32)
+		nr_requests = 32;
+	if (nr_requests > 1024)
+		nr_requests = 1024;
 	blk_grow_request_list(q, nr_requests);
 
 	init_waitqueue_head(&q->wait_for_requests[0]);
#2 lore raw

On Sunday 01 December 2002 22:25, you wrote:

Hi Rik,

Quoted 4 lines

That was my gut feeling as well, but I guess it's a good thing to quantify how much of a difference it makes. I wonder if we could convince Con to test a kernel both with and without this patch and look at the difference.

yep, would be a good idea. Con: *wake up ;)*

Quoted 3 lines
Quoted 1 line

So, here my patch proposal. Ontop of 2.4.20-rmap15a.

Looks good, now lets test it. If the patch is as needed as you say we should push it to marcelo ;)

yep, Andrew should do it. Anyway, all those patches do _not_ get rid of those I/O pauses/stops since 2.4.19-pre6. Andrea did a good approach with his lowlatency elevator, even if it drops throughput (needs more testing to become equivalent to throughput w/o it) and also Con and me did a Mini Lowlatency Elevator + Config option, so you can decide weather you are building for serverusage where interactive "desktop performance" is not needed ;) or not.

I wish I'll have the time to eleminate the broken code which went into 2.4.19 that causes those I/O stops.

*Repetition: those stopps do not occur with 2.4.18* ;)

ciao, Marc

#3 lore raw

Quoting Marc-Christian Petersen <[email protected]>:

Quoted 9 lines · you

Hi Rik,

Quoted 4 lines

That was my gut feeling as well, but I guess it's a good thing to quantify how much of a difference it makes. I wonder if we could convince Con to test a kernel both with and without this patch and look at the difference.

yep, would be a good idea. Con: *wake up ;)*

Sorry sleep and work intervene ;)

I have already tested it in -ck and was planning to put it into 2.4.20-ck1 when I finished it. I'll test it in vanilla to show how it works. It made a thunderous difference to io load.

Con

Quoted 22 lines
Quoted 4 lines
Quoted 2 lines

So, here my patch proposal. Ontop of 2.4.20-rmap15a.

Looks good, now lets test it. If the patch is as needed as you say we should push it to marcelo ;)

yep, Andrew should do it. Anyway, all those patches do _not_ get rid of those

I/O pauses/stops since 2.4.19-pre6. Andrea did a good approach with his lowlatency elevator, even if it drops throughput (needs more testing to become equivalent to throughput w/o it) and also Con and me did a Mini Lowlatency Elevator + Config option, so you can decide weather you are building for serverusage where interactive "desktop performance" is not needed ;) or not.

I wish I'll have the time to eleminate the broken code which went into 2.4.19

that causes those I/O stops.

*Repetition: those stopps do not occur with 2.4.18* ;)

ciao, Marc

#4 lore raw
In reply to Marc-Christian Petersen

Quoting Marc-Christian Petersen <[email protected]>:

Quoted 13 lines · you

Hi Rik,

Quoted 4 lines

That was my gut feeling as well, but I guess it's a good thing to quantify how much of a difference it makes. I wonder if we could convince Con to test a kernel both with and without this patch and look at the difference.

yep, would be a good idea. Con: *wake up ;)*

Quoted 3 lines
Quoted 1 line

So, here my patch proposal. Ontop of 2.4.20-rmap15a.

Looks good, now lets test it. If the patch is as needed as you say we should push it to marcelo ;)

Quoted 8 lines

yep, Andrew should do it. Anyway, all those patches do _not_ get rid of those

I/O pauses/stops since 2.4.19-pre6. Andrea did a good approach with his lowlatency elevator, even if it drops throughput (needs more testing to become equivalent to throughput w/o it) and also Con and me did a Mini Lowlatency Elevator + Config option, so you can decide weather you are building for serverusage where interactive "desktop performance" is not needed ;) or not.

Ok here are the contest results on the SMP machine with the read latency 2 patch (rl2) and my 3 line disk latency hack (dlh) which almost disables the elevator on vanilla 2.4.20:

io_load:

Kernel [runs]           Time    CPU%    Loads   LCPU%   Ratio
2.4.19 [5]              162.3   45      28      19      4.48
2.4.20 [5]              164.9   45      31      21      4.55
2.4.20-dlh [3]          47.3    157     0       1       1.31
2.4.20-rl2 [3]          101.8   76      19      22      2.81

io_other:

Kernel [runs]           Time    CPU%    Loads   LCPU%   Ratio
2.4.19 [5]              62.3    117     11      20      1.72
2.4.20 [5]              89.6    86      17      21      2.47
2.4.20-dlh [3]          45.0    159     0       1       1.24
2.4.20-rl2 [3]          51.8    142     10      21      1.43

Note no mention of throughput here - but implied drop off with dlh! My guess is the drop in throughput with dlh is much worse during kernel compilation than when the machine is idle. Nonetheless the dlh hack makes a significant improvement in responsivenss under IO load on a desktop.

Con

#5 lore raw
In reply to Rik van Riel
Quoted 4 lines · Rik van Riel
Quoted 1 line

So, here my patch proposal. Ontop of 2.4.20-rmap15a.

Looks good, now lets test it. If the patch is as needed as you say we should push it to marcelo ;)

Yes lets for heavens sake not fix the problem, merge the hack.

Signature · 1 line
Jens Axboe
#6 lore raw
Quoted 8 lines · Jens Axboe
Quoted 4 lines · Rik van Riel
Quoted 1 line

So, here my patch proposal. Ontop of 2.4.20-rmap15a.

Looks good, now lets test it. If the patch is as needed as you say we should push it to marcelo ;)

Yes lets for heavens sake not fix the problem, merge the hack.

If it fails to find a merge or insert the current 2.4 elevator will stick a read at the far end of the request queue. That's quite arbitrary, and is the worst possible thing to do with it.

read-latency2 will put the read a tunable distance from the head. Add a few embellishments to avoid permanent writer starvation, and that's basically all it does.

So rather than just keeping on calling it a "hack" could you please describe what is actually wrong with the idea?

#7 lore raw
Quoted 10 lines · Andrew Morton
Quoted 8 lines · Jens Axboe
Quoted 4 lines · Rik van Riel
Quoted 1 line

So, here my patch proposal. Ontop of 2.4.20-rmap15a.

Looks good, now lets test it. If the patch is as needed as you say we should push it to marcelo ;)

Yes lets for heavens sake not fix the problem, merge the hack.

Quoted 7 lines

If it fails to find a merge or insert the current 2.4 elevator will stick a read at the far end of the request queue. That's quite arbitrary, and is the worst possible thing to do with it.

read-latency2 will put the read a tunable distance from the head. Add a few embellishments to avoid permanent writer starvation, and that's basically all it does.

I just think that the design of the thing is ugly. It's clamped on to the current elevator instead of redoing the core based on the principles of read-starvation that it introduces (this is the only good thing that has come out of the patch).

Quoted 2 lines

So rather than just keeping on calling it a "hack" could you please describe what is actually wrong with the idea?

I've never said that the idea is wrong, it's the solution that is an ugly hack.

Signature · 1 line
Jens Axboe
#8 lore raw
Quoted 7 lines · Jens Axboe
Quoted 2 lines · Andrew Morton

So rather than just keeping on calling it a "hack" could you please describe what is actually wrong with the idea?

I've never said that the idea is wrong, it's the solution that is an ugly hack.

OK, do you have a better idea on how to implement this thing ?

I could be your code monkey if you don't have the time to implement something yourself.

regards,

Rik

Signature · 3 lines
Bravely reimplemented by the knights who say "NIH".
http://www.surriel.com/		http://guru.conectiva.com/
Current spamtrap:  <a href=mailto:"[email protected]">[email protected]</a>
#9 lore raw
Quoted 1 line · Rik van Riel

OK, do you have a better idea on how to implement this thing ?

Hello !

Please excuse my ignorance of the internals, but from a neutral view, I think that an efficient design could be like this :
- not one, but two elevators, one for read requests, one for write requests.
- may be one couple of these elevators for each physical device to ease parallelism, but I'm not sure.
- we would process one of the request queues (either reads or writes), and after a user-settable amount of requests processed, we would switch to the other one if it contains pending requests. For each request processed, we would take a look at the other queue, to see if a request for a very close location exists, in which case we would also switch.

This would bring the advantage of the latency/throughput balance being completely user-settable.

Please excuse me if it's impossible in the current design or if it's already done this way and fails. I just wanted to add my 2 euro-cents here.

Comments ?

Cheers,
Willy

#10 lore raw
Quoted 3 lines · Willy Tarreau

- not one, but two elevators, one for read requests, one for write requests.
- we would process one of the request queues (either reads or writes), and after a user-settable amount of requests processed, we would switch to the

OK, lets for the sake of the argument imagine such an elevator, with Read and Write queues for the following block numbers:

R: 1 3 4 5 6 20 21 22 100 110 111

W: 2 15 16 17 18 50 52 53

Now imagine what switching randomly between these queues would do for disk seeks. Especially considering that some of the writes can be "sandwiched" in-between the reads...

Rik

Signature · 3 lines
Bravely reimplemented by the knights who say "NIH".
http://www.surriel.com/		http://guru.conectiva.com/
Current spamtrap:  <a href=mailto:"[email protected]">[email protected]</a>
#11 lore raw
Quoted 10 lines · Rik van Riel
Quoted 3 lines · Willy Tarreau

- not one, but two elevators, one for read requests, one for write requests.
- we would process one of the request queues (either reads or writes), and after a user-settable amount of requests processed, we would switch to the

OK, lets for the sake of the argument imagine such an elevator, with Read and Write queues for the following block numbers:

Quoted 7 lines

R: 1 3 4 5 6 20 21 22 100 110 111

W: 2 15 16 17 18 50 52 53

Now imagine what switching randomly between these queues would do for disk seeks. Especially considering that some of the writes can be "sandwiched" in-between the reads...

Well, I don't speak about "random switching". My goal is exactly to reduce seek time *and* to bound latency. Look at your example, considering that we put no limit on the number of consecutive requests, just processing them in order would give :

R(1), W(2), R(3,4,5,6), W(15,16,17,18), R(20,21,22), W(50,52,53), R(100,110,111)

This is about what is currently done with a single elevator. Now, if we try to detect long runs of consecutive accesses based on seek length, we could optimize it this way :

W(2), R(1-22), W(15-53), R(100-111) => we only do one backwards seek

And now, if you want to lower latency for a particular usage, with a 3:1 read/write ratio, this would give :

R(1,3,4), W(2), R(5,6,20), W(15), R(21,22,100), W(16), R(110,111), W(17-53)

Of course, this won't be globally optimal, but could perhaps help *some* processes to wait less time for their data, which is the goal of inserting read requests near the head of the queue, isn't it ?

BTW, just for my understanding, what would your example look like with the current elevator (choose the ordering you like) ?

Cheers,
Willy

#12 lore raw
In reply to Rik van Riel
Quoted 6 lines · Rik van Riel
Quoted 5 lines · Jens Axboe
Quoted 2 lines · Andrew Morton

So rather than just keeping on calling it a "hack" could you please describe what is actually wrong with the idea?

Quoted 7 lines
Quoted 2 lines

I've never said that the idea is wrong, it's the solution that is an ugly hack.

OK, do you have a better idea on how to implement this thing ?

I could be your code monkey if you don't have the time to implement something yourself.

Clearly the patch addresses the problem. However, I have some doubt that it can be optimal however tuned. The more we put blocking io ahead of non-blocking io, the greater the chance that the system will get so low on memory that some of the non-blocking write may act like blocking write, or the system may smoothly become dead slow.

Ignoring all the reasons why a major change shouldn't be put into a frozen development, if there were a per-device and per-pid queue, then each time the elevator were loaded some minimum and maximum number of requests per pid could be queued. This would repeat until all requests were processed or some max number of requests was handled.

I think the effect of this would be that under light load many requests from a single process would be taken, getting the benefit of sequential io and good throughput. If the performance gain justified the overhead the per-pid queue could be sorted to keep contiguous requests grouped. If there were a lot of processes contending for the device, it would assure some degree of fair scheduling without having to scan down or insert in one large queue.

Just a thought, clearly this is too large a change to put in at the moment, and may have some drawback I missed, or less benefit. It has the advantage of not having to scan down all waiting requests for a device, which would clearly be too much overhead. The actual decisions aren't all that complex at any given point, hopefully CPU usage would reflext this.

Signature · 3 lines
bill davidsen <[email protected]>
  CTO, TMR Associates, Inc
Doing interesting things with little computers since 1979.