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

[Martin Wilck]

On Sat, 2021-09-11 at 11:28 +0800, lixiaokeng wrote:
> 
> 
> 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.

update_map() is called from other places like missing_uev_wait_tick(),
where it would also make sense to try and register PR keys. AFAICS this
holds for all callers.

We may want to set flags on the paths, to track whether a path is
already registered, so that we don't have to repeat that operation for
already registered paths.

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

I think this could happen, yes. It makes me wonder again why we
we never try to register keys from multipathd itself, except when
paths are added to an already registered map. IMO we could change
the logic such that if a registration_key was specified for a given
map, multipathd could register that key, even if it was not found
already registered on the storage. Multipathd should of course not
try to create a reservation.

Martin

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