* Re: FAILED: patch "[PATCH] btrfs: do not wait for short bulk allocation" failed to apply to 6.6-stable tree
[not found] <2024041951-sports-hula-f2a5@gregkh>
@ 2024-05-06 8:41 ` Julian Taylor
2024-05-14 17:13 ` David Sterba
0 siblings, 1 reply; 2+ messages in thread
From: Julian Taylor @ 2024-05-06 8:41 UTC (permalink / raw)
To: wqu; +Cc: linux-btrfs
Hello,
thank you for swiftly fixing the issue.
As the problem affects the stable releases 6.1 and 6.6 would it be
possible to backport the fix to these versions so it can make its way
into distributions?
It is a reasonably simple backport though 6.1 has some differences in
the ENOMEM situation.
This should apply to 6.6:
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -686,24 +686,14 @@ int btrfs_alloc_page_array(unsigned int nr_pages,
struct page **page_array)
unsigned int last = allocated;
allocated = alloc_pages_bulk_array(GFP_NOFS, nr_pages,
page_array);
-
- if (allocated == nr_pages)
- return 0;
-
- /*
- * During this iteration, no page could be allocated, even
- * though alloc_pages_bulk_array() falls back to alloc_page()
- * if it could not bulk-allocate. So we must be out of memory.
- */
- if (allocated == last) {
+ if (unlikely(allocated == last)) {
+ /* No progress, fail and do cleanup. */
for (int i = 0; i < allocated; i++) {
__free_page(page_array[i]);
page_array[i] = NULL;
}
return -ENOMEM;
}
-
- memalloc_retry_wait(GFP_NOFS);
}
return 0;
}
On 19.04.24 12:39, gregkh@linuxfoundation.org wrote:
> The patch below does not apply to the 6.6-stable tree.
> If someone wants it applied there, or to any other stable or longterm
> tree, then please email the backport, including the original git commit
> id to <stable@vger.kernel.org>.
>
> To reproduce the conflict and resubmit, you may use the following commands:
>
> git fetch https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/ linux-6.6.y
> git checkout FETCH_HEAD
> git cherry-pick -x 1db7959aacd905e6487d0478ac01d89f86eb1e51
> # <resolve conflicts, build, test, etc.>
> git commit -s
> git send-email --to '<stable@vger.kernel.org>' --in-reply-to '2024041951-sports-hula-f2a5@gregkh' --subject-prefix 'PATCH 6.6.y' HEAD^..
>
> Possible dependencies:
>
> 1db7959aacd9 ("btrfs: do not wait for short bulk allocation")
> 09e6cef19c9f ("btrfs: refactor alloc_extent_buffer() to allocate-then-attach method")
> 397239ed6a6c ("btrfs: allow extent buffer helpers to skip cross-page handling")
> 94dbf7c0871f ("btrfs: free the allocated memory if btrfs_alloc_page_array() fails")
>
> thanks,
>
> greg k-h
>
> ------------------ original commit in Linus's tree ------------------
>
> From 1db7959aacd905e6487d0478ac01d89f86eb1e51 Mon Sep 17 00:00:00 2001
> From: Qu Wenruo <wqu@suse.com>
> Date: Tue, 26 Mar 2024 09:16:46 +1030
> Subject: [PATCH] btrfs: do not wait for short bulk allocation
>
> [BUG]
> There is a recent report that when memory pressure is high (including
> cached pages), btrfs can spend most of its time on memory allocation in
> btrfs_alloc_page_array() for compressed read/write.
>
> [CAUSE]
> For btrfs_alloc_page_array() we always go alloc_pages_bulk_array(), and
> even if the bulk allocation failed (fell back to single page
> allocation) we still retry but with extra memalloc_retry_wait().
>
> If the bulk alloc only returned one page a time, we would spend a lot of
> time on the retry wait.
>
> The behavior was introduced in commit 395cb57e8560 ("btrfs: wait between
> incomplete batch memory allocations").
>
> [FIX]
> Although the commit mentioned that other filesystems do the wait, it's
> not the case at least nowadays.
>
> All the mainlined filesystems only call memalloc_retry_wait() if they
> failed to allocate any page (not only for bulk allocation).
> If there is any progress, they won't call memalloc_retry_wait() at all.
>
> For example, xfs_buf_alloc_pages() would only call memalloc_retry_wait()
> if there is no allocation progress at all, and the call is not for
> metadata readahead.
>
> So I don't believe we should call memalloc_retry_wait() unconditionally
> for short allocation.
>
> Call memalloc_retry_wait() if it fails to allocate any page for tree
> block allocation (which goes with __GFP_NOFAIL and may not need the
> special handling anyway), and reduce the latency for
> btrfs_alloc_page_array().
>
> Reported-by: Julian Taylor <julian.taylor@1und1.de>
> Tested-by: Julian Taylor <julian.taylor@1und1.de>
> Link: https://lore.kernel.org/all/8966c095-cbe7-4d22-9784-a647d1bf27c3@1und1.de/
> Fixes: 395cb57e8560 ("btrfs: wait between incomplete batch memory allocations")
> CC: stable@vger.kernel.org # 6.1+
> Reviewed-by: Sweet Tea Dorminy <sweettea-kernel@dorminy.me>
> Reviewed-by: Filipe Manana <fdmanana@suse.com>
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> Reviewed-by: David Sterba <dsterba@suse.com>
> Signed-off-by: David Sterba <dsterba@suse.com>
>
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index b18034f2ab80..2776112dbdf8 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -681,31 +681,21 @@ static void end_bbio_data_read(struct btrfs_bio *bbio)
> int btrfs_alloc_page_array(unsigned int nr_pages, struct page **page_array,
> gfp_t extra_gfp)
> {
> + const gfp_t gfp = GFP_NOFS | extra_gfp;
> unsigned int allocated;
>
> for (allocated = 0; allocated < nr_pages;) {
> unsigned int last = allocated;
>
> - allocated = alloc_pages_bulk_array(GFP_NOFS | extra_gfp,
> - nr_pages, page_array);
> -
> - if (allocated == nr_pages)
> - return 0;
> -
> - /*
> - * During this iteration, no page could be allocated, even
> - * though alloc_pages_bulk_array() falls back to alloc_page()
> - * if it could not bulk-allocate. So we must be out of memory.
> - */
> - if (allocated == last) {
> + allocated = alloc_pages_bulk_array(gfp, nr_pages, page_array);
> + if (unlikely(allocated == last)) {
> + /* No progress, fail and do cleanup. */
> for (int i = 0; i < allocated; i++) {
> __free_page(page_array[i]);
> page_array[i] = NULL;
> }
> return -ENOMEM;
> }
> -
> - memalloc_retry_wait(GFP_NOFS);
> }
> return 0;
> }
>
^ permalink raw reply [flat|nested] 2+ messages in thread