Re: [dm-devel] [PATCH 5/5] add prflag to path

[lixiaokeng <lixiaokeng@huawei.com>]

On 2021/11/19 0:57, Benjamin Marzinski wrote:
> On Tue, Nov 16, 2021 at 10:01:15PM +0800, lixiaokeng wrote:
>> The update_map will frequently be called and there will be
>> unnecessary checks of reseravtion. We add prflag to path
>> to avoid this.
>>
>> The pp->state changes from others to up or ghost, the
>> mpath_pr_event_handle should be called. The
>> mpath_pr_event_handle in ev_add_path may not be called,
>> so set pp->prkey PRKEY_NO when path is removed.
> 
> This patch kind of confuses me.  You only check pp->prkey before calling
> mpath_pr_event_handle() in update_map(). I get from your commit message
> that you are doing this to keep from frequent, unnecessary calls. But
> isn't update_map() only called when a multipath device is first created,
> or when multipathd stops waiting for something that it noticed during
> device creation? I don't see how this can be frequently called on a
> multipath device. What am I missing?
> 
> -Ben
> 

Discussed in the previous patch (multipathd: fix missing persistent
reseravtion for active path) as follows:

On Mon, 2021-09-13 at 10:32 -0500, Benjamin Marzinski wrote:
> On Mon, Sep 13, 2021 at 09:01:11AM +0200, Martin Wilck wrote:
>> Hello lixiaokeng,
>>
>> On Mon, 2021-09-13 at 10:43 +0800, lixiaokeng wrote:
>>> There are two paths(sucu as sda and adb) for one LUN. The two
>>> paths log in, but before the two uevents have been processed
>>> (for example there are many uevent), users use multipathd add
>>> path /dev/sda to cause mpatha and use mpathpersist -o -I to
>>> register prkey for mpatha. The add map uevent is after add path
>>> uevent, the the uevent(add sdb) will delay and missing persistent
>>> reseravtion check.
>>>
>>> Here, we add persistent reseravtion check in update_map() which
>>> is called ev_add_map().
>>>
>>> Signed-off-by: Lixiaokeng <lixiaokeng@huawei.com>
>>
>> Thank you, this looks ok to me. Have you tested it?
>>
>> I'll wait for Ben's opinion nonetheless, because he's more
>> exprerienced
>> with this part of the code than myself.
>>
>> This said, I would like to have multipathd record which paths have
>> already registered the key, to avoid doing that repeatedly.
>>
> Other than adding this, the patch looks fine.

There may be some mistakes. I mistakenly thought update_map would
be called multiple times

In check_path, mpath_pr_event_handle is only called pp->state
changes from others to up. Some prkey changes may happen when
pp->state is down, so mpath_pr_event_handle should be called
even pp->prkey is PRKEY_OK.

As Ben said, update_map will not be called repetitively. I think
this patch should be removed.

Thank Ben and Martin.

Regards,
Lixiaokeng

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel