Re: [PATCH] nvme: fix handling single range discard request

Chaitanya Kulkarni
2023-03-08T05:42:41Z

>>>> Was referring to this:
>>>> --
>>>> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
>>>> index 3345f866178e..dbc402587431 100644
>>>> --- a/drivers/nvme/host/core.c
>>>> +++ b/drivers/nvme/host/core.c
>>>> @@ -781,6 +781,7 @@ static blk_status_t nvme_setup_discard(struct nvme_ns
>>>> *ns, struct request *req,
>>>> range = page_address(ns->ctrl->discard_page);
>>>> }
>>>>
>>>> + segments = min(segments, queue_max_discard_segments(req->q));
>>>
>>> That can't work.
>>>
>>> In case of queue_max_discard_segments(req->q) == 1, the request still
>>> can have more than one bios since the normal merge is taken for discard
>>> IOs.
>>
>> Ah, I see, the bios are contiguous though right?
>
> Yes, the merge is just like normal RW.
>
>> We could add a contiguity check in the loop and conditionally
>> increment n, but maybe that would probably be more complicated...
>
> That is more complicated than this patch, and the same pattern
> has been applied on virtio-blk.
>

I'd very much keep the pattern in virio-blk in nvme that is easier
to read and simple than conditional increment of the n, unless there
is a strong reason for not doing that, which I failed to
understand ...

-ck