Re: [PATCH v2 1/2] blk-mq: Reduce the number of if-statements in blk_mq_mark_tag_wait()

[Hannes Reinecke]

On 01/10/2018 08:39 PM, Bart Van Assche wrote:
> This patch does not change any functionality but makes the
> blk_mq_mark_tag_wait() code slightly easier to read.
> 
> Signed-off-by: Bart Van Assche 
> Cc: Christoph Hellwig 
> Cc: Omar Sandoval 
> Cc: Hannes Reinecke 
> Cc: Johannes Thumshirn 
> ---
>  block/blk-mq.c | 30 +++---
>  1 file changed, 15 insertions(+), 15 deletions(-)
> 
Reviewed-by: Hannes Reinecke 

Cheers,

Hannes
-- 
Dr. Hannes ReineckeTeamlead Storage & Networking
h...@suse.de   +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH v2 1/2] blk-mq: Reduce the number of if-statements in blk_mq_mark_tag_wait()

[Bart Van Assche]

On Wed, 2018-01-10 at 13:30 -0700, Jens Axboe wrote:
> On 1/10/18 12:39 PM, Bart Van Assche wrote:
> > This patch does not change any functionality but makes the
> > blk_mq_mark_tag_wait() code slightly easier to read.
> 
> I agree it could do with a cleanup, but how about something like the
> below? I think that's easier to read.
> 
> [ ... ]

Hello Jens,

That sounds like a good idea to me. I will update the patch series, retest
it and repost.

Bart.

Re: [PATCH v2 1/2] blk-mq: Reduce the number of if-statements in blk_mq_mark_tag_wait()

[Jens Axboe]

On 1/10/18 12:39 PM, Bart Van Assche wrote:
> This patch does not change any functionality but makes the
> blk_mq_mark_tag_wait() code slightly easier to read.

I agree it could do with a cleanup, but how about something like the
below? I think that's easier to read.


diff --git a/block/blk-mq.c b/block/blk-mq.c
index 8000ba6db07d..afccd0848d6f 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -1104,58 +1104,59 @@ static bool blk_mq_mark_tag_wait(struct blk_mq_hw_ctx 
**hctx,
 struct request *rq)
 {
struct blk_mq_hw_ctx *this_hctx = *hctx;
-   bool shared_tags = (this_hctx->flags & BLK_MQ_F_TAG_SHARED) != 0;
struct sbq_wait_state *ws;
wait_queue_entry_t *wait;
bool ret;
 
-   if (!shared_tags) {
+   if (!(this_hctx->flags & BLK_MQ_F_TAG_SHARED)) {
if (!test_bit(BLK_MQ_S_SCHED_RESTART, _hctx->state))
set_bit(BLK_MQ_S_SCHED_RESTART, _hctx->state);
-   } else {
-   wait = _hctx->dispatch_wait;
-   if (!list_empty_careful(>entry))
-   return false;
 
-   spin_lock(_hctx->lock);
-   if (!list_empty(>entry)) {
-   spin_unlock(_hctx->lock);
-   return false;
-   }
+   /*
+* It's possible that a tag was freed in the window between the
+* allocation failure and adding the hardware queue to the wait
+* queue.
+*
+* Don't clear RESTART here, someone else could have set it.
+* At most this will cost an extra queue run.
+*/
+   return blk_mq_get_driver_tag(rq, hctx, false);
+   }
+
+   wait = _hctx->dispatch_wait;
+   if (!list_empty_careful(>entry))
+   return false;
 
-   ws = bt_wait_ptr(_hctx->tags->bitmap_tags, this_hctx);
-   add_wait_queue(>wait, wait);
+   spin_lock(_hctx->lock);
+   if (!list_empty(>entry)) {
+   spin_unlock(_hctx->lock);
+   return false;
}
 
+   ws = bt_wait_ptr(_hctx->tags->bitmap_tags, this_hctx);
+   add_wait_queue(>wait, wait);
+
/*
 * It's possible that a tag was freed in the window between the
 * allocation failure and adding the hardware queue to the wait
 * queue.
 */
ret = blk_mq_get_driver_tag(rq, hctx, false);
-
-   if (!shared_tags) {
-   /*
-* Don't clear RESTART here, someone else could have set it.
-* At most this will cost an extra queue run.
-*/
-   return ret;
-   } else {
-   if (!ret) {
-   spin_unlock(_hctx->lock);
-   return false;
-   }
-
-   /*
-* We got a tag, remove ourselves from the wait queue to ensure
-* someone else gets the wakeup.
-*/
-   spin_lock_irq(>wait.lock);
-   list_del_init(>entry);
-   spin_unlock_irq(>wait.lock);
+   if (!ret) {
spin_unlock(_hctx->lock);
-   return true;
+   return false;
}
+
+   /*
+* We got a tag, remove ourselves from the wait queue to ensure
+* someone else gets the wakeup.
+*/
+   spin_lock_irq(>wait.lock);
+   list_del_init(>entry);
+   spin_unlock_irq(>wait.lock);
+   spin_unlock(_hctx->lock);
+
+   return true;
 }
 
 bool blk_mq_dispatch_rq_list(struct request_queue *q, struct list_head *list,

-- 
Jens Axboe