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

[Hannes Reinecke <hare@suse.de>]

On 2/5/25 17:21, Hannes Reinecke wrote:
> 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?
> 
In the light of the discussion to the previous patch we should move that
to a later point. So:

Reviewed-by: Hannes Reinecke <hare@suse.de>

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