Skip to content

gfapi: wait for CHILD_DOWN on the graph's own mutex and condvar in glfs_fini - #4816

Open
ThalesBarretto wants to merge 2 commits into
gluster:develfrom
ThalesBarretto:fix/gfapi-fini-child-down-handshake
Open

ThalesBarretto wants to merge 2 commits into
gluster:develfrom
ThalesBarretto:fix/gfapi-fini-child-down-handshake

Conversation

@ThalesBarretto

Copy link
Copy Markdown
Contributor

gfapi: wait for CHILD_DOWN on the graph's own mutex and condvar in glfs_fini

Problem

pub_glfs_fini() waits for the active graph's CHILD_DOWN under fs->mutex on fs->child_down_cond, but the
flag it polls, graph->used, belongs to libglusterfs' graph handshake: protocol/client clears it and broadcasts
graph->child_down_cond under graph->mutex once the last client of the graph is down
(client_notify_dispatch_uniq(), client.c:106; the PARENT_DOWN handler's synchronous path, client.c:2394), and
glusterfs_graph_cleanup() waits for it under graph->mutex (graph.c:1433). The gfapi waiter (2014) predates
that handshake (2019) and was never moved onto it, so the same int is written under one mutex and read under
another. ThreadSanitizer reports it on every glfs_fini() of a healthy graph:

WARNING: ThreadSanitizer: data race
  Write of size 4 by thread T9 (mutexes: ctx->notify_lock, graph->mutex):
    #0 client_notify_dispatch_uniq xlators/protocol/client/src/client.c:106
    #1 client_rpc_notify client.c:2303
    #2 rpc_clnt_handle_disconnect rpc/rpc-lib/src/rpc-clnt.c:792
    ...
  Previous read of size 4 by main thread (mutexes: fs->mutex):
    #0 pub_glfs_fini api/src/glfs.c:1339

The only code that signals fs->child_down_cond is the gfapi top xlator's CHILD_DOWN handler
(glfs-primary.c), which runs at the end of the notification chain the client starts right after clearing the
flag. So the waiter can see the client's write and leave the loop while default_notify(CHILD_DOWN) is still
walking the graph on the poller thread; what keeps xlator fini() behind that walk today is only that
protocol/client is fini'd first and its fini() waits for RPC_CLNT_DESTROY, i.e. for the transport reference
the poller holds across the handler. Issue #4815.

Fix

Two commits:

  1. Use the graph's handshake. pub_glfs_fini() waits exactly like glusterfs_graph_cleanup():
    graph->mutex + while (graph->used) pthread_cond_wait(&graph->child_down_cond, …), then drains
    ctx->notifying before touching the xlators (graph.c:1446-1451 does the same). The top xlator's CHILD_UP /
    CHILD_DOWN handlers (glfs-primary.c) write graph->used under graph->mutex and broadcast
    graph->child_down_cond too — they are the only writers for a graph without a protocol/client leaf (a local
    posix volfile), so they must speak the same handshake. Two message ids are added for the graph-mutex
    lock/unlock failure logs (the existing ones are named and worded for fs->mutex).
  2. Drop fs->child_down_cond, now without waiter or signaller (field, init/destroy, GLFS_INIT_COND_CHILD).

Behaviour of the wait's outcome is unchanged: the flag is cleared by the same events as before. What changes is
that the read is synchronised with the writer, that the waiter is woken by every writer, and that teardown does
not start under an in-flight notification.

Lock order: graph->mutex is taken alone (graph.c, defaults-tmpl.c, and here) or under ctx->notify_lock
(client.c); nothing takes fs->mutex inside a graph->mutex section, and both gfapi PARENT_DOWN callers run
outside fs->mutex. No new nesting.

Why this shape

  • An atomic graph->used would only silence the report: the waiter would still leave the loop on the client's
    write ahead of the chain, and there is no liveness problem for an atomic to fix (the mismatched condvar cannot
    miss a wakeup: whenever the client de-duplicates its upward CHILD_DOWN the flag is already 0, so the waiter
    never sleeps — model-checked, see below).
  • Reusing the handshake the graph already owns is what glusterfs_graph_cleanup() does and keeps a single
    discipline for graph->used.

Verification

ThreadSanitizer builds (--enable-debug --without-tcmalloc CFLAGS='-fsanitize=thread -O1 -g -fno-omit-frame-pointer'),
single-brick distribute volume, performance.open-behind on.

exerciser before after
multi-threaded consumer (60 files × 40 iterations, glfs_dup() each, close all 4,800 handles from 4 threads, re-read, then glfs_fini()), 5 runs report present 5/5; 5 reports/run 0/5; 4 reports/run — the same set minus this one, nothing new; all reads correct
deterministic asynchronous-disconnect exerciser (kill brick, SIGSTOP glusterd, 4 s, glfs_fini()), 3–4 runs 7/7 across two unfixed trees 0/5 across two fixed trees
glfs_fini() with a brick down at the 2.7 / 3.0 / 3.2 s reconnect phases completes completes (no hang)

The remaining reports in the "after" column are pre-existing and unrelated (io_stats_flush /
syncop_flush / default_flush_resume from the concurrent closes; the event_reconfigure_threads_epoll vs
event_dispatch_epoll_worker pair in gf_event_dispatch_destroy()).

A SPIN model of the waiter / client writer / top-xlator handler (both scenarios: brick up at fini, brick down
before fini) confirms: the shipped code has the race and no missed wakeup; the fixed code has neither; the
notification drain narrows the "teardown under an in-flight notification" window to the same gap
glusterfs_graph_cleanup() has.

tests/basic/gfapi/: libgfapi-fini-hang, gfapi-graph-switch-open-fd, glfsxmp, gfapi-load-volfile,
bug1291259, upcall-cache-invalidate, open-behind-close-leak pass on the fixed tree, plus the fuse/protocol
fd-migration tests that go through graph teardown.

Test

No .t is added: the defect is a data race only observable under ThreadSanitizer. The reproduction recipe above
is deterministic and can be run against any TSan build.

Related: #4666 / #404 (the fini teardown sequence), #4720 / #4527 (the separate rpc_clnt_reconnect
timer use-after-free, not touched here).

Fixes: #4815

…fs_fini

pub_glfs_fini() waits for the active graph's CHILD_DOWN with

    pthread_mutex_lock(&fs->mutex);
    while (graph->used)
        pthread_cond_wait(&fs->child_down_cond, &fs->mutex);

but graph->used is the graph's handshake flag, owned by libglusterfs:
protocol/client clears it and broadcasts graph->child_down_cond under
graph->mutex once the last client of the graph has gone down
(client_notify_dispatch_uniq(), client notify PARENT_DOWN), and
glusterfs_graph_cleanup() waits for it under graph->mutex on
graph->child_down_cond.  The gfapi waiter predates that handshake
(2014 vs 2019) and was never unified with it, so the same int is
written under graph->mutex and read under fs->mutex -- a data race
ThreadSanitizer reports on every glfs_fini() of a healthy graph:

  WARNING: ThreadSanitizer: data race
    Read of size 4 by main thread (mutexes: fs->mutex):
      pub_glfs_fini api/src/glfs.c:1339
    Previous write of size 4 by thread glfs_epoll000
        (mutexes: graph->mutex, ctx->notify_lock):
      client_notify_dispatch_uniq xlators/protocol/client/src/client.c:106
      client_rpc_notify client.c:2303
      rpc_clnt_handle_disconnect rpc/rpc-lib/src/rpc-clnt.c:792

The only writer that ever signalled fs->child_down_cond is the top
xlator's CHILD_DOWN handler (glfs-primary.c), which runs at the end of
the notification chain the client starts right after clearing the
flag.  So the waiter could observe the client's write and leave the
loop before the CHILD_DOWN notification had finished walking the
graph on the poller thread, with nothing but the client's own
RPC_CLNT_DESTROY wait keeping the teardown behind it.

Wait on graph->mutex / graph->child_down_cond exactly like
glusterfs_graph_cleanup(), make the top xlator's CHILD_UP/CHILD_DOWN
handlers write graph->used under graph->mutex and broadcast the same
condvar (they are the only writers for a graph without a
protocol/client leaf, e.g. a local posix volfile), and drain
ctx->notifying before tearing the xlators down, as
glusterfs_graph_cleanup() does.  fs->child_down_cond is now unused.

No behaviour change for the wait's outcome: the flag is cleared by the
same events as before.  Verified with ThreadSanitizer on a
multi-threaded gfapi consumer followed by glfs_fini(): the
client_notify_dispatch_uniq/pub_glfs_fini report is gone, no new
report appears.

Fixes: gluster#4815
Signed-off-by: Thales Antunes de Oliveira Barretto <thales.barretto.git@gmail.com>
glfs_fini() now waits for CHILD_DOWN on graph->child_down_cond, and the
top xlator's CHILD_DOWN handler broadcasts that condvar, so
fs->child_down_cond has no waiter and no signaller left.  Remove the
field, its init/destroy and the GLFS_INIT_COND_CHILD flag.

Updates: gluster#4815
Signed-off-by: Thales Antunes de Oliveira Barretto <thales.barretto.git@gmail.com>
@ThalesBarretto
ThalesBarretto marked this pull request as draft September 22, 2026 10:03
@ThalesBarretto
ThalesBarretto marked this pull request as ready for review September 22, 2026 11:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

gfapi: glfs_fini() reads graph->used under fs->mutex while protocol/client writes it under graph->mutex (data race)

1 participant