[PATCH 0/5] More buffer_head cleanups

[PATCH 0/5] More buffer_head cleanups

[Matthew Wilcox (Oracle)]

The first patch is a left-over from last cycle.  The rest fix "obvious"
block size > PAGE_SIZE problems.  I haven't tested with a large block
size setup.

Matthew Wilcox (Oracle) (5):
  buffer: Return bool from grow_dev_folio()
  buffer: Calculate block number inside folio_init_buffers()
  buffer: Fix grow_buffers() for block size > PAGE_SIZE
  buffer: Cast block to loff_t before shifting it
  buffer: Fix various functions for block size > PAGE_SIZE

 fs/buffer.c | 86 +++++++++++++++++++++++++----------------------------
 1 file changed, 40 insertions(+), 46 deletions(-)

-- 
2.42.0

[PATCH 1/5] buffer: Return bool from grow_dev_folio()

[Matthew Wilcox (Oracle)]

Rename grow_dev_page() to grow_dev_folio() and make it return a bool.
Document what that bool means; it's more subtle than it first appears.
Also rename the 'failed' label to 'unlock' beacuse it's not exactly
'failed'.  It just hasn't succeeded.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 fs/buffer.c | 50 +++++++++++++++++++++++++-------------------------
 1 file changed, 25 insertions(+), 25 deletions(-)

diff --git a/fs/buffer.c b/fs/buffer.c
index 967f34b70aa8..8dad6c691e14 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -1024,40 +1024,43 @@ static sector_t folio_init_buffers(struct folio *folio,
 }
 
 /*
- * Create the page-cache page that contains the requested block.
+ * Create the page-cache folio that contains the requested block.
  *
  * This is used purely for blockdev mappings.
+ *
+ * Returns false if we have a 'permanent' failure.  Returns true if
+ * we succeeded, or the caller should retry.
  */
-static int
-grow_dev_page(struct block_device *bdev, sector_t block,
-	      pgoff_t index, int size, int sizebits, gfp_t gfp)
+static bool grow_dev_folio(struct block_device *bdev, sector_t block,
+		pgoff_t index, unsigned size, int sizebits, gfp_t gfp)
 {
 	struct inode *inode = bdev->bd_inode;
 	struct folio *folio;
 	struct buffer_head *bh;
-	sector_t end_block;
-	int ret = 0;
+	sector_t end_block = 0;
 
 	folio = __filemap_get_folio(inode->i_mapping, index,
 			FGP_LOCK | FGP_ACCESSED | FGP_CREAT, gfp);
 	if (IS_ERR(folio))
-		return PTR_ERR(folio);
+		return false;
 
 	bh = folio_buffers(folio);
 	if (bh) {
 		if (bh->b_size == size) {
 			end_block = folio_init_buffers(folio, bdev,
 					(sector_t)index << sizebits, size);
-			goto done;
+			goto unlock;
 		}
+
+		/* Caller should retry if this call fails */
+		end_block = ~0ULL;
 		if (!try_to_free_buffers(folio))
-			goto failed;
+			goto unlock;
 	}
 
-	ret = -ENOMEM;
 	bh = folio_alloc_buffers(folio, size, gfp | __GFP_ACCOUNT);
 	if (!bh)
-		goto failed;
+		goto unlock;
 
 	/*
 	 * Link the folio to the buffers and initialise them.  Take the
@@ -1069,20 +1072,19 @@ grow_dev_page(struct block_device *bdev, sector_t block,
 	end_block = folio_init_buffers(folio, bdev,
 			(sector_t)index << sizebits, size);
 	spin_unlock(&inode->i_mapping->private_lock);
-done:
-	ret = (block < end_block) ? 1 : -ENXIO;
-failed:
+unlock:
 	folio_unlock(folio);
 	folio_put(folio);
-	return ret;
+	return block < end_block;
 }
 
 /*
- * Create buffers for the specified block device block's page.  If
- * that page was dirty, the buffers are set dirty also.
+ * Create buffers for the specified block device block's folio.  If
+ * that folio was dirty, the buffers are set dirty also.  Returns false
+ * if we've hit a permanent error.
  */
-static int
-grow_buffers(struct block_device *bdev, sector_t block, int size, gfp_t gfp)
+static bool grow_buffers(struct block_device *bdev, sector_t block,
+		unsigned size, gfp_t gfp)
 {
 	pgoff_t index;
 	int sizebits;
@@ -1099,11 +1101,11 @@ grow_buffers(struct block_device *bdev, sector_t block, int size, gfp_t gfp)
 			"device %pg\n",
 			__func__, (unsigned long long)block,
 			bdev);
-		return -EIO;
+		return false;
 	}
 
-	/* Create a page with the proper size buffers.. */
-	return grow_dev_page(bdev, block, index, size, sizebits, gfp);
+	/* Create a folio with the proper size buffers */
+	return grow_dev_folio(bdev, block, index, size, sizebits, gfp);
 }
 
 static struct buffer_head *
@@ -1124,14 +1126,12 @@ __getblk_slow(struct block_device *bdev, sector_t block,
 
 	for (;;) {
 		struct buffer_head *bh;
-		int ret;
 
 		bh = __find_get_block(bdev, block, size);
 		if (bh)
 			return bh;
 
-		ret = grow_buffers(bdev, block, size, gfp);
-		if (ret < 0)
+		if (!grow_buffers(bdev, block, size, gfp))
 			return NULL;
 	}
 }
-- 
2.42.0

[PATCH 2/5] buffer: Calculate block number inside folio_init_buffers()

[Matthew Wilcox (Oracle)]

The calculation of block from index doesn't work for devices with a block
size larger than PAGE_SIZE as we end up shifting by a negative number.
Instead, calculate the number of the first block from the folio's
position in the block device.  We no longer need to pass sizebits to
grow_dev_folio().

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 fs/buffer.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/fs/buffer.c b/fs/buffer.c
index 8dad6c691e14..cd114110b27f 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -995,11 +995,12 @@ static sector_t blkdev_max_block(struct block_device *bdev, unsigned int size)
  * Initialise the state of a blockdev folio's buffers.
  */ 
 static sector_t folio_init_buffers(struct folio *folio,
-		struct block_device *bdev, sector_t block, int size)
+		struct block_device *bdev, int size)
 {
 	struct buffer_head *head = folio_buffers(folio);
 	struct buffer_head *bh = head;
 	bool uptodate = folio_test_uptodate(folio);
+	sector_t block = folio_pos(folio) / size;
 	sector_t end_block = blkdev_max_block(bdev, size);
 
 	do {
@@ -1032,7 +1033,7 @@ static sector_t folio_init_buffers(struct folio *folio,
  * we succeeded, or the caller should retry.
  */
 static bool grow_dev_folio(struct block_device *bdev, sector_t block,
-		pgoff_t index, unsigned size, int sizebits, gfp_t gfp)
+		pgoff_t index, unsigned size, gfp_t gfp)
 {
 	struct inode *inode = bdev->bd_inode;
 	struct folio *folio;
@@ -1047,8 +1048,7 @@ static bool grow_dev_folio(struct block_device *bdev, sector_t block,
 	bh = folio_buffers(folio);
 	if (bh) {
 		if (bh->b_size == size) {
-			end_block = folio_init_buffers(folio, bdev,
-					(sector_t)index << sizebits, size);
+			end_block = folio_init_buffers(folio, bdev, size);
 			goto unlock;
 		}
 
@@ -1069,8 +1069,7 @@ static bool grow_dev_folio(struct block_device *bdev, sector_t block,
 	 */
 	spin_lock(&inode->i_mapping->private_lock);
 	link_dev_buffers(folio, bh);
-	end_block = folio_init_buffers(folio, bdev,
-			(sector_t)index << sizebits, size);
+	end_block = folio_init_buffers(folio, bdev, size);
 	spin_unlock(&inode->i_mapping->private_lock);
 unlock:
 	folio_unlock(folio);
@@ -1105,7 +1104,7 @@ static bool grow_buffers(struct block_device *bdev, sector_t block,
 	}
 
 	/* Create a folio with the proper size buffers */
-	return grow_dev_folio(bdev, block, index, size, sizebits, gfp);
+	return grow_dev_folio(bdev, block, index, size, gfp);
 }
 
 static struct buffer_head *
-- 
2.42.0

[PATCH 3/5] buffer: Fix grow_buffers() for block size > PAGE_SIZE

[Matthew Wilcox (Oracle)]

We must not shift by a negative number so work in terms of a byte
offset to avoid the awkward shift left-or-right-depending-on-sign
option.  This means we need to use check_mul_overflow() to ensure
that a large block number does not result in a wrap.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 fs/buffer.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/fs/buffer.c b/fs/buffer.c
index cd114110b27f..c83bb89b2e24 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -1085,26 +1085,21 @@ static bool grow_dev_folio(struct block_device *bdev, sector_t block,
 static bool grow_buffers(struct block_device *bdev, sector_t block,
 		unsigned size, gfp_t gfp)
 {
-	pgoff_t index;
-	int sizebits;
-
-	sizebits = PAGE_SHIFT - __ffs(size);
-	index = block >> sizebits;
+	loff_t pos;
 
 	/*
-	 * Check for a block which wants to lie outside our maximum possible
-	 * pagecache index.  (this comparison is done using sector_t types).
+	 * Check for a block which lies outside our maximum possible
+	 * pagecache index.
 	 */
-	if (unlikely(index != block >> sizebits)) {
-		printk(KERN_ERR "%s: requested out-of-range block %llu for "
-			"device %pg\n",
+	if (check_mul_overflow(block, size, &pos) || pos > MAX_LFS_FILESIZE) {
+		printk(KERN_ERR "%s: requested out-of-range block %llu for device %pg\n",
 			__func__, (unsigned long long)block,
 			bdev);
 		return false;
 	}
 
 	/* Create a folio with the proper size buffers */
-	return grow_dev_folio(bdev, block, index, size, gfp);
+	return grow_dev_folio(bdev, block, pos / PAGE_SIZE, size, gfp);
 }
 
 static struct buffer_head *
-- 
2.42.0

[PATCH 4/5] buffer: Cast block to loff_t before shifting it

[Matthew Wilcox (Oracle)]

While sector_t is always defined as a u64 today, that hasn't always been
the case and it might not always be the same size as loff_t in the future.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 fs/buffer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/buffer.c b/fs/buffer.c
index c83bb89b2e24..2f08af3c47a2 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -2008,7 +2008,7 @@ static int
 iomap_to_bh(struct inode *inode, sector_t block, struct buffer_head *bh,
 		const struct iomap *iomap)
 {
-	loff_t offset = block << inode->i_blkbits;
+	loff_t offset = (loff_t)block << inode->i_blkbits;
 
 	bh->b_bdev = iomap->bdev;
 
-- 
2.42.0

[PATCH 5/5] buffer: Fix various functions for block size > PAGE_SIZE

[Matthew Wilcox (Oracle)]

If i_blkbits is larger than PAGE_SHIFT, we shift by a negative number,
which is undefined.  It is safe to shift the block left as a block
device must be smaller than MAX_LFS_FILESIZE, which is guaranteed to
fit in loff_t.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 fs/buffer.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/fs/buffer.c b/fs/buffer.c
index 2f08af3c47a2..5bdfcf8c6fe6 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -199,7 +199,7 @@ __find_get_block_slow(struct block_device *bdev, sector_t block)
 	int all_mapped = 1;
 	static DEFINE_RATELIMIT_STATE(last_warned, HZ, 1);
 
-	index = block >> (PAGE_SHIFT - bd_inode->i_blkbits);
+	index = ((loff_t)block << bd_inode->i_blkbits) / PAGE_SIZE;
 	folio = __filemap_get_folio(bd_mapping, index, FGP_ACCESSED, 0);
 	if (IS_ERR(folio))
 		goto out;
@@ -1693,13 +1693,13 @@ void clean_bdev_aliases(struct block_device *bdev, sector_t block, sector_t len)
 	struct inode *bd_inode = bdev->bd_inode;
 	struct address_space *bd_mapping = bd_inode->i_mapping;
 	struct folio_batch fbatch;
-	pgoff_t index = block >> (PAGE_SHIFT - bd_inode->i_blkbits);
+	pgoff_t index = ((loff_t)block << bd_inode->i_blkbits) / PAGE_SIZE;
 	pgoff_t end;
 	int i, count;
 	struct buffer_head *bh;
 	struct buffer_head *head;
 
-	end = (block + len - 1) >> (PAGE_SHIFT - bd_inode->i_blkbits);
+	end = ((loff_t)(block + len - 1) << bd_inode->i_blkbits) / PAGE_SIZE;
 	folio_batch_init(&fbatch);
 	while (filemap_get_folios(bd_mapping, &index, end, &fbatch)) {
 		count = folio_batch_count(&fbatch);
@@ -2660,8 +2660,8 @@ int block_truncate_page(struct address_space *mapping,
 		return 0;
 
 	length = blocksize - length;
-	iblock = (sector_t)index << (PAGE_SHIFT - inode->i_blkbits);
-	
+	iblock = ((loff_t)index * PAGE_SIZE) >> inode->i_blkbits;
+
 	folio = filemap_grab_folio(mapping, index);
 	if (IS_ERR(folio))
 		return PTR_ERR(folio);
-- 
2.42.0

Re: [PATCH 2/5] buffer: Calculate block number inside folio_init_buffers()

[Pankaj Raghav]

On Tue, Nov 07, 2023 at 07:41:49PM +0000, Matthew Wilcox (Oracle) wrote:
> The calculation of block from index doesn't work for devices with a block
> size larger than PAGE_SIZE as we end up shifting by a negative number.
> Instead, calculate the number of the first block from the folio's
> position in the block device.  We no longer need to pass sizebits to
> grow_dev_folio().
> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>

Not totally related to the patch but even though the variable "block"
is sector_t type, but it represents the block number in logical block
size unit of the device? My mind directly went to sector_t being 512
bytes blocks.

But the math checks out.
Reviewed-by: Pankaj Raghav <p.raghav@samsung.com>
> ---
>  fs/buffer.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/buffer.c b/fs/buffer.c
> index 8dad6c691e14..cd114110b27f 100644
> --- a/fs/buffer.c
> +++ b/fs/buffer.c
> @@ -995,11 +995,12 @@ static sector_t blkdev_max_block(struct block_device *bdev, unsigned int size)
>   * Initialise the state of a blockdev folio's buffers.
>   */ 
>  static sector_t folio_init_buffers(struct folio *folio,
> -		struct block_device *bdev, sector_t block, int size)
> +		struct block_device *bdev, int size)
>  {
>  	struct buffer_head *head = folio_buffers(folio);
>  	struct buffer_head *bh = head;
>  	bool uptodate = folio_test_uptodate(folio);
> +	sector_t block = folio_pos(folio) / size;
>  	sector_t end_block = blkdev_max_block(bdev, size);
>  
>  	do {
> @@ -1032,7 +1033,7 @@ static sector_t folio_init_buffers(struct folio *folio,
>   * we succeeded, or the caller should retry.
>   */
>  static bool grow_dev_folio(struct block_device *bdev, sector_t block,
> -		pgoff_t index, unsigned size, int sizebits, gfp_t gfp)
> +		pgoff_t index, unsigned size, gfp_t gfp)
>  {
>  	struct inode *inode = bdev->bd_inode;
>  	struct folio *folio;
> @@ -1047,8 +1048,7 @@ static bool grow_dev_folio(struct block_device *bdev, sector_t block,
>  	bh = folio_buffers(folio);
>  	if (bh) {
>  		if (bh->b_size == size) {
> -			end_block = folio_init_buffers(folio, bdev,
> -					(sector_t)index << sizebits, size);
> +			end_block = folio_init_buffers(folio, bdev, size);
>  			goto unlock;
>  		}
>  
> @@ -1069,8 +1069,7 @@ static bool grow_dev_folio(struct block_device *bdev, sector_t block,
>  	 */
>  	spin_lock(&inode->i_mapping->private_lock);
>  	link_dev_buffers(folio, bh);
> -	end_block = folio_init_buffers(folio, bdev,
> -			(sector_t)index << sizebits, size);
> +	end_block = folio_init_buffers(folio, bdev, size);
>  	spin_unlock(&inode->i_mapping->private_lock);
>  unlock:
>  	folio_unlock(folio);
> @@ -1105,7 +1104,7 @@ static bool grow_buffers(struct block_device *bdev, sector_t block,
>  	}
>  
>  	/* Create a folio with the proper size buffers */
> -	return grow_dev_folio(bdev, block, index, size, sizebits, gfp);
> +	return grow_dev_folio(bdev, block, index, size, gfp);
>  }
>  
>  static struct buffer_head *
> -- 
> 2.42.0
> 

-- 
Pankaj Raghav

Re: [PATCH 5/5] buffer: Fix various functions for block size > PAGE_SIZE

[Pankaj Raghav]

On Tue, Nov 07, 2023 at 07:41:52PM +0000, Matthew Wilcox (Oracle) wrote:
> If i_blkbits is larger than PAGE_SHIFT, we shift by a negative number,
> which is undefined.  It is safe to shift the block left as a block
> device must be smaller than MAX_LFS_FILESIZE, which is guaranteed to
> fit in loff_t.
> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>

Looks good,
Reviewed-by: Pankaj Raghav <p.raghav@samsung.com>

> ---
>  fs/buffer.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/buffer.c b/fs/buffer.c
> index 2f08af3c47a2..5bdfcf8c6fe6 100644
> --- a/fs/buffer.c
> +++ b/fs/buffer.c
> @@ -199,7 +199,7 @@ __find_get_block_slow(struct block_device *bdev, sector_t block)
>  	int all_mapped = 1;
>  	static DEFINE_RATELIMIT_STATE(last_warned, HZ, 1);
>  
> -	index = block >> (PAGE_SHIFT - bd_inode->i_blkbits);
> +	index = ((loff_t)block << bd_inode->i_blkbits) / PAGE_SIZE;

Re: [PATCH 2/5] buffer: Calculate block number inside folio_init_buffers()

[Matthew Wilcox]

On Wed, Nov 08, 2023 at 03:59:51PM +0100, Pankaj Raghav wrote:
> On Tue, Nov 07, 2023 at 07:41:49PM +0000, Matthew Wilcox (Oracle) wrote:
> > The calculation of block from index doesn't work for devices with a block
> > size larger than PAGE_SIZE as we end up shifting by a negative number.
> > Instead, calculate the number of the first block from the folio's
> > position in the block device.  We no longer need to pass sizebits to
> > grow_dev_folio().
> > 
> > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> 
> Not totally related to the patch but even though the variable "block"
> is sector_t type, but it represents the block number in logical block
> size unit of the device? My mind directly went to sector_t being 512
> bytes blocks.

Yes; it's confusing.  buffer_heads are always created for the logical
block size that the filesystem mounted on the device needs.  It's
never for the fixed-size 512 byte sectors (but might happen to be
512 bytes if that's what the fs has set the block device to).

> But the math checks out.
> Reviewed-by: Pankaj Raghav <p.raghav@samsung.com>

Thanks!

Re: [PATCH 2/5] buffer: Calculate block number inside folio_init_buffers()

[kernel test robot]

Hi Matthew,

kernel test robot noticed the following build errors:

[auto build test ERROR on akpm-mm/mm-everything]
[also build test ERROR on linus/master next-20231108]
[cannot apply to v6.6]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Matthew-Wilcox-Oracle/buffer-Return-bool-from-grow_dev_folio/20231108-035905
base:   https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git mm-everything
patch link:    https://lore.kernel.org/r/20231107194152.3374087-3-willy%40infradead.org
patch subject: [PATCH 2/5] buffer: Calculate block number inside folio_init_buffers()
config: i386-randconfig-141-20231108 (https://download.01.org/0day-ci/archive/20231109/202311090123.FRvXagQt-lkp@intel.com/config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231109/202311090123.FRvXagQt-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202311090123.FRvXagQt-lkp@intel.com/

All errors (new ones prefixed by >>):

   ld: fs/buffer.o: in function `folio_init_buffers':
>> fs/buffer.c:1003: undefined reference to `__divdi3'


vim +1003 fs/buffer.c

   993	
   994	/*
   995	 * Initialise the state of a blockdev folio's buffers.
   996	 */ 
   997	static sector_t folio_init_buffers(struct folio *folio,
   998			struct block_device *bdev, int size)
   999	{
  1000		struct buffer_head *head = folio_buffers(folio);
  1001		struct buffer_head *bh = head;
  1002		bool uptodate = folio_test_uptodate(folio);
> 1003		sector_t block = folio_pos(folio) / size;
  1004		sector_t end_block = blkdev_max_block(bdev, size);
  1005	
  1006		do {
  1007			if (!buffer_mapped(bh)) {
  1008				bh->b_end_io = NULL;
  1009				bh->b_private = NULL;
  1010				bh->b_bdev = bdev;
  1011				bh->b_blocknr = block;
  1012				if (uptodate)
  1013					set_buffer_uptodate(bh);
  1014				if (block < end_block)
  1015					set_buffer_mapped(bh);
  1016			}
  1017			block++;
  1018			bh = bh->b_this_page;
  1019		} while (bh != head);
  1020	
  1021		/*
  1022		 * Caller needs to validate requested block against end of device.
  1023		 */
  1024		return end_block;
  1025	}
  1026	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki