[PATCH 3/3] nvme-core: delete the dependency on REQ_FAILFAST_TRANSPORT

[PATCH 3/3] nvme-core: delete the dependency on REQ_FAILFAST_TRANSPORT

[Chao Leng]

REQ_FAILFAST_TRANSPORT may be designed for scsi, because scsi protocol
do not difine the local retry mechanism. SCSI implements a fuzzy local
retry mechanism, so need the REQ_FAILFAST_TRANSPORT for multipath
software, if work with multipath software, ultraPath determines
whether to retry and how to retry.

Nvme is different with scsi about this. It define local retry mechanism
and path error code, so nvme should retry local for non path error.
If path related error, whether to retry and how to retry is still
determined by ultraPath. REQ_FAILFAST_TRANSPORT just for non nvme
multipath software(like dm-multipath), but we do not need return an
error for REQ_FAILFAST_TRANSPORT first, because we need retry local
for non path error first.

In this way, the mechanism of nvme multipath or other multipath will be
same. The mechanism is: non path related error will be retry local,
path related error treated by multipath.

If work with dm-multipath, The patch can also fix io interrupt when io
return NVME_SC_CMD_INTERRUPTED or NVME_SC_NS_NOT_READY. Because
NVME_SC_CMD_INTERRUPTED or NVME_SC_NS_NOT_READY will be translated to
BLK_STS_TARGET. blk_path_error will treat BLK_STS_TARGET as retrying
failover path will not help, will cause io interrupt.

Signed-off-by: Chao Leng <lengchao@huawei.com>
---
 drivers/nvme/host/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 246988091c05..07471bd37f60 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -243,7 +243,7 @@ static blk_status_t nvme_error_status(u16 status)
 
 static inline bool nvme_req_needs_retry(struct request *req)
 {
-	if (blk_noretry_request(req))
+	if (req->cmd_flags & (REQ_FAILFAST_DEV | REQ_FAILFAST_DRIVER))
 		return false;
 	if (nvme_req(req)->status & NVME_SC_DNR)
 		return false;
-- 
2.16.4

Re: [PATCH 3/3] nvme-core: delete the dependency on REQ_FAILFAST_TRANSPORT

[Christoph Hellwig]

On Wed, Aug 12, 2020 at 04:18:55PM +0800, Chao Leng wrote:
> REQ_FAILFAST_TRANSPORT may be designed for scsi, because scsi protocol
> do not difine the local retry mechanism. SCSI implements a fuzzy local
> retry mechanism, so need the REQ_FAILFAST_TRANSPORT for multipath
> software, if work with multipath software, ultraPath determines
> whether to retry and how to retry.
> 
> Nvme is different with scsi about this. It define local retry mechanism
> and path error code, so nvme should retry local for non path error.
> If path related error, whether to retry and how to retry is still
> determined by ultraPath. REQ_FAILFAST_TRANSPORT just for non nvme
> multipath software(like dm-multipath), but we do not need return an
> error for REQ_FAILFAST_TRANSPORT first, because we need retry local
> for non path error first.

This doesn't look wrong, but these kinds of changes really need to
go along with block layer documentation of the intended uses of the
flags.  In fact the SCSI usage also looks really confused to me and at
very least needs better documentation if not changes.  So I think
you need to do a lot code archaeology, ping the authors and current
maintainers and sort this out as well.

More importantly if the above explanation makes sense we really need
to kill blk_noretry_request off entirely and replace it with a check
of the right set of flags in each caller as well.

Re: [PATCH 3/3] nvme-core: delete the dependency on REQ_FAILFAST_TRANSPORT

[Chao Leng]

On 2020/8/12 23:13, Christoph Hellwig wrote:
> On Wed, Aug 12, 2020 at 04:18:55PM +0800, Chao Leng wrote:
>> REQ_FAILFAST_TRANSPORT may be designed for scsi, because scsi protocol
>> do not difine the local retry mechanism. SCSI implements a fuzzy local
>> retry mechanism, so need the REQ_FAILFAST_TRANSPORT for multipath
>> software, if work with multipath software, ultraPath determines
>> whether to retry and how to retry.
>>
>> Nvme is different with scsi about this. It define local retry mechanism
>> and path error code, so nvme should retry local for non path error.
>> If path related error, whether to retry and how to retry is still
>> determined by ultraPath. REQ_FAILFAST_TRANSPORT just for non nvme
>> multipath software(like dm-multipath), but we do not need return an
>> error for REQ_FAILFAST_TRANSPORT first, because we need retry local
>> for non path error first.
> 
> This doesn't look wrong, but these kinds of changes really need to
> go along with block layer documentation of the intended uses of the
> flags.  In fact the SCSI usage also looks really confused to me and at
> very least needs better documentation if not changes.  So I think
Yes, SCSI do not define local retry, so complex processing logic is
required.
> you need to do a lot code archaeology, ping the authors and current
> maintainers and sort this out as well.
Now REQ_FAILFAST_TRANSPORT just used for multipath software, the patch
use the advantages of nvme, looks no bad effect.
> 
> More importantly if the above explanation makes sense we really need
> to kill blk_noretry_request off entirely and replace it with a check
> of the right set of flags in each caller as well.