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. :-)
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 <[email protected]>
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;
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 <[email protected]>
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 */
};
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 <[email protected]>
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 */
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 <[email protected]>
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
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