All of lore.kernel.org
 help / color / mirror / Atom feed
From: Martin Wilck <mwilck@suse.com>
To: Benjamin Marzinski <bmarzins@redhat.com>
Cc: dm-devel@redhat.com
Subject: Re: [RFC PATCH 14/16] multipath.rules: find_multipaths+ignore_wwids logic
Date: Tue, 30 Jan 2018 14:07:57 +0100	[thread overview]
Message-ID: <1517317677.4072.42.camel@suse.com> (raw)
In-Reply-To: <20180129222802.GJ14513@octiron.msp.redhat.com>

On Mon, 2018-01-29 at 16:28 -0600, Benjamin Marzinski wrote:
> On Fri, Jan 26, 2018 at 06:29:04PM +0100, Martin Wilck wrote:
> > On Thu, 2018-01-25 at 07:40 -0600, Benjamin Marzinski wrote:
> > > 
> > > Possibly another option is to check if something else is using
> > > the
> > > device
> > > and to not claim the device in this case. That will solve the
> > > initramfs
> > > case, with is the really bad one.
> > 
> > I agree it's bad, but AFAICS "i" only eliminates a part of the
> > problem.
> 
> I agree "i" isn't a perfect solution.  But adding code to never claim
> a
> device that is currently in use will help things, regardless of any
> other changes we do or whatever mode we are in.  If we are in
> agreement
> that reassign_maps isn't safe, then there's never a good reason for
> multipathd to set up on a path that is already in use. If we don't do
> that, then we should never claim these paths either. 

If a path is in use, multipathd fails to claim it. I've been pondering
to simply try "open(device, O_EXCL)" to test for "is in use". 
If the open() fails, the problem is to determine whether the device is 
permanently in use (e.g. already mounted or claimed by MD) or only
temporarily (because some other process is probing it at the same time
as us). Retrying, at least over more than a minimal time span, is out
of the question. Maybe multipathd could try to claim a single-path map
(if should_multipath is true), and just try the open() call in the case
of failure, to test whether the reason for the failure was that the
path was in use.

Btw, I've been wondering what the additional "in use" logic based on
lock_multipath()/flock() is useful for these days. We can either grab a
path (which comes down to open(..., O_EXCL)), or we can't. It seems odd
to have an additional, not fully equivalent "in use" logic
(lock_multipath() success doesn't imply subsequent O_EXCL succcess!). 

IMO we should determine if we're supposed to grab the path
(should_multipath(), using the best heuristics we can), and if yes, we
should immediately do so without doing other checks first. If that
fails, we might either retry or fall back. Once we have the O_EXCL fd,
we may  also (try to) flock() the paths to notify other processes using
flock(), but that would be fully optional.

>  
> > > It will still leave the case where
> > > multipath will never be able to create the device for some other
> > > reason,
> > > but that is almost always a configuration issues, where for
> > > instance
> > > multipath should be blacklisting a whole class of devices that it
> > > isn't.
> > 
> > Hannes and I have recently discussed an idea how to deal with this
> > situation. It's quite high on my todo list. The basic idea is to
> > that
> > if multipathd fails to set up a device, it leaves a flag somewhere
> > and
> > retrigger an uevent. When multipath sees the flag, it doesn't claim
> > the
> > device next time.
> 
> How is this different from the pathes you just wrote to deal with the
> find_multipaths case? It claims the path device immediately, sets a
> timer, and if multipathd doesn't set itself up within that time, it
> unclaims the path.
>   The only addition you need is for multipathd to
> trigger that change event for devices that it fails set up on, so
> that
> you don't have to wait for the timeout. You still do need a backup

That's the idea. But a flag is necessary that multipath -u will see.

> timeout to deal with the case where mutipathd doesn't start up
> correctly.

Hm. IMO we have to assume that multipathd will start up. If it doesn't,
all the schemes we've discussed so far would fail, even strict fiN. But
you've just hinted that my "retry timer" idea might actually be a
solution to this problem :-)

> 
> > 
> > If you insist, we'll just let "fin" survive as, say,
> > 'find_multipaths
> > "conservative"'. Or we call "fIn" "greedy" and "fin" "no".
> 
> I'm fine with this. But, assuming you agree with my two above
> comments,
> then I'm not sure why you don't like the five options outlined in my
> last email: 
> 
> fin like we have now
> Fin like we have now
> fIn with the ability to go back and unclaim the device
> Fin with the ability to go back and unclaim the device
> xxN where f/F and i/I are ignored

I think we should differentiate "what we try to claim" and "what we do
if we fail". The ability to go back in the error case is highly
desirable, but orthogonal to the options which describe the "try to"
side. Luckily, for "go back and unclaim" we need no option - it seems
obvious that failure recovery shouldn't be disabled (assuming we get
this right).

> * all with the additional check that we don't claim or assemble on
> paths
> that are currently in use.
> 
> I really don't care how we expose these options to the user. I was
> just
> pointing out that if we used "find_multipaths", "ignore_wwids", and
> "no_new_devs", where "ignore_wwids" implied the ability to go back
> and
> unclaim the device, then every combination of the three is valid. But
> whether we use one config option with 5 keywords, or three config
> options that are boolean, or whatever, is fine by me.

I think one config option is far better, be it only for the sake of
documenting it. I hate stuff like "option X does Y if set to Z, but
only if option A is set to B, ...". It's bad for readers and even worse
for writers.

>  
> > We had a case where the customer ran "dracut --add multipath", but
> > forgot to run "systemctl enable multipath.service". Because we use
> > "fIn", multipathd grabbed the device in the initrd, and afterwards
> > the
> > root device's subvolumes couldn't be mounted, because systemd would
> > try
> > to mount the members, which were locked by the multipath device.
> 
> So, I'm still a little fuzzy on what happened here. Are you saying
> that
> this case was also an instance of 4C.2 after all? If not that, was
> the
> problem that multipath was assembled on the path device, but not
> having
> multipathd enabled made "multipath -u" not claim the device after the
> switch-root, making it appear available for others to use. 

Exactly. systemd would have been able to mount the other files systems
through the mpath device, but the path devices show up earlier during
"coldplug", and once the mount unit is in FAILED state, systemd heads
towards emergency.

>  Perhaps in
> that case, if we fail the check to see if multipathd is
> running/wanted
> in "multipath -u", we should still check if the path device is
> multipathed already, and if it is, we should claim it regardless,
> because nobody else will be able to.

Yes, that might have helped in this failure scenario.

>  
> > >  As an
> > > aside, I am personally very wary about reassign_maps. Multipath
> > > doesn't
> > > own the other devices it is reloading. There is nothing to
> > > guarantee
> > > that someone else isn't trying to modify the LVM device at the
> > > same
> > > time. I don't know of a specific bug with this (we never turn it
> > > on),
> > > but it seems very risky to start changing devices we don't own
> > > with
> > > no
> > > coordination.
> > 
> > I've no experience with reassign_maps. It's tempting to think that
> > it
> > could solve all these nasty issues, but my gut feeling, like yours,
> > says that it might be dangerous. We don't turn it on, either.
> > 
> 
> If neither of us find this safe, we should deprecate it at the least,
> and probably disable it and print a warning message instead, if it is
> set in multipath.conf.

I'm unsure. I don't have the experience to say it does work or it
doesn't.

@ALL: (if anyone except Ben read this far :-), can you contribute
experience with reassign_maps?

> > I agree your approach can't hurt, although I still think it will
> > just
> > make a very unlikely outcome even more unlikely (the 4C.3 case
> > described above is obviously very special, and your patch wouldn't
> > avoid it). I hope that our mutual ideas can go together.
> > 
> 
> If we can cut down the cases where we end up waiting, I'm fine with
> dropping my idea. I had some discussions this week, and people do
> recognize that there are problems with the existing device assembling
> code in udev. I'm now pretty confident that sooner or later, we will
> have the ability to find about how a device was labelled in the past,
> even between boots. 

I'd be interested to learn more about this. Do you have a reference?

Cheers,
Martin

-- 
Dr. Martin Wilck <mwilck@suse.com>, 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@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

  reply	other threads:[~2018-01-30 13:07 UTC|newest]

Thread overview: 30+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-01-19  0:29 [RFC PATCH 00/16] multipath path classification Martin Wilck
2018-01-19  0:29 ` [RFC PATCH 01/16] Revert "multipath: ignore -i if find_multipaths is set" Martin Wilck
2018-01-19  0:29 ` [RFC PATCH 02/16] Revert "multipathd: imply -n " Martin Wilck
2018-01-19  0:29 ` [RFC PATCH 03/16] libmultipath: add mpvec param to should_multipath() Martin Wilck
2018-01-19  0:29 ` [RFC PATCH 04/16] libmultipath: should_multipath: keep existing maps Martin Wilck
2018-01-19 16:06   ` Benjamin Marzinski
2018-01-19  0:29 ` [RFC PATCH 05/16] multipath -u -i: change logic for find_multipaths Martin Wilck
2018-01-19  0:29 ` [RFC PATCH 06/16] libmultipath: let ignore_wwids be set in config file Martin Wilck
2018-01-19  0:29 ` [RFC PATCH 07/16] multipathd: replace -n with !ignore_wwids Martin Wilck
2018-01-19  0:29 ` [RFC PATCH 08/16] multipath.conf.5: document "ignore_wwids" Martin Wilck
2018-01-19  0:29 ` [RFC PATCH 09/16] multipath.8: adapt documentation of '-i' Martin Wilck
2018-01-19  0:29 ` [RFC PATCH 10/16] multipathd.8: document that '-n' is now ignored Martin Wilck
2018-01-19  0:29 ` [RFC PATCH 11/16] multipath: common code path for CMD_VALID_PATH Martin Wilck
2018-01-19  0:29 ` [RFC PATCH 12/16] multipath -u/-c: change output to environment/key format Martin Wilck
2018-01-19  0:29 ` [RFC PATCH 13/16] multipath -u/-c: add "$DEV is maybe a valid path" Martin Wilck
2018-01-19  0:29 ` [RFC PATCH 14/16] multipath.rules: find_multipaths+ignore_wwids logic Martin Wilck
2018-01-19 18:12   ` Benjamin Marzinski
2018-01-20  1:20     ` Martin Wilck
2018-01-21  3:21       ` Benjamin Marzinski
2018-01-22 21:56         ` Martin Wilck
2018-01-25 13:40           ` Benjamin Marzinski
2018-01-26 17:29             ` Martin Wilck
2018-01-29 22:28               ` Benjamin Marzinski
2018-01-30 13:07                 ` Martin Wilck [this message]
2018-01-30 23:40                   ` Benjamin Marzinski
2018-01-20  0:27   ` [FIX for 14/16] multipath.rules: set job properties for systemd-run correctly Martin Wilck
2018-01-19  0:29 ` [RFC PATCH 15/16] libmultipath: trigger change uevent on new device creation Martin Wilck
2018-01-19  0:29 ` [RFC PATCH 16/16] libmultipath: trigger path uevent only when necessary Martin Wilck
2018-03-07  8:53 ` [RFC PATCH 00/16] multipath path classification Christophe Varoqui
2018-03-07  9:26   ` Martin Wilck

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1517317677.4072.42.camel@suse.com \
    --to=mwilck@suse.com \
    --cc=bmarzins@redhat.com \
    --cc=dm-devel@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.