[dm-devel] [PATCH] multipathd: fix no checking persistent reservation registration

[dm-devel] [PATCH] multipathd: fix no checking persistent reservation registration

[lixiaokeng]

If one multipath device has two paths, the first is down (network
failure) and the second is up, then we register a prkey to the
device. The first path will register successfully but the second
won't. Then fix the network. The uev_update_path will race with
check_path. If uev_update_path -> pathinfo is called before
check_path, the state of the first path will be set PATH_UP
without checking persistent reservation registration.

Here we add checking in uev_update_path.

Signed-off-by: Lixiaokeng <lixiaokeng@huawei.com>
---
 multipathd/main.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/multipathd/main.c b/multipathd/main.c
index 2251e02c..0948bf81 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -1316,6 +1316,7 @@ uev_update_path (struct uevent *uev, struct vectors * vecs)
 	struct path * pp;
 	struct config *conf;
 	int needs_reinit = 0;
+	int oldstate;

 	switch ((rc = change_foreign(uev->udev))) {
 	case FOREIGN_OK:
@@ -1366,9 +1367,22 @@ uev_update_path (struct uevent *uev, struct vectors * vecs)
 			pp->udev = udev_device_ref(uev->udev);
 			conf = get_multipath_config();
 			pthread_cleanup_push(put_multipath_config, conf);
+			oldstate = pp->state;
+
 			if (pathinfo(pp, conf, DI_SYSFS|DI_NOIO) != PATHINFO_OK)
 				condlog(1, "%s: pathinfo failed after change uevent",
 					uev->kernel);
+
+			if (pp->state != oldstate && (pp->state == PATH_UP || pp->state == PATH_GHOST)) {
+				if (pp->mpp && pp->mpp->prflag) {
+					/*
+					 * Check Persistent Reservation.
+					 */
+					condlog(2, "%s: checking persistent reservation "
+						"registration when update path", pp->dev);
+					mpath_pr_event_handle(pp);
+				}
+			}
 			pthread_cleanup_pop(1);
 		}

-- 

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

Re: [dm-devel] [PATCH] multipathd: fix no checking persistent reservation registration

[Martin Wilck]

On Do, 2021-06-24 at 16:47 +0800, lixiaokeng wrote:
> If one multipath device has two paths, the first is down (network
> failure) and the second is up, then we register a prkey to the
> device. The first path will register successfully but the second
> won't. Then fix the network. The uev_update_path will race with
> check_path. If uev_update_path -> pathinfo is called before
> check_path, the state of the first path will be set PATH_UP
> without checking persistent reservation registration.
> 
> Here we add checking in uev_update_path.
> 
> Signed-off-by: Lixiaokeng <lixiaokeng@huawei.com>
> ---
>  multipathd/main.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/multipathd/main.c b/multipathd/main.c
> index 2251e02c..0948bf81 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -1316,6 +1316,7 @@ uev_update_path (struct uevent *uev, struct
> vectors * vecs)
>         struct path * pp;
>         struct config *conf;
>         int needs_reinit = 0;
> +       int oldstate;
> 
>         switch ((rc = change_foreign(uev->udev))) {
>         case FOREIGN_OK:
> @@ -1366,9 +1367,22 @@ uev_update_path (struct uevent *uev, struct
> vectors * vecs)
>                         pp->udev = udev_device_ref(uev->udev);
>                         conf = get_multipath_config();
>                         pthread_cleanup_push(put_multipath_config,
> conf);
> +                       oldstate = pp->state;
> +
>                         if (pathinfo(pp, conf, DI_SYSFS|DI_NOIO) !=
> PATHINFO_OK)
>                                 condlog(1, "%s: pathinfo failed after
> change uevent",
>                                         uev->kernel);
> +
> +                       if (pp->state != oldstate && (pp->state ==
> PATH_UP || pp->state == PATH_GHOST)) {


I don't understand. pathinfo(DI_SYSFS|DI_NOIO) doesn't modify 
pp->state.  So your first condition should always be false.
Am I overlooking something?

Regards
Martin



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

Re: [dm-devel] [PATCH] multipathd: fix no checking persistent reservation registration

[lixiaokeng]

Hi Martin,

You are right!! This problem was found in 0.7.7. Now I know it
is fixed with commit 9b715bf9 in 0.8.1.
I will use the commit 9b715bf9 to fix it. Thanks for your
review and answer very much.

Regarts
Lixiaokeng

On 2021/6/24 18:19, Martin Wilck wrote:
> On Do, 2021-06-24 at 16:47 +0800, lixiaokeng wrote:
>> If one multipath device has two paths, the first is down (network
>> failure) and the second is up, then we register a prkey to the
>> device. The first path will register successfully but the second
>> won't. Then fix the network. The uev_update_path will race with
>> check_path. If uev_update_path -> pathinfo is called before
>> check_path, the state of the first path will be set PATH_UP
>> without checking persistent reservation registration.
>>
>> Here we add checking in uev_update_path.
>>
>> Signed-off-by: Lixiaokeng <lixiaokeng@huawei.com>
>> ---
>>  multipathd/main.c | 14 ++++++++++++++
>>  1 file changed, 14 insertions(+)
>>
>> diff --git a/multipathd/main.c b/multipathd/main.c
>> index 2251e02c..0948bf81 100644
>> --- a/multipathd/main.c
>> +++ b/multipathd/main.c
>> @@ -1316,6 +1316,7 @@ uev_update_path (struct uevent *uev, struct
>> vectors * vecs)
>>         struct path * pp;
>>         struct config *conf;
>>         int needs_reinit = 0;
>> +       int oldstate;
>>
>>         switch ((rc = change_foreign(uev->udev))) {
>>         case FOREIGN_OK:
>> @@ -1366,9 +1367,22 @@ uev_update_path (struct uevent *uev, struct
>> vectors * vecs)
>>                         pp->udev = udev_device_ref(uev->udev);
>>                         conf = get_multipath_config();
>>                         pthread_cleanup_push(put_multipath_config,
>> conf);
>> +                       oldstate = pp->state;
>> +
>>                         if (pathinfo(pp, conf, DI_SYSFS|DI_NOIO) !=
>> PATHINFO_OK)
>>                                 condlog(1, "%s: pathinfo failed after
>> change uevent",
>>                                         uev->kernel);
>> +
>> +                       if (pp->state != oldstate && (pp->state ==
>> PATH_UP || pp->state == PATH_GHOST)) {
> 
> 
> I don't understand. pathinfo(DI_SYSFS|DI_NOIO) doesn't modify 
> pp->state.  So your first condition should always be false.
> Am I overlooking something?
> 
> Regards
> Martin
> 
> 
> .
> 


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