Re: [PATCH v8 1/1] nvmet: support reservation feature

[Guixin Liu <kanie@linux.alibaba.com>]

在 2024/9/18 21:41, Dmitry Bogdanov 写道:
> On Wed, Sep 18, 2024 at 04:26:10PM +0800, Guixin Liu wrote:
>> Hi Dmitry,
>>
>>      My thanks for your advice, I will work on this back recently.
>>
>>
>> 在 2024/9/17 16:43, Dmitry Bogdanov 写道:
>>> Hi,
>>>
>>> Waiting for the final solution half an year we taken this patch as is.
>>> Here is my comments on it. Hope, this will bring the life to the patch.
>>>
>>>> +void nvmet_execute_get_log_page_resv(struct nvmet_req *req)
>>>> +{
>>>> +       struct nvmet_pr_log_mgr *log_mgr = &req->sq->ctrl->pr_log_mgr;
>>>> +       struct nvme_pr_log next_log = {0};
>>>> +       struct nvme_pr_log log = {0};
>>>> +       u16 status = NVME_SC_SUCCESS;
>>>> +       u64 lost_count;
>>>> +       u64 cur_count;
>>>> +       u64 next_count;
>>>> +
>>>> +       mutex_lock(&log_mgr->lock);
>>>> +       if (!kfifo_get(&log_mgr->log_queue, &log))
>>>> +               goto out;
>>>> +
>>>> +       /*
>>>> +        * We can't get the last in kfifo.
>>>> +        * Utilize the current count and the count from the next log to
>>>> +        * calculate the number of lost logs, while also addressing cases
>>>> +        * of overflow. If there is no subsequent log, the number of lost
>>>> +        * logs is equal to the lost_count within the nvmet_pr_log_mgr.
>>>> +        */
>>>> +       cur_count = le64_to_cpu(log.count);
>>>> +       if (kfifo_peek(&log_mgr->log_queue, &next_log)) {
>>>> +               next_count = le64_to_cpu(next_log.count);
>>>> +               if (next_count > cur_count)
>>>> +                       lost_count = next_count - cur_count - 1;
>>>> +               else
>>>> +                       lost_count = U64_MAX - cur_count + next_count - 1;
>>>> +       } else {
>>>> +               lost_count = log_mgr->lost_count;
>>>> +       }
>>>> +
>>>> +       log.count = cpu_to_le64(cur_count + lost_count);
>>> This value should be wrapped by U64_MAX too but in reverse direction
>>> (count is next_count-1). If == 0 then = -1.
>>>
>> The next_count will never be 0, you can see in nvmet_pr_add_resv_log, when
>>
>> the log_mgr->counter == 0, I set the log_mgr->counter to 1.
> I mean that herer tou should add the same logic as in nvmet_pr_add_resv_log.
> I am talking about this case:
> next_count = 2
> cur_count  = 0xffffffff
> lost_count = U64_MAX - cur_count + next_count - 1 =  0xffffffff - 0xffffffff + 2 - 1 = 1
> log.count = cpu_to_le64(cur_count + lost_count) = 0xffffffff + 1 = 0
>
> 0 in this case shall became 1 (not -1 as in my original comment)
>
> if (log.count == 0)
> 	log.count = 1;
>
> would fix that.
Your right, it will be changed in v9.
>>>> +static u16 nvmet_pr_create_new_reservation(struct nvmet_pr *pr, u8 new_rtype,
>>>> +                                          struct nvmet_pr_registrant *reg)
>>>> +{
>>>> +       u16 status;
>>>> +
>>>> +       status = nvmet_pr_validate_rtype(new_rtype);
>>> You shall check the validity of the command in the very beginning of
>>> command handling, not at the very end.
>> In some situations, we dont care the value of rtype, for example
>>
>> using preempt to unregister other host.
>>
>> So I only check it if needed.
> I have two arguemnts for:
> 1. It is more convenient to check a validity of a command in the beginning -
> there will be less changes to rollback. Now you do some unregistrations
> and then may fail the command due to invalid rtype, but Reservation data
> you does not change back.
>
> 2. Yes, the spec does not state that RTYPE shall be valid in Reservation Acquire Action (RACQA) field to 001b (Preempt)
> when reservation is not going to be changed. But it doesnot state that it
> might be ignored too :). In some places I see that such a statement there is.
> Also, there is such general statement for reserved values
> 	1.4.1.6 reserved
> 	Receipt of reserved coded values in defined fields in
> 	commands shall be reported as an error.
I changed rtype check to the beginning of acquire to see maintainer`s 
opinion.
>>>> +       if (!status) {
>>>> +               reg->rtype = new_rtype;
>>>> +               rcu_assign_pointer(pr->holder, reg);
>>>> +       }
>>>> +       return status;
>>>> +}
>>>> +
> BR,
>   Dmitry

Best Regards,

Guixin Liu