Re: [RFC] [PATCH] A clean aEvgeniy pproach to writeout throttling

On Mon, Dec 10 2007, Daniel Phillips wrote:
On Monday 10 December 2007 04:32, Jens Axboe wrote:
I honestly don't know how to make this any clearer than I already did
above.

Sure you do, you could cut out the rhetoric and save lots of bandwidth
thereby.

I spent 3 mail explaining it as clearly as I could. So you're welcome
for the review and the reminder of why it's impossible to have a normal
conversation with you.

Yes, the q = bdev_get_queue(bio->bi_bdev) needs to be repeated inside
the submission loop, that was a flaw, thanks for the catch.

Precisely. So forgive me for thinking this patch hasn't seen very varied
testing, that's 2 errors (one simple, one bad - broken was NOT a gross
exageration, thanks) in very few lines.


Regards,

Daniel

--- 2.6.24-rc3-mm.clean/block/ll_rw_blk.c 2007-12-04 14:45:25.000000000 -0800
+++ 2.6.24-rc3-mm/block/ll_rw_blk.c 2007-12-10 04:49:56.000000000 -0800
@@ -3210,9 +3210,9 @@ static inline int bio_check_eod(struct b
*/
static inline void __generic_make_request(struct bio *bio)
{
- struct request_queue *q;
+ struct request_queue *q = bdev_get_queue(bio->bi_bdev);
sector_t old_sector;
- int ret, nr_sectors = bio_sectors(bio);
+ int nr_sectors = bio_sectors(bio);
dev_t old_dev;
int err = -EIO;

@@ -3221,6 +3221,13 @@ static inline void __generic_make_reques
if (bio_check_eod(bio, nr_sectors))
goto end_io;

+ if (q && q->metric && !bio->bi_queue) {
+ int need = bio->bi_throttle = q->metric(bio);
+ bio->bi_queue = q;
+ /* FIXME: potential race if atomic_sub is called in the middle of condition check */
+ wait_event(q->throttle_wait, atomic_read(&q->available) >= need);
+ atomic_sub(need, &q->available);
+ }
/*
* Resolve the mapping until finished. (drivers are
* still free to implement/resolve their own stacking
@@ -3231,10 +3238,9 @@ static inline void __generic_make_reques
*/
old_sector = -1;
old_dev = 0;
- do {
+ while (1) {
char b[BDEVNAME_SIZE];

- q = bdev_get_queue(bio->bi_bdev);
if (!q) {
printk(KERN_ERR
"generic_make_request: Trying to access "
@@ -3282,8 +3288,10 @@ end_io:
goto end_io;
}

- ret = q->make_request_fn(q, bio);
- } while (ret);
+ if (!q->make_request_fn(q, bio))
+ return;
+ q = bdev_get_queue(bio->bi_bdev);
+ }

break here please.

--- 2.6.24-rc3-mm.clean/include/linux/bio.h 2007-12-04 14:39:31.000000000 -0800
+++ 2.6.24-rc3-mm/include/linux/bio.h 2007-12-04 23:31:41.000000000 -0800
@@ -111,6 +111,9 @@ struct bio {
bio_end_io_t *bi_end_io;
atomic_t bi_cnt; /* pin count */

+ struct request_queue *bi_queue; /* for throttling */
+ unsigned bi_throttle; /* throttle metric */
+

I still wish there was a way around this, you are bloating the bio by
about 15% (yeah I know you rambled on about this, but still). Better
placement would help, so there's still low hanging fruit available.

--
Jens Axboe

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Relevant Pages

  • Re: icmp echo_user
    ... receiving an icmp echo. ... tunes itself around the bandwidth limit in effect or c) tunes the bandwidth ... bandwidth limit did not seem very useful in the mtund case. ... 'goto reflect'. ...
    (freebsd-net)
  • Re: Never witnessed so many fucking morons...
    ... Daemon Roxx spewed out this bit, and i'll scatter a few bits myself: ... entirely too much bandwidth here? ... PRINT "Windows Vista ERROR" ... GOTO 1 ...
    (alt.os.windows-xp)