Re: [RFC v2 01/11] fs/buffer: move async batch read code into a helper

Hannes Reinecke
2024-12-17T09:56:11Z

On 12/14/24 04:10, Luis Chamberlain wrote:
> Move the code from block_read_full_folio() which does a batch of async
> reads into a helper.
>
> No functional changes.
>
> Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
> ---
> fs/buffer.c | 73 +++++++++++++++++++++++++++++++----------------------
> 1 file changed, 43 insertions(+), 30 deletions(-)
>
> diff --git a/fs/buffer.c b/fs/buffer.c
> index cc8452f60251..580451337efa 100644
> --- a/fs/buffer.c
> +++ b/fs/buffer.c
> @@ -2350,6 +2350,48 @@ bool block_is_partially_uptodate(struct folio *folio, size_t from, size_t count)
> }
> EXPORT_SYMBOL(block_is_partially_uptodate);
>
> +static void bh_read_batch_async(struct folio *folio,
> + int nr, struct buffer_head *arr[],
> + bool fully_mapped, bool no_reads,
> + bool any_get_block_error)
> +{
> + int i;
> + struct buffer_head *bh;
> +
> + if (fully_mapped)
> + folio_set_mappedtodisk(folio);
> +
> + if (no_reads) {
> + /*
> + * All buffers are uptodate or get_block() returned an
> + * error when trying to map them *all* buffers we can
> + * finish the read.
> + */
> + folio_end_read(folio, !any_get_block_error);
> + return;
> + }
> +
> + /* Stage one: lock the buffers */

Now you messed up documentation:
Originally this was 'stage two', so now we have two 'stage one'
comments.
Please use the original documentation convention and add a note
to the helper that it's contingent on the 'stage 1' in the
calling function.

> + for (i = 0; i < nr; i++) {
> + bh = arr[i];
> + lock_buffer(bh);
> + mark_buffer_async_read(bh);
> + }
> +
> + /*
> + * Stage 2: start the IO. Check for uptodateness
> + * inside the buffer lock in case another process reading
> + * the underlying blockdev brought it uptodate (the sct fix).
> + */
Same here; should be 'stage 3' to be consistent.

> + 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);
> + }
> +}
> +
> /*
> * Generic "read_folio" function for block devices that have the normal
> * get_block functionality. This is most of the block device filesystems.
> @@ -2414,37 +2456,8 @@ int block_read_full_folio(struct folio *folio, get_block_t *get_block)
> arr[nr++] = bh;
> } while (i++, 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);
> - }
> + bh_read_batch_async(folio, nr, arr, fully_mapped, nr == 0, page_error);
>
> - /*
> - * 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).
> - */
> - 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);
> - }
> return 0;
> }
> EXPORT_SYMBOL(block_read_full_folio);

Otherwise looks good.

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