| Message ID | 20231107194152.3374087-3-willy@infradead.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | More buffer_head cleanups | expand |
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 >
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!
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
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 *
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(-)