On Fri, Apr 14, 2023 at 08:24:56PM -0700, Luis Chamberlain wrote:
> I thought of that but I saw that the loop that assigns the arr only
> pegs a bh if we don't "continue" for certain conditions, which made me
> believe that we only wanted to keep on the array as non-null items which
> meet the initial loop's criteria. If that is not accurate then yes,
> the simplication is nice!
Uh, right. A little bit more carefully this time ... how does this
look?
diff --git a/fs/buffer.c b/fs/buffer.c
index 5e67e21b350a..dff671079b02 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -2282,7 +2282,7 @@ 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;
unsigned int blocksize, bbits;
int nr, i;
int fully_mapped = 1;
@@ -2335,7 +2335,7 @@ int block_read_full_folio(struct folio *folio, get_block_t *get_block)
if (buffer_uptodate(bh))
continue;
}
- arr[nr++] = bh;
+ nr++;
} while (i++, iblock++, (bh = bh->b_this_page) != head);
if (fully_mapped)
@@ -2352,25 +2352,29 @@ int block_read_full_folio(struct folio *folio, get_block_t *get_block)
return 0;
}
- /* Stage two: lock the buffers */
- for (i = 0; i < nr; i++) {
- bh = arr[i];
+ /*
+ * Stage two: lock the buffers. Recheck the uptodate flag under
+ * the lock in case somebody else brought it uptodate first.
+ */
+ bh = head;
+ do {
+ if (buffer_uptodate(bh))
+ continue;
lock_buffer(bh);
+ if (buffer_uptodate(bh)) {
+ unlock_buffer(bh);
+ continue;
+ }
mark_buffer_async_read(bh);
- }
+ } while ((bh = bh->b_this_page) != head);
- /*
- * 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
+ /* Stage 3: start the IO */
+ bh = head;
+ do {
+ if (buffer_async_read(bh))
submit_bh(REQ_OP_READ, bh);
- }
+ } while ((bh = bh->b_this_page) != head);
+
return 0;
}
EXPORT_SYMBOL(block_read_full_folio);
I do wonder how much it's worth doing this vs switching to non-BH methods.
I appreciate that's a lot of work still.
