Re: [PATCH v2 2/8] fs/buffer: remove batching from async read

Hannes Reinecke
2025-02-05T16:21:04Z

On 2/5/25 00:12, Luis Chamberlain wrote:
> From: Matthew Wilcox <willy@infradead.org>
>
> The current implementation of a folio async read in block_read_full_folio()
> first batches all buffer-heads which need IOs issued for by putting them on an
> array of max size MAX_BUF_PER_PAGE. After collection it locks the batched
> buffer-heads and finally submits the pending reads. On systems with CPUs
> where the system page size is quite larger like Hexagon with 256 KiB this
> batching can lead stack growth warnings so we want to avoid that.
>
> Note the use of folio_end_read() through block_read_full_folio(), its
> used either when the folio is determined to be fully uptodate and no
> pending read is needed, an IO error happened on get_block(), or an out of
> bound read raced against batching collection to make our required reads
> uptodate.
>
> We can simplify this logic considerably and remove the stack growth
> issues of MAX_BUF_PER_PAGE by just replacing the batched logic with
> one which only issues IO for the previous buffer-head keeping in mind
> we'll always have one buffer-head (the current one) on the folio with
> an async flag, this will prevent any calls to folio_end_read().
>
> So we accomplish two things with this:
>
> o Avoid large stacks arrays with MAX_BUF_PER_PAGE
> o Make the need for folio_end_read() explicit and easier to read
>
> Suggested-by: Matthew Wilcox <willy@infradead.org>
> Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
> ---
> fs/buffer.c | 51 +++++++++++++++++++++------------------------------
> 1 file changed, 21 insertions(+), 30 deletions(-)
>
> diff --git a/fs/buffer.c b/fs/buffer.c
> index b99560e8a142..167fa3e33566 100644
> --- a/fs/buffer.c
> +++ b/fs/buffer.c
> @@ -2361,9 +2361,8 @@ int block_read_full_folio(struct folio *folio, get_block_t *get_block)
> {
> struct inode *inode = folio->mapping->host;
> sector_t iblock, lblock;
> - struct buffer_head *bh, *head, *arr[MAX_BUF_PER_PAGE];
> + struct buffer_head *bh, *head, *prev = NULL;
> size_t blocksize;
> - int nr, i;
> int fully_mapped = 1;
> bool page_error = false;
> loff_t limit = i_size_read(inode);
> @@ -2380,7 +2379,6 @@ int block_read_full_folio(struct folio *folio, get_block_t *get_block)
> iblock = div_u64(folio_pos(folio), blocksize);
> lblock = div_u64(limit + blocksize - 1, blocksize);
> bh = head;
> - nr = 0;
>
> do {
> if (buffer_uptodate(bh))
> @@ -2410,40 +2408,33 @@ int block_read_full_folio(struct folio *folio, get_block_t *get_block)
> if (buffer_uptodate(bh))
> continue;
> }
> - arr[nr++] = bh;
> +
> + lock_buffer(bh);
> + if (buffer_uptodate(bh)) {
> + unlock_buffer(bh);
> + continue;
> + }
> +
> + mark_buffer_async_read(bh);
> + if (prev)
> + submit_bh(REQ_OP_READ, prev);
> + prev = bh;
> } while (iblock++, (bh = bh->b_this_page) != head);
>
> if (fully_mapped)
> folio_set_mappedtodisk(folio);
>
> - if (!nr) {
> - /*
> - * All buffers are uptodate or get_block() returned an
> - * error when trying to map them - we can finish the read.
> - */
> - folio_end_read(folio, !page_error);
> - return 0;
> - }
> -
> - /* Stage two: lock the buffers */
> - for (i = 0; i < nr; i++) {
> - bh = arr[i];
> - lock_buffer(bh);
> - mark_buffer_async_read(bh);
> - }
> -
> /*
> - * Stage 3: start the IO. Check for uptodateness
> - * inside the buffer lock in case another process reading
> - * the underlying blockdev brought it uptodate (the sct fix).
> + * All buffers are uptodate or get_block() returned an error
> + * when trying to map them - we must finish the read because
> + * end_buffer_async_read() will never be called on any buffer
> + * in this folio.
> */
> - for (i = 0; i < nr; i++) {
> - bh = arr[i];
> - if (buffer_uptodate(bh))
> - end_buffer_async_read(bh, 1);
> - else
> - submit_bh(REQ_OP_READ, bh);
> - }
> + if (prev)
> + submit_bh(REQ_OP_READ, prev);
> + else
> + folio_end_read(folio, !page_error);
> +
> return 0;
> }
> EXPORT_SYMBOL(block_read_full_folio);

Similar here; as we now removed batching (which technically could result
in I/O being completed while executing the various stages) there really
is nothing preventing us to use plugging here, no?

Cheers,

Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
hare@suse.de +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich