[PATCH 2/2] 2.6.19-rc1-mm1 pktcdvd: bio write congestion

[PATCH 2/2] 2.6.19-rc1-mm1 pktcdvd: bio write congestion

[Thomas Maier]

[-- Attachment #1: Type: text/plain, Size: 228 bytes --]

Hello,

this adds a bio write queue congestion control
to the pktcdvd driver with fixed on/off marks.
It prevents that the driver consumes a unlimited
amount of write requests.

Signed-off-by: Thomas Maier <balagi@justmail.de>


[-- Attachment #2: pktcdvd-2-wqueue-congestion.patch --]
[-- Type: application/octet-stream, Size: 4111 bytes --]

diff -urpN 1-init-pktdev_major/drivers/block/pktcdvd.c 2-wqueue-congestion/drivers/block/pktcdvd.c
--- 1-init-pktdev_major/drivers/block/pktcdvd.c	2006-10-14 13:50:54.000000000 +0200
+++ 2-wqueue-congestion/drivers/block/pktcdvd.c	2006-10-14 14:05:28.000000000 +0200
@@ -84,6 +84,8 @@
 static struct pktcdvd_device *pkt_devs[MAX_WRITERS];
 static struct proc_dir_entry *pkt_proc;
 static int pktdev_major = 0;
+static int write_congestion_on  = PKT_WRITE_CONGESTION_ON;
+static int write_congestion_off = PKT_WRITE_CONGESTION_OFF;
 static struct mutex ctl_mutex;	/* Serialize open/close/setup/teardown */
 static mempool_t *psd_pool;
 
@@ -894,6 +896,7 @@ static int pkt_handle_queue(struct pktcd
 	sector_t zone = 0; /* Suppress gcc warning */
 	struct pkt_rb_node *node, *first_node;
 	struct rb_node *n;
+	int wakeup;
 
 	VPRINTK("handle_queue\n");
 
@@ -966,7 +969,14 @@ try_next_bio:
 		pkt->write_size += bio->bi_size / CD_FRAMESIZE;
 		spin_unlock(&pkt->lock);
 	}
+	/* check write congestion marks, and if bio_queue_size is
+	   below, wake up any waiters */
+	wakeup = (pd->write_congestion_on > 0
+	 		&& pd->bio_queue_size <= pd->write_congestion_off
+			&& waitqueue_active(&pd->write_congestion_wqueue));
 	spin_unlock(&pd->lock);
+	if (wakeup)
+		wake_up(&pd->write_congestion_wqueue);
 
 	pkt->sleep_time = max(PACKET_WAIT_TIME, 1);
 	pkt_set_state(pkt, PACKET_WAITING_STATE);
@@ -2180,6 +2190,31 @@ static int pkt_make_request(request_queu
 	spin_unlock(&pd->cdrw.active_list_lock);
 
 	/*
+	 * Test if there is enough room left in the bio work queue
+	 * (queue size >= congestion on mark).
+	 * If not, wait till the work queue size is below the congestion off mark.
+	 * This is similar to the get_request_wait() call made in the block
+	 * layer function __make_request() used for normal block i/o request
+	 * handling.
+	 */
+	spin_lock(&pd->lock);
+	if (pd->write_congestion_on > 0
+	     && pd->bio_queue_size >= pd->write_congestion_on) {
+		DEFINE_WAIT(wait);
+		do { /* wait till number of bio requests is low enough */
+			spin_unlock(&pd->lock);
+			prepare_to_wait_exclusive(&pd->write_congestion_wqueue,
+					&wait, TASK_UNINTERRUPTIBLE);
+			io_schedule();
+			/* if we are here, bio_queue_size should be below
+			   congestion_off, but be sure and do a test */
+			spin_lock(&pd->lock);
+		} while(pd->bio_queue_size > pd->write_congestion_off);
+		finish_wait(&pd->write_congestion_wqueue, &wait);
+	}
+	spin_unlock(&pd->lock);
+
+	/*
 	 * No matching packet found. Store the bio in the work queue.
 	 */
 	node = mempool_alloc(pd->rb_pool, GFP_NOIO);
@@ -2298,6 +2333,9 @@ static int pkt_seq_show(struct seq_file 
 	seq_printf(m, "\tstate:\t\t\ti:%d ow:%d rw:%d ww:%d rec:%d fin:%d\n",
 		   states[0], states[1], states[2], states[3], states[4], states[5]);
 
+	seq_printf(m, "\twrite congestion marks:\toff=%d on=%d\n",
+			pd->write_congestion_off,
+			pd->write_congestion_on);
 	return 0;
 }
 
@@ -2474,6 +2512,10 @@ static int pkt_setup_dev(dev_t dev, dev_
 	init_waitqueue_head(&pd->wqueue);
 	pd->bio_queue = RB_ROOT;
 
+	init_waitqueue_head(&pd->write_congestion_wqueue);
+	pd->write_congestion_on  = write_congestion_on;
+	pd->write_congestion_off = write_congestion_off;
+
 	disk = alloc_disk(1);
 	if (!disk)
 		goto out_mem;
diff -urpN 1-init-pktdev_major/include/linux/pktcdvd.h 2-wqueue-congestion/include/linux/pktcdvd.h
--- 1-init-pktdev_major/include/linux/pktcdvd.h	2006-10-14 13:49:52.000000000 +0200
+++ 2-wqueue-congestion/include/linux/pktcdvd.h	2006-10-14 13:58:16.000000000 +0200
@@ -112,6 +112,12 @@ struct pkt_ctrl_command {
 #include <linux/completion.h>
 #include <linux/cdrom.h>
 
+
+/* default bio write queue congestion marks */
+#define PKT_WRITE_CONGESTION_ON    10000
+#define PKT_WRITE_CONGESTION_OFF   9000
+
+
 struct packet_settings
 {
 	__u32			size;		/* packet size in (512 byte) sectors */
@@ -271,6 +277,10 @@ struct pktcdvd_device
 
 	struct packet_iosched   iosched;
 	struct gendisk		*disk;
+
+	wait_queue_head_t	write_congestion_wqueue;
+	int			write_congestion_off;
+	int			write_congestion_on;
 };
 
 #endif /* __KERNEL__ */

Re: [PATCH 2/2] 2.6.19-rc1-mm1 pktcdvd: bio write congestion

[Peter Osterlund]

"Thomas Maier" <balagi@justmail.de> writes:

> Hello,
> 
> this adds a bio write queue congestion control
> to the pktcdvd driver with fixed on/off marks.
> It prevents that the driver consumes a unlimited
> amount of write requests.

Thanks, this looks good in principle, but I think it can be
implemented in a simpler way.

Jens, can I please ask your opinion. Would it make sense to export the
clear_queue_congested() and set_queue_congested() functions in
ll_rw_blk.c so that the pktcdvd driver can use them? Something like
this patch from a few years ago:

        http://marc.theaimsgroup.com/?l=linux-kernel&m=109378210610508&w=2

-- 
Peter Osterlund - petero2@telia.com
http://web.telia.com/~u89404340

Re: [PATCH 2/2] 2.6.19-rc1-mm1 pktcdvd: bio write congestion

[Jens Axboe]

On Sat, Oct 14 2006, Peter Osterlund wrote:
> "Thomas Maier" <balagi@justmail.de> writes:
> 
> > Hello,
> > 
> > this adds a bio write queue congestion control
> > to the pktcdvd driver with fixed on/off marks.
> > It prevents that the driver consumes a unlimited
> > amount of write requests.
> 
> Thanks, this looks good in principle, but I think it can be
> implemented in a simpler way.
> 
> Jens, can I please ask your opinion. Would it make sense to export the
> clear_queue_congested() and set_queue_congested() functions in
> ll_rw_blk.c so that the pktcdvd driver can use them? Something like
> this patch from a few years ago:
> 
>         http://marc.theaimsgroup.com/?l=linux-kernel&m=109378210610508&w=2

It definitely would. Did you test, and did it work well for you? I think
it's the right approach.

-- 
Jens Axboe