From: Martin Wilck <mwilck@xxxxxxxx>
Hello Christophe, hello Ben,
The current multipathd unix listener code has various deficiencies.
- client disconnects aren't handled correctly,
- the uxsock_timeout is applied for receiving, handling, and
responding to the client requests separately, rather than for
the entire operation,
- timeouts are logged, but not acted upon, causing the timeout
to be noticed in the client rather than in the server.
- clients may see a timeout while "reconfigure" is running,
- unpriviledged (non-root) client connections don't work
correctly
- most importantly, the code busy-loops, polls, or waits in
various places in called subroutines, which is a no-go in a
piece of code designed as an event handler and may lead
to spurious timeouts and delayed reaction e.g. to signals
or client requests.
This patch set approaches all these issues. Fixing the last one,
in particular, requires a major refactoring of the uxlsnr code.
Overall, the reliability and latency of client request handling
and signal handling by multipathd should be noticeably improved
by this patch set.
The biggest problem (waiting for the vecs lock in a client handler)
can only be fixed by moving this wait into the handlers ppoll()
loop (another possible fix would have been to handle all clients
in separate threads, but that would have required even more
complexity). The patch set achieves this by adding an eventfd-based
notification mechanism to the vecs lock, which can be passed to
ppoll() to wake up when the lock is freed.
Furthermore, client requests can't be handled in a single poll
iteration any more. Therefore the client connection becomes stateful,
and is handled by a state machine using the states RECEIVE, PARSE,
WAIT FOR LOCK, WORK, and SEND.
The refactoring is done step by step for ease (hopefully) of
review. 1/35-4/35 add utility code that will be used by the uxlsnr
refactoring. 5/35-7/35 are some independent patches that
aren't directly related to uxlnsr, but fix issues that I observed
while working on this set.
8/35-13/35 are minor fixups in the client handling code. This code is
strongly related to the uxlsnr, thus I thought I'd rather fix it
before making the other changes. In 25/35, the cli-handlers are
converted to use the strbuf API everywhere instead of separate "reply"
and "len" arguments. 15/35-18/35 are minor fixes for the
uxlsnr. 19/35-34/35 are the actual refactoring patches for the uxlsnr
code. First I move some code around unchanged, then I add the
state machine (handle_client()) and move the code into it piece
by piece. 35/35 adds a fix for the client side (multipathd -k).
CC'ing Lixiaokeng and Chongyun Wu, as they have test cases that use
the client code heavily AFAIR. Testing by 3rd parties would be
very welcome.
Cheers,
Martin
---
Changes wrt v2 (Ben Marzinski):
- Rebased the series upon Lixiaokeng's recent patch 'remove unuseful
MALLOC/REALLOC/STRDUP/FREE'.
- changed indentation from spaces to tabs in multiple patches
- 03/35: Fixed comment in libmultipath.version
- 18/35, 30/35: Renamed CLT_WAIT_LOCK to CLT_LOCKED_WORK
- 30/35: fatal error if idle_fd allocation fails
- 32/35: check for POLLOUT before trying to send reply
I didn't change the switch statement in 30/35 to an if because another
switch clause is added in 32/35, as discussed during the review of the v2 series.
I don't repost Ben's reconfigure series which I added to the v2 submission;
it's unchanged and unaffected by the rebase.
While there are numerous minor changes mostly because of the rebase and the
whitespace fixes, I took the liberty to keep Ben's Reviewed-by: tags in the patches.
Changes wrt v1 (Ben Marzinski):
03: this is a major library version change.
07: make set_config_state() static
12: further simplify add_handler, make it static, and use assert
to check for multiply-defined handlers
14: dropped in favor of Ben's "reconfigure all" set, numbering changes
from here on
29 (was 30): don't use fallthrough; call state machine in a loop instead.
fix signedness of return codes. Fix double messages.
30 (was 31): The lock handling in this patch was broken. It could happen that
the uxlsnr was cancelled without releasing the lock. Fixed by
simplification.
35 (new): Use recv() for getting the command length, as suggested by Ben.
Comments welcome, regards,
Martin
Martin Wilck (35):
libmultipath: add timespeccmp() utility function
libmultipath: add trylock() helper
libmultipath: add optional wakeup functionality to lock.c
libmultipath: print: add __snprint_config()
libmultipath: improve cleanup of uevent queues on exit
multipathd: fix systemd notification when stopping while reloading
multipathd: improve delayed reconfigure
multipathd: cli.h: formatting improvements
multipathd: cli_del_map: fix reply for delayed action
multipathd: add prototype for cli_handler functions
multipathd: make all cli_handlers static
multipathd: add and set cli_handlers in a single step
multipathd: cli.c: use ESRCH for "command not found"
multipathd: uxlsnr: avoid stalled clients during reconfigure
multipathd: uxlsnr: handle client HUP
multipathd: uxlsnr: use symbolic values for pollfd indices
multipathd: uxlsnr: avoid using fd -1 in ppoll()
multipathd: uxlsnr: data structure for stateful client connection
multipathd: move uxsock_trigger() to uxlsnr.c
multipathd: move parse_cmd() to uxlsnr.c
multipathd: uxlsnr: remove check_timeout()
multipathd: uxlsnr: move client handling to separate function
multipathd: uxlsnr: use main poll loop for receiving
multipathd: use strbuf in cli_handler functions
multipathd: uxlsnr: check root on connection startup
multipathd: uxlsnr: pass struct client to uxsock_trigger() and
parse_cmd()
multipathd: uxlsnr: move handler execution to separate function
multipathd: uxlsnr: use parser to determine non-root commands
multipathd: uxlsnr: merge uxsock_trigger() into state machine
multipathd: uxlsnr: add idle notification
multipathd: uxlsnr: add timeout handling
multipathd: uxlsnr: use poll loop for sending, too
multipathd: uxlsnr: drop client_lock
multipathd: uxclt: allow client mode for non-root, too
multipathd: uxlsnr: use recv() for command length
libmultipath/libmultipath.version | 13 +-
libmultipath/lock.c | 12 +-
libmultipath/lock.h | 11 +-
libmultipath/print.c | 34 +-
libmultipath/print.h | 2 +
libmultipath/structs_vec.h | 2 +-
libmultipath/time-util.c | 12 +
libmultipath/time-util.h | 1 +
libmultipath/uevent.c | 49 ++-
multipathd/cli.c | 180 ++--------
multipathd/cli.h | 100 +++---
multipathd/cli_handlers.c | 553 ++++++++++++++----------------
multipathd/cli_handlers.h | 61 +---
multipathd/main.c | 220 +++++-------
multipathd/main.h | 2 +-
multipathd/uxlsnr.c | 528 +++++++++++++++++++++-------
multipathd/uxlsnr.h | 4 +-
17 files changed, 957 insertions(+), 827 deletions(-)
--
2.33.1
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/dm-devel
