[RFC] [PATCH] Potential bug fix for blk_tag_queue

[RFC] [PATCH] Potential bug fix for blk_tag_queue

[Abhijeet Joglekar]

The following patch fixes a potential bug in the blk_tag_queue structure.
"busy" is used to keep track of outstanding tags, is declared as int,
and updated inside queue lock. For host-wide shared tag map, this corrupts
the value of busy, which hits BUG_ON during __blk_free_tags.

Recommend converting busy to atomic_t and using atomic_macros to access it.

---

Abhijeet Joglekar (1):
      blk-tag: Use atomic_t type for bqt->busy.


 block/blk-tag.c        |    8 ++++----
 include/linux/blkdev.h |    2 +-
 2 files changed, 5 insertions(+), 5 deletions(-)

-- 
Signature

[RFC][PATCH] blk-tag: Use atomic_t type for bqt->busy.

[Abhijeet Joglekar]

blk-tag: Use atomic_t type for bqt->busy.

bqt->busy is an integer that keeps count of number of outstanding tags. For
host-wide shared tag map, busy is accessed in a queue lock (not host lock)
which corrupts the busy value. Change busy to atomic_t and use atomic
macros to access it.

Signed-off-by: Abhijeet Joglekar <abjoglek@cisco.com>
---
 block/blk-tag.c        |    8 ++++----
 include/linux/blkdev.h |    2 +-
 2 files changed, 5 insertions(+), 5 deletions(-)


diff --git a/block/blk-tag.c b/block/blk-tag.c
index 32667be..8f37d08 100644
--- a/block/blk-tag.c
+++ b/block/blk-tag.c
@@ -38,7 +38,7 @@ static int __blk_free_tags(struct blk_queue_tag *bqt)
 
 	retval = atomic_dec_and_test(&bqt->refcnt);
 	if (retval) {
-		BUG_ON(bqt->busy);
+		BUG_ON(atomic_read(&bqt->busy));
 
 		kfree(bqt->tag_index);
 		bqt->tag_index = NULL;
@@ -147,7 +147,7 @@ static struct blk_queue_tag *__blk_queue_init_tags(struct request_queue *q,
 	if (init_tag_map(q, tags, depth))
 		goto fail;
 
-	tags->busy = 0;
+	atomic_set(&tags->busy, 0);
 	atomic_set(&tags->refcnt, 1);
 	return tags;
 fail:
@@ -313,7 +313,7 @@ void blk_queue_end_tag(struct request_queue *q, struct request *rq)
 	 * unlock memory barrier semantics.
 	 */
 	clear_bit_unlock(tag, bqt->tag_map);
-	bqt->busy--;
+	atomic_dec(&bqt->busy);
 }
 EXPORT_SYMBOL(blk_queue_end_tag);
 
@@ -368,7 +368,7 @@ int blk_queue_start_tag(struct request_queue *q, struct request *rq)
 	bqt->tag_index[tag] = rq;
 	blkdev_dequeue_request(rq);
 	list_add(&rq->queuelist, &q->tag_busy_list);
-	bqt->busy++;
+	atomic_inc(&bqt->busy);
 	return 0;
 }
 EXPORT_SYMBOL(blk_queue_start_tag);
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 88d6808..106531e 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -274,7 +274,7 @@ enum blk_queue_state {
 struct blk_queue_tag {
 	struct request **tag_index;	/* map of busy tags */
 	unsigned long *tag_map;		/* bit map of free/busy tags */
-	int busy;			/* current depth */
+	atomic_t 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: [RFC] [PATCH] Potential bug fix for blk_tag_queue

[Matthew Wilcox]

On Mon, Aug 25, 2008 at 06:03:01PM -0700, Abhijeet Joglekar wrote:
> The following patch fixes a potential bug in the blk_tag_queue structure.
> "busy" is used to keep track of outstanding tags, is declared as int,
> and updated inside queue lock. For host-wide shared tag map, this corrupts
> the value of busy, which hits BUG_ON during __blk_free_tags.

I believe you to be right.

> Recommend converting busy to atomic_t and using atomic_macros to access it.

ugh, more atomic ops.  How about just getting rid of it?  Patch
untested:

diff --git a/block/blk-tag.c b/block/blk-tag.c
index 32667be..ed5166f 100644
--- a/block/blk-tag.c
+++ b/block/blk-tag.c
@@ -38,7 +38,8 @@ static int __blk_free_tags(struct blk_queue_tag *bqt)
 
 	retval = atomic_dec_and_test(&bqt->refcnt);
 	if (retval) {
-		BUG_ON(bqt->busy);
+		BUG_ON(find_first_bit(bqt->tag_map, bqt->max_depth) <
+							bqt->max_depth);
 
 		kfree(bqt->tag_index);
 		bqt->tag_index = NULL;
@@ -147,7 +148,6 @@ static struct blk_queue_tag *__blk_queue_init_tags(struct request_queue *q,
 	if (init_tag_map(q, tags, depth))
 		goto fail;
 
-	tags->busy = 0;
 	atomic_set(&tags->refcnt, 1);
 	return tags;
 fail:
@@ -313,7 +313,6 @@ void blk_queue_end_tag(struct request_queue *q, struct request *rq)
 	 * unlock memory barrier semantics.
 	 */
 	clear_bit_unlock(tag, bqt->tag_map);
-	bqt->busy--;
 }
 EXPORT_SYMBOL(blk_queue_end_tag);
 
@@ -368,7 +367,6 @@ int blk_queue_start_tag(struct request_queue *q, struct request *rq)
 	bqt->tag_index[tag] = rq;
 	blkdev_dequeue_request(rq);
 	list_add(&rq->queuelist, &q->tag_busy_list);
-	bqt->busy++;
 	return 0;
 }
 EXPORT_SYMBOL(blk_queue_start_tag);
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index e61f22b..c66c664 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -274,7 +274,6 @@ enum blk_queue_state {
 struct blk_queue_tag {
 	struct request **tag_index;	/* map of busy tags */
 	unsigned long *tag_map;		/* bit map of free/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 */

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [RFC] [PATCH] Potential bug fix for blk_tag_queue

[Jens Axboe]

On Mon, Aug 25 2008, Matthew Wilcox wrote:
> On Mon, Aug 25, 2008 at 06:03:01PM -0700, Abhijeet Joglekar wrote:
> > The following patch fixes a potential bug in the blk_tag_queue structure.
> > "busy" is used to keep track of outstanding tags, is declared as int,
> > and updated inside queue lock. For host-wide shared tag map, this corrupts
> > the value of busy, which hits BUG_ON during __blk_free_tags.
> 
> I believe you to be right.
> 
> > Recommend converting busy to atomic_t and using atomic_macros to access it.
> 
> ugh, more atomic ops.  How about just getting rid of it?  Patch
> untested:
> 
> diff --git a/block/blk-tag.c b/block/blk-tag.c
> index 32667be..ed5166f 100644
> --- a/block/blk-tag.c
> +++ b/block/blk-tag.c
> @@ -38,7 +38,8 @@ static int __blk_free_tags(struct blk_queue_tag *bqt)
>  
>  	retval = atomic_dec_and_test(&bqt->refcnt);
>  	if (retval) {
> -		BUG_ON(bqt->busy);
> +		BUG_ON(find_first_bit(bqt->tag_map, bqt->max_depth) <
> +							bqt->max_depth);
>  
>  		kfree(bqt->tag_index);
>  		bqt->tag_index = NULL;
> @@ -147,7 +148,6 @@ static struct blk_queue_tag *__blk_queue_init_tags(struct request_queue *q,
>  	if (init_tag_map(q, tags, depth))
>  		goto fail;
>  
> -	tags->busy = 0;
>  	atomic_set(&tags->refcnt, 1);
>  	return tags;
>  fail:
> @@ -313,7 +313,6 @@ void blk_queue_end_tag(struct request_queue *q, struct request *rq)
>  	 * unlock memory barrier semantics.
>  	 */
>  	clear_bit_unlock(tag, bqt->tag_map);
> -	bqt->busy--;
>  }
>  EXPORT_SYMBOL(blk_queue_end_tag);
>  
> @@ -368,7 +367,6 @@ int blk_queue_start_tag(struct request_queue *q, struct request *rq)
>  	bqt->tag_index[tag] = rq;
>  	blkdev_dequeue_request(rq);
>  	list_add(&rq->queuelist, &q->tag_busy_list);
> -	bqt->busy++;
>  	return 0;
>  }
>  EXPORT_SYMBOL(blk_queue_start_tag);
> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> index e61f22b..c66c664 100644
> --- a/include/linux/blkdev.h
> +++ b/include/linux/blkdev.h
> @@ -274,7 +274,6 @@ enum blk_queue_state {
>  struct blk_queue_tag {
>  	struct request **tag_index;	/* map of busy tags */
>  	unsigned long *tag_map;		/* bit map of free/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 */

Thanks, we can easily kill it. We should remove blk_queue_tag_depth()
and blk_queue_tag_queue() as well, they were the 'exported' way of
getting this information. I added this for the IDE TCQ bits back in
olden days, but that stuff is no longer there. So it's probably been
unused since then.

I've applied your patch.

-- 
Jens Axboe

RE: [RFC] [PATCH] Potential bug fix for blk_tag_queue

[Abhijeet Joglekar]

Ok, removing that works too. 

Shouldn't the BUG_ON be checking upto bqt->real_max_depth instead of
bqt->depth?  

BUG_ON(find_first_bit(bqt->tag_map, bqt->real_max_depth) <
                      bqt->real_max_depth);


In cases where the tag map gets resized to a value less than the
originally allocated tag map, blk_queue_resize_tags seems to be setting
bqt->max_depth to the new_depth. 

There could still be outstanding tags max_depth <= t <= real_max_depth
which might not get freed, which the BUG_ON will not capture if it
checks against max_depth.

abhijeet

-----Original Message-----
From: Jens Axboe [mailto:jens.axboe@oracle.com] 
Sent: Tuesday, August 26, 2008 12:01 AM
To: Matthew Wilcox
Cc: Abhijeet Joglekar; linux-scsi@vger.kernel.org
Subject: Re: [RFC] [PATCH] Potential bug fix for blk_tag_queue

On Mon, Aug 25 2008, Matthew Wilcox wrote:
> On Mon, Aug 25, 2008 at 06:03:01PM -0700, Abhijeet Joglekar wrote:
> > The following patch fixes a potential bug in the blk_tag_queue
structure.
> > "busy" is used to keep track of outstanding tags, is declared as
int,
> > and updated inside queue lock. For host-wide shared tag map, this
corrupts
> > the value of busy, which hits BUG_ON during __blk_free_tags.
> 
> I believe you to be right.
> 
> > Recommend converting busy to atomic_t and using atomic_macros to
access it.
> 
> ugh, more atomic ops.  How about just getting rid of it?  Patch
> untested:
> 
> diff --git a/block/blk-tag.c b/block/blk-tag.c
> index 32667be..ed5166f 100644
> --- a/block/blk-tag.c
> +++ b/block/blk-tag.c
> @@ -38,7 +38,8 @@ static int __blk_free_tags(struct blk_queue_tag
*bqt)
>  
>  	retval = atomic_dec_and_test(&bqt->refcnt);
>  	if (retval) {
> -		BUG_ON(bqt->busy);
> +		BUG_ON(find_first_bit(bqt->tag_map, bqt->max_depth) <
> +							bqt->max_depth);
>  
>  		kfree(bqt->tag_index);
>  		bqt->tag_index = NULL;
> @@ -147,7 +148,6 @@ static struct blk_queue_tag
*__blk_queue_init_tags(struct request_queue *q,
>  	if (init_tag_map(q, tags, depth))
>  		goto fail;
>  
> -	tags->busy = 0;
>  	atomic_set(&tags->refcnt, 1);
>  	return tags;
>  fail:
> @@ -313,7 +313,6 @@ void blk_queue_end_tag(struct request_queue *q,
struct request *rq)
>  	 * unlock memory barrier semantics.
>  	 */
>  	clear_bit_unlock(tag, bqt->tag_map);
> -	bqt->busy--;
>  }
>  EXPORT_SYMBOL(blk_queue_end_tag);
>  
> @@ -368,7 +367,6 @@ int blk_queue_start_tag(struct request_queue *q,
struct request *rq)
>  	bqt->tag_index[tag] = rq;
>  	blkdev_dequeue_request(rq);
>  	list_add(&rq->queuelist, &q->tag_busy_list);
> -	bqt->busy++;
>  	return 0;
>  }
>  EXPORT_SYMBOL(blk_queue_start_tag);
> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> index e61f22b..c66c664 100644
> --- a/include/linux/blkdev.h
> +++ b/include/linux/blkdev.h
> @@ -274,7 +274,6 @@ enum blk_queue_state {
>  struct blk_queue_tag {
>  	struct request **tag_index;	/* map of busy tags */
>  	unsigned long *tag_map;		/* bit map of free/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 */

Thanks, we can easily kill it. We should remove blk_queue_tag_depth()
and blk_queue_tag_queue() as well, they were the 'exported' way of
getting this information. I added this for the IDE TCQ bits back in
olden days, but that stuff is no longer there. So it's probably been
unused since then.

I've applied your patch.

-- 
Jens Axboe

Re: [RFC] [PATCH] Potential bug fix for blk_tag_queue

[Jens Axboe]

(please don't top post!)

On Tue, Aug 26 2008, Abhijeet Joglekar wrote:
> Ok, removing that works too. 
> 
> Shouldn't the BUG_ON be checking upto bqt->real_max_depth instead of
> bqt->depth?  
> 
> BUG_ON(find_first_bit(bqt->tag_map, bqt->real_max_depth) <
>                       bqt->real_max_depth);
> 
> 
> In cases where the tag map gets resized to a value less than the
> originally allocated tag map, blk_queue_resize_tags seems to be setting
> bqt->max_depth to the new_depth. 
> 
> There could still be outstanding tags max_depth <= t <= real_max_depth
> which might not get freed, which the BUG_ON will not capture if it
> checks against max_depth.

max_depth is fine, it's the safer choice. For the real_max_depth to
potentially catch any extra offenders, you would have to do a resize to
a larger depth, get bunch of IO issued, then resize down and shut down
the queue. Using real_max_depth should work as well, but then you have
to audit the ending of tags > max/real_max_depth. It looks ok, though.

-- 
Jens Axboe