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

[Dmitry Bogdanov <d.bogdanov@yadro.com>]

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.

> > > +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.

> > > +       if (!status) {
> > > +               reg->rtype = new_rtype;
> > > +               rcu_assign_pointer(pr->holder, reg);
> > > +       }
> > > +       return status;
> > > +}
> > > +

BR,
 Dmitry