Queue congestion: passing down vs passing up [PATCH]

linux-lvm Miquel van Smoorenburg 1 participant 1 message Feb 25, 2004

Subsystems (2)

Block layer Device mapper

lore

Share & tools

lore

Miquel van Smoorenburg

author

2004-02-25 14:15 UTC #0 lore raw

[Resent since I didn't see it show up on the linux-lvm list yet-
apologies if you get this twice]
[weeded out the enormous Cc: list]

Quoted 27 lines · Jens Axboe

Quoted 18 lines · Miquel van Smoorenburg
Quoted 10 lines
Quoted 3 lines
Even if it isn't happening
a lot, and something isn't bust it might be a good idea to do this.
Seems OK from a quick check. pdflush will block in get_request_wait() occasionally, but not at all often. Perhaps we could move the write_congested test into the mpage_writepages() inner loop but it hardly seems worth the risk.
Maybe things are different on Miquel's clockwork controller.
I haven't tested it yet because of the "This patch isn't actually so good" comment, but I found another explanation.
Quoted 3 lines
 drivers/block/ll_rw_blk.c |    2 ++
 fs/fs-writeback.c         |    2 ++
 2 files changed, 4 insertions(+)
Quoted 7 lines
*Lightbulb on* .. I just read fs-writeback.c. As I said, this happens with an LVM device. Could it be that because LVM and the actual device have different struct request_queue's things go awry ?
In fs-writeback.c, your're looking at the LVM device (and its request_queue, and its backing_dev_info). In__make_request, you're looking at the SCSI device.

Quoted 6 lines

In principle, the lvm/md queues themselves will never be congested. But the underlying queues can be, of course.
Now this approach is _much_ better, imo. I don't particularly care very much for how you solved it, though, I'd much rather just see both setting and testing passed down (and kill the ->aux as well).

I just sent a patch to linux-lvm / Joe Thornber that does this. It adds a void *request_queue to the struct backing_dev_info, and a congestion_fn to the request_queue_t struct. bdi_{read,write}_congested got moved to ll_rw_blk.c. dm.c sets the congestion_fn to an internal function that checks the congestion of all the underlying block devices.

See https://www.redhat.com/archives/linux-lvm/2004-February/msg00203.html
for the actual patch.

So that's testing being passed down.

The setting you cannot pass down - you need to pass it "up". I implemented that too, added a list of "parent queues" to the request_queue, and a function so that for example dm can register its queue as "parent" of an underlying device.

It needs to be a list, since different partitions on one device can be members of different meta-devices.

When the congestion functions in ll_rw_blk.c are called, if a queue is congested, that info is also passed "up" to all of the "parent" queues. backing_dev_info uses a counter instead of a bit to mark read/write as congested. Perhaps it would be better to call a function (congestion_fn).

I think that we only need one of the two - either testing being passed down or setting being passed up. The first one is the easiest. The second one keeps the struct backing_dev_info clean (no request_queue pointer).

What would you prefer ?

For reference, a first cut at the "passing up queue congestion" is below.

Mike.

queue-parent.patch

include/linux/backing-dev.h +4 -2

@@ -24,6 +24,8 @@
 	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 */
+	int read_congested;	/* Write queue or child write queue congested */
+	int write_congested;	/* Read queue or child read queue congested */
 };
 
 extern struct backing_dev_info default_backing_dev_info;

@@ -34,12 +36,12 @@
 
 static inline int bdi_read_congested(struct backing_dev_info *bdi)
 {
-	return test_bit(BDI_read_congested, &bdi->state);
+	return bdi->read_congested;
 }
 
 static inline int bdi_write_congested(struct backing_dev_info *bdi)
 {
-	return test_bit(BDI_write_congested, &bdi->state);
+	return bdi->write_congested;
 }
 
 #endif		/* _LINUX_BACKING_DEV_H */

include/linux/blkdev.h +9 -0

@@ -267,6 +267,11 @@
 	atomic_t refcnt;		/* map can be shared */
 };
 
+struct rqueue_list {
+	struct list_head	rqueue_list_head;
+	struct request_queue	*rqueue;
+};
+
 struct request_queue
 {
 	/*

@@ -301,6 +306,8 @@
 
 	struct backing_dev_info	backing_dev_info;
 
+	struct list_head	parents;
+
 	/*
 	 * The queue owner gets to use this for whatever they like.
 	 * ll_rw_blk doesn't touch it.

@@ -514,6 +521,8 @@
 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 int blk_queue_add_parent(request_queue_t *q, request_queue_t *parent);
+extern void blk_queue_remove_parent(request_queue_t *q, request_queue_t *parent);
 
 static inline request_queue_t *bdev_get_queue(struct block_device *bdev)
 {

drivers/block/ll_rw_blk.c +105 -8

157 lines not on this page · raw message

drivers/md/dm.h +2 -0

@@ -110,6 +110,8 @@
 struct dm_target *dm_table_get_target(struct dm_table *t, unsigned int index);
 struct dm_target *dm_table_find_target(struct dm_table *t, sector_t sector);
 void dm_table_set_restrictions(struct dm_table *t, struct request_queue *q);
+void dm_table_add_parent(struct dm_table *t, struct request_queue *q);
+void dm_table_remove_parent(struct dm_table *t, struct request_queue *q);
 unsigned int dm_table_get_num_targets(struct dm_table *t);
 struct list_head *dm_table_get_devices(struct dm_table *t);
 int dm_table_get_mode(struct dm_table *t);

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

@@ -818,6 +818,30 @@
 	q->seg_boundary_mask = t->limits.seg_boundary_mask;
 }
 
+void dm_table_add_parent(struct dm_table *t, struct request_queue *q)
+{
+	struct list_head *d, *devices;
+
+	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 *bq = bdev_get_queue(dd->bdev);
+		blk_queue_add_parent(bq, q);
+	}
+}
+
+void dm_table_remove_parent(struct dm_table *t, struct request_queue *q)
+{
+	struct list_head *d, *devices;
+
+	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 *bq = bdev_get_queue(dd->bdev);
+		blk_queue_remove_parent(bq, q);
+	}
+}
+
 unsigned int dm_table_get_num_targets(struct dm_table *t)
 {
 	return t->num_targets;

drivers/md/dm.c +5 -1

@@ -41,6 +41,7 @@
 struct mapped_device {
 	struct rw_semaphore lock;
 	atomic_t holders;
+	spinlock_t queue_lock;
 
 	unsigned long flags;

@@ -600,7 +601,7 @@
 	memset(md, 0, sizeof(*md));
 	init_rwsem(&md->lock);
 	atomic_set(&md->holders, 1);
-
+	md->queue_lock = SPIN_LOCK_UNLOCKED;
 	md->queue = blk_alloc_queue(GFP_KERNEL);
 	if (!md->queue) {
 		kfree(md);

@@ -608,6 +609,7 @@
 	}
 
 	md->queue->queuedata = md;
+	md->queue->queue_lock = &md->queue_lock;
 	blk_queue_make_request(md->queue, dm_request);
 
 	md->io_pool = mempool_create(MIN_IOS, mempool_alloc_slab,

@@ -695,6 +697,7 @@
 
 	dm_table_get(t);
 	dm_table_set_restrictions(t, q);
+	dm_table_add_parent(t, q);
 	return 0;
 }

@@ -704,6 +707,7 @@
 		return;
 
 	dm_table_event_callback(md->map, NULL, NULL);
+	dm_table_remove_parent(md->map, md->queue);
 	dm_table_put(md->map);
 	md->map = NULL;
 }