Re: [dm-devel] [PATCH 3/5] multipathd: make ev_remove_path return success on path removal

From: Martin Wilck <martin.wilck@suse.com>
To: "bmarzins@redhat.com" <bmarzins@redhat.com>,
	"christophe.varoqui@opensvc.com" <christophe.varoqui@opensvc.com>
Cc: "dm-devel@redhat.com" <dm-devel@redhat.com>
Subject: Re: [dm-devel] [PATCH 3/5] multipathd: make ev_remove_path return success on path removal
Date: Wed, 12 May 2021 11:38:08 +0000	[thread overview]
Message-ID: <66c4ca02bb90a7a4c18819082d2ec554ddc56205.camel@suse.com> (raw)
In-Reply-To: <1620775324-23984-4-git-send-email-bmarzins@redhat.com>

On Tue, 2021-05-11 at 18:22 -0500, Benjamin Marzinski wrote:
> When ev_remove_path() returns success, callers assume that the path
> (and
> possibly the map) has been removed.  When ev_remove_path() returns
> failure, callers assume that the path has not been removed. However,
> the
> path could be removed on both success or failure. This could cause
> callers to dereference the path after it was removed. Change
> ev_remove_path() to return success whenever the path is removed, even
> if
> the map was removed due to a failure when trying to reload it. Found by
> coverity.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>

This looks ok, but I'd like to discuss it in some more depth.

We need to clarify the meaning of the return code of ev_remove_path().
We guarantee that, when ev_remove_path() returns, either the path is
removed from internal data structures and kernel maps, or INIT_REMOVED
is set. We can't guarantee whether the path

 a) is not referenced any more by any kernel map,
 b) was actually removed from pathvec and other data structures in
multipathd.

We have to agree on whether it means a) or b) if we can't make these
two cases equivalent. Assuming multipathd has correct information about
the kernel mappings, the only difference between a) and b) is the
unlikely failure in setup_multipath(), where a) is true and b) is not
(because sync_map_state() won't be called). Your patch assumes the
semantics of a), which is correct AFAICS, but should be more clearly
documented.

Actually, I think we can fix the discrepancy between a) and b) - if
domap() was successful, we should be able to orphan the path, even if
update_multipath_strings() failed for some reason.

IMO we should consequently change the retval for the cases where
ev_remove_path() returns without deleting the path for a non-"failure"
case (wait_for_udev and !need_do_map).

Thoughts? Whatever we decide wrt the semantics of the return code, we
should document it clearly in comments at the function header.

Here's a quick review of callers:

 - cli_add_path(): AFAICS this needs b) semantics. We shouldn't set up
a new path unless it had been successfully removed internally.
 - cli_del_path(): needs a) semantics.
 - handle_path_wwid_change(): needs a).
 - uev_add_path(): needs a).
 - uev_remove_path(): the return code of ev_remove_path doesn't matter
much here. This is the only caller that sets need_do_map = false.
 - uev_update_path(): we currently don't look at the return code.
uev_add_path() will make another attempt at removing the path if
necessary.

Regards
Martin

P.S.: The remaining failure cases in ev_remove_path() are the failures
in update_mpp_paths() and setup_map(). The former can only fail with
ENOMEM, afaics. The latter, likewise, or if the map is fundamentally
misconfigured (which to me means that a previous call to setup_map()
would have failed as well). I'm wondering why we remove the entire map
in these failure cases. This goes back all the way to 45eb316 
("[multipathd] DM configuration final cut") from 2005. It's true that
both failures are pretty much fatal, but why is removing the map the
answer here?

However, this goes beyond the purpose of your patch. *If* we remove the
map, returning 0 is correct for either a) or b).

P.S. 2: I wonder if the logic in uev_update_path() is correct. Rather
than calling uev_add_path() after rescan_path() directly, I think we
should rather wait for another uevent (and possibly trigger another
"add" event, I don't think "rescan" automatically generates one).

> ---
>  multipathd/main.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/multipathd/main.c b/multipathd/main.c
> index 6090434c..4bdf14bd 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -1284,7 +1284,7 @@ ev_remove_path (struct path *pp, struct vectors *
> vecs, int need_do_map)
>  
>                         strlcpy(devt, pp->dev_t, sizeof(devt));
>                         if (setup_multipath(vecs, mpp))
> -                               return 1;
> +                               return 0;
>                         /*
>                          * Successful map reload without this path:
>                          * sync_map_state() will free it.
> @@ -1304,8 +1304,10 @@ out:
>         return retval;
>  
>  fail:
> +       condlog(0, "%s: error removing path. removing map %s", pp->dev,
> +               mpp->alias);
>         remove_map_and_stop_waiter(mpp, vecs);
> -       return 1;
> +       return 0;
>  }
>  
>  static int

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