Re: [PATCH] blk-mq: plug request for shared sbitmap

From: John Garry <john.garry@huawei.com>
To: Ming Lei <ming.lei@redhat.com>, Jens Axboe <axboe@kernel.dk>,
	"Christoph Hellwig" <hch@lst.de>
Cc: <linux-block@vger.kernel.org>, Yanhui Ma <yama@redhat.com>,
	"Bart Van Assche" <bvanassche@acm.org>,
	<kashyap.desai@broadcom.com>,
	chenxiang <chenxiang66@hisilicon.com>
Subject: Re: [PATCH] blk-mq: plug request for shared sbitmap
Date: Tue, 18 May 2021 10:44:43 +0100	[thread overview]
Message-ID: <b38d671a-530b-244a-bc0f-0b926c796243@huawei.com> (raw)
In-Reply-To: <20210514022052.1047665-1-ming.lei@redhat.com>

On 14/05/2021 03:20, Ming Lei wrote:
> In case of shared sbitmap, request won't be held in plug list any more
> sine commit 32bc15afed04 ("blk-mq: Facilitate a shared sbitmap per
> tagset"), this way makes request merge from flush plug list & batching
> submission not possible, so cause performance regression.
> 
> Yanhui reports performance regression when running sequential IO
> test(libaio, 16 jobs, 8 depth for each job) in VM, and the VM disk
> is emulated with image stored on xfs/megaraid_sas.
> 
> Fix the issue by recovering original behavior to allow to hold request
> in plug list.

Hi Ming,

Since testing v5.13-rc2, I noticed that this patch made the hang I was 
seeing disappear:
https://lore.kernel.org/linux-scsi/3d72d64d-314f-9d34-e039-7e508b2abe1b@huawei.com/

I don't think that problem is solved, though.

So I wonder about throughput performance (I had hoped to test before it 
was merged). I only have 6x SAS SSDs at hand, but I see some significant 
changes (good and bad) for mq-deadline for hisi_sas:
Before 620K (read), 300K IOPs (randread)
After 430K (read), 460-490K IOPs (randread)

none IO sched is always about 450K (read) and 500K (randread)

Do you guys have any figures? Are my results as expected?

Thanks,
John

> 
> Cc: Yanhui Ma <yama@redhat.com>
> Cc: John Garry <john.garry@huawei.com>
> Cc: Bart Van Assche <bvanassche@acm.org>
> Cc: kashyap.desai@broadcom.com
> Fixes: 32bc15afed04 ("blk-mq: Facilitate a shared sbitmap per tagset")
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
>   block/blk-mq.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index ae7f5ee41cd3..baf7a9546068 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -2236,8 +2236,9 @@ blk_qc_t blk_mq_submit_bio(struct bio *bio)
>   		/* Bypass scheduler for flush requests */
>   		blk_insert_flush(rq);
>   		blk_mq_run_hw_queue(data.hctx, true);
> -	} else if (plug && (q->nr_hw_queues == 1 || q->mq_ops->commit_rqs ||
> -				!blk_queue_nonrot(q))) {
> +	} else if (plug && (q->nr_hw_queues == 1 ||
> +		   blk_mq_is_sbitmap_shared(rq->mq_hctx->flags) ||
> +		   q->mq_ops->commit_rqs || !blk_queue_nonrot(q))) {
>   		/*
>   		 * Use plugging if we have a ->commit_rqs() hook as well, as
>   		 * we know the driver uses bd->last in a smart fashion.
> 