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;
MIME-Version: 1.0
In-Reply-To: <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>
 <20230808-unsensibel-scham-c61a71622ae7@brauner>
From:   Mateusz Guzik <mjguzik@gmail.com>
Date:   Tue, 8 Aug 2023 11:21:46 +0200
Message-ID: <CAGudoHEQ6Tq=88VKqurypjHqOzfU2eBmPts4+H8C7iNu96MRKQ@mail.gmail.com>
Subject: Re: [PATCH] fs: use __fput_sync in close(2)
To:     Christian Brauner <brauner@kernel.org>
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>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On 8/8/23, Christian Brauner <brauner@kernel.org> wrote:
>> 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.
>

There is some work I'm considering doing, but before that happens I'm
sanity checking performance of various syscalls and I keep finding
problems, some of which are trivially avoidable.

I'm genuinely confused with the strong opposition to the very notion
of making close(2) a special case (which I consider conceptually
trivial), but as you noted below I'm not ultimately the person on the
hook for any problems.

> 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.
>

Ok, I missed this e-mail. It further discourages patching filp_close,
but does not make an argument against *just* close(2) rolling with
sync which is what I'm proposing.

> If you care about it enough send a patch that just makes close(2) go
> sync.

But this is precisely what the submitted patch is doing. It adds
file_fput_sync, then adds close_fd_sync which is the only consumer and
only makes close(2) use it. *nobody* else has sync added.

One can argue the way this is sorted out is crap and I'm not going to
defend it. I am saying making *just* close(2) roll with sync is very
easy, there are numerous ways to do it and anyone involved with
maintaining vfs can write their own variant in minutes. Basically I
don't see *technical* problems here.

> 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.
>

That would be nice. Given the patch is already doing what you asked,
can you just take it as is?

I'll note though what I mentioned elsewhere
(https://lore.kernel.org/all/CAGudoHEG7vtCRWjn0yR5LMUsaw3KJANfa+Hkke9gy0imXQz6tg@mail.gmail.com/):
can they make sure to whack CONFIG_RANDOMIZE_KSTACK_OFFSET=y from
their kernel config? It is an *optional* measure and it comes at a
massive premium, so single-threaded changes are easily diminished.

-- 
Mateusz Guzik <mjguzik gmail.com>