From: Laura Abbott <[email protected]>
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().
Signed-off-by: Laura Abbott <[email protected]>
Signed-off-by: Chris Goldsworthy <[email protected]>
Cc: Matthew Wilcox <[email protected]>
Reported-by: kernel test robot <[email protected]>
---
fs/buffer.c | 81 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++----
1 file changed, 76 insertions(+), 5 deletions(-)
diff --git a/fs/buffer.c b/fs/buffer.c
index 96c7604..d2d1237 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -48,6 +48,7 @@
#include <linux/sched/mm.h>
#include <trace/events/block.h>
#include <linux/fscrypt.h>
+#include <linux/xarray.h>
#include "internal.h"
@@ -1471,12 +1472,59 @@ static bool has_bh_in_lru(int cpu, void *dummy)
return false;
}
+static void __evict_bhs_lru(void *arg)
+{
+ struct bh_lru *b = &get_cpu_var(bh_lrus);
+ struct xarray *busy_bhs = arg;
+ struct buffer_head *bh;
+ unsigned long i, xarray_index;
+
+ xa_for_each(busy_bhs, xarray_index, bh) {
+ for (i = 0; i < BH_LRU_SIZE; i++) {
+ if (b->bhs[i] == bh) {
+ brelse(b->bhs[i]);
+ b->bhs[i] = NULL;
+ break;
+ }
+ }
+
+ bh = bh->b_this_page;
+ }
+
+ put_cpu_var(bh_lrus);
+}
+
+static bool page_has_bhs_in_lru(int cpu, void *arg)
+{
+ struct bh_lru *b = per_cpu_ptr(&bh_lrus, cpu);
+ struct xarray *busy_bhs = arg;
+ struct buffer_head *bh;
+ unsigned long i, xarray_index;
+
+ xa_for_each(busy_bhs, xarray_index, bh) {
+ for (i = 0; i < BH_LRU_SIZE; i++) {
+ if (b->bhs[i] == bh)
+ return true;
+ }
+
+ bh = bh->b_this_page;
+ }
+
+ return false;
+
+}
void invalidate_bh_lrus(void)
{
on_each_cpu_cond(has_bh_in_lru, invalidate_bh_lru, NULL, 1);
}
EXPORT_SYMBOL_GPL(invalidate_bh_lrus);
+static void evict_bh_lrus(struct xarray *busy_bhs)
+{
+ on_each_cpu_cond(page_has_bhs_in_lru, __evict_bhs_lru,
+ busy_bhs, 1);
+}
+
void set_bh_page(struct buffer_head *bh,
struct page *page, unsigned long offset)
{
@@ -3242,14 +3290,36 @@ drop_buffers(struct page *page, struct buffer_head **buffers_to_free)
{
struct buffer_head *head = page_buffers(page);
struct buffer_head *bh;
+ struct xarray busy_bhs;
+ int bh_count = 0;
+ int xa_ret, ret = 0;
+
+ xa_init(&busy_bhs);
bh = head;
do {
- if (buffer_busy(bh))
- goto failed;
+ if (buffer_busy(bh)) {
+ xa_ret = xa_err(xa_store(&busy_bhs, bh_count++,
+ bh, GFP_ATOMIC));
+ if (xa_ret)
+ goto out;
+ }
bh = bh->b_this_page;
} while (bh != head);
+ if (bh_count) {
+ /*
+ * Check if the busy failure was due to an outstanding
+ * LRU reference
+ */
+ evict_bh_lrus(&busy_bhs);
+ do {
+ if (buffer_busy(bh))
+ goto out;
+ } while (bh != head);
+ }
+
+ ret = 1;
do {
struct buffer_head *next = bh->b_this_page;
@@ -3259,9 +3329,10 @@ drop_buffers(struct page *page, struct buffer_head **buffers_to_free)
} while (bh != head);
*buffers_to_free = head;
detach_page_private(page);
- return 1;
-failed:
- return 0;
+out:
+ xa_destroy(&busy_bhs);
+
+ return ret;
}
int try_to_free_buffers(struct page *page)
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
On Wed, 13 Jan 2021 13:17:30 -0800 Chris Goldsworthy <[email protected]> wrote:
> From: Laura Abbott <[email protected]>
>
> 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.
On Mon, Mar 15, 2021 at 04:41:38PM -0700, Andrew Morton wrote:
> > 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.
>
> 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.
This patch set is obsoleted by Minchan Kim's more recent patch-set.