Re: [PATCH 03/15] mm: Add file_offset_of_ helpers
On Thu, Sep 26, 2019 at 05:02:11PM +0300, Kirill A. Shutemov wrote: > On Tue, Sep 24, 2019 at 05:52:02PM -0700, Matthew Wilcox wrote: > > From: "Matthew Wilcox (Oracle)" > > > > 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. > > This should be trivial for coccinelle, right? Yes, should be. I'd prefer not to do conversions for now to minimise conflicts when rebasing. > > +static inline loff_t file_offset_of_next_page(struct page *page) > > +{ > > + return ((loff_t)page->index + compound_nr(page)) << PAGE_SHIFT; > > Wouldn't it be more readable as > > return file_offset_of_page(page) + page_size(page); > > ? Good idea. I'll fix that up.
Re: [PATCH 03/15] mm: Add file_offset_of_ helpers
Your mail program is still broken. This shows up as a reply to the 0/15 email instead of as a reply to the 3/15 email. On Wed, Oct 02, 2019 at 09:07:53PM +0800, Hillf Danton wrote: > On Tue, 24 Sep 2019 17:52:02 -0700 From: Matthew Wilcox (Oracle) > > +/** > > + * 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; > > } > > > > static inline loff_t page_file_offset(struct page *page) > > { > > return ((loff_t)page_index(page)) << PAGE_SHIFT; > > Would you like to specify the need to build a moon on the moon, > with another name though? I have no idea what you mean. Is this an idiom in your native language, perhaps?
Re: [PATCH 03/15] mm: Add file_offset_of_ helpers
On Tue, Sep 24, 2019 at 05:52:02PM -0700, Matthew Wilcox wrote: > From: "Matthew Wilcox (Oracle)" > > 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. This should be trivial for coccinelle, right? > --- > drivers/net/ethernet/ibm/ibmveth.c | 2 -- > include/linux/pagemap.h| 25 ++--- > 2 files changed, 22 insertions(+), 5 deletions(-) > > diff --git a/drivers/net/ethernet/ibm/ibmveth.c > b/drivers/net/ethernet/ibm/ibmveth.c > index c5be4ebd8437..bf98aeaf9a45 100644 > --- a/drivers/net/ethernet/ibm/ibmveth.c > +++ b/drivers/net/ethernet/ibm/ibmveth.c > @@ -978,8 +978,6 @@ static int ibmveth_ioctl(struct net_device *dev, struct > ifreq *ifr, int cmd) > return -EOPNOTSUPP; > } > > -#define page_offset(v) ((unsigned long)(v) & ((1 << 12) - 1)) > - > static int ibmveth_send(struct ibmveth_adapter *adapter, > union ibmveth_buf_desc *descs, unsigned long mss) > { > diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h > index 750770a2c685..103205494ea0 100644 > --- a/include/linux/pagemap.h > +++ b/include/linux/pagemap.h > @@ -428,14 +428,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; Wouldn't it be more readable as return file_offset_of_page(page) + page_size(page); ? > +} > + > static inline loff_t page_file_offset(struct page *page) > { > return ((loff_t)page_index(page)) << PAGE_SHIFT; > -- > 2.23.0 > > -- Kirill A. Shutemov
[PATCH 03/15] mm: Add file_offset_of_ helpers
From: "Matthew Wilcox (Oracle)" 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. --- drivers/net/ethernet/ibm/ibmveth.c | 2 -- include/linux/pagemap.h| 25 ++--- 2 files changed, 22 insertions(+), 5 deletions(-) diff --git a/drivers/net/ethernet/ibm/ibmveth.c b/drivers/net/ethernet/ibm/ibmveth.c index c5be4ebd8437..bf98aeaf9a45 100644 --- a/drivers/net/ethernet/ibm/ibmveth.c +++ b/drivers/net/ethernet/ibm/ibmveth.c @@ -978,8 +978,6 @@ static int ibmveth_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd) return -EOPNOTSUPP; } -#define page_offset(v) ((unsigned long)(v) & ((1 << 12) - 1)) - static int ibmveth_send(struct ibmveth_adapter *adapter, union ibmveth_buf_desc *descs, unsigned long mss) { diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h index 750770a2c685..103205494ea0 100644 --- a/include/linux/pagemap.h +++ b/include/linux/pagemap.h @@ -428,14 +428,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