enable bs > ps for block devices

When we read over all buffers in a folio we currently use the buffer index on the folio and blocksize to get the offset. Simplify this with bh_offset(). This simplifies the loop while making no functional changes.

Suggested-by Matthew Wilcox <[email protected]>

Signed-off-by Luis Chamberlain <[email protected]>

---
  fs/buffer.c | 5 ++---
  1 file changed, 2 insertions(+), 3 deletions(-)

fs/buffer.c +1 -2

index cc8452f60251..b99560e8a142 100644
@@ -2381,7 +2381,6 @@ int block_read_full_folio(struct folio *folio, get_block_t *get_block)
  	lblock = div_u64(limit + blocksize - 1, blocksize);
  	bh = head;
  	nr = 0;
-	i = 0;
 
  	do {
  		if (buffer_uptodate(bh))

@@ -2398,7 +2397,7 @@ int block_read_full_folio(struct folio *folio, get_block_t *get_block)
  					page_error = true;
  			}
  			if (!buffer_mapped(bh)) {
-				folio_zero_range(folio, i * blocksize,
+				folio_zero_range(folio, bh_offset(bh),
  						blocksize);
  				if (!err)
  					set_buffer_uptodate(bh);

One wonders: shouldn't we use plugging here to make I/O more efficient?

Quoted 1 line · Hannes Reinecke

One wonders: shouldn't we use plugging here to make I/O more efficient?

Should we plug at a higher level?

Opposite question: What if getblk() needs to do a read (ie ext2 indirect block)?

Quoted 7 lines · Matthew Wilcox

Quoted 1 line · Hannes Reinecke

One wonders: shouldn't we use plugging here to make I/O more efficient?

Should we plug at a higher level?

Opposite question: What if getblk() needs to do a read (ie ext2 indirect block)?

Ah, that. Yes, plugging on higher level would be a good idea. (And can we check for nested plugs? _Should_ we check for nested plugs?)

Quoted 6 lines · Hannes Reinecke

Quoted 5 lines · Matthew Wilcox

Quoted 1 line · Hannes Reinecke

One wonders: shouldn't we use plugging here to make I/O more efficient?

Should we plug at a higher level?

Quoted 5 lines

Quoted 2 lines

Opposite question: What if getblk() needs to do a read (ie ext2 indirect block)?

Ah, that. Yes, plugging on higher level would be a good idea. (And can we check for nested plugs? _Should_ we check for nested plugs?)

I think given the discussion less is more for now, and if we really want this we can add it later. Thoughts?

From: Matthew Wilcox <[email protected]>

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 <[email protected]>

Signed-off-by Luis Chamberlain <[email protected]>

---
  fs/buffer.c | 51 +++++++++++++++++++++------------------------------
  1 file changed, 21 insertions(+), 30 deletions(-)

fs/buffer.c +16 -28

index b99560e8a142..167fa3e33566 100644
@@ -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

Quoted 112 lines · Luis Chamberlain

From: Matthew Wilcox <[email protected]>

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 <[email protected]>

Signed-off-by Luis Chamberlain <[email protected]>

---
  fs/buffer.c | 51 +++++++++++++++++++++------------------------------

  1 file changed, 21 insertions(+), 30 deletions(-)

fs/buffer.c +1 -2

index b99560e8a142..167fa3e33566 100644
@@ -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);

fs/buffer.c +19 -28

@@ -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);

fs/buffer.c int block_read_full_folio(struct folio *folio, get_block_t *get_block) · line 2440

Quoted 4 lines

fs/buffer.c +1 -0

+
       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?

From: Matthew Wilcox <[email protected]>

fs/buffer.c +21 -30

index b99560e8a142..167fa3e33566 100644
@@ -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);

Signature · 1 line

2.45.2

From: Hannes Reinecke <[email protected]>

For large blocksizes the number of block bits is larger than PAGE_SHIFT, so use instead use folio_pos(folio) >> blkbits to calculate the sector

number. This is required to enable large folios with buffer-heads.

Signed-off-by Luis Chamberlain <[email protected]>

Signed-off-by Hannes Reinecke <[email protected]>

fs/mpage.c static struct bio *do_mpage_readpage(struct mpage_readpage_args *args) · line 185

Quoted 6 lines · Luis Chamberlain

fs/mpage.c +1 -1

@@ -182,7 +182,7 @@ static struct bio *do_mpage_readpage(struct mpage_readpage_args *args)
  		goto confused;
 
  	block_in_file = folio_pos(folio) >> blkbits;
-	last_block = block_in_file + args->nr_pages * blocks_per_page;
+	last_block = block_in_file + args->nr_pages * blocks_per_folio;

In mpage_readahead(), we set args->nr_pages to the nunber of pages (not folios) being requested. In mpage_read_folio() we currently set it to
1. So this is going to read too far ahead for readahead if using large folios.

I think we need to make nr_pages continue to mean nr_pages. Or we pass in nr_bytes or nr_blocks.

Quoted 12 lines · Matthew Wilcox

Review static struct bio *do_mpage_readpage(struct mpage_readpage_args *args) · line 184

Quoted 5 lines · Luis Chamberlain

(quoted diff) +1 -1

@@ -182,7 +182,7 @@ static struct bio *do_mpage_readpage(struct mpage_readpage_args *args)
  		goto confused;
  	block_in_file = folio_pos(folio) >> blkbits;
-	last_block = block_in_file + args->nr_pages * blocks_per_page;
+	last_block = block_in_file + args->nr_pages * blocks_per_folio;

In mpage_readahead(), we set args->nr_pages to the nunber of pages (not folios) being requested. In mpage_read_folio() we currently set it to
1. So this is going to read too far ahead for readahead if using large folios.

Quoted 3 lines

I think we need to make nr_pages continue to mean nr_pages. Or we pass in nr_bytes or nr_blocks.

I had been pondering this, too, while developing the patch. The idea I had here was to change counting by pages over to counting by folios, as then the logic is essentially unchanged.

Not a big fan of 'nr_pages', as then the question really is how much data we should read at the end of the day. So I'd rather go with 'nr_blocks' to avoid any confusion.

fs/mpage.c +0 -0

 		.get_block = get_block,
 	};
 

fs/mpage.c int mpage_read_folio(struct folio *folio, get_block_t get_block) · line 391

Quoted 6 lines · Luis Chamberlain

fs/mpage.c +1 -1

@@ -385,7 +388,7 @@ int mpage_read_folio(struct folio *folio, get_block_t get_block)
 {
 	struct mpage_readpage_args args = {
 		.folio = folio,
-		.nr_pages = 1,
+		.nr_pages = mapping_min_folio_nrpages(folio->mapping),

		.nr_pages = folio_nr_pages(folio);

since the folio is not necessarily the minimum size.

+++ b/fs/mpage.c

fs/mpage.c +1 -0

@@ -152,6 +152,7 @@ static struct bio *do_mpage_readpage(struct mpage_readpage_args *args)
 {
 	struct folio *folio = args->folio;
 	struct inode *inode = folio->mapping->host;
+	const unsigned min_nrpages = mapping_min_folio_nrpages(folio->mapping);
 	const unsigned blkbits = inode->i_blkbits;
 	const unsigned blocks_per_folio = folio_size(folio) >> blkbits;
 	const unsigned blocksize = 1 << blkbits;

Quoted 16 lines · Luis Chamberlain

+++ b/fs/mpage.c

fs/mpage.c +3 -0

@@ -152,6 +152,7 @@ static struct bio *do_mpage_readpage(struct mpage_readpage_args *args)
 {
 	struct folio *folio = args->folio;
 	struct inode *inode = folio->mapping->host;
+	const unsigned min_nrpages = mapping_min_folio_nrpages(folio->mapping);
 	const unsigned blkbits = inode->i_blkbits;
 	const unsigned blocks_per_folio = folio_size(folio) >> blkbits;
 	const unsigned blocksize = 1 << blkbits;

@@ -172,6 +173,8 @@ static struct bio *do_mpage_readpage(struct mpage_readpage_args *args)
 
 	/* MAX_BUF_PER_PAGE, for example */
 	VM_BUG_ON_FOLIO(folio_test_large(folio), folio);
+	VM_BUG_ON_FOLIO(args->nr_pages < min_nrpages, folio);
+	VM_BUG_ON_FOLIO(!IS_ALIGNED(args->nr_pages, min_nrpages), folio);
 

fs/mpage.c static struct bio *do_mpage_readpage(struct mpage_readpage_args *args) · line 180

Quoted 2 lines

fs/mpage.c +0 -0

 	if (args->is_readahead) {
 		opf |= REQ_RAHEAD;

Also, I don't think these assertions add any value; we already assert these things are true in other places.

Now that buffer-heads has been converted over to support large folios we can remove the built-in VM_BUG_ON_FOLIO() checks which prevents their use.

Signed-off-by Luis Chamberlain <[email protected]>

---
  fs/buffer.c | 2 --
  fs/mpage.c  | 3 ---
  2 files changed, 5 deletions(-)

Now that buffer-heads has been converted over to support large folios we can remove the built-in VM_BUG_ON_FOLIO() checks which prevents their use.

Signed-off-by Luis Chamberlain <[email protected]>

From: Hannes Reinecke <[email protected]>

Call mapping_set_folio_min_order() when modifying the logical block size to ensure folios are allocated with the correct size.

Signed-off-by Hannes Reinecke <[email protected]>

We now can support blocksizes larger than PAGE_SIZE, so in theory we should be able to lift the restriction up to the max supported page cache order. However bound ourselves to what we can currently validate and test. Through blktests and fstest we can validate up to 64k today.

Reviewed-by Hannes Reinecke <[email protected]>

Signed-off-by Luis Chamberlain <[email protected]>

You can use lsblk to query for a block device block device block size:

lsblk -o MIN-IO /dev/nvme0n1
MIN-IO
4096

The min-io is the minimum IO the block device prefers for optimal performance. In turn we map this to the block device block size. The current block size exposed even for block devices with an LBA format of 16k is 4k. Likewise devices which support 4k LBA format but have a larger Indirection Unit of 16k have an exposed block size of 4k.

This incurs read-modify-writes on direct IO against devices with a min-io larger than the page size. To fix this, use the block device min io, which is the minimal optimal IO the device prefers.

With this we now get:

lsblk -o MIN-IO /dev/nvme0n1
MIN-IO
16384

And so userspace gets the appropriate information it needs for optimal performance. This is verified with blkalgn against mkfs against a device with LBA format of 4k but an NPWG of 16k (min io size)

mkfs.xfs -f -b size=16k /dev/nvme3n1
blkalgn -d nvme3n1 --ops Write

      Block size          : count     distribution
          0 -> 1          : 0        |                                        |
          2 -> 3          : 0        |                                        |
          4 -> 7          : 0        |                                        |
          8 -> 15         : 0        |                                        |
         16 -> 31         : 0        |                                        |
         32 -> 63         : 0        |                                        |
         64 -> 127        : 0        |                                        |
        128 -> 255        : 0        |                                        |
        256 -> 511        : 0        |                                        |
        512 -> 1023       : 0        |                                        |
       1024 -> 2047       : 0        |                                        |
       2048 -> 4095       : 0        |                                        |
       4096 -> 8191       : 0        |                                        |
       8192 -> 16383      : 0        |                                        |
      16384 -> 32767      : 66       |****************************************|
      32768 -> 65535      : 0        |                                        |
      65536 -> 131071     : 0        |                                        |
     131072 -> 262143     : 2        |*                                       |

Block size: 14 - 66
Block size: 17 - 2

      Algn size           : count     distribution
          0 -> 1          : 0        |                                        |
          2 -> 3          : 0        |                                        |
          4 -> 7          : 0        |                                        |
          8 -> 15         : 0        |                                        |
         16 -> 31         : 0        |                                        |
         32 -> 63         : 0        |                                        |
         64 -> 127        : 0        |                                        |
        128 -> 255        : 0        |                                        |
        256 -> 511        : 0        |                                        |
        512 -> 1023       : 0        |                                        |
       1024 -> 2047       : 0        |                                        |
       2048 -> 4095       : 0        |                                        |
       4096 -> 8191       : 0        |                                        |
       8192 -> 16383      : 0        |                                        |
      16384 -> 32767      : 66       |****************************************|
      32768 -> 65535      : 0        |                                        |
      65536 -> 131071     : 0        |                                        |
     131072 -> 262143     : 2        |*                                       |

Algn size: 14 - 66
Algn size: 17 - 2

Signed-off-by Luis Chamberlain <[email protected]>

---
  block/bdev.c | 5 ++---
  1 file changed, 2 insertions(+), 3 deletions(-)