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

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

On Tue, Sep 24, 2024 at 05:45:12PM +0800, Guixin Liu wrote:
> 在 2024/9/24 16:24, Dmitry Bogdanov 写道:
> > > I take a look again, if we set self new holder before call
> > > nvmet_pr_unreg_all_others_by_prkey(), the
> > > nvmet_pr_unreg_all_others_by_prkey() will
> > > 
> > > not unregister self, so this will not goto nvmet_pr_unregister_one()'s
> > > calling nvmet_pr_resv_released().
> > Yes, and this is a reason not to try to fix non-atomicity (anothter my
> > comment) by setting new holder before unregistering.
> > 
> > Regarding this place, here nvmet_pr_resv_released should be called for
> > original_rtype !=*_REG_ONLY with a note that _REG_ONLY handled in nvmet_pr_unregister_one.
> > 
> > Please, do not take my suggestions "how to fix" as a direct order, it's
> > just suggestion.
> > 
> I'm a little confused, if we dont set new holder before unregistering,
> how do we fix the non-atomicity problem?
> 
> My opinion is that setting current host to holder first can not only
> make sure that during unregistering other host can not access, but also
> ensure that nvmet_pr_unregister_one will not unregiter the new holder(In
> nvmet_pr_unreg_all_others_by_prkey, I exclude current host),
> so that we dont need to worry about doule call nvmet_pr_resv_released.

I didnot mean that that will not fix the non-atomicity.
I was worrying that in that case you will miss the existing logic for the
changing the reservation - that reservation released notification in nvmet_pr_unregister_one.

But, looking into that now I see that keeping nvmet_pr_resv_released()
here and setting the holder before unregistring others will actualy
solve both my comments.

BR,
 Dmitry