Re: [PATCH 7/8] blk-mq: add blk_mq_all_tag_iter

From: Bart Van Assche <bvanassche@acm.org>
To: Christoph Hellwig <hch@lst.de>
Cc: linux-block@vger.kernel.org, John Garry <john.garry@huawei.com>,
	Hannes Reinecke <hare@suse.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	Ming Lei <ming.lei@redhat.com>
Subject: Re: [PATCH 7/8] blk-mq: add blk_mq_all_tag_iter
Date: Wed, 27 May 2020 15:52:44 -0700	[thread overview]
Message-ID: <413f73b7-436b-9319-b589-e6fef370fb39@acm.org> (raw)
In-Reply-To: <20200527180644.514302-8-hch@lst.de>

On 2020-05-27 11:06, Christoph Hellwig wrote:
> @@ -308,21 +312,28 @@ static void bt_tags_for_each(struct blk_mq_tags *tags, struct sbitmap_queue *bt,
>  }
>  
>  /**
> - * blk_mq_all_tag_busy_iter - iterate over all started requests in a tag map
> + * blk_mq_all_tag_iter - iterate over all requests in a tag map
>   * @tags:	Tag map to iterate over.
> - * @fn:		Pointer to the function that will be called for each started
> + * @fn:		Pointer to the function that will be called for each
>   *		request. @fn will be called as follows: @fn(rq, @priv,
>   *		reserved) where rq is a pointer to a request. 'reserved'
>   *		indicates whether or not @rq is a reserved request. Return
>   *		true to continue iterating tags, false to stop.
>   * @priv:	Will be passed as second argument to @fn.
>   */

Please document the new 'flags' argument in the kernel-doc header.

> -static void blk_mq_all_tag_busy_iter(struct blk_mq_tags *tags,
> -		busy_tag_iter_fn *fn, void *priv)
> +static void __blk_mq_all_tag_iter(struct blk_mq_tags *tags,
> +		busy_tag_iter_fn *fn, void *priv, unsigned int flags)
>  {
>  	if (tags->nr_reserved_tags)
> -		bt_tags_for_each(tags, &tags->breserved_tags, fn, priv, true);
> -	bt_tags_for_each(tags, &tags->bitmap_tags, fn, priv, false);
> +		bt_tags_for_each(tags, &tags->breserved_tags, fn, priv,
> +				 flags | BT_TAG_ITER_RESERVED);
> +	bt_tags_for_each(tags, &tags->bitmap_tags, fn, priv, flags);
> +}

The meaning of the 'reserved' argument passed to bt_for_each() and also
to busy_iter_fn is 'the request argument is a reserved request'. If
BT_TAG_ITER_RESERVED would be set in the 'flags' argument then the wrong
value will be passed for the 'reserved' argument in the second
bt_tags_for_each() call. So I think we need something like the following
at the start of __blk_mq_all_tag_iter():

	WARN_ON_ONCE(flags & BT_TAG_ITER_RESERVED)

Otherwise this patch looks good to me.

Thanks,

Bart.