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

Sagi Grimberg
2023-03-07T11:39:38Z

On 3/6/23 23:49, Ming Lei wrote:
> On Mon, Mar 06, 2023 at 04:21:08PM +0200, Sagi Grimberg wrote:
>>
>>
>> On 3/4/23 01:13, Ming Lei wrote:
>>> When investigating one customer report on warning in nvme_setup_discard,
>>> we observed the controller(nvme/tcp) actually exposes
>>> queue_max_discard_segments(req->q) == 1.
>>>
>>> Obviously the current code can't handle this situation, since contiguity
>>> merge like normal RW request is taken.
>>>
>>> Fix the issue by building range from request sector/nr_sectors directly.
>>>
>>> Fixes: b35ba01ea697 ("nvme: support ranged discard requests")
>>> Signed-off-by: Ming Lei <ming.lei@redhat.com>
>>> ---
>>> drivers/nvme/host/core.c | 28 +++++++++++++++++++---------
>>> 1 file changed, 19 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
>>> index c2730b116dc6..d4be525f8100 100644
>>> --- a/drivers/nvme/host/core.c
>>> +++ b/drivers/nvme/host/core.c
>>> @@ -781,16 +781,26 @@ static blk_status_t nvme_setup_discard(struct nvme_ns *ns, struct request *req,
>>> range = page_address(ns->ctrl->discard_page);
>>> }
>>> - __rq_for_each_bio(bio, req) {
>>> - u64 slba = nvme_sect_to_lba(ns, bio->bi_iter.bi_sector);
>>> - u32 nlb = bio->bi_iter.bi_size >> ns->lba_shift;
>>> -
>>> - if (n < segments) {
>>> - range[n].cattr = cpu_to_le32(0);
>>> - range[n].nlb = cpu_to_le32(nlb);
>>> - range[n].slba = cpu_to_le64(slba);
>>> + if (queue_max_discard_segments(req->q) == 1) {
>>> + u64 slba = nvme_sect_to_lba(ns, blk_rq_pos(req));
>>> + u32 nlb = blk_rq_sectors(req) >> (ns->lba_shift - 9);
>>> +
>>> + range[0].cattr = cpu_to_le32(0);
>>> + range[0].nlb = cpu_to_le32(nlb);
>>> + range[0].slba = cpu_to_le64(slba);
>>> + n = 1;
>>> + } else {
>>> + __rq_for_each_bio(bio, req) {
>>> + u64 slba = nvme_sect_to_lba(ns, bio->bi_iter.bi_sector);
>>> + u32 nlb = bio->bi_iter.bi_size >> ns->lba_shift;
>>> +
>>> + if (n < segments) {
>>> + range[n].cattr = cpu_to_le32(0);
>>> + range[n].nlb = cpu_to_le32(nlb);
>>> + range[n].slba = cpu_to_le64(slba);
>>> + }
>>> + n++;
>>> }
>>> - n++;
>>> }
>>> if (WARN_ON_ONCE(n != segments)) {
>>
>>
>> Maybe just set segments to min(blk_rq_nr_discard_segments(req),
>> queue_max_discard_segments(req->q)) and let the existing code do
>> its thing?
>
> What is the existing code for applying min()?

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));
__rq_for_each_bio(bio, req) {
u64 slba = nvme_sect_to_lba(ns, bio->bi_iter.bi_sector);
u32 nlb = bio->bi_iter.bi_size >> ns->lba_shift;
--