On Thu, May 06, 2021 at 09:38:41PM +0800, Ming Lei wrote:
> > So. Even a different LUN shares the same tagset. So I can see the
> > need for the cmpxchg (please document it!), but I don't see the need
> > for the complex iteration. All the rqs are freed in one single loop,
> > so we can just iterate through them sequentially.
>
> That is exactly what the patch is doing, requests are stored in page
> list, so check if one request(covered in page list) reference in
> drv_tags->rq[i] exists, if yes, we clear the request reference.
>
> The code is actually sort of self-document: before we free requests,
> clear the reference in hostwide drv->rqs[].
What the patch does it to do a completely pointless nested loop.
Instead of just looping through all requests which is simple and fast
it loops through each page, and then does another loop inside that,
just increasing complexity and runtime. We should at least do something
like the incremental patch below instead which is simpler, faster and
easier to understand:
diff --git a/block/blk-mq.c b/block/blk-mq.c
index c1b28e09a27e..598fe82cfbcf 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -2311,29 +2311,20 @@ static size_t order_to_size(unsigned int order)
return (size_t)PAGE_SIZE << order;
}
-/* called before freeing request pool in @tags */
+/* ensure that blk_mq_find_and_get_req can't find the tags any more */
static void blk_mq_clear_rq_mapping(struct blk_mq_tag_set *set,
struct blk_mq_tags *tags, unsigned int hctx_idx)
{
struct blk_mq_tags *drv_tags = set->tags[hctx_idx];
- struct page *page;
unsigned long flags;
+ int i;
spin_lock_irqsave(&drv_tags->lock, flags);
- list_for_each_entry(page, &tags->page_list, lru) {
- unsigned long start = (unsigned long)page_address(page);
- unsigned long end = start + order_to_size(page->private);
- int i;
+ for (i = 0; i < set->queue_depth; i++) {
+ struct request *rq = drv_tags->rqs[i];
- for (i = 0; i < set->queue_depth; i++) {
- struct request *rq = drv_tags->rqs[i];
- unsigned long rq_addr = (unsigned long)rq;
-
- if (rq_addr >= start && rq_addr < end) {
- WARN_ON_ONCE(refcount_read(&rq->ref) != 0);
- cmpxchg(&drv_tags->rqs[i], rq, NULL);
- }
- }
+ WARN_ON_ONCE(refcount_read(&rq->ref) != 0);
+ cmpxchg(&drv_tags->rqs[i], rq, NULL);
}
spin_unlock_irqrestore(&drv_tags->lock, flags);
}
