From: Benjamin Marzinski <bmarzins@redhat.com>
To: Martin Wilck <martin.wilck@suse.com>
Cc: Christophe Varoqui <christophe.varoqui@opensvc.com>,
dm-devel@lists.linux.dev, linux-lvm@lists.linux.dev,
Zdenek Kabelac <zkabelac@redhat.com>,
Peter Rajnoha <prajnoha@redhat.com>
Subject: Re: [PATCH 4/6] 11-dm-mpath.rules: handle reloads during coldplug events
Date: Fri, 9 Feb 2024 11:15:24 -0500 [thread overview]
Message-ID: <ZcZPnHkOJUWrvp1e@bmarzins-01.fast.eng.rdu2.dc.redhat.com> (raw)
In-Reply-To: <7fc6996c93def8360d27c60f6503ff2dc42d073c.camel@suse.com>
On Fri, Feb 09, 2024 at 04:53:50PM +0100, Martin Wilck wrote:
> On Thu, 2024-02-08 at 20:03 -0500, Benjamin Marzinski wrote:
> > On Mon, Feb 05, 2024 at 01:46:36PM +0100, Martin Wilck wrote:
> > > If a map reload happens while udev is processing rules for a
> > > coldplug
> > > event, DM_SUSPENDED may be set if the respective test in 10-
> > > dm.rules
> > > happens while the device is suspened. This will cause the rules for
> > > all
> > > higher block device layers to be skipped. Record this situation in
> > > an udev
> > > property. The reload operation will trigger another "change" uevent
> > > later,
> > > which would normally be treated as a reload, and be ignored without
> > > rescanning the device. If a previous "coldplug while suspended"
> > > situation is
> > > detected, perform a full device rescan instead.
> >
> > For what it's worth, this seems reasonable.
>
> If that's so, I'd appreciate a Reviewed-by: :-)
> See below.
>
> > But I do see the issue you
> > pointed out where we can't trust DM_SUSPENDED.
>
> That was my suspicion originally, but the actual problem that was
> observed was different, see my recent response to 1/6.
>
> Yes, it can happen any time that a device gets suspended between the
> DM_SUSPENDED check and an attempt to do actual I/O in later udev rules,
> but that is much less likely than the other case. So far I haven't been
> able to actually observe it.
>
> Btw am I understanding correctly that you don't like the idea of going
> back to exclusive locking?
Oh, and yeah, I'd rather avoid exclusive locking if possible, just out
of a hunch that it could lead to other unexpected problems.
-Ben
>
> > Perhaps we could track
> > in multipathd if an add event occured for a path between when we
> > issued
> > a reload, and when we got the change event (unfortunately, change
> > events
> > can occur for things other than reloads that multipathd triggers, and
> > the only way I know to accurately associate a uevent with a reload is
> > by
> > using dm udev cookies. We have those turned off in multipathd and I
> > think it would be a good bit of work to turn them back on without
> > causing lots of waiting with locks held in multipathd).
>
> IMO doing this right in multipathd will be hard. We must be careful not
> to trigger too many additional uevents just because something *might*
> have gone wrong during udev handling (most of the time all is well,
> even if a reload happens). I believe to do this properly we must listen
> for *kernel* uevents, too, and that's something we've been trying to
> avoid for good reason.
>
> I had a different idea: to track a "reload count" for a map somehow
> (e.g. in a file under /run/multipath) and checking that at the
> beginning and end of uevent handling in order to see if a reload
> occurred while the uevent was being processed (which would be detected
> by seeing a different the reload count change during the uevent).
>
> For now, I hope that this change plus my latest addition will make the
> practical issue go away. All else can be discussed later.
> We haven't got any reports about this sort of race for years, so it's
> safe to assume that happens very rarely.
>
> Regards
> Martin
next prev parent reply other threads:[~2024-02-09 16:15 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-02-05 12:46 [PATCH 0/6] multipath-tools: udev rules and service improvements Martin Wilck
2024-02-05 12:46 ` [PATCH 1/6] 11-dm-mpath.rules: don't import properties that are already set Martin Wilck
2024-02-05 20:44 ` Benjamin Marzinski
2024-02-06 10:50 ` Martin Wilck
2024-02-09 0:30 ` Benjamin Marzinski
2024-02-09 15:35 ` Martin Wilck
2024-02-05 12:46 ` [PATCH 2/6] 11-dm-mpath.rules: fix list of imported properties Martin Wilck
2024-02-09 0:32 ` Benjamin Marzinski
2024-02-05 12:46 ` [PATCH 3/6] 11-dm-mpath.rules: use import logic like 13-dm-disk.rules Martin Wilck
2024-02-09 0:36 ` Benjamin Marzinski
2024-02-09 15:38 ` Martin Wilck
2024-02-05 12:46 ` [PATCH 4/6] 11-dm-mpath.rules: handle reloads during coldplug events Martin Wilck
2024-02-09 1:03 ` Benjamin Marzinski
2024-02-09 15:53 ` Martin Wilck
2024-02-09 16:13 ` Benjamin Marzinski
2024-02-09 16:15 ` Benjamin Marzinski [this message]
2024-02-05 12:46 ` [PATCH 5/6] multipath: udev rules: use configured $(bindir) in udev rules Martin Wilck
2024-02-09 1:04 ` Benjamin Marzinski
2024-02-05 12:46 ` [PATCH 6/6] multipathd: don't activate socket activation by default Martin Wilck
2024-02-09 1:05 ` Benjamin Marzinski
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=ZcZPnHkOJUWrvp1e@bmarzins-01.fast.eng.rdu2.dc.redhat.com \
--to=bmarzins@redhat.com \
--cc=christophe.varoqui@opensvc.com \
--cc=dm-devel@lists.linux.dev \
--cc=linux-lvm@lists.linux.dev \
--cc=martin.wilck@suse.com \
--cc=prajnoha@redhat.com \
--cc=zkabelac@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 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 read-only IMAP folder(s) and NNTP newsgroup(s).