Re: [PATCH v2 1/4] mm/filemap: Add folio_lock_timeout()

From: Linus Torvalds <torvalds@linux-foundation.org>
To: Mel Gorman <mgorman@techsingularity.net>
Cc: Doug Anderson <dianders@chromium.org>,
	Hillf Danton <hdanton@sina.com>,
	 Andrew Morton <akpm@linux-foundation.org>,
	Alexander Viro <viro@zeniv.linux.org.uk>,
	 Christian Brauner <brauner@kernel.org>,
	linux-kernel@vger.kernel.org, linux-mm@kvack.org,
	 Yu Zhao <yuzhao@google.com>,
	Matthew Wilcox <willy@infradead.org>
Subject: Re: [PATCH v2 1/4] mm/filemap: Add folio_lock_timeout()
Date: Wed, 26 Apr 2023 08:24:36 -0700	[thread overview]
Message-ID: <CAHk-=wiTw-Ct60_P+jnOfjKBAvpNJyvZENhA477Th0K6b6S3fA@mail.gmail.com> (raw)
In-Reply-To: <20230426100918.ku32k6mqoogsnijn@techsingularity.net>

On Wed, Apr 26, 2023 at 3:09 AM Mel Gorman <mgorman@techsingularity.net> wrote:
>
> The reason I don't consider this patch a NAK candidate is that this is not
> conditional locking as such because no special action is taken if the lock
> cannot be acquired.

If this comes from my rant against not having conditional locking for
the O_NDELAY (well, the io_uring equivalent) IO path, then no, I don't
think something like lock_page_timout() is "conditional locking".

Some caller wanting to get the lock, but being willing to just go on
and do something else after a timeout is fine. Within the context of
something like memory compaction internally for the kernel that is
fundamentally opportunistic anyway, that sounds fine to me.

In fact, in contexts like that, even trylock is fine. We obviously
have trylock in lots of places of the kernel.

My "no conditional locking" is really that I do not think it's sane to
have user IO fail with EAGAIN just because some data structure
happened to be busy. It's a debugging nightmare with unlikely things
that happen only in special conditions. Doing IO is not some
"opportunistic" thing.

We've actually had things like that before where people tried to make
O_NDELAY mean "no locking" (I think that was driven at least partly by
the old in-kernel web server patches), and it also causes actual
problems with user space then busy-looping in a select() loop, because
the select doesn't consider some low-level lock to be a waiting event.

(The io_uring case is _slightly_ different from our historical issues
in this area, in that the kernel can fall back to the user worker
thread case, but it's all mixed up in that same IO path and that's why
I absolutely hated that "if (X) trylock else proper_lock" approach).

So a unconditional "lock with timeout" in the context of "we can just
skip this if it times out" is perfectly fine by me.

That said - the kcompactd code is not code I know, so maybe there are
*other* issues with that patch, so this is also not an ACK from me.

So please consider this just a "that is a very different case from the
one I complained about".

                Linus