[PATCH Linux 2.6.12-rc2 00/04] blk: generic tag support fixes

[PATCH Linux 2.6.12-rc2 00/04] blk: generic tag support fixes

[Tejun Heo]

 Hello, Jens.

 These are fixes to generic tag support in the blk layer.  They all
compile okay and I've proof read it, but as I don't have any HBA which
uses generic tag support, I wasn't able to test directly.  However,
all changes are fairly straight-forward.

[ Start of patch descriptions ]

01_blk_tag_map_use_find_first_zero_bit.patch
	: use find_first_zero_bit() in blk_queue_start_tag()

	blk_queue_start_tag() hand-coded searching for the first zero
	bit in the tag map.  Replace it with find_first_zero_bit().
	With this patch, blk_queue_star_tag() doesn't need to fill
	remains of tag map with 1, thus allowing it to work properly
	with the next remove_real_max_depth patch.

02_blk_tag_map_remove_real_max_depth.patch
	: remove blk_queue_tag->real_max_depth optimization

	blk_queue_tag->real_max_depth was used to optimize out
	unnecessary allocations/frees on tag resize.  However, the
	whole thing was very broken - tag_map was never allocated to
	real_max_depth resulting in access beyond the end of the map,
	bits in [max_depth..real_max_depth] were set when initializing
	a map and copied when resizing resulting in pre-occupied tags.

	As the gain of the optimization is very small, well, almost
	nill, remove the whole thing.

03_blk_tag_map_remove_BLK_TAGS_PER_LONG.patch
	: remove BLK_TAGS_{PER_LONG|MASK}

	Replace BLK_TAGS_PER_LONG with BITS_PER_LONG and remove unused
	BLK_TAGS_MASK.

04_blk_tag_map_error_handling_cleanup.patch
	: cleanup generic tag support error messages

	Add KERN_ERR and __FUNCTION__ to generic tag error messages,
	and add a comment in blk_queue_end_tag() which explains the
	silent failure path.

[ End of patch descriptions ]

 Thanks a lot. :-)

Re: [PATCH Linux 2.6.12-rc2 01/04] blk: use find_first_zero_bit() in blk_queue_start_tag()

[Tejun Heo]

01_blk_tag_map_use_find_first_zero_bit.patch

	blk_queue_start_tag() hand-coded searching for the first zero
	bit in the tag map.  Replace it with find_first_zero_bit().
	With this patch, blk_queue_star_tag() doesn't need to fill
	remains of tag map with 1, thus allowing it to work properly
	with the next remove_real_max_depth patch.

Signed-off-by: Tejun Heo <htejun@gmail.com>

 ll_rw_blk.c |   13 ++++---------
 1 files changed, 4 insertions(+), 9 deletions(-)

Index: blk-fixes/drivers/block/ll_rw_blk.c
===================================================================
--- blk-fixes.orig/drivers/block/ll_rw_blk.c	2005-04-20 20:36:35.000000000 +0900
+++ blk-fixes/drivers/block/ll_rw_blk.c	2005-04-20 20:36:36.000000000 +0900
@@ -967,8 +967,7 @@ EXPORT_SYMBOL(blk_queue_end_tag);
 int blk_queue_start_tag(request_queue_t *q, struct request *rq)
 {
 	struct blk_queue_tag *bqt = q->queue_tags;
-	unsigned long *map = bqt->tag_map;
-	int tag = 0;
+	int tag;
 
 	if (unlikely((rq->flags & REQ_QUEUED))) {
 		printk(KERN_ERR 
@@ -977,14 +976,10 @@ int blk_queue_start_tag(request_queue_t 
 		BUG();
 	}
 
-	for (map = bqt->tag_map; *map == -1UL; map++) {
-		tag += BLK_TAGS_PER_LONG;
-
-		if (tag >= bqt->max_depth)
-			return 1;
-	}
+	tag = find_first_zero_bit(bqt->tag_map, bqt->max_depth);
+	if (tag >= bqt->max_depth)
+		return 1;
 
-	tag += ffz(*map);
 	__set_bit(tag, bqt->tag_map);
 
 	rq->flags |= REQ_QUEUED;

Re: [PATCH Linux 2.6.12-rc2 02/04] blk: remove blk_queue_tag->real_max_depth optimization

[Tejun Heo]

02_blk_tag_map_remove_real_max_depth.patch

	blk_queue_tag->real_max_depth was used to optimize out
	unnecessary allocations/frees on tag resize.  However, the
	whole thing was very broken - tag_map was never allocated to
	real_max_depth resulting in access beyond the end of the map,
	bits in [max_depth..real_max_depth] were set when initializing
	a map and copied when resizing resulting in pre-occupied tags.

	As the gain of the optimization is very small, well, almost
	nill, remove the whole thing.

Signed-off-by: Tejun Heo <htejun@gmail.com>

 drivers/block/ll_rw_blk.c |   35 ++++++++++-------------------------
 include/linux/blkdev.h    |    1 -
 2 files changed, 10 insertions(+), 26 deletions(-)

Index: blk-fixes/drivers/block/ll_rw_blk.c
===================================================================
--- blk-fixes.orig/drivers/block/ll_rw_blk.c	2005-04-20 20:36:36.000000000 +0900
+++ blk-fixes/drivers/block/ll_rw_blk.c	2005-04-20 20:36:37.000000000 +0900
@@ -716,7 +716,7 @@ struct request *blk_queue_find_tag(reque
 {
 	struct blk_queue_tag *bqt = q->queue_tags;
 
-	if (unlikely(bqt == NULL || tag >= bqt->real_max_depth))
+	if (unlikely(bqt == NULL || tag >= bqt->max_depth))
 		return NULL;
 
 	return bqt->tag_index[tag];
@@ -774,9 +774,9 @@ EXPORT_SYMBOL(blk_queue_free_tags);
 static int
 init_tag_map(request_queue_t *q, struct blk_queue_tag *tags, int depth)
 {
-	int bits, i;
 	struct request **tag_index;
 	unsigned long *tag_map;
+	int nr_ulongs;
 
 	if (depth > q->nr_requests * 2) {
 		depth = q->nr_requests * 2;
@@ -788,24 +788,17 @@ init_tag_map(request_queue_t *q, struct 
 	if (!tag_index)
 		goto fail;
 
-	bits = (depth / BLK_TAGS_PER_LONG) + 1;
-	tag_map = kmalloc(bits * sizeof(unsigned long), GFP_ATOMIC);
+	nr_ulongs = ALIGN(depth, BLK_TAGS_PER_LONG) / BLK_TAGS_PER_LONG;
+	tag_map = kmalloc(nr_ulongs * sizeof(unsigned long), GFP_ATOMIC);
 	if (!tag_map)
 		goto fail;
 
 	memset(tag_index, 0, depth * sizeof(struct request *));
-	memset(tag_map, 0, bits * sizeof(unsigned long));
+	memset(tag_map, 0, nr_ulongs * sizeof(unsigned long));
 	tags->max_depth = depth;
-	tags->real_max_depth = bits * BITS_PER_LONG;
 	tags->tag_index = tag_index;
 	tags->tag_map = tag_map;
 
-	/*
-	 * set the upper bits if the depth isn't a multiple of the word size
-	 */
-	for (i = depth; i < bits * BLK_TAGS_PER_LONG; i++)
-		__set_bit(i, tag_map);
-
 	return 0;
 fail:
 	kfree(tag_index);
@@ -870,32 +863,24 @@ int blk_queue_resize_tags(request_queue_
 	struct blk_queue_tag *bqt = q->queue_tags;
 	struct request **tag_index;
 	unsigned long *tag_map;
-	int bits, max_depth;
+	int max_depth, nr_ulongs;
 
 	if (!bqt)
 		return -ENXIO;
 
 	/*
-	 * don't bother sizing down
-	 */
-	if (new_depth <= bqt->real_max_depth) {
-		bqt->max_depth = new_depth;
-		return 0;
-	}
-
-	/*
 	 * save the old state info, so we can copy it back
 	 */
 	tag_index = bqt->tag_index;
 	tag_map = bqt->tag_map;
-	max_depth = bqt->real_max_depth;
+	max_depth = bqt->max_depth;
 
 	if (init_tag_map(q, bqt, new_depth))
 		return -ENOMEM;
 
 	memcpy(bqt->tag_index, tag_index, max_depth * sizeof(struct request *));
-	bits = max_depth / BLK_TAGS_PER_LONG;
-	memcpy(bqt->tag_map, tag_map, bits * sizeof(unsigned long));
+	nr_ulongs = ALIGN(max_depth, BLK_TAGS_PER_LONG) / BLK_TAGS_PER_LONG;
+	memcpy(bqt->tag_map, tag_map, nr_ulongs * sizeof(unsigned long));
 
 	kfree(tag_index);
 	kfree(tag_map);
@@ -925,7 +910,7 @@ void blk_queue_end_tag(request_queue_t *
 
 	BUG_ON(tag == -1);
 
-	if (unlikely(tag >= bqt->real_max_depth))
+	if (unlikely(tag >= bqt->max_depth))
 		return;
 
 	if (unlikely(!__test_and_clear_bit(tag, bqt->tag_map))) {
Index: blk-fixes/include/linux/blkdev.h
===================================================================
--- blk-fixes.orig/include/linux/blkdev.h	2005-04-20 20:36:35.000000000 +0900
+++ blk-fixes/include/linux/blkdev.h	2005-04-20 20:36:37.000000000 +0900
@@ -294,7 +294,6 @@ struct blk_queue_tag {
 	struct list_head busy_list;	/* fifo list of busy tags */
 	int busy;			/* current depth */
 	int max_depth;			/* what we will send to device */
-	int real_max_depth;		/* what the array can hold */
 	atomic_t refcnt;		/* map can be shared */
 };
 

Re: [PATCH Linux 2.6.12-rc2 03/04] blk: remove BLK_TAGS_{PER_LONG|MASK}

[Tejun Heo]

03_blk_tag_map_remove_BLK_TAGS_PER_LONG.patch

	Replace BLK_TAGS_PER_LONG with BITS_PER_LONG and remove unused
	BLK_TAGS_MASK.

Signed-off-by: Tejun Heo <htejun@gmail.com>

 drivers/block/ll_rw_blk.c |    4 ++--
 include/linux/blkdev.h    |    3 ---
 2 files changed, 2 insertions(+), 5 deletions(-)

Index: blk-fixes/drivers/block/ll_rw_blk.c
===================================================================
--- blk-fixes.orig/drivers/block/ll_rw_blk.c	2005-04-20 20:36:37.000000000 +0900
+++ blk-fixes/drivers/block/ll_rw_blk.c	2005-04-20 20:36:38.000000000 +0900
@@ -788,7 +788,7 @@ init_tag_map(request_queue_t *q, struct 
 	if (!tag_index)
 		goto fail;
 
-	nr_ulongs = ALIGN(depth, BLK_TAGS_PER_LONG) / BLK_TAGS_PER_LONG;
+	nr_ulongs = ALIGN(depth, BITS_PER_LONG) / BITS_PER_LONG;
 	tag_map = kmalloc(nr_ulongs * sizeof(unsigned long), GFP_ATOMIC);
 	if (!tag_map)
 		goto fail;
@@ -879,7 +879,7 @@ int blk_queue_resize_tags(request_queue_
 		return -ENOMEM;
 
 	memcpy(bqt->tag_index, tag_index, max_depth * sizeof(struct request *));
-	nr_ulongs = ALIGN(max_depth, BLK_TAGS_PER_LONG) / BLK_TAGS_PER_LONG;
+	nr_ulongs = ALIGN(max_depth, BITS_PER_LONG) / BITS_PER_LONG;
 	memcpy(bqt->tag_map, tag_map, nr_ulongs * sizeof(unsigned long));
 
 	kfree(tag_index);
Index: blk-fixes/include/linux/blkdev.h
===================================================================
--- blk-fixes.orig/include/linux/blkdev.h	2005-04-20 20:36:37.000000000 +0900
+++ blk-fixes/include/linux/blkdev.h	2005-04-20 20:36:38.000000000 +0900
@@ -285,9 +285,6 @@ enum blk_queue_state {
 	Queue_up,
 };
 
-#define BLK_TAGS_PER_LONG	(sizeof(unsigned long) * 8)
-#define BLK_TAGS_MASK		(BLK_TAGS_PER_LONG - 1)
-
 struct blk_queue_tag {
 	struct request **tag_index;	/* map of busy tags */
 	unsigned long *tag_map;		/* bit map of free/busy tags */

Re: [PATCH Linux 2.6.12-rc2 04/04] blk: cleanup generic tag support error messages

[Tejun Heo]

04_blk_tag_map_error_handling_cleanup.patch

	Add KERN_ERR and __FUNCTION__ to generic tag error messages,
	and add a comment in blk_queue_end_tag() which explains the
	silent failure path.

Signed-off-by: Tejun Heo <htejun@gmail.com>

 ll_rw_blk.c |   18 +++++++++++++-----
 1 files changed, 13 insertions(+), 5 deletions(-)

Index: blk-fixes/drivers/block/ll_rw_blk.c
===================================================================
--- blk-fixes.orig/drivers/block/ll_rw_blk.c	2005-04-20 20:36:38.000000000 +0900
+++ blk-fixes/drivers/block/ll_rw_blk.c	2005-04-20 20:36:40.000000000 +0900
@@ -911,10 +911,15 @@ void blk_queue_end_tag(request_queue_t *
 	BUG_ON(tag == -1);
 
 	if (unlikely(tag >= bqt->max_depth))
+		/*
+		 * This can happen after tag depth has been reduced.
+		 * FIXME: how about a warning or info message here?
+		 */
 		return;
 
 	if (unlikely(!__test_and_clear_bit(tag, bqt->tag_map))) {
-		printk("attempt to clear non-busy tag (%d)\n", tag);
+		printk(KERN_ERR "%s: attempt to clear non-busy tag (%d)\n",
+		       __FUNCTION__, tag);
 		return;
 	}
 
@@ -923,7 +928,8 @@ void blk_queue_end_tag(request_queue_t *
 	rq->tag = -1;
 
 	if (unlikely(bqt->tag_index[tag] == NULL))
-		printk("tag %d is missing\n", tag);
+		printk(KERN_ERR "%s: tag %d is missing\n",
+		       __FUNCTION__, tag);
 
 	bqt->tag_index[tag] = NULL;
 	bqt->busy--;
@@ -956,8 +962,9 @@ int blk_queue_start_tag(request_queue_t 
 
 	if (unlikely((rq->flags & REQ_QUEUED))) {
 		printk(KERN_ERR 
-		       "request %p for device [%s] already tagged %d",
-		       rq, rq->rq_disk ? rq->rq_disk->disk_name : "?", rq->tag);
+		       "%s: request %p for device [%s] already tagged %d",
+		       __FUNCTION__, rq,
+		       rq->rq_disk ? rq->rq_disk->disk_name : "?", rq->tag);
 		BUG();
 	}
 
@@ -1000,7 +1007,8 @@ void blk_queue_invalidate_tags(request_q
 		rq = list_entry_rq(tmp);
 
 		if (rq->tag == -1) {
-			printk("bad tag found on list\n");
+			printk(KERN_ERR
+			       "%s: bad tag found on list\n", __FUNCTION__);
 			list_del_init(&rq->queuelist);
 			rq->flags &= ~REQ_QUEUED;
 		} else

Re: [PATCH Linux 2.6.12-rc2 00/04] blk: generic tag support fixes

[Jens Axboe]

On Wed, Apr 20 2005, Tejun Heo wrote:
>  Hello, Jens.
> 
>  These are fixes to generic tag support in the blk layer.  They all
> compile okay and I've proof read it, but as I don't have any HBA which
> uses generic tag support, I wasn't able to test directly.  However,
> all changes are fairly straight-forward.

All patches look good, thanks!

-- 
Jens Axboe