[dm-devel] [PATCH] multipathd: fix missing persistent reseravtion for active path

[lixiaokeng]

On 2021/9/11 0:17, Martin Wilck wrote:
> Hello lixiaokeng,
> 
> thanks for your patch.
> 
> On Fri, 2021-09-10 at 20:31 +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 ev_add_map if
>> mpp->wait_for_udev > 1.
>>
>> Signed-off-by: Lixiaokeng <lixiaokeng at huawei.com>
>> ---
>>  multipathd/main.c | 9 +++++++++
>>  1 file changed, 9 insertions(+)
>>
>> diff --git a/multipathd/main.c b/multipathd/main.c
>> index 3aff241d..ef456c34 100644
>> --- a/multipathd/main.c
>> +++ b/multipathd/main.c
>> @@ -706,6 +706,8 @@ ev_add_map (char * dev, const char * alias,
>> struct vectors * vecs)
>>         struct multipath * mpp;
>>         int delayed_reconfig, reassign_maps;
>>         struct config *conf;
>> +       struct path *pp;
>> +       int i;
>>
>>         if (dm_is_mpath(alias) != 1) {
>>                 condlog(4, "%s: not a multipath map", alias);
>> @@ -721,6 +723,13 @@ ev_add_map (char * dev, const char * alias,
>> struct vectors * vecs)
>>                         if (update_map(mpp, vecs, 0))
>>                                 /* setup multipathd removed the map
>> */
>>                                 return 1;
> 
> Should you make this conditional on mpp->prflag, perhaps?
> After all, sda has already been added, sp prflag should should be set
> if that was successful.
> 
Hi Martin:

Thanks for your reply. I will add conditional on mpp->prflag.

> I think this should rather be added to update_map(). Ben?

I have considered putting it after adopt_paths() in update_map().
But I'm not sure which one is better. If you and Ben think adding
to update_map() is better, I will move it to there.

Here is anotherthing. If two new paths(sdc) for maptha
(with sda sdb path) log in. mpathpersist -o -I for mpatha, the
sda and sdb will be registered. Before update_prflag() and
update_prkey() in do_mpath_persistent_reserve_out, the uevent
(add sdc) is finshed, then sdc will missing registering.

This is just my theoretical analysis. I'm not sure if this is
a real problem.

> 
> 
>> +
>> +                       vector_foreach_slot(mpp->paths, pp, i) {
>> +                               if ((pp->state == PATH_UP)  || (pp-
>>> state == PATH_GHOST)) {
>> +                                       /* persistent reseravtion
>> check*/
> 
> Typo above.
> 
>> +                                       mpath_pr_event_handle(pp);
>> +                               }
>> +                       }
>>                 }
>>                 conf = get_multipath_config();
>>                 delayed_reconfig = conf->delayed_reconfig;
> 
> 
> Thanks,
> Martin
> 
> .
>