public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
From: Christian Brauner <brauner@kernel.org>
To: Mateusz Guzik <mjguzik@gmail.com>
Cc: "Eric W. Biederman" <ebiederm@xmission.com>,
	viro@zeniv.linux.org.uk, linux-fsdevel@vger.kernel.org,
	linux-kernel@vger.kernel.org, oleg@redhat.com,
	Matthew Wilcox <willy@infradead.org>,
	Linus Torvalds <torvalds@linux-foundation.org>
Subject: Re: [PATCH] fs: use __fput_sync in close(2)
Date: Tue, 8 Aug 2023 10:13:04 +0200	[thread overview]
Message-ID: <20230808-eingaben-lumpen-e3d227386e23@brauner> (raw)
In-Reply-To: <87o7jidqlg.fsf@email.froward.int.ebiederm.org>

> Are there any real world gains if close(2) is the only place this
> optimization can be applied?  Is the added maintenance burden worth the
> speed up?

Tbh, none of this patch makes me exited.

It adds two new exports of filp_close_sync() and close_fd_sync() without
any users. That's not something we do and we also shouldn't encourage
random drivers to switch to sync behavior.

That rseq thing is completely orthogonal and maybe that needs to be
fixed and you can go and convince the glibc people to do it.

And making filp_close() fully sync again is also really not great.
Simplicity wins out and if all codepaths share the same behavior we're
better off then having parts use task work and other parts just not.

Yes, we just did re-added the f_pos optimization because it may have had
an impact. And that makes more sense because that was something we had
changed just a few days/weeks before.

But this is over 10 year old behavior and this micro benchmarking isn't
a strong selling point imho. We could make close(2) go sync, sure. But
I'm skeptical even about this without real-world data or from a proper
testsuite.

(There's also a stray sysctl_fput_sync there which is scary to think that
we'd ever allow a sysctl that allows userspace to control how we close
fds.)

> Unless you can find some real world performance gains this looks like
> a bad idea.

I agree.

  parent reply	other threads:[~2023-08-08 19:09 UTC|newest]

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-08-06 23:06 [PATCH] fs: use __fput_sync in close(2) Mateusz Guzik
2023-08-07  3:18 ` Matthew Wilcox
2023-08-08  5:56 ` Eric W. Biederman
2023-08-08  7:32   ` Mateusz Guzik
2023-08-08  8:13   ` Christian Brauner [this message]
2023-08-08  8:23     ` Mateusz Guzik
2023-08-08  8:40       ` Christian Brauner
2023-08-08  9:21         ` Mateusz Guzik
2023-08-08 15:07           ` [PATCH v2 (kindof)] " Mateusz Guzik
2023-08-08 16:30             ` Christian Brauner
2023-08-08 17:00               ` Christian Brauner
2023-08-08 17:05               ` Linus Torvalds
2023-08-08 17:06                 ` Christian Brauner
2023-08-09  9:03                 ` David Laight
2023-08-08 16:57   ` [PATCH] " Linus Torvalds
2023-08-08 17:10     ` Mateusz Guzik
2023-08-08 17:18       ` Linus Torvalds
2023-08-08 17:24         ` Mateusz Guzik
2023-08-08 17:35           ` Christian Brauner
2023-08-08 17:48             ` Linus Torvalds
2023-08-08 17:15     ` Christian Brauner
2023-08-08 17:22       ` Linus Torvalds
2023-08-08 17:48         ` Eric W. Biederman

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=20230808-eingaben-lumpen-e3d227386e23@brauner \
    --to=brauner@kernel.org \
    --cc=ebiederm@xmission.com \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mjguzik@gmail.com \
    --cc=oleg@redhat.com \
    --cc=torvalds@linux-foundation.org \
    --cc=viro@zeniv.linux.org.uk \
    --cc=willy@infradead.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