Re: [PATCH v7 3/9] nvme-multipath: pass I/O type to nvme_find_path()

[Nilay Shroff <nilay@linux.ibm.com>]

On 8/11/26 4:06 PM, John Garry wrote:
> On 10/08/2026 18:19, Nilay Shroff wrote:
>>> It's a but unfortunate that we have to find op_type even for when not using the latency iopolicy.
>>>
>> I looked at a few alternatives to avoid passing op_type into
>> nvme_find_path(), but couldn't find a cleaner approach.
>> Fortunately, determining op_type is inexpensive, so I don't
>> expect it to have any measurable performance impact.
> 
> All I can think of is a variant of nvme_find_path() which accepts the bio, but that means more complication and code duplication - but not too much.
> 
> As you mentioned, the processing should be small for this.
> 
>>
>>>>        if (likely(ns)) {
>>>>            bio_set_dev(bio, ns->disk->part0);
>>>>            /*
>>>> @@ -572,7 +573,7 @@ static int nvme_ns_head_get_unique_id(struct gendisk *disk, u8 id[16],
>>>>        int srcu_idx, ret = -EWOULDBLOCK;
>>>>        srcu_idx = srcu_read_lock(&head->srcu);
>>>> -    ns = nvme_find_path(head);
>>>> +    ns = nvme_find_path(head, NVME_STAT_OTHER);
>>>>        if (ns)
>>>>            ret = nvme_ns_get_unique_id(ns, id, type);
>>>>        srcu_read_unlock(&head->srcu, srcu_idx);
>>>> @@ -588,7 +589,7 @@ static int nvme_ns_head_report_zones(struct gendisk *disk, sector_t sector,
>>>>        int srcu_idx, ret = -EWOULDBLOCK;
>>>>        srcu_idx = srcu_read_lock(&head->srcu);
>>>> -    ns = nvme_find_path(head);
>>>> +    ns = nvme_find_path(head, NVME_STAT_OTHER);
>>>>        if (ns)
>>>>            ret = nvme_ns_report_zones(ns, sector, nr_zones, args);
>>>>        srcu_read_unlock(&head->srcu, srcu_idx);
>>>> diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
>>>> index 824651cc898d..8a9ec502912d 100644
>>>> --- a/drivers/nvme/host/nvme.h
>>>> +++ b/drivers/nvme/host/nvme.h
>>>> @@ -520,6 +520,13 @@ struct nvme_ns_ids {
>>>>        u8    csi;
>>>>    };
>>>> +enum nvme_stat_group {
>>>> +    NVME_STAT_READ,
>>>> +    NVME_STAT_WRITE,
>>>> +    NVME_STAT_OTHER,
>>>
>>> Would NVME_STAT_OTHER ever be used in high frequency scenarios such that it is worth having its own type? If not, could NVME_STAT_READ be reused?
>>>
>> It may not be used in high-throughput scenarios, but treating these
>> commands as READ or WRITE would unnecessarily skew the latency statistics
>> for actual read/write workloads. Keeping them in a separate category
>> avoids that distortion, so I think having NVME_STAT_OTHER makes sense.
> 
> I can't imagine that NVME_STAT_OTHER could skew the READ/WRITE results too much, but I don't know the full range of IO for which NVME_STAT_OTHER would be used (so I may be wrong).
> 
There're many opcodes which falls in other catogory here for instance,
flush/write-zeros/compare/verify/dsm and few others...

> BTW, it would be nice to mention the reason behind NVME_STAT_OTHER in the commit message. None of enum nvme_stat_group is described in much detail.
> 
Yes make sense, I'd do the needful.

Thanks,
--Nilay