Re: [PATCH] block: re-introduce blk_mq_complete_request_sync

From: Ming Lei <ming.lei@redhat.com>
To: Chao Leng <lengchao@huawei.com>
Cc: Jens Axboe <axboe@kernel.dk>, Yi Zhang <yi.zhang@redhat.com>,
	Sagi Grimberg <sagi@grimberg.me>,
	linux-nvme@lists.infradead.org, linux-block@vger.kernel.org,
	Keith Busch <kbusch@kernel.org>, Christoph Hellwig <hch@lst.de>
Subject: Re: [PATCH] block: re-introduce blk_mq_complete_request_sync
Date: Mon, 12 Oct 2020 16:13:06 +0800	[thread overview]
Message-ID: <20201012081306.GB556731@T590> (raw)
In-Reply-To: <6f2a5ae2-2e6a-0386-691c-baefeecb5478@huawei.com>

On Mon, Oct 12, 2020 at 11:59:21AM +0800, Chao Leng wrote:
> 
> 
> On 2020/10/10 14:08, Yi Zhang wrote:
> > 
> > 
> > On 10/10/20 2:29 AM, Sagi Grimberg wrote:
> > > 
> > > 
> > > On 10/9/20 6:55 AM, Yi Zhang wrote:
> > > > Hi Sagi
> > > > 
> > > > On 10/9/20 4:09 PM, Sagi Grimberg wrote:
> > > > > > Hi Sagi
> > > > > > 
> > > > > > I applied this patch on block origin/for-next and still can reproduce it.
> > > > > 
> > > > > That's unexpected, can you try this patch?
> > > > > -- 
> > > > > diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
> > > > > index 629b025685d1..46428ff0b0fc 100644
> > > > > --- a/drivers/nvme/host/tcp.c
> > > > > +++ b/drivers/nvme/host/tcp.c
> > > > > @@ -2175,7 +2175,7 @@ static void nvme_tcp_complete_timed_out(struct request *rq)
> > > > >         /* fence other contexts that may complete the command */
> > > > >         mutex_lock(&to_tcp_ctrl(ctrl)->teardown_lock);
> > > > >         nvme_tcp_stop_queue(ctrl, nvme_tcp_queue_id(req->queue));
> > > > > -       if (!blk_mq_request_completed(rq)) {
> > > > > +       if (blk_mq_request_started(rq) && !blk_mq_request_completed(rq)) {
> > > > >                 nvme_req(rq)->status = NVME_SC_HOST_ABORTED_CMD;
> > > > >                 blk_mq_complete_request_sync(rq);
> > > > >         }
> This may just reduce the probability. The concurrency of timeout and teardown will cause the same request
> be treated repeatly, this is not we expected.

That is right, not like SCSI, NVME doesn't apply atomic request completion, so
request may be completed/freed from both timeout & nvme_cancel_request().

.teardown_lock still may cover the race with Sagi's patch because teardown
actually cancels requests in sync style.

> In the teardown process, after quiesced queues delete the timer and cancel the timeout work maybe a better option.

Seems better solution, given it is aligned with NVME PCI's reset
handling. nvme_sync_queues() may be called in nvme_tcp_teardown_io_queues() to
avoid this race.

Thanks, 
Ming

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme