[PATCH] pktcdvd: use BIO list management functions

[PATCH] pktcdvd: use BIO list management functions

[Akinobu Mita]

Now that the bio list management stuff is generic, convert pktcdvd to
use bio lists instead of its own private bio list implementation.

Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
Cc:  Jens Axboe <axboe@kernel.dk>
Cc: Christoph Hellwig <hch@infradead.org>
---
 drivers/block/pktcdvd.c |   89 +++++++++++------------------------------------
 include/linux/pktcdvd.h |   10 ++---
 2 files changed, 25 insertions(+), 74 deletions(-)

diff --git a/drivers/block/pktcdvd.c b/drivers/block/pktcdvd.c
index 68b5957..7cd2973 100644
--- a/drivers/block/pktcdvd.c
+++ b/drivers/block/pktcdvd.c
@@ -569,6 +569,7 @@ static struct packet_data *pkt_alloc_packet_data(int frames)
 	}
 
 	spin_lock_init(&pkt->lock);
+	bio_list_init(&pkt->orig_bios);
 
 	for (i = 0; i < frames; i++) {
 		struct bio *bio = pkt_bio_alloc(1);
@@ -721,43 +722,6 @@ static void pkt_rbtree_insert(struct pktcdvd_device *pd, struct pkt_rb_node *nod
 }
 
 /*
- * Add a bio to a single linked list defined by its head and tail pointers.
- */
-static void pkt_add_list_last(struct bio *bio, struct bio **list_head, struct bio **list_tail)
-{
-	bio->bi_next = NULL;
-	if (*list_tail) {
-		BUG_ON((*list_head) == NULL);
-		(*list_tail)->bi_next = bio;
-		(*list_tail) = bio;
-	} else {
-		BUG_ON((*list_head) != NULL);
-		(*list_head) = bio;
-		(*list_tail) = bio;
-	}
-}
-
-/*
- * Remove and return the first bio from a single linked list defined by its
- * head and tail pointers.
- */
-static inline struct bio *pkt_get_list_first(struct bio **list_head, struct bio **list_tail)
-{
-	struct bio *bio;
-
-	if (*list_head == NULL)
-		return NULL;
-
-	bio = *list_head;
-	*list_head = bio->bi_next;
-	if (*list_head == NULL)
-		*list_tail = NULL;
-
-	bio->bi_next = NULL;
-	return bio;
-}
-
-/*
  * Send a packet_command to the underlying block device and
  * wait for completion.
  */
@@ -876,13 +840,10 @@ static noinline_for_stack int pkt_set_speed(struct pktcdvd_device *pd,
 static void pkt_queue_bio(struct pktcdvd_device *pd, struct bio *bio)
 {
 	spin_lock(&pd->iosched.lock);
-	if (bio_data_dir(bio) == READ) {
-		pkt_add_list_last(bio, &pd->iosched.read_queue,
-				  &pd->iosched.read_queue_tail);
-	} else {
-		pkt_add_list_last(bio, &pd->iosched.write_queue,
-				  &pd->iosched.write_queue_tail);
-	}
+	if (bio_data_dir(bio) == READ)
+		bio_list_add(&pd->iosched.read_queue, bio);
+	else
+		bio_list_add(&pd->iosched.write_queue, bio);
 	spin_unlock(&pd->iosched.lock);
 
 	atomic_set(&pd->iosched.attention, 1);
@@ -917,8 +878,8 @@ static void pkt_iosched_process_queue(struct pktcdvd_device *pd)
 		int reads_queued, writes_queued;
 
 		spin_lock(&pd->iosched.lock);
-		reads_queued = (pd->iosched.read_queue != NULL);
-		writes_queued = (pd->iosched.write_queue != NULL);
+		reads_queued = !bio_list_empty(&pd->iosched.read_queue);
+		writes_queued = !bio_list_empty(&pd->iosched.write_queue);
 		spin_unlock(&pd->iosched.lock);
 
 		if (!reads_queued && !writes_queued)
@@ -927,7 +888,7 @@ static void pkt_iosched_process_queue(struct pktcdvd_device *pd)
 		if (pd->iosched.writing) {
 			int need_write_seek = 1;
 			spin_lock(&pd->iosched.lock);
-			bio = pd->iosched.write_queue;
+			bio = bio_list_peek(&pd->iosched.write_queue);
 			spin_unlock(&pd->iosched.lock);
 			if (bio && (bio->bi_sector == pd->iosched.last_write))
 				need_write_seek = 0;
@@ -950,13 +911,10 @@ static void pkt_iosched_process_queue(struct pktcdvd_device *pd)
 		}
 
 		spin_lock(&pd->iosched.lock);
-		if (pd->iosched.writing) {
-			bio = pkt_get_list_first(&pd->iosched.write_queue,
-						 &pd->iosched.write_queue_tail);
-		} else {
-			bio = pkt_get_list_first(&pd->iosched.read_queue,
-						 &pd->iosched.read_queue_tail);
-		}
+		if (pd->iosched.writing)
+			bio = bio_list_pop(&pd->iosched.write_queue);
+		else
+			bio = bio_list_pop(&pd->iosched.read_queue);
 		spin_unlock(&pd->iosched.lock);
 
 		if (!bio)
@@ -1114,7 +1072,7 @@ static void pkt_gather_data(struct pktcdvd_device *pd, struct packet_data *pkt)
 	int f;
 	char written[PACKET_MAX_SIZE];
 
-	BUG_ON(!pkt->orig_bios);
+	BUG_ON(bio_list_empty(&pkt->orig_bios));
 
 	atomic_set(&pkt->io_wait, 0);
 	atomic_set(&pkt->io_errors, 0);
@@ -1124,7 +1082,7 @@ static void pkt_gather_data(struct pktcdvd_device *pd, struct packet_data *pkt)
 	 */
 	memset(written, 0, sizeof(written));
 	spin_lock(&pkt->lock);
-	for (bio = pkt->orig_bios; bio; bio = bio->bi_next) {
+	bio_list_for_each(bio, &pkt->orig_bios) {
 		int first_frame = (bio->bi_sector - pkt->sector) / (CD_FRAMESIZE >> 9);
 		int num_frames = bio->bi_size / CD_FRAMESIZE;
 		pd->stats.secs_w += num_frames * (CD_FRAMESIZE >> 9);
@@ -1363,7 +1321,7 @@ try_next_bio:
 			break;
 		pkt_rbtree_erase(pd, node);
 		spin_lock(&pkt->lock);
-		pkt_add_list_last(bio, &pkt->orig_bios, &pkt->orig_bios_tail);
+		bio_list_add(&pkt->orig_bios, bio);
 		pkt->write_size += bio->bi_size / CD_FRAMESIZE;
 		spin_unlock(&pkt->lock);
 	}
@@ -1409,7 +1367,7 @@ static void pkt_start_write(struct pktcdvd_device *pd, struct packet_data *pkt)
 	 */
 	frames_write = 0;
 	spin_lock(&pkt->lock);
-	for (bio = pkt->orig_bios; bio; bio = bio->bi_next) {
+	bio_list_for_each(bio, &pkt->orig_bios) {
 		int segment = bio->bi_idx;
 		int src_offs = 0;
 		int first_frame = (bio->bi_sector - pkt->sector) / (CD_FRAMESIZE >> 9);
@@ -1472,20 +1430,14 @@ static void pkt_start_write(struct pktcdvd_device *pd, struct packet_data *pkt)
 
 static void pkt_finish_packet(struct packet_data *pkt, int uptodate)
 {
-	struct bio *bio, *next;
+	struct bio *bio;
 
 	if (!uptodate)
 		pkt->cache_valid = 0;
 
 	/* Finish all bios corresponding to this packet */
-	bio = pkt->orig_bios;
-	while (bio) {
-		next = bio->bi_next;
-		bio->bi_next = NULL;
+	while ((bio = bio_list_pop(&pkt->orig_bios)))
 		bio_endio(bio, uptodate ? 0 : -EIO);
-		bio = next;
-	}
-	pkt->orig_bios = pkt->orig_bios_tail = NULL;
 }
 
 static void pkt_run_state_machine(struct pktcdvd_device *pd, struct packet_data *pkt)
@@ -2567,8 +2519,7 @@ static int pkt_make_request(struct request_queue *q, struct bio *bio)
 			spin_lock(&pkt->lock);
 			if ((pkt->state == PACKET_WAITING_STATE) ||
 			    (pkt->state == PACKET_READ_WAIT_STATE)) {
-				pkt_add_list_last(bio, &pkt->orig_bios,
-						  &pkt->orig_bios_tail);
+				bio_list_add(&pkt->orig_bios, bio);
 				pkt->write_size += bio->bi_size / CD_FRAMESIZE;
 				if ((pkt->write_size >= pkt->frames) &&
 				    (pkt->state == PACKET_WAITING_STATE)) {
@@ -2898,6 +2849,8 @@ static int pkt_setup_dev(dev_t dev, dev_t* pkt_dev)
 
 	spin_lock_init(&pd->lock);
 	spin_lock_init(&pd->iosched.lock);
+	bio_list_init(&pd->iosched.read_queue);
+	bio_list_init(&pd->iosched.write_queue);
 	sprintf(pd->name, DRIVER_NAME"%d", idx);
 	init_waitqueue_head(&pd->wqueue);
 	pd->bio_queue = RB_ROOT;
diff --git a/include/linux/pktcdvd.h b/include/linux/pktcdvd.h
index 76e5053..721301b 100644
--- a/include/linux/pktcdvd.h
+++ b/include/linux/pktcdvd.h
@@ -163,10 +163,8 @@ struct packet_iosched
 	atomic_t		attention;	/* Set to non-zero when queue processing is needed */
 	int			writing;	/* Non-zero when writing, zero when reading */
 	spinlock_t		lock;		/* Protecting read/write queue manipulations */
-	struct bio		*read_queue;
-	struct bio		*read_queue_tail;
-	struct bio		*write_queue;
-	struct bio		*write_queue_tail;
+	struct bio_list		read_queue;
+	struct bio_list		write_queue;
 	sector_t		last_write;	/* The sector where the last write ended */
 	int			successive_reads;
 };
@@ -206,8 +204,8 @@ struct packet_data
 	spinlock_t		lock;		/* Lock protecting state transitions and */
 						/* orig_bios list */
 
-	struct bio		*orig_bios;	/* Original bios passed to pkt_make_request */
-	struct bio		*orig_bios_tail;/* that will be handled by this packet */
+	struct bio_list		orig_bios;	/* Original bios passed to pkt_make_request */
+						/* that will be handled by this packet */
 	int			write_size;	/* Total size of all bios in the orig_bios */
 						/* list, measured in number of frames */
 
-- 
1.6.0.6

[PATCH] blk-core: use BIO list management functions

[Akinobu Mita]

Now that the bio list management stuff is generic, convert
generic_make_request to use bio lists instead of its own private bio
list implementation.

Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
Cc:  Jens Axboe <axboe@kernel.dk>
Cc: Christoph Hellwig <hch@infradead.org>
---
 block/blk-core.c      |   31 ++++++++++++++-----------------
 include/linux/sched.h |    4 ++--
 2 files changed, 16 insertions(+), 19 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 718897e..44b6d69 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -1490,9 +1490,9 @@ end_io:
 /*
  * We only want one ->make_request_fn to be active at a time,
  * else stack usage with stacked devices could be a problem.
- * So use current->bio_{list,tail} to keep a list of requests
+ * So use current->bio_list to keep a list of requests
  * submited by a make_request_fn function.
- * current->bio_tail is also used as a flag to say if
+ * current->bio_list is also used as a flag to say if
  * generic_make_request is currently active in this task or not.
  * If it is NULL, then no make_request is active.  If it is non-NULL,
  * then a make_request is active, and new requests should be added
@@ -1500,11 +1500,11 @@ end_io:
  */
 void generic_make_request(struct bio *bio)
 {
-	if (current->bio_tail) {
+	struct bio_list bio_list_on_stack;
+
+	if (current->bio_list) {
 		/* make_request is active */
-		*(current->bio_tail) = bio;
-		bio->bi_next = NULL;
-		current->bio_tail = &bio->bi_next;
+		bio_list_add(current->bio_list, bio);
 		return;
 	}
 	/* following loop may be a bit non-obvious, and so deserves some
@@ -1512,30 +1512,27 @@ void generic_make_request(struct bio *bio)
 	 * Before entering the loop, bio->bi_next is NULL (as all callers
 	 * ensure that) so we have a list with a single bio.
 	 * We pretend that we have just taken it off a longer list, so
-	 * we assign bio_list to the next (which is NULL) and bio_tail
-	 * to &bio_list, thus initialising the bio_list of new bios to be
+	 * we assign bio_list to a pointer to the bio_list_on_stack,
+	 * thus initialising the bio_list of new bios to be
 	 * added.  __generic_make_request may indeed add some more bios
 	 * through a recursive call to generic_make_request.  If it
 	 * did, we find a non-NULL value in bio_list and re-enter the loop
 	 * from the top.  In this case we really did just take the bio
-	 * of the top of the list (no pretending) and so fixup bio_list and
-	 * bio_tail or bi_next, and call into __generic_make_request again.
+	 * of the top of the list (no pretending) and so remove it from
+	 * bio_list, and call into __generic_make_request again.
 	 *
 	 * The loop was structured like this to make only one call to
 	 * __generic_make_request (which is important as it is large and
 	 * inlined) and to keep the structure simple.
 	 */
 	BUG_ON(bio->bi_next);
+	bio_list_init(&bio_list_on_stack);
+	current->bio_list = &bio_list_on_stack;
 	do {
-		current->bio_list = bio->bi_next;
-		if (bio->bi_next == NULL)
-			current->bio_tail = &current->bio_list;
-		else
-			bio->bi_next = NULL;
 		__generic_make_request(bio);
-		bio = current->bio_list;
+		bio = bio_list_pop(current->bio_list);
 	} while (bio);
-	current->bio_tail = NULL; /* deactivate */
+	current->bio_list = NULL; /* deactivate */
 }
 EXPORT_SYMBOL(generic_make_request);
 
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 78efe7c..7eb8297 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -97,7 +97,7 @@ struct sched_param {
 struct exec_domain;
 struct futex_pi_state;
 struct robust_list_head;
-struct bio;
+struct bio_list;
 struct fs_struct;
 struct bts_context;
 struct perf_event_context;
@@ -1466,7 +1466,7 @@ struct task_struct {
 	void *journal_info;
 
 /* stacked block device info */
-	struct bio *bio_list, **bio_tail;
+	struct bio_list *bio_list;
 
 /* VM state */
 	struct reclaim_state *reclaim_state;
-- 
1.6.0.6

Re: [PATCH] blk-core: use BIO list management functions

[Jens Axboe]

On Mon, Feb 22 2010, Akinobu Mita wrote:
> Now that the bio list management stuff is generic, convert
> generic_make_request to use bio lists instead of its own private bio
> list implementation.

Looks good, applied. Also saves 4 bytes in task_struct, not a bad thing
:-)

-- 
Jens Axboe

Re: [PATCH] blk-core: use BIO list management functions

[Akinobu Mita]

2010/2/23 Jens Axboe <jens.axboe@oracle.com>:
> On Mon, Feb 22 2010, Akinobu Mita wrote:
>> Now that the bio list management stuff is generic, convert
>> generic_make_request to use bio lists instead of its own private bio
>> list implementation.
>
> Looks good, applied. Also saves 4 bytes in task_struct, not a bad thing
> :-)

Thanks.

How about another cleanup patch for pktcdvd?
http://lkml.org/lkml/2010/2/22/93

Re: [PATCH] blk-core: use BIO list management functions

[Jens Axboe]

On Tue, Feb 23 2010, Akinobu Mita wrote:
> 2010/2/23 Jens Axboe <jens.axboe@oracle.com>:
> > On Mon, Feb 22 2010, Akinobu Mita wrote:
> >> Now that the bio list management stuff is generic, convert
> >> generic_make_request to use bio lists instead of its own private bio
> >> list implementation.
> >
> > Looks good, applied. Also saves 4 bytes in task_struct, not a bad thing
> > :-)
> 
> Thanks.
> 
> How about another cleanup patch for pktcdvd?
> http://lkml.org/lkml/2010/2/22/93

Looks fine to me, I can pick it up if need be. Peter?

-- 
Jens Axboe

Re: [PATCH] blk-core: use BIO list management functions

[Peter Osterlund]

On Tue, 23 Feb 2010, Jens Axboe wrote:

> On Tue, Feb 23 2010, Akinobu Mita wrote:
>> 2010/2/23 Jens Axboe <jens.axboe@oracle.com>:
>>> On Mon, Feb 22 2010, Akinobu Mita wrote:
>>>> Now that the bio list management stuff is generic, convert
>>>> generic_make_request to use bio lists instead of its own private bio
>>>> list implementation.
>>>
>>> Looks good, applied. Also saves 4 bytes in task_struct, not a bad thing
>>> :-)
>>
>> Thanks.
>>
>> How about another cleanup patch for pktcdvd?
>> http://lkml.org/lkml/2010/2/22/93
>
> Looks fine to me, I can pick it up if need be. Peter?

It looks fine to me too. Please pick it up, Jens.

Thanks.

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

Re: [PATCH] blk-core: use BIO list management functions

[Jens Axboe]

On Tue, Feb 23 2010, Peter Osterlund wrote:
> On Tue, 23 Feb 2010, Jens Axboe wrote:
>
>> On Tue, Feb 23 2010, Akinobu Mita wrote:
>>> 2010/2/23 Jens Axboe <jens.axboe@oracle.com>:
>>>> On Mon, Feb 22 2010, Akinobu Mita wrote:
>>>>> Now that the bio list management stuff is generic, convert
>>>>> generic_make_request to use bio lists instead of its own private bio
>>>>> list implementation.
>>>>
>>>> Looks good, applied. Also saves 4 bytes in task_struct, not a bad thing
>>>> :-)
>>>
>>> Thanks.
>>>
>>> How about another cleanup patch for pktcdvd?
>>> http://lkml.org/lkml/2010/2/22/93
>>
>> Looks fine to me, I can pick it up if need be. Peter?
>
> It looks fine to me too. Please pick it up, Jens.

Certainly, I'll queue it up for 2.6.34. I'm assuming I can add your
acked-by?

-- 
Jens Axboe

Re: [PATCH] blk-core: use BIO list management functions

[Peter Osterlund]

On Tue, 23 Feb 2010, Jens Axboe wrote:

> On Tue, Feb 23 2010, Peter Osterlund wrote:
>> On Tue, 23 Feb 2010, Jens Axboe wrote:
>>
>>> On Tue, Feb 23 2010, Akinobu Mita wrote:
>>>> 2010/2/23 Jens Axboe <jens.axboe@oracle.com>:
>>>>> On Mon, Feb 22 2010, Akinobu Mita wrote:
>>>>>> Now that the bio list management stuff is generic, convert
>>>>>> generic_make_request to use bio lists instead of its own private bio
>>>>>> list implementation.
>>>>>
>>>>> Looks good, applied. Also saves 4 bytes in task_struct, not a bad thing
>>>>> :-)
>>>>
>>>> Thanks.
>>>>
>>>> How about another cleanup patch for pktcdvd?
>>>> http://lkml.org/lkml/2010/2/22/93
>>>
>>> Looks fine to me, I can pick it up if need be. Peter?
>>
>> It looks fine to me too. Please pick it up, Jens.
>
> Certainly, I'll queue it up for 2.6.34. I'm assuming I can add your
> acked-by?

Yes.

Acked-by: Peter Osterlund <petero2@telia.com>

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