| Message ID | 20190821003039.12555-3-willy@infradead.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iomap & xfs support for large pages | expand |
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.
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 >
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.
[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
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;
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(-)