[v2,2/5] mm: Add file_offset_of_ helpers
diff mbox series

Message ID 20190821003039.12555-3-willy@infradead.org
State New
Headers show
Series
  • iomap & xfs support for large pages
Related show

Commit Message

Matthew Wilcox Aug. 21, 2019, 12:30 a.m. UTC
From: "Matthew Wilcox (Oracle)" <willy@infradead.org>

The page_offset function is badly named for people reading the functions
which call it.  The natural meaning of a function with this name would
be 'offset within a page', not 'page offset in bytes within a file'.
Dave Chinner suggests file_offset_of_page() as a replacement function
name and I'm also adding file_offset_of_next_page() as a helper for the
large page work.  Also add kernel-doc for these functions so they show
up in the kernel API book.

page_offset() is retained as a compatibility define for now.
---
 include/linux/pagemap.h | 25 ++++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)

Comments

Matthew Wilcox Aug. 24, 2019, 3:28 p.m. UTC | #1
On Sat, Aug 24, 2019 at 07:48:24PM +0800, kbuild test robot wrote:
> Hi Matthew,
> 
> Thank you for the patch! Perhaps something to improve:
> 
> [auto build test WARNING on linus/master]
> [cannot apply to v5.3-rc5 next-20190823]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

It depends on various patches which are in -next, although I didn't
generate them against -next.
Darrick J. Wong Sept. 18, 2019, 9:17 p.m. UTC | #2
On Tue, Aug 20, 2019 at 05:30:36PM -0700, Matthew Wilcox wrote:
> From: "Matthew Wilcox (Oracle)" <willy@infradead.org>
> 
> The page_offset function is badly named for people reading the functions
> which call it.  The natural meaning of a function with this name would
> be 'offset within a page', not 'page offset in bytes within a file'.
> Dave Chinner suggests file_offset_of_page() as a replacement function
> name and I'm also adding file_offset_of_next_page() as a helper for the
> large page work.  Also add kernel-doc for these functions so they show
> up in the kernel API book.
> 
> page_offset() is retained as a compatibility define for now.

No SOB?

Looks fine to me, and I appreciate the much less confusing name.  I was
hoping for a page_offset conversion for fs/iomap/ (and not a treewide
change because yuck), but I guess that can be done if and when this
lands.

--D

> ---
>  include/linux/pagemap.h | 25 ++++++++++++++++++++++---
>  1 file changed, 22 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h
> index 2728f20fbc49..84f341109710 100644
> --- a/include/linux/pagemap.h
> +++ b/include/linux/pagemap.h
> @@ -436,14 +436,33 @@ static inline pgoff_t page_to_pgoff(struct page *page)
>  	return page_to_index(page);
>  }
>  
> -/*
> - * Return byte-offset into filesystem object for page.
> +/**
> + * file_offset_of_page - File offset of this page.
> + * @page: Page cache page.
> + *
> + * Context: Any context.
> + * Return: The offset of the first byte of this page.
>   */
> -static inline loff_t page_offset(struct page *page)
> +static inline loff_t file_offset_of_page(struct page *page)
>  {
>  	return ((loff_t)page->index) << PAGE_SHIFT;
>  }
>  
> +/* Legacy; please convert callers */
> +#define page_offset(page)	file_offset_of_page(page)
> +
> +/**
> + * file_offset_of_next_page - File offset of the next page.
> + * @page: Page cache page.
> + *
> + * Context: Any context.
> + * Return: The offset of the first byte after this page.
> + */
> +static inline loff_t file_offset_of_next_page(struct page *page)
> +{
> +	return ((loff_t)page->index + compound_nr(page)) << PAGE_SHIFT;
> +}
> +
>  static inline loff_t page_file_offset(struct page *page)
>  {
>  	return ((loff_t)page_index(page)) << PAGE_SHIFT;
> -- 
> 2.23.0.rc1
>
Matthew Wilcox Sept. 18, 2019, 11:49 p.m. UTC | #3
On Wed, Sep 18, 2019 at 02:17:55PM -0700, Darrick J. Wong wrote:
> On Tue, Aug 20, 2019 at 05:30:36PM -0700, Matthew Wilcox wrote:
> > From: "Matthew Wilcox (Oracle)" <willy@infradead.org>
> > 
> > The page_offset function is badly named for people reading the functions
> > which call it.  The natural meaning of a function with this name would
> > be 'offset within a page', not 'page offset in bytes within a file'.
> > Dave Chinner suggests file_offset_of_page() as a replacement function
> > name and I'm also adding file_offset_of_next_page() as a helper for the
> > large page work.  Also add kernel-doc for these functions so they show
> > up in the kernel API book.
> > 
> > page_offset() is retained as a compatibility define for now.
> 
> No SOB?
> 
> Looks fine to me, and I appreciate the much less confusing name.  I was
> hoping for a page_offset conversion for fs/iomap/ (and not a treewide
> change because yuck), but I guess that can be done if and when this
> lands.

Sure, I'll do that once everything else has landed.
Darrick J. Wong Sept. 19, 2019, 12:04 a.m. UTC | #4
[add dave to cc]

On Wed, Sep 18, 2019 at 04:49:24PM -0700, Matthew Wilcox wrote:
> On Wed, Sep 18, 2019 at 02:17:55PM -0700, Darrick J. Wong wrote:
> > On Tue, Aug 20, 2019 at 05:30:36PM -0700, Matthew Wilcox wrote:
> > > From: "Matthew Wilcox (Oracle)" <willy@infradead.org>
> > > 
> > > The page_offset function is badly named for people reading the functions
> > > which call it.  The natural meaning of a function with this name would
> > > be 'offset within a page', not 'page offset in bytes within a file'.
> > > Dave Chinner suggests file_offset_of_page() as a replacement function
> > > name and I'm also adding file_offset_of_next_page() as a helper for the
> > > large page work.  Also add kernel-doc for these functions so they show
> > > up in the kernel API book.
> > > 
> > > page_offset() is retained as a compatibility define for now.
> > 
> > No SOB?
> > 
> > Looks fine to me, and I appreciate the much less confusing name.  I was
> > hoping for a page_offset conversion for fs/iomap/ (and not a treewide
> > change because yuck), but I guess that can be done if and when this
> > lands.
> 
> Sure, I'll do that once everything else has landed.

You might also want to ask Dave Chinner what changes he's making to
iomap to support blocksize > pagesize filesystems, since that's
/definitely/ going to clash. :)

--D

Patch
diff mbox series

diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h
index 2728f20fbc49..84f341109710 100644
--- a/include/linux/pagemap.h
+++ b/include/linux/pagemap.h
@@ -436,14 +436,33 @@  static inline pgoff_t page_to_pgoff(struct page *page)
 	return page_to_index(page);
 }
 
-/*
- * Return byte-offset into filesystem object for page.
+/**
+ * file_offset_of_page - File offset of this page.
+ * @page: Page cache page.
+ *
+ * Context: Any context.
+ * Return: The offset of the first byte of this page.
  */
-static inline loff_t page_offset(struct page *page)
+static inline loff_t file_offset_of_page(struct page *page)
 {
 	return ((loff_t)page->index) << PAGE_SHIFT;
 }
 
+/* Legacy; please convert callers */
+#define page_offset(page)	file_offset_of_page(page)
+
+/**
+ * file_offset_of_next_page - File offset of the next page.
+ * @page: Page cache page.
+ *
+ * Context: Any context.
+ * Return: The offset of the first byte after this page.
+ */
+static inline loff_t file_offset_of_next_page(struct page *page)
+{
+	return ((loff_t)page->index + compound_nr(page)) << PAGE_SHIFT;
+}
+
 static inline loff_t page_file_offset(struct page *page)
 {
 	return ((loff_t)page_index(page)) << PAGE_SHIFT;