dm, dm_table, request_queue

linux-lvm Miquel van Smoorenburg 2 participants 5 messages Feb 23, 2004 – Feb 24, 2004

Subsystems (2)

Block layer Device mapper

lore

Share & tools

lore

Miquel van Smoorenburg

author

2004-02-23 16:31 UTC #0 lore raw

I'm implementing congestion queue management as discussed in the "IO scheduler, queue depth, nr_requests" thread.

I've added a list of "parent queues" to the struct request_queue, and if the queue is marked as congested, so are the "parent queues".

Now dm needs to be able to say "add this request_queue to the list of parent" for a device.

But in dm_table, I cannot get at the request_queue of the dm device. From a quick look at the code, it seems that tables are never changed "on the fly", but built in advance and then __bind is used to bind a table to a device.

If that is the case, then I could iterate over the devices in __bind and call blk_queue_add_parent(dm_dev->bdev->queue, mapped_device->queue) (and blk_queue_remove_parent in __unbind)

Is that correct ?

And, in general, do you feel this is the correct solution ? Now request_queues know what their parents are, does that complicate things like "dm" too much or is it OK ?

Mike.

Joe Thornber

2004-02-23 20:37 UTC #1 lore raw

Quoted 1 line · Miquel van Smoorenburg

And, in general, do you feel this is the correct solution ?

No, I'm going to do what Jens Axboe suggested and add a congestion function into the request_queue itself. dm will therefore provide its own congestion function that takes the targets into account. I think this is the opposite of what you're suggesting, devices look downwards to their children devices rather than up to their parents.

- Joe

Miquel van Smoorenburg

author

2004-02-24 00:21 UTC #2 lore raw

Quoted 5 lines · Joe Thornber

Quoted 1 line · Miquel van Smoorenburg
And, in general, do you feel this is the correct solution ?
No, I'm going to do what Jens Axboe suggested and add a congestion function into the request_queue itself.

Okay, but that means that we need to add a request_queue pointer to the struct backing_dev_info. My impression was that Jens didn't like that either.

Quoted 4 lines

dm will therefore provide its
own congestion function that takes the targets into account. I think this is the opposite of what you're suggesting, devices look downwards to their children devices rather than up to their parents.

That was what I suggested, and I also implemented it this night. It works, but it is quite complicated - more so than the other way around. It does keep the simplicity in struct backing_dev_info, that's readable directly without calling indirect functions.

All in all, I think that the first approach is the simplest and therefor the best.

So how does this look:

queue_congestion_fn.patch

include/linux/backing-dev.h +3 -9

@@ -24,6 +24,7 @@
 	unsigned long ra_pages;	/* max readahead in PAGE_CACHE_SIZE units */
 	unsigned long state;	/* Always use atomic bitops on this */
 	int memory_backed;	/* Cannot clean pages with writepage */
+	void *request_queue;	/* Request queue (if this device has one) */
 };
 
 extern struct backing_dev_info default_backing_dev_info;

@@ -32,14 +33,7 @@
 int writeback_in_progress(struct backing_dev_info *bdi);
 void writeback_release(struct backing_dev_info *bdi);
 
-static inline int bdi_read_congested(struct backing_dev_info *bdi)
-{
-	return test_bit(BDI_read_congested, &bdi->state);
-}
-
-static inline int bdi_write_congested(struct backing_dev_info *bdi)
-{
-	return test_bit(BDI_write_congested, &bdi->state);
-}
+extern int bdi_read_congested(struct backing_dev_info *bdi);
+extern int bdi_write_congested(struct backing_dev_info *bdi);
 
 #endif		/* _LINUX_BACKING_DEV_H */

include/linux/blkdev.h +3 -0

@@ -244,6 +244,7 @@
 typedef int (make_request_fn) (request_queue_t *q, struct bio *bio);
 typedef int (prep_rq_fn) (request_queue_t *, struct request *);
 typedef void (unplug_fn) (void *q);
+typedef int (congested_fn) (request_queue_t *, int rw);
 
 struct bio_vec;
 typedef int (merge_bvec_fn) (request_queue_t *, struct bio *, struct bio_vec *);

@@ -290,6 +291,7 @@
 	unplug_fn		*unplug_fn;
 	merge_bvec_fn		*merge_bvec_fn;
 	activity_fn		*activity_fn;
+	congested_fn		*congested_fn;
 
 	/*
 	 * Auto-unplugging state

@@ -514,6 +516,7 @@
 extern void __blk_stop_queue(request_queue_t *q);
 extern void blk_run_queue(request_queue_t *q);
 extern void blk_queue_activity_fn(request_queue_t *, activity_fn *, void *);
+extern void blk_queue_congested_fn(request_queue_t *, congested_fn *);
 
 static inline request_queue_t *bdev_get_queue(struct block_device *bdev)
 {

drivers/block/ll_rw_blk.c +30 -0

@@ -147,6 +147,35 @@
 
 EXPORT_SYMBOL(blk_queue_activity_fn);
 
+void blk_queue_congested_fn(request_queue_t *q, congested_fn *fn)
+{
+	q->congested_fn = fn;
+}
+
+EXPORT_SYMBOL(blk_queue_congested_fn);
+
+int bdi_read_congested(struct backing_dev_info *bdi)
+{
+	request_queue_t *q = bdi->request_queue;
+
+	if (!q || !q->congested_fn)
+		return test_bit(BDI_read_congested, &bdi->state);
+	return q->congested_fn(q, READ);
+}
+
+EXPORT_SYMBOL(bdi_read_congested);
+
+int bdi_write_congested(struct backing_dev_info *bdi)
+{
+	request_queue_t *q = bdi->request_queue;
+
+	if (!q || !q->congested_fn)
+		return test_bit(BDI_write_congested, &bdi->state);
+	return q->congested_fn(q, WRITE);
+}
+
+EXPORT_SYMBOL(bdi_write_congested);
+
 /**
  * blk_queue_prep_rq - set a prepare_request function for queue
  * @q:		queue

@@ -1372,6 +1401,7 @@
 	memset(q, 0, sizeof(*q));
 	init_timer(&q->unplug_timer);
 	atomic_set(&q->refcnt, 1);
+	q->backing_dev_info.request_queue = q;
 	return q;
 }

drivers/md/dm.h +1 -0

@@ -115,6 +115,7 @@
 int dm_table_get_mode(struct dm_table *t);
 void dm_table_suspend_targets(struct dm_table *t);
 void dm_table_resume_targets(struct dm_table *t);
+int dm_table_any_congested(struct dm_table *t, int rw);
 
 /*-----------------------------------------------------------------
  * A registry of target types.

drivers/md/dm-table.c +18 -0

@@ -13,6 +13,7 @@
 #include <linux/ctype.h>
 #include <linux/slab.h>
 #include <linux/interrupt.h>
+#include <linux/backing-dev.h>
 #include <asm/atomic.h>
 
 #define MAX_DEPTH 16

@@ -857,6 +858,23 @@
 	}
 }
 
+int dm_table_any_congested(struct dm_table *t, int rw)
+{
+	struct list_head *d, *devices;
+	int r = 0;
+
+	devices = dm_table_get_devices(t);
+	for (d = devices->next; d != devices; d = d->next) {
+		struct dm_dev *dd = list_entry(d, struct dm_dev, list);
+		request_queue_t *q = bdev_get_queue(dd->bdev);
+		if (rw == WRITE)
+			r |= bdi_write_congested(&q->backing_dev_info);
+		else
+			r |= bdi_read_congested(&q->backing_dev_info);
+	}
+
+	return r;
+}
 
 EXPORT_SYMBOL(dm_get_device);
 EXPORT_SYMBOL(dm_put_device);

drivers/md/dm.c +13 -0

@@ -526,6 +526,18 @@
 	return 0;
 }
 
+static int dm_any_congested(struct request_queue *q, int rw)
+{
+	int r;
+	struct mapped_device *md = q->queuedata;
+
+	down_read(&md->lock);
+	r = dm_table_any_congested(md->map, rw);
+	up_read(&md->lock);
+
+	return r;
+}
+
 /*-----------------------------------------------------------------
  * A bitset is used to keep track of allocated minor numbers.
  *---------------------------------------------------------------*/

@@ -608,6 +620,7 @@
 	}
 
 	md->queue->queuedata = md;
+	blk_queue_congested_fn(md->queue, dm_any_congested);
 	blk_queue_make_request(md->queue, dm_request);
 
 	md->io_pool = mempool_create(MIN_IOS, mempool_alloc_slab,

Joe Thornber

2004-02-24 09:03 UTC #3 lore raw

Quoted 4 lines · Miquel van Smoorenburg

All in all, I think that the first approach is the simplest and therefor the best.
So how does this look:

Better wrt keeping mapped_device out of dm-table.c. But it still has the problem that it treats congestion in any subdevices equally (which may be a good approximation to start with).

I'll stick this in my pending queue, and look at it later in the week. I think you're quite close, given that Jens doesn't want the backing_dev pointer in the queue.

Thanks,

- Joe

Miquel van Smoorenburg

author

2004-02-24 10:27 UTC #4 lore raw

Quoted 9 lines · Joe Thornber

Quoted 4 lines · Miquel van Smoorenburg
All in all, I think that the first approach is the simplest and therefor the best.
So how does this look:
Better wrt keeping mapped_device out of dm-table.c. But it still has the problem that it treats congestion in any subdevices equally (which may be a good approximation to start with).

The only other solution I see is to 1) add a mechanism for non-blocking generic_make_request() calls (returning EAGAIN) and 2) a way to check for congestion of a block device with (offset, len) parameters.

It's hard though. You'd need request reservations on a queue that you can cancel later on if you discover that the whole multi-device request cannot be made. Something like a transaction with multiple requests.

But that's currently a bit over my head at least and not 2.6 material either I think.

Mike.