From: Al Viro <viro@zeniv.linux.org.uk>
To: Christian Brauner <brauner@kernel.org>
Cc: linux-fsdevel@vger.kernel.org
Subject: [RFC] what's the reason to delay switching current->files in close_range(2)?
Date: Mon, 6 Oct 2025 23:18:28 +0100 [thread overview]
Message-ID: <20251006221828.GH2441659@ZenIV> (raw)
In 60997c3d45d9 "close_range: add CLOSE_RANGE_UNSHARE":
The patch makes it so that if CLOSE_RANGE_UNSHARE is requested and we do in
fact currently share our file descriptor table we create a new private copy.
We then close all fds in the requested range and finally after we're done we
install the new fd table.
Is there any reason to do things in that order? IOW, what's wrong with
replacing the descriptor table as soon as we'd done dup_fd() (unshared_fd()
in your original version) and doing the closing after that?
I'm trying to come up with reasons for that, and I don't see any, TBH...
The reason I'm asking is that doing it your way complicates the analysis
in there - e.g. __put_unused_fd(files, _) almost always has files equal to
current->files with files->file_lock held by caller; the sole exception
is __put_unused_fd() called from file_close_fd_locked(), called from
__range_close(), called from close_range(2) with CLOSE_RANGE_UNSHARE
with current->files->count greater than 1. On that call chain and on that
call chain alone it is given the result of dup_fd() that will shortly
(and unconditionally) be installed as current->files. ->file_lock
is, thankfully, held.
Sure, it's visible only to the caller, so we can simply adjust the
predicate to "equal to current->files or not visible in any shared
data structures", but... what's the point?
Note that delayed replacement of descriptor table does not buy us
anything in terms of concurrent modifications - another thread could
modify our descriptor table only if they had their ->files pointing
to it, which obviously won't be possible until our thread spawns
a new child with CLONE_FILES.
Am I missing something subtle here?
reply other threads:[~2025-10-06 22:18 UTC|newest]
Thread overview: [no followups] expand[flat|nested] mbox.gz Atom feed
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=20251006221828.GH2441659@ZenIV \
--to=viro@zeniv.linux.org.uk \
--cc=brauner@kernel.org \
--cc=linux-fsdevel@vger.kernel.org \
/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 NNTP newsgroup(s).