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, Guozhonghua <guozh88@chinatelecom.cn>
Subject: Re: [dm-devel] [PATCH v2 07/11] multipathd: use remove_map_callback for delayed reconfigure
Date: Tue, 22 Mar 2022 10:08:35 +0100	[thread overview]
Message-ID: <7294c7d0fb76bae0bea9ddbd7545d48449f8eece.camel@suse.com> (raw)
In-Reply-To: <CAPt1nt5ntVjCJYVF8B7+uvjoypoUSdGChc3G+jycRHte-V2BOA@mail.gmail.com>

On Mon, 2022-03-21 at 19:34 -0500, Benjamin Marzinski wrote:
> On Fri, Mar 18, 2022 at 5:33 PM <mwilck@suse.com> wrote:
> > 
> > From: Martin Wilck <mwilck@suse.com>
> > 
> > If multipathd needs to delay reconfigure() because it's waiting for
> > a map
> > creation uevent, it can happen that child() isn't woken up if the
> > map
> > being waited for is removed before the uevent arrives. Fix this by
> > unblocking reconfigure() with the remove_map_callback() function.
> > 
> > Signed-off-by: Martin Wilck <mwilck@suse.com>
> > ---
> >  multipathd/main.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> > 
> > diff --git a/multipathd/main.c b/multipathd/main.c
> > index f3b8eb4..4721cd8 100644
> > --- a/multipathd/main.c
> > +++ b/multipathd/main.c
> > @@ -319,6 +319,17 @@ static bool unblock_reconfigure(void)
> >         return was_delayed;
> >  }
> > 
> > +/*
> > + * Make sure child() is woken up when a map is removed that
> > multipathd
> > + * is currently waiting for.
> > + * Overrides libmultipath's weak symbol by the same name
> > + */
> > +void remove_map_callback(struct multipath *mpp)
> > +{
> > +       if (mpp->wait_for_udev > 0)
> 
> Is there a reason why you don't decrement wait_for_udev, and check
> need_to_delay_reconfig() like in ev_add_map()? I realize that it
> doesn't hurt anything to unblock the reconfigure even if there are
> other devices that need a delay. The main thread will notice that it
> still needs to delay. I'm just wondering why we do it differently
> here?

The main reason was to keep it simple. need_to_delay_reconfig() needs
to be passed a "vecs" pointer, and requires the vecs lock to be
held. remove_map() is deep down in the call stack and has lots of
direct and indirect callers. It can be called with mpvec == NULL and
(in theory) also with pathvec == NULL, in which case it simply frees
the memory obtained by the map, without unlinking itself or its members
from any vectors. In that case it *could* be called without the vecs
lock held (AFAICS, that's not the case today, but the function could be
used this way, e.g. in an error handling code path).

I thought it was easier and safer not to have to assert that every
current and future caller holds the vecs lock, in particular because
this is called indirectly from libmultipath, the function that grabs
the lock is usually high up in the call stack.

I had indeed pondered whether I should remove the call to
need_to_delay_reconfig() from the ev_add_map(), too. I decided against
it, because I realized that waking up child() for nothing is not
cheap,as child() needs to grab the vecs lock just for calling
need_to_delay_reconfig(). We should avoid this for the common case of 
an uevent which we are waiting for. 
The remove_map() code path, OTOH, is a corner case (map being removed
while in the process of being added). We need to cover it, but we know
that this code path will be rarely executed in practice, and thus
unlikely to cause vecs lock contention.

I hope this makes sense.

Regards
Martin

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


  reply	other threads:[~2022-03-22  9:08 UTC|newest]

Thread overview: 29+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-03-18 22:33 [dm-devel] [PATCH v2 00/11] multipathd: fix __delayed_reconfig logic mwilck
2022-03-18 22:33 ` [dm-devel] [PATCH v2 01/11] multipathd: child(): remove superfluous if condition mwilck
2022-03-22  0:18   ` Benjamin Marzinski
2022-03-18 22:33 ` [dm-devel] [PATCH v2 02/11] multipathd: set reload_type in when calling reconfigure() mwilck
2022-03-18 22:33 ` [dm-devel] [PATCH v2 03/11] multipathd: avoid busy loop in child() for delayed reconfigure mwilck
2022-03-22  0:19   ` Benjamin Marzinski
2022-03-18 22:33 ` [dm-devel] [PATCH v2 04/11] multipathd: reset __delayed_reconfig from ev_add_map() mwilck
2022-03-22  0:21   ` Benjamin Marzinski
2022-03-18 22:33 ` [dm-devel] [PATCH v2 05/11] multipathd: remove volatile qualifier from running_state mwilck
2022-03-22  0:23   ` Benjamin Marzinski
2022-03-18 22:33 ` [dm-devel] [PATCH v2 06/11] libmultipath: add callback for remove_map() mwilck
2022-03-22  0:28   ` Benjamin Marzinski
2022-03-22  8:35     ` Martin Wilck
2022-03-22 14:59       ` Benjamin Marzinski
2022-03-22 16:37         ` Martin Wilck
2022-03-18 22:33 ` [dm-devel] [PATCH v2 07/11] multipathd: use remove_map_callback for delayed reconfigure mwilck
2022-03-22  0:34   ` Benjamin Marzinski
2022-03-22  9:08     ` Martin Wilck [this message]
2022-03-22 15:36       ` Benjamin Marzinski
2022-03-18 22:33 ` [dm-devel] [PATCH v2 08/11] libmultipath: warn only once about deprecated options mwilck
2022-03-22  0:34   ` Benjamin Marzinski
2022-03-18 22:33 ` [dm-devel] [PATCH v2 09/11] multipathd: improve logging of reconfigure() mwilck
2022-03-22  0:35   ` Benjamin Marzinski
2022-03-18 22:33 ` [dm-devel] [PATCH v2 10/11] multipathd: log state changes mwilck
2022-03-22  0:37   ` Benjamin Marzinski
2022-03-18 22:33 ` [dm-devel] [PATCH v2 11/11] multipathd: remove unhelpful startup / shutdown messages mwilck
2022-03-22  0:43   ` Benjamin Marzinski
2022-03-22  9:30     ` Martin Wilck
2022-03-22 15:38       ` 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=7294c7d0fb76bae0bea9ddbd7545d48449f8eece.camel@suse.com \
    --to=mwilck@suse.com \
    --cc=bmarzins@redhat.com \
    --cc=dm-devel@redhat.com \
    --cc=guozh88@chinatelecom.cn \
    /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.