Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 15 Mar 2021 16:41:38 -0700
From:   Andrew Morton <akpm@linux-foundation.org>
To:     Chris Goldsworthy <cgoldswo@codeaurora.org>
Cc:     Alexander Viro <viro@zeniv.linux.org.uk>,
        Matthew Wilcox <willy@infradead.org>, linux-mm@kvack.org,
        linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
        Laura Abbott <lauraa@codeaurora.org>
Subject: Re: [PATCH v3] fs/buffer.c: Revoke LRU when trying to drop buffers
Message-Id: <20210315164138.c15727adeb184313f5e7e9f6@linux-foundation.org>
In-Reply-To: <2f13c006ad12b047e9e4d5de008e5d5c41322754.1610572007.git.cgoldswo@codeaurora.org>
References: <cover.1610572007.git.cgoldswo@codeaurora.org>
        <2f13c006ad12b047e9e4d5de008e5d5c41322754.1610572007.git.cgoldswo@codeaurora.org>
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Wed, 13 Jan 2021 13:17:30 -0800 Chris Goldsworthy <cgoldswo@codeaurora.org> wrote:

> From: Laura Abbott <lauraa@codeaurora.org>
> 
> When a buffer is added to the LRU list, a reference is taken which is
> not dropped until the buffer is evicted from the LRU list. This is the
> correct behavior, however this LRU reference will prevent the buffer
> from being dropped. This means that the buffer can't actually be dropped
> until it is selected for eviction. There's no bound on the time spent
> on the LRU list, which means that the buffer may be undroppable for
> very long periods of time. Given that migration involves dropping
> buffers, the associated page is now unmigratible for long periods of
> time as well. CMA relies on being able to migrate a specific range
> of pages, so these types of failures make CMA significantly
> less reliable, especially under high filesystem usage.
> 
> Rather than waiting for the LRU algorithm to eventually kick out
> the buffer, explicitly remove the buffer from the LRU list when trying
> to drop it. There is still the possibility that the buffer
> could be added back on the list, but that indicates the buffer is
> still in use and would probably have other 'in use' indicates to
> prevent dropping.
> 
> Note: a bug reported by "kernel test robot" lead to a switch from
> using xas_for_each() to xa_for_each().

(hm, why isn't drop_buffers() static to fs/buffer.c??)

It looks like patch this turns drop_buffers() into a very expensive
operation.  And that expensive operation occurs under the
address_space-wide private_lock, which is more ouch.

How carefully has this been tested for performance?  In pathological
circumstances (which are always someone's common case :()


Just thinking out loud...

If a buffer_head* is sitting in one or more of the LRUs, what is
stopping us from stripping it from the page anyway?  Then
try_to_free_buffers() can mark the bh as buffer_dead(), declare success
and leave the bh sitting in the LRU, with the LRU as the only reference
to that buffer.  Teach lookup_bh_lru() to skip over buffer_dead()
buffers and our now-dead buffer will eventually reach the tail of the
lru and get freed for real.