[PATCH] pktdvd: stop using bdi congestion framework.

[PATCH] pktdvd: stop using bdi congestion framework.

[NeilBrown]

The bdi congestion framework isn't widely used and should be
deprecated.

pktdvd makes use of it to track congestion, but this can be done
entirely internally to pktdvd, so it doesn't need to use the framework.

So introduce a "congested" flag.  When waiting for bio_queue_size to
drop, set this flag and use wait_var_event() to wait for it.
When bio_queue_size does drop and this flag is set, clear the flag
and call wake_up_var().

Signed-off-by: NeilBrown <neilb@suse.de>
---

Note that I haven't testing this - just confirmed that it compiles.

 drivers/block/pktcdvd.c | 30 +++++++++++++++++-------------
 include/linux/pktcdvd.h |  2 ++
 2 files changed, 19 insertions(+), 13 deletions(-)

diff --git a/drivers/block/pktcdvd.c b/drivers/block/pktcdvd.c
index b53f648302c1..bf4135e52bf1 100644
--- a/drivers/block/pktcdvd.c
+++ b/drivers/block/pktcdvd.c
@@ -1107,7 +1107,6 @@ static int pkt_handle_queue(struct pktcdvd_device *pd)
 	sector_t zone = 0; /* Suppress gcc warning */
 	struct pkt_rb_node *node, *first_node;
 	struct rb_node *n;
-	int wakeup;
 
 	atomic_set(&pd->scan_queue, 0);
 
@@ -1179,12 +1178,14 @@ static int pkt_handle_queue(struct pktcdvd_device *pd)
 		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);
+	 * below, wake up any waiters
+	 */
+	if (pd->congested &&
+	    pd->bio_queue_size <= pd->write_congestion_off) {
+		pd->congested = false;
+		wake_up_var(&pd->congested);
+	}
 	spin_unlock(&pd->lock);
-	if (wakeup)
-		clear_bdi_congested(pd->disk->bdi, BLK_RW_ASYNC);
 
 	pkt->sleep_time = max(PACKET_WAIT_TIME, 1);
 	pkt_set_state(pkt, PACKET_WAITING_STATE);
@@ -2356,7 +2357,7 @@ static void pkt_make_request_write(struct request_queue *q, struct bio *bio)
 	}
 	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.
@@ -2364,12 +2365,15 @@ static void pkt_make_request_write(struct request_queue *q, struct bio *bio)
 	spin_lock(&pd->lock);
 	if (pd->write_congestion_on > 0
 	    && pd->bio_queue_size >= pd->write_congestion_on) {
-		set_bdi_congested(bio->bi_bdev->bd_disk->bdi, BLK_RW_ASYNC);
-		do {
-			spin_unlock(&pd->lock);
-			congestion_wait(BLK_RW_ASYNC, HZ);
-			spin_lock(&pd->lock);
-		} while(pd->bio_queue_size > pd->write_congestion_off);
+		___wait_var_event(&pd->congested,
+				  pd->bio_queue_size <= pd->write_congestion_off,
+				  TASK_UNINTERRUPTIBLE, 0, 0,
+				  ({pd->congested = true;
+				    spin_unlock(&pd->lock);
+				    schedule();
+				    spin_lock(&pd->lock);
+				  })
+		);
 	}
 	spin_unlock(&pd->lock);
 
diff --git a/include/linux/pktcdvd.h b/include/linux/pktcdvd.h
index 174601554b06..c391e694aa26 100644
--- a/include/linux/pktcdvd.h
+++ b/include/linux/pktcdvd.h
@@ -183,6 +183,8 @@ struct pktcdvd_device
 	spinlock_t		lock;		/* Serialize access to bio_queue */
 	struct rb_root		bio_queue;	/* Work queue of bios we need to handle */
 	int			bio_queue_size;	/* Number of nodes in bio_queue */
+	bool			congested;	/* Someone is waiting for bio_queue_size
+						 * to drop. */
 	sector_t		current_sector;	/* Keep track of where the elevator is */
 	atomic_t		scan_queue;	/* Set to non-zero when pkt_handle_queue */
 						/* needs to be run. */
-- 
2.33.1

Re: [PATCH] pktdvd: stop using bdi congestion framework.

[Christoph Hellwig]

> -		set_bdi_congested(bio->bi_bdev->bd_disk->bdi, BLK_RW_ASYNC);
> -		do {
> -			spin_unlock(&pd->lock);
> -			congestion_wait(BLK_RW_ASYNC, HZ);
> -			spin_lock(&pd->lock);
> -		} while(pd->bio_queue_size > pd->write_congestion_off);
> +		___wait_var_event(&pd->congested,
> +				  pd->bio_queue_size <= pd->write_congestion_off,
> +				  TASK_UNINTERRUPTIBLE, 0, 0,
> +				  ({pd->congested = true;
> +				    spin_unlock(&pd->lock);
> +				    schedule();
> +				    spin_lock(&pd->lock);
> +				  })

I'd be tempted to open code the ___wait_var_event here as this is pretty
unreadable.  But otherwise this change looks good to me:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH] pktdvd: stop using bdi congestion framework.

[NeilBrown]

On Wed, 17 Nov 2021, Christoph Hellwig wrote:
> > -		set_bdi_congested(bio->bi_bdev->bd_disk->bdi, BLK_RW_ASYNC);
> > -		do {
> > -			spin_unlock(&pd->lock);
> > -			congestion_wait(BLK_RW_ASYNC, HZ);
> > -			spin_lock(&pd->lock);
> > -		} while(pd->bio_queue_size > pd->write_congestion_off);
> > +		___wait_var_event(&pd->congested,
> > +				  pd->bio_queue_size <= pd->write_congestion_off,
> > +				  TASK_UNINTERRUPTIBLE, 0, 0,
> > +				  ({pd->congested = true;
> > +				    spin_unlock(&pd->lock);
> > +				    schedule();
> > +				    spin_lock(&pd->lock);
> > +				  })
> 
> I'd be tempted to open code the ___wait_var_event here as this is pretty
> unreadable.  But otherwise this change looks good to me:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>

Thanks for the review.  I'm comfortable with eiuther __wait_var_event()
or an open-coded version.  I'll follow-up with the latter.

Jens: I don't see this in linux-next.  Are you happy to take one version
or the other?

Thanks,
NeilBrown

[PATCH v3] pktdvd: stop using bdi congestion framework.

[NeilBrown]

From afac2a88e6a256ffde7040c4191a2bae5df7f5f3 Mon Sep 17 00:00:00 2001
From: NeilBrown <neilb@suse.de>
Date: Wed, 29 Sep 2021 14:24:01 +1000
Subject: [PATCH] pktdvd: stop using bdi congestion framework.

The bdi congestion framework isn't widely used and should be
deprecated.

pktdvd makes use of it to track congestion, but this can be done
entirely internally to pktdvd, so it doesn't need to use the framework.

So introduce a "congested" flag.  When waiting for bio_queue_size to
drop, set this flag and a var_waitqueue() to wait for it.  When
bio_queue_size does drop and this flag is set, clear the flag and call
wake_up_var().

We don't use a wait_var_event macro for the waiting as we need to set
the flag and drop the spinlock before calling schedule() and while that
is possible with __wait_var_event(), result is not easy to read.

Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: NeilBrown <neilb@suse.de>
---
 drivers/block/pktcdvd.c | 31 ++++++++++++++++++++-----------
 include/linux/pktcdvd.h |  2 ++
 2 files changed, 22 insertions(+), 11 deletions(-)

diff --git a/drivers/block/pktcdvd.c b/drivers/block/pktcdvd.c
index b53f648302c1..96fe68dbd6c7 100644
--- a/drivers/block/pktcdvd.c
+++ b/drivers/block/pktcdvd.c
@@ -1107,7 +1107,6 @@ static int pkt_handle_queue(struct pktcdvd_device *pd)
 	sector_t zone = 0; /* Suppress gcc warning */
 	struct pkt_rb_node *node, *first_node;
 	struct rb_node *n;
-	int wakeup;
 
 	atomic_set(&pd->scan_queue, 0);
 
@@ -1179,12 +1178,14 @@ static int pkt_handle_queue(struct pktcdvd_device *pd)
 		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);
+	 * below, wake up any waiters
+	 */
+	if (pd->congested &&
+	    pd->bio_queue_size <= pd->write_congestion_off) {
+		pd->congested = false;
+		wake_up_var(&pd->congested);
+	}
 	spin_unlock(&pd->lock);
-	if (wakeup)
-		clear_bdi_congested(pd->disk->bdi, BLK_RW_ASYNC);
 
 	pkt->sleep_time = max(PACKET_WAIT_TIME, 1);
 	pkt_set_state(pkt, PACKET_WAITING_STATE);
@@ -2356,7 +2357,7 @@ static void pkt_make_request_write(struct request_queue *q, struct bio *bio)
 	}
 	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.
@@ -2364,12 +2365,20 @@ static void pkt_make_request_write(struct request_queue *q, struct bio *bio)
 	spin_lock(&pd->lock);
 	if (pd->write_congestion_on > 0
 	    && pd->bio_queue_size >= pd->write_congestion_on) {
-		set_bdi_congested(bio->bi_bdev->bd_disk->bdi, BLK_RW_ASYNC);
-		do {
+		struct wait_bit_queue_entry wqe;
+
+		init_wait_var_entry(&wqe, &pd->congested, 0);
+		for (;;) {
+			prepare_to_wait_event(__var_waitqueue(&pd->congested),
+					      &wqe.wq_entry,
+					      TASK_UNINTERRUPTIBLE);
+			if (pd->bio_queue_size <= pd->write_congestion_off)
+				break;
+			pd->congested = true;
 			spin_unlock(&pd->lock);
-			congestion_wait(BLK_RW_ASYNC, HZ);
+			schedule();
 			spin_lock(&pd->lock);
-		} while(pd->bio_queue_size > pd->write_congestion_off);
+		}
 	}
 	spin_unlock(&pd->lock);
 
diff --git a/include/linux/pktcdvd.h b/include/linux/pktcdvd.h
index 174601554b06..c391e694aa26 100644
--- a/include/linux/pktcdvd.h
+++ b/include/linux/pktcdvd.h
@@ -183,6 +183,8 @@ struct pktcdvd_device
 	spinlock_t		lock;		/* Serialize access to bio_queue */
 	struct rb_root		bio_queue;	/* Work queue of bios we need to handle */
 	int			bio_queue_size;	/* Number of nodes in bio_queue */
+	bool			congested;	/* Someone is waiting for bio_queue_size
+						 * to drop. */
 	sector_t		current_sector;	/* Keep track of where the elevator is */
 	atomic_t		scan_queue;	/* Set to non-zero when pkt_handle_queue */
 						/* needs to be run. */
-- 
2.34.1

Re: [PATCH v3] pktdvd: stop using bdi congestion framework.

[Jens Axboe]

On Fri, 10 Dec 2021 14:53:55 +1100, NeilBrown wrote:
> From afac2a88e6a256ffde7040c4191a2bae5df7f5f3 Mon Sep 17 00:00:00 2001
> From: NeilBrown <neilb@suse.de>
> Date: Wed, 29 Sep 2021 14:24:01 +1000
> Subject: [PATCH] pktdvd: stop using bdi congestion framework.
> 
> The bdi congestion framework isn't widely used and should be
> deprecated.
> 
> [...]

Applied, thanks!

[1/1] pktdvd: stop using bdi congestion framework.
      commit: db67097aa6f2587b44055f2e16db72a11e17faef

Best regards,
-- 
Jens Axboe