diff mbox series

[v2,10/25] fs: Introduce i_blocks_per_page

Message ID 20200212041845.25879-11-willy@infradead.org (mailing list archive)
State New, archived
Headers show
Series Large pages in the page cache | expand

Commit Message

Matthew Wilcox Feb. 12, 2020, 4:18 a.m. UTC 
From: "Matthew Wilcox (Oracle)" <willy@infradead.org>

This helper is useful for both large pages in the page cache and for
supporting block size larger than page size.  Convert some example
users (we have a few different ways of writing this idiom).

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 fs/iomap/buffered-io.c  |  8 ++++----
 fs/jfs/jfs_metapage.c   |  2 +-
 fs/xfs/xfs_aops.c       |  2 +-
 include/linux/pagemap.h | 16 ++++++++++++++++
 4 files changed, 22 insertions(+), 6 deletions(-)

Comments

Christoph Hellwig Feb. 12, 2020, 7:44 a.m. UTC | #1 
Looks good modulo some nitpicks below:

Reviewed-by: Christoph Hellwig <hch@lst.de>

> + * Context: Any context.

Does this add any value for a trivial helper like this?

> + * Return: The number of filesystem blocks covered by this page.
> + */
> +static inline
> +unsigned int i_blocks_per_page(struct inode *inode, struct page *page)

static inline unisnged int
i_blocks_per_page(struct inode *inode, struct page *page)
Matthew Wilcox Feb. 12, 2020, 3:05 p.m. UTC | #2 
On Tue, Feb 11, 2020 at 11:44:53PM -0800, Christoph Hellwig wrote:
> Looks good modulo some nitpicks below:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> 
> > + * Context: Any context.
> 
> Does this add any value for a trivial helper like this?

I think it's good to put them in to remind people they should be putting
them in for more complex functions.  Just like the Return: section.

> > + * Return: The number of filesystem blocks covered by this page.
> > + */
> > +static inline
> > +unsigned int i_blocks_per_page(struct inode *inode, struct page *page)
> 
> static inline unisnged int
> i_blocks_per_page(struct inode *inode, struct page *page)

That's XFS coding style.  Linus has specifically forbidden that:

https://lore.kernel.org/lkml/1054519757.161606@palladium.transmeta.com/
Christoph Hellwig Feb. 12, 2020, 5:54 p.m. UTC | #3 
On Wed, Feb 12, 2020 at 07:05:40AM -0800, Matthew Wilcox wrote:
> > > + * Return: The number of filesystem blocks covered by this page.
> > > + */
> > > +static inline
> > > +unsigned int i_blocks_per_page(struct inode *inode, struct page *page)
> > 
> > static inline unisnged int
> > i_blocks_per_page(struct inode *inode, struct page *page)
> 
> That's XFS coding style.  Linus has specifically forbidden that:
> 
> https://lore.kernel.org/lkml/1054519757.161606@palladium.transmeta.com/

Not just xfs but lots of places.  But if you don't like that follow
the real Linus style we use elsewhere:

static inline unsigned int i_blocks_per_page(struct inode *inode,
		struct page *page)
Kirill A . Shutemov Feb. 13, 2020, 3:40 p.m. UTC | #4 
On Tue, Feb 11, 2020 at 08:18:30PM -0800, Matthew Wilcox wrote:
> From: "Matthew Wilcox (Oracle)" <willy@infradead.org>
> 
> This helper is useful for both large pages in the page cache and for
> supporting block size larger than page size.  Convert some example
> users (we have a few different ways of writing this idiom).

Maybe we should list what was converted and what wasn't. Like it's
important to know that fs/buffer.c is not covered.
Matthew Wilcox Feb. 13, 2020, 4:07 p.m. UTC | #5 
On Thu, Feb 13, 2020 at 06:40:10PM +0300, Kirill A. Shutemov wrote:
> On Tue, Feb 11, 2020 at 08:18:30PM -0800, Matthew Wilcox wrote:
> > From: "Matthew Wilcox (Oracle)" <willy@infradead.org>
> > 
> > This helper is useful for both large pages in the page cache and for
> > supporting block size larger than page size.  Convert some example
> > users (we have a few different ways of writing this idiom).
> 
> Maybe we should list what was converted and what wasn't. Like it's
> important to know that fs/buffer.c is not covered.

I don't know what could have been converted and wasn't ... I just went
looking for a few places that use idioms like this.  Happy to add some
more examples.
diff mbox series

Patch

diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
index e40eb45230fa..c551a48e2a81 100644
--- a/fs/iomap/buffered-io.c
+++ b/fs/iomap/buffered-io.c
@@ -46,7 +46,7 @@  iomap_page_create(struct inode *inode, struct page *page)
 {
 	struct iomap_page *iop = to_iomap_page(page);
 
-	if (iop || i_blocksize(inode) == PAGE_SIZE)
+	if (iop || i_blocks_per_page(inode, page) <= 1)
 		return iop;
 
 	iop = kmalloc(sizeof(*iop), GFP_NOFS | __GFP_NOFAIL);
@@ -152,7 +152,7 @@  iomap_iop_set_range_uptodate(struct page *page, unsigned off, unsigned len)
 	unsigned int i;
 
 	spin_lock_irqsave(&iop->uptodate_lock, flags);
-	for (i = 0; i < PAGE_SIZE / i_blocksize(inode); i++) {
+	for (i = 0; i < i_blocks_per_page(inode, page); i++) {
 		if (i >= first && i <= last)
 			set_bit(i, iop->uptodate);
 		else if (!test_bit(i, iop->uptodate))
@@ -1073,7 +1073,7 @@  iomap_finish_page_writeback(struct inode *inode, struct page *page,
 		mapping_set_error(inode->i_mapping, -EIO);
 	}
 
-	WARN_ON_ONCE(i_blocksize(inode) < PAGE_SIZE && !iop);
+	WARN_ON_ONCE(i_blocks_per_page(inode, page) > 1 && !iop);
 	WARN_ON_ONCE(iop && atomic_read(&iop->write_count) <= 0);
 
 	if (!iop || atomic_dec_and_test(&iop->write_count))
@@ -1369,7 +1369,7 @@  iomap_writepage_map(struct iomap_writepage_ctx *wpc,
 	int error = 0, count = 0, i;
 	LIST_HEAD(submit_list);
 
-	WARN_ON_ONCE(i_blocksize(inode) < PAGE_SIZE && !iop);
+	WARN_ON_ONCE(i_blocks_per_page(inode, page) > 1 && !iop);
 	WARN_ON_ONCE(iop && atomic_read(&iop->write_count) != 0);
 
 	/*
diff --git a/fs/jfs/jfs_metapage.c b/fs/jfs/jfs_metapage.c
index a2f5338a5ea1..176580f54af9 100644
--- a/fs/jfs/jfs_metapage.c
+++ b/fs/jfs/jfs_metapage.c
@@ -473,7 +473,7 @@  static int metapage_readpage(struct file *fp, struct page *page)
 	struct inode *inode = page->mapping->host;
 	struct bio *bio = NULL;
 	int block_offset;
-	int blocks_per_page = PAGE_SIZE >> inode->i_blkbits;
+	int blocks_per_page = i_blocks_per_page(inode, page);
 	sector_t page_start;	/* address of page in fs blocks */
 	sector_t pblock;
 	int xlen;
diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c
index 0897dd71c622..5573bf2957dd 100644
--- a/fs/xfs/xfs_aops.c
+++ b/fs/xfs/xfs_aops.c
@@ -544,7 +544,7 @@  xfs_discard_page(
 			page, ip->i_ino, offset);
 
 	error = xfs_bmap_punch_delalloc_range(ip, start_fsb,
-			PAGE_SIZE / i_blocksize(inode));
+			i_blocks_per_page(inode, page));
 	if (error && !XFS_FORCED_SHUTDOWN(mp))
 		xfs_alert(mp, "page discard unable to remove delalloc mapping.");
 out_invalidate:
diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h
index 0842622cca90..aa925295347c 100644
--- a/include/linux/pagemap.h
+++ b/include/linux/pagemap.h
@@ -748,4 +748,20 @@  static inline int page_mkwrite_check_truncate(struct page *page,
 	return offset;
 }
 
+/**
+ * i_blocks_per_page - How many blocks fit in this page.
+ * @inode: The inode which contains the blocks.
+ * @page: The (potentially large) page.
+ *
+ * If the block size is larger than the size of this page, will return
+ * zero,
+ *
+ * Context: Any context.
+ * Return: The number of filesystem blocks covered by this page.
+ */
+static inline
+unsigned int i_blocks_per_page(struct inode *inode, struct page *page)
+{
+	return thp_size(page) >> inode->i_blkbits;
+}
 #endif /* _LINUX_PAGEMAP_H */