Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 8 Aug 2023 10:40:57 +0200
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)
Message-ID: <20230808-unsensibel-scham-c61a71622ae7@brauner>
References: <20230806230627.1394689-1-mjguzik@gmail.com>
 <87o7jidqlg.fsf@email.froward.int.ebiederm.org>
 <20230808-eingaben-lumpen-e3d227386e23@brauner>
 <CAGudoHF=cEvXy3v96dN_ruXHnPv33BA6fA+dCWCm-9L3xgMPNQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <CAGudoHF=cEvXy3v96dN_ruXHnPv33BA6fA+dCWCm-9L3xgMPNQ@mail.gmail.com>
Precedence: bulk

> > And making filp_close() fully sync again is also really not great.
> 
> The patch is not doing it.

No, but you were referencing the proposed patch as an alternative in
your commit message.

> > 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.
> >
> 
> I don't think perf tax on something becomes more sensible the longer
> it is there.

One does need to answer the question why it does suddenly become
relevant after all these years though.

The original discussion was triggered by fifo ordering in task work
which led to a noticable regression and why it was ultimately reverted.
The sync proposal for fput() was an orthogonal proposal and the
conclusion was that it wasn't safe generally
https://lore.kernel.org/all/20150905051915.GC22011@ZenIV.linux.org.uk
even though it wasn't a direct response to the patch you linked.

Sure, for f_pos it was obvious when and how that happend. Here? It needs
a bit more justification.

If you care about it enough send a patch that just makes close(2) go
sync. We'll stuff it in a branch and we'll see what LKP has to say about
it or whether this gets lost in noise. I really don't think letting
micro-benchmarks become a decisive factor for code churn is a good
idea.

And fwiw, yes, maybe the difference between close(2) and other parts
doesn't matter for you but for use mortals that maintain a bunch more
then just a few lines of code in file.c if you have a tiny collection of
differences in behavior everywhere it adds up. The fact that you think
it's irrelevant doesn't mean we have that luxury.

That's not to say your patches haven't been useful. Not at all. The
close_range() tweak was very much appreciated and that f_pos thing was
good to fix as well.