dm-devel.redhat.com archive mirror
 help / color / mirror / Atom feed
* [dm-devel] [PATCH v2] multipathd: fix missing persistent reseravtion for active path
@ 2021-09-13  2:43 lixiaokeng
  2021-09-13  7:01 ` Martin Wilck
  2021-09-15 11:23 ` Martin Wilck
  0 siblings, 2 replies; 6+ messages in thread
From: lixiaokeng @ 2021-09-13  2:43 UTC (permalink / raw)
  To: Christophe Varoqui, Martin Wilck, Benjamin Marzinski,
	dm-devel mailing list
  Cc: linfeilong, liuzhiqiang (I)

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [dm-devel] [PATCH v2] multipathd: fix missing persistent reseravtion for active path
  2021-09-13  2:43 [dm-devel] [PATCH v2] multipathd: fix missing persistent reseravtion for active path lixiaokeng
@ 2021-09-13  7:01 ` Martin Wilck
  2021-09-13 15:32   ` Benjamin Marzinski
  2021-09-15 11:23 ` Martin Wilck
  1 sibling, 1 reply; 6+ messages in thread
From: Martin Wilck @ 2021-09-13  7:01 UTC (permalink / raw)
  To: lixiaokeng, Christophe Varoqui, Benjamin Marzinski,
	dm-devel mailing list
  Cc: linfeilong, liuzhiqiang (I)

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [dm-devel] [PATCH v2] multipathd: fix missing persistent reseravtion for active path
  2021-09-13  7:01 ` Martin Wilck
@ 2021-09-13 15:32   ` Benjamin Marzinski
  2021-09-13 16:32     ` Martin Wilck
  0 siblings, 1 reply; 6+ messages in thread
From: Benjamin Marzinski @ 2021-09-13 15:32 UTC (permalink / raw)
  To: Martin Wilck
  Cc: lixiaokeng, dm-devel mailing list, linfeilong, liuzhiqiang (I)

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [dm-devel] [PATCH v2] multipathd: fix missing persistent reseravtion for active path
  2021-09-13 15:32   ` Benjamin Marzinski
@ 2021-09-13 16:32     ` Martin Wilck
  2021-09-14  7:56       ` lixiaokeng
  0 siblings, 1 reply; 6+ messages in thread
From: Martin Wilck @ 2021-09-13 16:32 UTC (permalink / raw)
  To: Benjamin Marzinski
  Cc: lixiaokeng, dm-devel mailing list, linfeilong, liuzhiqiang (I)

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [dm-devel] [PATCH v2] multipathd: fix missing persistent reseravtion for active path
  2021-09-13 16:32     ` Martin Wilck
@ 2021-09-14  7:56       ` lixiaokeng
  0 siblings, 0 replies; 6+ messages in thread
From: lixiaokeng @ 2021-09-14  7:56 UTC (permalink / raw)
  To: Martin Wilck, Benjamin Marzinski
  Cc: linfeilong, dm-devel mailing list, liuzhiqiang (I)



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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [dm-devel] [PATCH v2] multipathd: fix missing persistent reseravtion for active path
  2021-09-13  2:43 [dm-devel] [PATCH v2] multipathd: fix missing persistent reseravtion for active path lixiaokeng
  2021-09-13  7:01 ` Martin Wilck
@ 2021-09-15 11:23 ` Martin Wilck
  1 sibling, 0 replies; 6+ messages in thread
From: Martin Wilck @ 2021-09-15 11:23 UTC (permalink / raw)
  To: lixiaokeng, Christophe Varoqui, Benjamin Marzinski,
	dm-devel mailing list
  Cc: linfeilong, liuzhiqiang (I)

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2021-09-15 11:29 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-13  2:43 [dm-devel] [PATCH v2] multipathd: fix missing persistent reseravtion for active path lixiaokeng
2021-09-13  7:01 ` Martin Wilck
2021-09-13 15:32   ` Benjamin Marzinski
2021-09-13 16:32     ` Martin Wilck
2021-09-14  7:56       ` lixiaokeng
2021-09-15 11:23 ` Martin Wilck

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).