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

[Benjamin Marzinski]

On Sat, Sep 11, 2021 at 09:13:27PM +0200, Martin Wilck wrote:
> 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.
>

Yeah. This is a real problem. I wonder if mpathpersist should try
delegating work to multipathd, like multipath does. I don't use
persistent reservations enough to know if it would be problematic to
have multipathd automatically register paths.  It would be different
from how persistent reservations work on scsi devices, and the original
idea for mpathpersist was to make persistent reservations on a multipath
device work just like they do on scsi devices (where devices appear with
no registrations, and you must register them).

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