| Message ID | 20190925005214.27240-4-willy@infradead.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Large pages in the page-cache | expand |
On Tue, Sep 24, 2019 at 05:52:02PM -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. 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 > >
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)" <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. > > 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.
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;
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. --- drivers/net/ethernet/ibm/ibmveth.c | 2 -- include/linux/pagemap.h | 25 ++++++++++++++++++++++--- 2 files changed, 22 insertions(+), 5 deletions(-)