On Fri, 2019-03-08 at 17:12 -0600, Benjamin Marzinski wrote:
> Instead of ignoring failed get_uid() calls, multipathd now fails the
> path as it originally did.
This patch reverts much of 07/12; I'd appreciate if you could merge the
two into one, that would make the review easier.
> However, if the result of calling get_uid()
> is a blank wwid (which is always the case when it fails), multipathd
> now
> tries to get the wwid in check_path() as well, using the
> uid_fallback()
> methods to attempt to get a valid wwid.
>
> Multipathd can't use the uid_attribute methods, since pp->udev still
> has
> the old uevent information with the uid_attribute of the original
> wwid.
Which is actually wrong, IMO. It means that udev's (and the kernel's)
view of the device are now different from multipathd's, and will remain
so unless a new change event arrives. What bad can happen if we update
pp->udev?
Perhaps we should rather update pp->udev always, and set the
wwid_changed flag if necessary. The case wwid_changed==WWID_ZEROED
could be treated like INIT_MISSING_UDEV (it _is_ almost the same case,
actually), by retriggering uevents.
> This means that the uid_attribute methods would always return the
> original wwid, even if it had changed.
>
> To make the get_uid() use the fallback methods, pathinfo now sets
> pp->retriggers to the retrigger_tries once a WWID has be successfully
> obtained, so that it uid_fallback() doesn't need to be called
> retrigger_tries times before trying the fallback methods.
I don't like this much, see below.
> Signed-off-by: Benjamin Marzinski <bmarzins@xxxxxxxxxx>
> ---
> libmultipath/discovery.c | 4 +++-
> libmultipath/structs.h | 6 ++++++
> multipathd/main.c | 36 +++++++++++++++++++++++++-----------
> 3 files changed, 34 insertions(+), 12 deletions(-)
>
> diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
> index bece67c..15568ca 100644
> --- a/libmultipath/discovery.c
> +++ b/libmultipath/discovery.c
> @@ -2018,8 +2018,10 @@ int pathinfo(struct path *pp, struct config
> *conf, int mask)
> }
> }
>
> - if ((mask & DI_ALL) == DI_ALL)
> + if ((mask & DI_ALL) == DI_ALL) {
> + pp->retriggers = conf->retrigger_tries;
> pp->initialized = INIT_OK;
> + }
> return PATHINFO_OK;
This abuse of retrigger_tries looks like a hack to me. You want
uid_fallback() to try and get a uid, even if retrigger_tries isn't
exhausted. So you you should check another condition besides
(retriggers > retriggers_tries) in uid_callback().
At the very least, this would call for comments explaining the logic
in both pathinfo() and uid_fallback(). I think you'd also have to set
retriggers = 0 when you set initialized = INIT_MISSING_UDEV
(be it only for code clarity). But I'd prefer not to use the retrigger
counter for this different condition.
> [...]
> @@ -2017,10 +2017,24 @@ check_path (struct vectors * vecs, struct
> path * pp, int ticks)
> if (newstate == PATH_REMOVED)
> newstate = PATH_DOWN;
>
> - if (pp->wwid_changed) {
> - condlog(2, "%s: path wwid has changed. Refusing to
> use",
> - pp->dev);
> - newstate = PATH_DOWN;
> + if (pp->wwid_changed != WWID_SAME) {
> + if (pp->wwid_changed == WWID_ZEROED) {
> + char wwid[WWID_SIZE];
> +
> + strcpy(wwid, pp->wwid);
> + get_uid(pp, newstate, NULL);
> + if (strncmp(wwid, pp->wwid, WWID_SIZE) == 0)
> + pp->wwid_changed = WWID_SAME;
> + else {
> + pp->wwid_changed = (strlen(pp->wwid))?
> WWID_CHANGED : WWID_ZEROED;
> + strcpy(pp->wwid, wwid);
> + }
> + }
> + if (pp->wwid_changed != WWID_SAME) {
> + condlog(2, "%s: path wwid has changed. Refusing
> to use",
> + pp->dev);
Please don't log the same condition at -v2 in every check_path()
iteration. We log the actual change at -v0 already in
uev_update_path().
Regards
Martin
--
Dr. Martin Wilck <mwilck@xxxxxxxx>, Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/dm-devel
