Skip to content

gfapi: deliver fdclose before dropping an fd in glfs_fd_destroy() - #4840

Draft
ThalesBarretto wants to merge 2 commits into
gluster:develfrom
ThalesBarretto:leak/fdmig/gfapi-close-without-fdclose
Draft

ThalesBarretto wants to merge 2 commits into
gluster:develfrom
ThalesBarretto:leak/fdmig/gfapi-close-without-fdclose

Conversation

@ThalesBarretto

Copy link
Copy Markdown
Contributor

gfapi: deliver fdclose before dropping an fd in glfs_fd_destroy()

Problem

fd_close() (added with the open-behind rewrite, db95388706) notifies the xlators of an fd's graph that
the application's handle is closing; open-behind uses it to cancel a deferred open and drop the fd reference
and the open stub it keeps for it. Only the FUSE bridge ever calls it. gfapi releases its fds with a bare
fd_unref(), so with performance.open-behind on (the default) the first open of an inode in a gfapi process
that is closed without an fd fop reaching open-behind — glfs_open() followed by glfs_close(), or a
small-file read served by quick-read in between; glfs_close()'s flush is answered locally — leaves the fd_t
alive with open-behind's two references, pinning its inode, until the file is opened again through that graph
(which triggers the stale open and releases it) or the process exits. open-behind's open_count for the inode
never comes back down either, so later opens of that file are all sent synchronously.

This is #3977, reported by @Ratio2 against 10.1 with the open_count symptom ("caching don't work in
nfs-ganesha") and the two-line fd_close() in glfs_close() of #3978, which got no review and was closed by the
stale bot.

Fix

fd_close(glfd->fd) in glfs_fd_destroy() before the final fd_unref(). Compared with #3978's placement in
pub_glfs_close():

  • glfs_fd_destroy() is the single point where every gfapi handle releases its fd — glfs_close(), the error
    paths, and a fini-time drain (gfapi: drain open fds in glfs_fini before graph teardown #4669) all end there.
  • It fires once per fd_t: a glfs_dup()ed handle shares the fd_t with its siblings (dupfd->fd = fd_ref(fd),
    both on fs->openfds), so the fdclose is sent only by the last glfd still referencing it (checked under the
    fs lock that glfs_fd_destroy() already takes to unlink itself from openfds). This matters: with the
    glfs_close() placement, closing the first handle of a dup pair cancels the deferred open, and the second
    handle's close then fails with EBADFD (client_pre_flush_v2: remote_fd is -1) while open-behind's
    open_count is decremented twice — measured with a trace in ob_fdclose() on a build carrying api: Added missing fd_close call to glfs_close #3978's change.
    With this change every fd_t receives exactly one fdclose, from its last handle, and reads through a surviving
    dup keep working.

The sibling check walks fs->openfds, which is O(open-fd-count), under the fs lock the close already
holds. glfs_dup() is uncommon, so this is a short scan in the common case; on a process that deliberately holds very
many fds (Ganesha, Samba) it adds a linear scan per close. If that ever matters it can be made O(1) by having
glfs_dup()/glfs_fd_bind() maintain a per-fd_t handle count; kept as a scan here to keep the change minimal and
self-contained. Correctness does not depend on the representation.

Walking the fd's graph in fd_close() is safe for a straggler fd on an old graph: gfapi never frees a graph
before glfs_fini() (glusterfs_graph_destroy is only called on volfile-fetch errors and in fini), and
pub_glfs_close() already dereferences the old graph for such fds through glfs_migrate_fd_safe().

Reproduction and A/B

Stock builds of devel a482a8578a and of this branch (default ./configure: tcmalloc, no mem-pools, no debug),
performance.open-behind on, a gfapi program that opens N existing files read-only and closes them without any
I/O, then a statedump via glfs_sysrq(). On a default build the dump has no fd_t mem-pool (tcmalloc) and no
inode table (gfapi never had one in statedumps), so the count below is the gfapi master xlator's memory
accounting: one gf_common_mt_fd_ctx array per live fd_t.

scenario [mount/api.gfapi - usage-type gf_common_mt_fd_ctx memusage] num_allocs
devel, N = 10 10
devel, N = 1000 1000
devel, N = 10, performance.open-behind off 0
this change, N = 10 0
this change, N = 1000 0

With an inode table in the dump (a separate small patch giving the gfapi master xlator a .inode dumper like
fuse's), the same N = 10 run shows the pin itself: active_size=11, and every file inode
fd-count=1 active-fd-count=1 ref=2 ref_by_xl:.gfapi=1 ref_by_xl:.vol4490-open-behind=1 after all handles were
closed; with this change active_size=1, the ten inodes idle on the LRU list with fd-count=0 ref=0.

Earlier runs on --enable-debug --enable-asan --without-tcmalloc builds, where the mem-pools exist: fd_t active-count=10 after 10 open+close, 9 after reopening one file (the next open of the same inode triggers the
stale open and releases it), 0 with the change; 5 fds held across a graph switch: 5 on the new graph and 0 on the
old one (the eager migration fsyncs the old fd on its graph, which triggers the old deferred open), 0 with the
change.

Test

tests/basic/gfapi/open-behind-close-leak.{c,t}: a tester opens three files (created through a FUSE mount
so the gfapi process sees their inodes for the first time) read-only and closes them without I/O; then opens
three more, glfs_dup()s each handle and closes both in alternating order, reading through the dup after the
original was closed; then dumps state through glfs_sysrq(). The test expects the gfapi xlator's
gf_common_mt_fd_ctx allocation count to be 0 (that count exists on every build; the fd_t mem-pool does not
on tcmalloc builds). Fails on stock devel (Got "6" instead of "0"), passes with the change.

The FUSE side of the same gap — the fd migrated across a graph switch never receives fdclose — is a separate
issue and PR (#4836).

Related: #404 (the "implement proper cleanup sequence" epic, which names gfapi applications as its motivating
case). This leak surfaces downstream as fd accumulation on long-lived gfapi processes — nfs-ganesha and Samba
vfs_glusterfs in particular.

Fixes: #3977

fd_close() (added with the open-behind rewrite, db95388) notifies
the xlators of an fd's graph that the application's handle is closing;
open-behind uses it to cancel a deferred open and drop the fd reference
and the open stub it keeps for it. Only the FUSE bridge ever calls it.
gfapi releases its fds with a bare fd_unref(), so with
performance.open-behind on (the default volume setting) the first open
of an inode in a gfapi process that is closed without an fd fop
reaching open-behind -- glfs_open() followed by glfs_close(), or a
small-file read served by quick-read in between; glfs_close()'s flush is
answered locally -- leaves the fd_t alive with open-behind's two
references, pinning its inode, until the file is opened again through
that graph (which triggers the stale open and releases it) or the
process exits. open-behind's open_count for the inode never comes back
down either, so later opens of that file are all sent synchronously.

Aleksey Vasenev reported the open_count symptom against 10.1 in gluster#3977
("caching don't work in nfs-ganesha") and proposed the two-line
fd_close() in glfs_close() (gluster#3978); the PR got no review and the stale
bot closed it. This change puts the call in glfs_fd_destroy() instead,
the single point where every gfapi handle releases its fd (glfs_close,
the error paths, a fini-time drain), and delivers it once per fd_t: a
glfs_dup()ed handle shares the fd_t with its siblings, so only the last
glfd left on fs->openfds sends the fdclose.

Reproducible with any gfapi program: open N existing files O_RDONLY and
close them without reading; a statedump (glfs_sysrq) shows the fd_t
pool active-count = N and N pinned inodes; 0 with this change.

Fixes: gluster#3977
Reported-by: Aleksey Vasenev <margtu-fivt@ya.ru>
Signed-off-by: Thales Antunes de Oliveira Barretto <thales.barretto.git@gmail.com>
Regression test for the fdclose delivery in glfs_fd_destroy(): a small
gfapi program opens N existing files read-only, closes them without any
I/O, then opens N more, glfs_dup()s each handle and closes both in
alternating order (reading through the dup after the original was closed:
a premature fdclose would leave open-behind unable to serve it), and takes
a statedump through glfs_sysrq(); the test expects the gfapi xlator's
gf_common_mt_fd_ctx allocation count in that dump to be 0 (open-behind's
deferred opens were cancelled by the closes, once per fd_t). That count
exists on every build; the fd_t mem-pool does not on tcmalloc builds. The
files are created through a FUSE mount first so that the gfapi process
sees their inodes for the first time on open.

Updates: gluster#3977
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:00
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.

Incorrect file close in api

1 participant