From: Hannes Reinecke <hare@suse.de>
To: Keith Busch <keith.busch@intel.com>, Christoph Hellwig <hch@lst.de>
Cc: Jens Axboe <axboe@kernel.dk>, Ming Lei <ming.lei@redhat.com>,
linux-scsi@vger.kernel.org, linux-block@vger.kernel.org,
Martin Petersen <martin.petersen@oracle.com>,
Bart Van Assche <bvanassche@acm.org>
Subject: Re: [PATCHv4 0/3] scsi timeout handling updates
Date: Fri, 28 Dec 2018 13:47:48 +0100 [thread overview]
Message-ID: <70460671-289e-b6b0-fc3c-9bfdad00bc53@suse.de> (raw)
In-Reply-To: <20181129172034.GD8332@localhost.localdomain>
On 11/29/18 6:20 PM, Keith Busch wrote:
> On Thu, Nov 29, 2018 at 06:11:59PM +0100, Christoph Hellwig wrote:
>>> diff --git a/block/blk-mq.c b/block/blk-mq.c
>>> index a82830f39933..d0ef540711c7 100644
>>> --- a/block/blk-mq.c
>>> +++ b/block/blk-mq.c
>>> @@ -647,7 +647,7 @@ EXPORT_SYMBOL(blk_mq_complete_request);
>>>
>>> int blk_mq_request_started(struct request *rq)
>>> {
>>> - return blk_mq_rq_state(rq) != MQ_RQ_IDLE;
>>> + return blk_mq_rq_state(rq) == MQ_RQ_IN_FLIGHT;
>>> }
>>> EXPORT_SYMBOL_GPL(blk_mq_request_started);
>>
>> Independ of this series this change looks like the right thing to do.
>> But this whole area is a mine field, so separate testing would be
>> very helpful.
>>
>> I also wonder why we even bother with the above helper, a direct
>> state comparism seems a lot more obvious to the reader.
>
> I think it's just because blk_mq_rq_state() is a private interface. The
> enum mq_rq_state is defined under include/linux/, so it looks okay to
> make getting the state public too.
>
>> Last but not least the blk_mq_request_started check in nbd
>> should probably be lifted into blk_mq_tag_to_rq while we're at it..
>>
>> As for the nvme issue - it seems to me like we need to decouple the
>> nvme loop frontend request from the target backend request. In case
>> of a timeout/reset we'll complete struct request like all other nvme
>> transport drivers, but we leave the backend target state around, which
>> will be freed when it completes (or leaks when the completion is lost).
>
> I don't think nvme's loop target should do anything to help a command
> complete. It shouldn't even implement a timeout for the same reason
> no stacking block driver implements these. If a request is stuck, the
> lowest level is the only driver that should have the responsibility to
> make it unstuck.
>
Not quite.
You still need to be able to reset the controller, which you can't if
you have to wait for the lowest level.
Cheers,
Hannes
next prev parent reply other threads:[~2018-12-28 12:47 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-11-26 16:54 [PATCHv4 0/3] scsi timeout handling updates Keith Busch
2018-11-26 16:54 ` [PATCHv4 1/3] blk-mq: Return true if request was completed Keith Busch
2018-11-26 17:00 ` Christoph Hellwig
2018-11-26 16:54 ` [PATCHv4 2/3] scsi: Do not rely on blk-mq for double completions Keith Busch
2018-11-26 17:01 ` Christoph Hellwig
2018-11-26 16:54 ` [PATCHv4 3/3] blk-mq: Simplify request completion state Keith Busch
2018-11-26 17:01 ` Christoph Hellwig
2018-11-26 17:33 ` [PATCHv4 0/3] scsi timeout handling updates Jens Axboe
2018-11-28 2:20 ` Ming Lei
2018-11-28 7:00 ` Christoph Hellwig
2018-11-28 10:07 ` Ming Lei
2018-11-28 10:08 ` Christoph Hellwig
2018-11-28 15:49 ` Keith Busch
2018-11-28 15:58 ` Jens Axboe
2018-11-28 16:26 ` Keith Busch
2018-11-28 16:31 ` Jens Axboe
2018-11-28 17:56 ` Keith Busch
2018-11-29 1:18 ` Ming Lei
2018-11-28 22:31 ` Keith Busch
2018-11-28 23:36 ` Keith Busch
2018-11-29 17:11 ` Christoph Hellwig
2018-11-29 17:20 ` Keith Busch
2018-12-28 12:47 ` Hannes Reinecke [this message]
2018-11-29 2:15 ` Ming Lei
2018-11-29 2:39 ` Martin K. Petersen
2018-11-29 8:12 ` Ming Lei
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=70460671-289e-b6b0-fc3c-9bfdad00bc53@suse.de \
--to=hare@suse.de \
--cc=axboe@kernel.dk \
--cc=bvanassche@acm.org \
--cc=hch@lst.de \
--cc=keith.busch@intel.com \
--cc=linux-block@vger.kernel.org \
--cc=linux-scsi@vger.kernel.org \
--cc=martin.petersen@oracle.com \
--cc=ming.lei@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).