__blk_mq_tag_busy() weirdness

__blk_mq_tag_busy() weirdness

[Hannes Reinecke]

Hi Jens,

while hunting yet another queue stall I've been looking at
__blk_mq_tag_busy():

bool __blk_mq_tag_busy(struct blk_mq_hw_ctx *hctx)
{
	if (!test_bit(BLK_MQ_S_TAG_ACTIVE, &hctx->state) &&
	    !test_and_set_bit(BLK_MQ_S_TAG_ACTIVE, &hctx->state))
		atomic_inc(&hctx->tags->active_queues);

	return true;
}

Is this some subtle usage of bitops (and I just didn't grasp it) or is
the first test_bit() pointless?
If the former I _really_ would like to have some comments on why this is
required.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@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: __blk_mq_tag_busy() weirdness

[Jens Axboe]

On 01/23/2017 07:07 AM, Hannes Reinecke wrote:
> Hi Jens,
> 
> while hunting yet another queue stall I've been looking at
> __blk_mq_tag_busy():
> 
> bool __blk_mq_tag_busy(struct blk_mq_hw_ctx *hctx)
> {
> 	if (!test_bit(BLK_MQ_S_TAG_ACTIVE, &hctx->state) &&
> 	    !test_and_set_bit(BLK_MQ_S_TAG_ACTIVE, &hctx->state))
> 		atomic_inc(&hctx->tags->active_queues);
> 
> 	return true;
> }
> 
> Is this some subtle usage of bitops (and I just didn't grasp it) or is
> the first test_bit() pointless?

Is it subtle? I've used that in a few places in blk-mq, and in mm/filemap.c
as well:

commit 7fcbbaf18392f0b17c95e2f033c8ccf87eecde1d
Author: Jens Axboe <axboe@fb.com>
Date:   Thu May 22 11:54:16 2014 -0700

    mm/filemap.c: avoid always dirtying mapping->flags on O_DIRECT

the test_and_* functions always need to be locked, and there's no point
in doing that if we only rarely need to change the value.

> If the former I _really_ would like to have some comments on why this is
> required.

Sure, but I really didn't think it was black magic.

-- 
Jens Axboe