| Message ID | 2f13c006ad12b047e9e4d5de008e5d5c41322754.1610572007.git.cgoldswo@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] fs/buffer.c: Revoke LRU when trying to drop buffers | expand |
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.
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.
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)