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

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.