[32/69] buffer: Rewrite nobh_truncate_page() to use folios

Commit Message

Matthew Wilcox April 29, 2022, 5:25 p.m. UTC

- Calculate iblock directly instead of using a while loop
 - Move has_buffers to the end to remove a backwards jump
 - Use __filemap_get_folio() instead of grab_cache_page(), which
   removes a spurious FGP_ACCESSED flag.
 - Eliminate length and pos variables
 - Use folio APIs where they exist

Signed-off-by: Matthew Wilcox (Oracle) <[e-mail]>
---
 fs/buffer.c | 64 ++++++++++++++++++++++-------------------------------
 1 file changed, 27 insertions(+), 37 deletions(-)

Comments

Christoph Hellwig May 3, 2022, 2:41 p.m. UTC | #1

The changes looks good:

Reviewed-by: Christoph Hellwig <[e-mail]>

But I wonder if we shouldn't really just drop these nobh helpers
entirely.  They were added to save lowmem on highmem systems with a lot
of highmem, but I really don't think we should continue optimizaing for
that.

Matthew Wilcox May 8, 2022, 6:37 p.m. UTC | #2

On Tue, May 03, 2022 at 07:41:24AM -0700, Christoph Hellwig wrote:
> The changes looks good:
> 
> Reviewed-by: Christoph Hellwig <[e-mail]>
> 
> But I wonder if we shouldn't really just drop these nobh helpers
> entirely.  They were added to save lowmem on highmem systems with a lot
> of highmem, but I really don't think we should continue optimizaing for
> that.

They certainly don't have a lot of users:

$ git grep -w nobh_truncate_page
fs/buffer.c:int nobh_truncate_page(struct address_space *mapping,
fs/buffer.c:EXPORT_SYMBOL(nobh_truncate_page);
fs/ext2/inode.c:                error = nobh_truncate_page(inode->i_mapping,
fs/jfs/inode.c: nobh_truncate_page(ip->i_mapping, ip->i_size, jfs_get_block);
include/linux/buffer_head.h:int nobh_truncate_page(struct address_space *, loff_t, get_block_t *);

So just two for this helper.  The other nobh_ helpers are in a similar
position (just ext2 and jfs).  I had a quick look at converting jfs to
iomap, and that seems quite reasonable.  So ... once ext2 is converted
to iomap, all the nobh helpers can go away as being unused?

Patch

diff --git a/fs/buffer.c b/fs/buffer.c
index fb4df259c92d..9737e0dbe3ec 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -2791,44 +2791,28 @@  int nobh_truncate_page(struct address_space *mapping,
 			loff_t from, get_block_t *get_block)
 {
 	pgoff_t index = from >> PAGE_SHIFT;
-	unsigned offset = from & (PAGE_SIZE-1);
-	unsigned blocksize;
-	sector_t iblock;
-	unsigned length, pos;
 	struct inode *inode = mapping->host;
-	struct page *page;
+	unsigned blocksize = i_blocksize(inode);
+	struct folio *folio;
 	struct buffer_head map_bh;
+	size_t offset;
+	sector_t iblock;
 	int err;
 
-	blocksize = i_blocksize(inode);
-	length = offset & (blocksize - 1);
-
 	/* Block boundary? Nothing to do */
-	if (!length)
+	if (!(from & (blocksize - 1)))
 		return 0;
 
-	length = blocksize - length;
-	iblock = (sector_t)index << (PAGE_SHIFT - inode->i_blkbits);
-
-	page = grab_cache_page(mapping, index);
+	folio = __filemap_get_folio(mapping, index, FGP_LOCK | FGP_CREAT,
+			mapping_gfp_mask(mapping));
 	err = -ENOMEM;
-	if (!page)
+	if (!folio)
 		goto out;
 
-	if (page_has_buffers(page)) {
-has_buffers:
-		unlock_page(page);
-		put_page(page);
-		return block_truncate_page(mapping, from, get_block);
-	}
-
-	/* Find the buffer that contains "offset" */
-	pos = blocksize;
-	while (offset >= pos) {
-		iblock++;
-		pos += blocksize;
-	}
+	if (folio_buffers(folio))
+		goto has_buffers;
 
+	iblock = from >> inode->i_blkbits;
 	map_bh.b_size = blocksize;
 	map_bh.b_state = 0;
 	err = get_block(inode, iblock, &map_bh, 0);
@@ -2839,29 +2823,35 @@  int nobh_truncate_page(struct address_space *mapping,
 		goto unlock;
 
 	/* Ok, it's mapped. Make sure it's up-to-date */
-	if (!PageUptodate(page)) {
-		err = mapping->a_ops->readpage(NULL, page);
+	if (!folio_test_uptodate(folio)) {
+		err = mapping->a_ops->readpage(NULL, &folio->page);
 		if (err) {
-			put_page(page);
+			folio_put(folio);
 			goto out;
 		}
-		lock_page(page);
-		if (!PageUptodate(page)) {
+		folio_lock(folio);
+		if (!folio_test_uptodate(folio)) {
 			err = -EIO;
 			goto unlock;
 		}
-		if (page_has_buffers(page))
+		if (folio_buffers(folio))
 			goto has_buffers;
 	}
-	zero_user(page, offset, length);
-	set_page_dirty(page);
+	offset = offset_in_folio(folio, from);
+	folio_zero_segment(folio, offset, round_up(offset, blocksize));
+	folio_mark_dirty(folio);
 	err = 0;
 
 unlock:
-	unlock_page(page);
-	put_page(page);
+	folio_unlock(folio);
+	folio_put(folio);
 out:
 	return err;
+
+has_buffers:
+	folio_unlock(folio);
+	folio_put(folio);
+	return block_truncate_page(mapping, from, get_block);
 }
 EXPORT_SYMBOL(nobh_truncate_page);