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

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

[lixiaokeng]

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>
---
 multipathd/main.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/multipathd/main.c b/multipathd/main.c
index 3aff241d..1defeaf1 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -490,6 +490,8 @@ update_map (struct multipath *mpp, struct vectors *vecs, int new_map)
 {
 	int retries = 3;
 	char *params __attribute__((cleanup(cleanup_charp))) = NULL;
+	struct path *pp;
+	int i;

 retry:
 	condlog(4, "%s: updating new map", mpp->alias);
@@ -502,6 +504,15 @@ retry:
 	verify_paths(mpp);
 	mpp->action = ACT_RELOAD;

+	if (mpp->prflag) {
+		vector_foreach_slot(mpp->paths, pp, i) {
+			if ((pp->state == PATH_UP)  || (pp->state == PATH_GHOST)) {
+				/* persistent reseravtion check*/
+				mpath_pr_event_handle(pp);
+			}
+		}
+	}
+
 	if (setup_map(mpp, &params, vecs)) {
 		condlog(0, "%s: failed to setup new map in update", mpp->alias);
 		retries = -1;
-- 

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

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

[Martin Wilck]

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.

Additional question to Ben in this context: what's the reason that we
don't actively register keys (that we found in multipath.conf or
prkeys) during multipathd startup / reconfigure?

Regards
Martin



> ---
>  multipathd/main.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/multipathd/main.c b/multipathd/main.c
> index 3aff241d..1defeaf1 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -490,6 +490,8 @@ update_map (struct multipath *mpp, struct vectors
> *vecs, int new_map)
>  {
>         int retries = 3;
>         char *params __attribute__((cleanup(cleanup_charp))) = NULL;
> +       struct path *pp;
> +       int i;
> 
>  retry:
>         condlog(4, "%s: updating new map", mpp->alias);
> @@ -502,6 +504,15 @@ retry:
>         verify_paths(mpp);
>         mpp->action = ACT_RELOAD;
> 
> +       if (mpp->prflag) {
> +               vector_foreach_slot(mpp->paths, pp, i) {
> +                       if ((pp->state == PATH_UP)  || (pp->state ==
> PATH_GHOST)) {
> +                               /* persistent reseravtion check*/
> +                               mpath_pr_event_handle(pp);
> +                       }
> +               }
> +       }
> +
>         if (setup_map(mpp, &params, vecs)) {
>                 condlog(0, "%s: failed to setup new map in update",
> mpp->alias);
>                 retries = -1;



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

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

[Benjamin Marzinski]

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.
 
> Additional question to Ben in this context: what's the reason that we
> don't actively register keys (that we found in multipath.conf or
> prkeys) during multipathd startup / reconfigure?

Like I said in my reply to the first patch, the goal was to make
persitent reservations to multipath devices work just like with scsi
devices.

-Ben

> Regards
> Martin
> 
> 
> 
> > ---
> >  multipathd/main.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> > 
> > diff --git a/multipathd/main.c b/multipathd/main.c
> > index 3aff241d..1defeaf1 100644
> > --- a/multipathd/main.c
> > +++ b/multipathd/main.c
> > @@ -490,6 +490,8 @@ update_map (struct multipath *mpp, struct vectors
> > *vecs, int new_map)
> >  {
> >         int retries = 3;
> >         char *params __attribute__((cleanup(cleanup_charp))) = NULL;
> > +       struct path *pp;
> > +       int i;
> > 
> >  retry:
> >         condlog(4, "%s: updating new map", mpp->alias);
> > @@ -502,6 +504,15 @@ retry:
> >         verify_paths(mpp);
> >         mpp->action = ACT_RELOAD;
> > 
> > +       if (mpp->prflag) {
> > +               vector_foreach_slot(mpp->paths, pp, i) {
> > +                       if ((pp->state == PATH_UP)  || (pp->state ==
> > PATH_GHOST)) {
> > +                               /* persistent reseravtion check*/
> > +                               mpath_pr_event_handle(pp);
> > +                       }
> > +               }
> > +       }
> > +
> >         if (setup_map(mpp, &params, vecs)) {
> >                 condlog(0, "%s: failed to setup new map in update",
> > mpp->alias);
> >                 retries = -1;
> 

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

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

[Martin Wilck]

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.

I would say we can take the patch, then. We can add the record-keeping
later, I suppose it needs some deeper considerations. I wouldn't be
against lixiaokeng giving it a shot ;-)

> > Additional question to Ben in this context: what's the reason that
> > we
> > don't actively register keys (that we found in multipath.conf or
> > prkeys) during multipathd startup / reconfigure?
> 
> Like I said in my reply to the first patch, the goal was to make
> persitent reservations to multipath devices work just like with scsi
> devices.

There's no obvious way to do it for SCSI other than writing a custom 
udev rule. For multipath, we could. I see no problem with adding the
automatic registration, making it depend on a new configuration setting
(well _almost_ no problem - yet another configuration option). The
question is whether anyone would be interested in such a feature.

Regards
Martin


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

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

[lixiaokeng]

On 2021/9/14 0:32, Martin Wilck wrote:
> 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.
> 
> I would say we can take the patch, then. We can add the record-keeping
> later, I suppose it needs some deeper considerations. I wouldn't be
> against lixiaokeng giving it a shot ;-)
> 

Hello Martin,

Thanks for your trust. I will try to do it. But as you said, it needs some
deeper considerations and may take some time. If you have good solution
for it firstly, please fix it.

Regards
Lixiaokeng

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

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

[Martin Wilck]

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>

As dicussed previously:

Reviewed-by: Martin Wilck <mwilck@suse.com>


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