[PATCH 0/3] Unify filler_t and read_folio

[PATCH 0/3] Unify filler_t and read_folio

[Matthew Wilcox (Oracle)]

I realised there was no good reason for any of the three filesystems which
actually use read_cache_page() to pass in something that wasn't a struct
file pointer.  Indeed, it made each of them more complex.  These aren't
filesystems I test regularly, so please scrutinise carefully.  This is
on top of the read_folio() patchset that I posted recently and can be
found at git://git.infradead.org/users/willy/pagecache.git for-next

Matthew Wilcox (Oracle) (3):
  jffs2: Pass the file pointer to jffs2_do_readpage_unlock()
  nfs: Pass the file pointer to nfs_symlink_filler()
  fs: Change the type of filler_t

 fs/gfs2/aops.c          | 29 +++++++++++------------------
 fs/jffs2/file.c         |  9 ++++-----
 fs/jffs2/gc.c           |  2 +-
 fs/jffs2/os-linux.h     |  2 +-
 fs/nfs/symlink.c        | 16 ++++++++--------
 include/linux/pagemap.h |  6 +++---
 mm/filemap.c            | 40 ++++++++++++++++++++--------------------
 7 files changed, 48 insertions(+), 56 deletions(-)

-- 
2.34.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 1/3] jffs2: Pass the file pointer to jffs2_do_readpage_unlock()

[Matthew Wilcox (Oracle)]

In preparation for unifying the read_cache_page() and read_folio()
implementations, make jffs2_do_readpage_unlock() get the inode
from the page instead of passing it in from read_cache_page().

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 fs/jffs2/file.c | 4 ++--
 fs/jffs2/gc.c   | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/jffs2/file.c b/fs/jffs2/file.c
index f8616683fbee..492fb2da0403 100644
--- a/fs/jffs2/file.c
+++ b/fs/jffs2/file.c
@@ -112,7 +112,7 @@ static int jffs2_do_readpage_nolock (struct inode *inode, struct page *pg)
 
 int jffs2_do_readpage_unlock(void *data, struct page *pg)
 {
-	int ret = jffs2_do_readpage_nolock(data, pg);
+	int ret = jffs2_do_readpage_nolock(pg->mapping->host, pg);
 	unlock_page(pg);
 	return ret;
 }
@@ -124,7 +124,7 @@ static int jffs2_read_folio(struct file *file, struct folio *folio)
 	int ret;
 
 	mutex_lock(&f->sem);
-	ret = jffs2_do_readpage_unlock(folio->mapping->host, &folio->page);
+	ret = jffs2_do_readpage_unlock(file, &folio->page);
 	mutex_unlock(&f->sem);
 	return ret;
 }
diff --git a/fs/jffs2/gc.c b/fs/jffs2/gc.c
index 373b3b7c9f44..a53bac7569b6 100644
--- a/fs/jffs2/gc.c
+++ b/fs/jffs2/gc.c
@@ -1327,7 +1327,7 @@ static int jffs2_garbage_collect_dnode(struct jffs2_sb_info *c, struct jffs2_era
 	 * trying to write out, read_cache_page() will not deadlock. */
 	mutex_unlock(&f->sem);
 	page = read_cache_page(inode->i_mapping, start >> PAGE_SHIFT,
-			       jffs2_do_readpage_unlock, inode);
+			       jffs2_do_readpage_unlock, NULL);
 	if (IS_ERR(page)) {
 		pr_warn("read_cache_page() returned error: %ld\n",
 			PTR_ERR(page));
-- 
2.34.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 1/3] jffs2: Pass the file pointer to jffs2_do_readpage_unlock()

[Christoph Hellwig]

Looks good:

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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 2/3] nfs: Pass the file pointer to nfs_symlink_filler()

[Matthew Wilcox (Oracle)]

In preparation for unifying the read_cache_page() and read_folio()
implementations, make nfs_symlink_filler() get the inode
from the page instead of passing it in from read_cache_page().

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 fs/nfs/symlink.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/nfs/symlink.c b/fs/nfs/symlink.c
index 25ba299fdac2..8b53538bcc75 100644
--- a/fs/nfs/symlink.c
+++ b/fs/nfs/symlink.c
@@ -28,7 +28,7 @@
 
 static int nfs_symlink_filler(void *data, struct page *page)
 {
-	struct inode *inode = data;
+	struct inode *inode = page->mapping->host;
 	int error;
 
 	error = NFS_PROTO(inode)->readlink(inode, page, 0, PAGE_SIZE);
@@ -67,7 +67,7 @@ static const char *nfs_get_link(struct dentry *dentry,
 		if (err)
 			return err;
 		page = read_cache_page(&inode->i_data, 0, nfs_symlink_filler,
-				inode);
+				NULL);
 		if (IS_ERR(page))
 			return ERR_CAST(page);
 	}
-- 
2.34.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 2/3] nfs: Pass the file pointer to nfs_symlink_filler()

[Christoph Hellwig]

On Mon, May 02, 2022 at 06:41:58AM +0100, Matthew Wilcox (Oracle) wrote:
> In preparation for unifying the read_cache_page() and read_folio()
> implementations, make nfs_symlink_filler() get the inode
> from the page instead of passing it in from read_cache_page().
> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>

Looks good:

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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 3/3] fs: Change the type of filler_t

[Matthew Wilcox (Oracle)]

By making filler_t the same as read_folio, we can use the same function
for both in gfs2.  We can push the use of folios down one more level
in jffs2 and nfs.  We also increase type safety for future users of the
various read_cache_page() family of functions by forcing the parameter
to be a pointer to struct file (or NULL).

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 fs/gfs2/aops.c          | 29 +++++++++++------------------
 fs/jffs2/file.c         |  9 ++++-----
 fs/jffs2/gc.c           |  2 +-
 fs/jffs2/os-linux.h     |  2 +-
 fs/nfs/symlink.c        | 14 +++++++-------
 include/linux/pagemap.h |  6 +++---
 mm/filemap.c            | 40 ++++++++++++++++++++--------------------
 7 files changed, 47 insertions(+), 55 deletions(-)

diff --git a/fs/gfs2/aops.c b/fs/gfs2/aops.c
index 340bf5d0e835..1016631bcbdc 100644
--- a/fs/gfs2/aops.c
+++ b/fs/gfs2/aops.c
@@ -464,21 +464,24 @@ static int stuffed_readpage(struct gfs2_inode *ip, struct page *page)
 	return 0;
 }
 
-
-static int __gfs2_readpage(void *file, struct page *page)
+/**
+ * gfs2_read_folio - read a folio from a file
+ * @file: The file to read
+ * @folio: The folio in the file
+ */
+static int gfs2_read_folio(struct file *file, struct folio *folio)
 {
-	struct folio *folio = page_folio(page);
-	struct inode *inode = page->mapping->host;
+	struct inode *inode = folio->mapping->host;
 	struct gfs2_inode *ip = GFS2_I(inode);
 	struct gfs2_sbd *sdp = GFS2_SB(inode);
 	int error;
 
 	if (!gfs2_is_jdata(ip) ||
-	    (i_blocksize(inode) == PAGE_SIZE && !page_has_buffers(page))) {
+	    (i_blocksize(inode) == PAGE_SIZE && !folio_buffers(folio))) {
 		error = iomap_read_folio(folio, &gfs2_iomap_ops);
 	} else if (gfs2_is_stuffed(ip)) {
-		error = stuffed_readpage(ip, page);
-		unlock_page(page);
+		error = stuffed_readpage(ip, &folio->page);
+		folio_unlock(folio);
 	} else {
 		error = mpage_read_folio(folio, gfs2_block_map);
 	}
@@ -489,16 +492,6 @@ static int __gfs2_readpage(void *file, struct page *page)
 	return error;
 }
 
-/**
- * gfs2_read_folio - read a folio from a file
- * @file: The file to read
- * @folio: The folio in the file
- */
-static int gfs2_read_folio(struct file *file, struct folio *folio)
-{
-	return __gfs2_readpage(file, &folio->page);
-}
-
 /**
  * gfs2_internal_read - read an internal file
  * @ip: The gfs2 inode
@@ -523,7 +516,7 @@ int gfs2_internal_read(struct gfs2_inode *ip, char *buf, loff_t *pos,
 		amt = size - copied;
 		if (offset + size > PAGE_SIZE)
 			amt = PAGE_SIZE - offset;
-		page = read_cache_page(mapping, index, __gfs2_readpage, NULL);
+		page = read_cache_page(mapping, index, gfs2_read_folio, NULL);
 		if (IS_ERR(page))
 			return PTR_ERR(page);
 		p = kmap_atomic(page);
diff --git a/fs/jffs2/file.c b/fs/jffs2/file.c
index 492fb2da0403..ba86acbe12d3 100644
--- a/fs/jffs2/file.c
+++ b/fs/jffs2/file.c
@@ -110,21 +110,20 @@ static int jffs2_do_readpage_nolock (struct inode *inode, struct page *pg)
 	return ret;
 }
 
-int jffs2_do_readpage_unlock(void *data, struct page *pg)
+int __jffs2_read_folio(struct file *file, struct folio *folio)
 {
-	int ret = jffs2_do_readpage_nolock(pg->mapping->host, pg);
-	unlock_page(pg);
+	int ret = jffs2_do_readpage_nolock(folio->mapping->host, &folio->page);
+	folio_unlock(folio);
 	return ret;
 }
 
-
 static int jffs2_read_folio(struct file *file, struct folio *folio)
 {
 	struct jffs2_inode_info *f = JFFS2_INODE_INFO(folio->mapping->host);
 	int ret;
 
 	mutex_lock(&f->sem);
-	ret = jffs2_do_readpage_unlock(file, &folio->page);
+	ret = __jffs2_read_folio(file, folio);
 	mutex_unlock(&f->sem);
 	return ret;
 }
diff --git a/fs/jffs2/gc.c b/fs/jffs2/gc.c
index a53bac7569b6..5c6602f3c189 100644
--- a/fs/jffs2/gc.c
+++ b/fs/jffs2/gc.c
@@ -1327,7 +1327,7 @@ static int jffs2_garbage_collect_dnode(struct jffs2_sb_info *c, struct jffs2_era
 	 * trying to write out, read_cache_page() will not deadlock. */
 	mutex_unlock(&f->sem);
 	page = read_cache_page(inode->i_mapping, start >> PAGE_SHIFT,
-			       jffs2_do_readpage_unlock, NULL);
+			       __jffs2_read_folio, NULL);
 	if (IS_ERR(page)) {
 		pr_warn("read_cache_page() returned error: %ld\n",
 			PTR_ERR(page));
diff --git a/fs/jffs2/os-linux.h b/fs/jffs2/os-linux.h
index 173eccac691d..921d782583d6 100644
--- a/fs/jffs2/os-linux.h
+++ b/fs/jffs2/os-linux.h
@@ -155,7 +155,7 @@ extern const struct file_operations jffs2_file_operations;
 extern const struct inode_operations jffs2_file_inode_operations;
 extern const struct address_space_operations jffs2_file_address_operations;
 int jffs2_fsync(struct file *, loff_t, loff_t, int);
-int jffs2_do_readpage_unlock(void *data, struct page *pg);
+int __jffs2_read_folio(struct file *file, struct folio *folio);
 
 /* ioctl.c */
 long jffs2_ioctl(struct file *, unsigned int, unsigned long);
diff --git a/fs/nfs/symlink.c b/fs/nfs/symlink.c
index 8b53538bcc75..0e27a2e4e68b 100644
--- a/fs/nfs/symlink.c
+++ b/fs/nfs/symlink.c
@@ -26,21 +26,21 @@
  * and straight-forward than readdir caching.
  */
 
-static int nfs_symlink_filler(void *data, struct page *page)
+static int nfs_symlink_filler(struct file *file, struct folio *folio)
 {
-	struct inode *inode = page->mapping->host;
+	struct inode *inode = folio->mapping->host;
 	int error;
 
-	error = NFS_PROTO(inode)->readlink(inode, page, 0, PAGE_SIZE);
+	error = NFS_PROTO(inode)->readlink(inode, &folio->page, 0, PAGE_SIZE);
 	if (error < 0)
 		goto error;
-	SetPageUptodate(page);
-	unlock_page(page);
+	folio_mark_uptodate(folio);
+	folio_unlock(folio);
 	return 0;
 
 error:
-	SetPageError(page);
-	unlock_page(page);
+	folio_set_error(folio);
+	folio_unlock(folio);
 	return -EIO;
 }
 
diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h
index b70192f56454..831b28dab01a 100644
--- a/include/linux/pagemap.h
+++ b/include/linux/pagemap.h
@@ -492,7 +492,7 @@ static inline gfp_t readahead_gfp_mask(struct address_space *x)
 	return mapping_gfp_mask(x) | __GFP_NORETRY | __GFP_NOWARN;
 }
 
-typedef int filler_t(void *, struct page *);
+typedef int filler_t(struct file *, struct folio *);
 
 pgoff_t page_cache_next_miss(struct address_space *mapping,
 			     pgoff_t index, unsigned long max_scan);
@@ -747,9 +747,9 @@ static inline struct page *grab_cache_page(struct address_space *mapping,
 }
 
 struct folio *read_cache_folio(struct address_space *, pgoff_t index,
-		filler_t *filler, void *data);
+		filler_t *filler, struct file *file);
 struct page *read_cache_page(struct address_space *, pgoff_t index,
-		filler_t *filler, void *data);
+		filler_t *filler, struct file *file);
 extern struct page * read_cache_page_gfp(struct address_space *mapping,
 				pgoff_t index, gfp_t gfp_mask);
 
diff --git a/mm/filemap.c b/mm/filemap.c
index 079f8cca7959..81a0ed08a82c 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -3483,7 +3483,7 @@ EXPORT_SYMBOL(generic_file_mmap);
 EXPORT_SYMBOL(generic_file_readonly_mmap);
 
 static struct folio *do_read_cache_folio(struct address_space *mapping,
-		pgoff_t index, filler_t filler, void *data, gfp_t gfp)
+		pgoff_t index, filler_t filler, struct file *file, gfp_t gfp)
 {
 	struct folio *folio;
 	int err;
@@ -3504,9 +3504,9 @@ static struct folio *do_read_cache_folio(struct address_space *mapping,
 
 filler:
 		if (filler)
-			err = filler(data, &folio->page);
+			err = filler(file, folio);
 		else
-			err = mapping->a_ops->read_folio(data, folio);
+			err = mapping->a_ops->read_folio(file, folio);
 
 		if (err < 0) {
 			folio_put(folio);
@@ -3557,44 +3557,44 @@ static struct folio *do_read_cache_folio(struct address_space *mapping,
 }
 
 /**
- * read_cache_folio - read into page cache, fill it if needed
- * @mapping:	the page's address_space
- * @index:	the page index
- * @filler:	function to perform the read
- * @data:	first arg to filler(data, page) function, often left as NULL
- *
- * Read into the page cache. If a page already exists, and PageUptodate() is
- * not set, try to fill the page and wait for it to become unlocked.
+ * read_cache_folio - Read into page cache, fill it if needed.
+ * @mapping: The address_space to read from.
+ * @index: The index to read.
+ * @filler: Function to perform the read, or NULL to use aops->read_folio().
+ * @file: Passed to filler function, may be NULL if not required.
  *
- * If the page does not get brought uptodate, return -EIO.
+ * Read one page into the page cache.  If it succeeds, the folio returned
+ * will contain @index, but it may not be the first page of the folio.
  *
- * The function expects mapping->invalidate_lock to be already held.
+ * If the filler function returns an error, it will be returned to the
+ * caller.
  *
- * Return: up to date page on success, ERR_PTR() on failure.
+ * Context: May sleep.  Expects mapping->invalidate_lock to be held.
+ * Return: An uptodate folio on success, ERR_PTR() on failure.
  */
 struct folio *read_cache_folio(struct address_space *mapping, pgoff_t index,
-		filler_t filler, void *data)
+		filler_t filler, struct file *file)
 {
-	return do_read_cache_folio(mapping, index, filler, data,
+	return do_read_cache_folio(mapping, index, filler, file,
 			mapping_gfp_mask(mapping));
 }
 EXPORT_SYMBOL(read_cache_folio);
 
 static struct page *do_read_cache_page(struct address_space *mapping,
-		pgoff_t index, filler_t *filler, void *data, gfp_t gfp)
+		pgoff_t index, filler_t *filler, struct file *file, gfp_t gfp)
 {
 	struct folio *folio;
 
-	folio = do_read_cache_folio(mapping, index, filler, data, gfp);
+	folio = do_read_cache_folio(mapping, index, filler, file, gfp);
 	if (IS_ERR(folio))
 		return &folio->page;
 	return folio_file_page(folio, index);
 }
 
 struct page *read_cache_page(struct address_space *mapping,
-				pgoff_t index, filler_t *filler, void *data)
+			pgoff_t index, filler_t *filler, struct file *file)
 {
-	return do_read_cache_page(mapping, index, filler, data,
+	return do_read_cache_page(mapping, index, filler, file,
 			mapping_gfp_mask(mapping));
 }
 EXPORT_SYMBOL(read_cache_page);
-- 
2.34.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 3/3] fs: Change the type of filler_t

[Christoph Hellwig]

On Mon, May 02, 2022 at 06:41:59AM +0100, Matthew Wilcox (Oracle) wrote:
> By making filler_t the same as read_folio, we can use the same function
> for both in gfs2.  We can push the use of folios down one more level
> in jffs2 and nfs.  We also increase type safety for future users of the
> various read_cache_page() family of functions by forcing the parameter
> to be a pointer to struct file (or NULL).
> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> ---
>  fs/gfs2/aops.c          | 29 +++++++++++------------------
>  fs/jffs2/file.c         |  9 ++++-----
>  fs/jffs2/gc.c           |  2 +-
>  fs/jffs2/os-linux.h     |  2 +-
>  fs/nfs/symlink.c        | 14 +++++++-------
>  include/linux/pagemap.h |  6 +++---
>  mm/filemap.c            | 40 ++++++++++++++++++++--------------------
>  7 files changed, 47 insertions(+), 55 deletions(-)
> 
> diff --git a/fs/gfs2/aops.c b/fs/gfs2/aops.c
> index 340bf5d0e835..1016631bcbdc 100644
> --- a/fs/gfs2/aops.c
> +++ b/fs/gfs2/aops.c
> @@ -464,21 +464,24 @@ static int stuffed_readpage(struct gfs2_inode *ip, struct page *page)
>  	return 0;
>  }
>  
> -
> -static int __gfs2_readpage(void *file, struct page *page)
> +/**
> + * gfs2_read_folio - read a folio from a file
> + * @file: The file to read
> + * @folio: The folio in the file
> + */
> +static int gfs2_read_folio(struct file *file, struct folio *folio)
>  {
> -	struct folio *folio = page_folio(page);
> -	struct inode *inode = page->mapping->host;
> +	struct inode *inode = folio->mapping->host;
>  	struct gfs2_inode *ip = GFS2_I(inode);
>  	struct gfs2_sbd *sdp = GFS2_SB(inode);
>  	int error;
>  
>  	if (!gfs2_is_jdata(ip) ||
> -	    (i_blocksize(inode) == PAGE_SIZE && !page_has_buffers(page))) {
> +	    (i_blocksize(inode) == PAGE_SIZE && !folio_buffers(folio))) {
>  		error = iomap_read_folio(folio, &gfs2_iomap_ops);
>  	} else if (gfs2_is_stuffed(ip)) {
> -		error = stuffed_readpage(ip, page);
> -		unlock_page(page);
> +		error = stuffed_readpage(ip, &folio->page);
> +		folio_unlock(folio);
>  	} else {
>  		error = mpage_read_folio(folio, gfs2_block_map);
>  	}
> @@ -489,16 +492,6 @@ static int __gfs2_readpage(void *file, struct page *page)
>  	return error;
>  }
>  
> -/**
> - * gfs2_read_folio - read a folio from a file
> - * @file: The file to read
> - * @folio: The folio in the file
> - */
> -static int gfs2_read_folio(struct file *file, struct folio *folio)
> -{
> -	return __gfs2_readpage(file, &folio->page);
> -}
> -
>  /**
>   * gfs2_internal_read - read an internal file
>   * @ip: The gfs2 inode
> @@ -523,7 +516,7 @@ int gfs2_internal_read(struct gfs2_inode *ip, char *buf, loff_t *pos,
>  		amt = size - copied;
>  		if (offset + size > PAGE_SIZE)
>  			amt = PAGE_SIZE - offset;
> -		page = read_cache_page(mapping, index, __gfs2_readpage, NULL);
> +		page = read_cache_page(mapping, index, gfs2_read_folio, NULL);
>  		if (IS_ERR(page))
>  			return PTR_ERR(page);
>  		p = kmap_atomic(page);
> diff --git a/fs/jffs2/file.c b/fs/jffs2/file.c
> index 492fb2da0403..ba86acbe12d3 100644
> --- a/fs/jffs2/file.c
> +++ b/fs/jffs2/file.c
> @@ -110,21 +110,20 @@ static int jffs2_do_readpage_nolock (struct inode *inode, struct page *pg)
>  	return ret;
>  }
>  
> -int jffs2_do_readpage_unlock(void *data, struct page *pg)
> +int __jffs2_read_folio(struct file *file, struct folio *folio)
>  {
> -	int ret = jffs2_do_readpage_nolock(pg->mapping->host, pg);
> -	unlock_page(pg);
> +	int ret = jffs2_do_readpage_nolock(folio->mapping->host, &folio->page);
> +	folio_unlock(folio);
>  	return ret;
>  }
>  
> -
>  static int jffs2_read_folio(struct file *file, struct folio *folio)
>  {
>  	struct jffs2_inode_info *f = JFFS2_INODE_INFO(folio->mapping->host);
>  	int ret;
>  
>  	mutex_lock(&f->sem);
> -	ret = jffs2_do_readpage_unlock(file, &folio->page);
> +	ret = __jffs2_read_folio(file, folio);
>  	mutex_unlock(&f->sem);
>  	return ret;
>  }
> diff --git a/fs/jffs2/gc.c b/fs/jffs2/gc.c
> index a53bac7569b6..5c6602f3c189 100644
> --- a/fs/jffs2/gc.c
> +++ b/fs/jffs2/gc.c
> @@ -1327,7 +1327,7 @@ static int jffs2_garbage_collect_dnode(struct jffs2_sb_info *c, struct jffs2_era
>  	 * trying to write out, read_cache_page() will not deadlock. */
>  	mutex_unlock(&f->sem);
>  	page = read_cache_page(inode->i_mapping, start >> PAGE_SHIFT,
> -			       jffs2_do_readpage_unlock, NULL);
> +			       __jffs2_read_folio, NULL);
>  	if (IS_ERR(page)) {
>  		pr_warn("read_cache_page() returned error: %ld\n",
>  			PTR_ERR(page));
> diff --git a/fs/jffs2/os-linux.h b/fs/jffs2/os-linux.h
> index 173eccac691d..921d782583d6 100644
> --- a/fs/jffs2/os-linux.h
> +++ b/fs/jffs2/os-linux.h
> @@ -155,7 +155,7 @@ extern const struct file_operations jffs2_file_operations;
>  extern const struct inode_operations jffs2_file_inode_operations;
>  extern const struct address_space_operations jffs2_file_address_operations;
>  int jffs2_fsync(struct file *, loff_t, loff_t, int);
> -int jffs2_do_readpage_unlock(void *data, struct page *pg);
> +int __jffs2_read_folio(struct file *file, struct folio *folio);
>  
>  /* ioctl.c */
>  long jffs2_ioctl(struct file *, unsigned int, unsigned long);
> diff --git a/fs/nfs/symlink.c b/fs/nfs/symlink.c
> index 8b53538bcc75..0e27a2e4e68b 100644
> --- a/fs/nfs/symlink.c
> +++ b/fs/nfs/symlink.c
> @@ -26,21 +26,21 @@
>   * and straight-forward than readdir caching.
>   */
>  
> -static int nfs_symlink_filler(void *data, struct page *page)
> +static int nfs_symlink_filler(struct file *file, struct folio *folio)
>  {
> -	struct inode *inode = page->mapping->host;
> +	struct inode *inode = folio->mapping->host;
>  	int error;
>  
> -	error = NFS_PROTO(inode)->readlink(inode, page, 0, PAGE_SIZE);
> +	error = NFS_PROTO(inode)->readlink(inode, &folio->page, 0, PAGE_SIZE);
>  	if (error < 0)
>  		goto error;
> -	SetPageUptodate(page);
> -	unlock_page(page);
> +	folio_mark_uptodate(folio);
> +	folio_unlock(folio);
>  	return 0;
>  
>  error:
> -	SetPageError(page);
> -	unlock_page(page);
> +	folio_set_error(folio);
> +	folio_unlock(folio);
>  	return -EIO;
>  }
>  
> diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h
> index b70192f56454..831b28dab01a 100644
> --- a/include/linux/pagemap.h
> +++ b/include/linux/pagemap.h
> @@ -492,7 +492,7 @@ static inline gfp_t readahead_gfp_mask(struct address_space *x)
>  	return mapping_gfp_mask(x) | __GFP_NORETRY | __GFP_NOWARN;
>  }
>  
> -typedef int filler_t(void *, struct page *);
> +typedef int filler_t(struct file *, struct folio *);
>  
>  pgoff_t page_cache_next_miss(struct address_space *mapping,
>  			     pgoff_t index, unsigned long max_scan);
> @@ -747,9 +747,9 @@ static inline struct page *grab_cache_page(struct address_space *mapping,
>  }
>  
>  struct folio *read_cache_folio(struct address_space *, pgoff_t index,
> -		filler_t *filler, void *data);
> +		filler_t *filler, struct file *file);
>  struct page *read_cache_page(struct address_space *, pgoff_t index,
> -		filler_t *filler, void *data);
> +		filler_t *filler, struct file *file);
>  extern struct page * read_cache_page_gfp(struct address_space *mapping,
>  				pgoff_t index, gfp_t gfp_mask);
>  
> diff --git a/mm/filemap.c b/mm/filemap.c
> index 079f8cca7959..81a0ed08a82c 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -3483,7 +3483,7 @@ EXPORT_SYMBOL(generic_file_mmap);
>  EXPORT_SYMBOL(generic_file_readonly_mmap);
>  
>  static struct folio *do_read_cache_folio(struct address_space *mapping,
> -		pgoff_t index, filler_t filler, void *data, gfp_t gfp)
> +		pgoff_t index, filler_t filler, struct file *file, gfp_t gfp)
>  {
>  	struct folio *folio;
>  	int err;
> @@ -3504,9 +3504,9 @@ static struct folio *do_read_cache_folio(struct address_space *mapping,
>  
>  filler:
>  		if (filler)
> -			err = filler(data, &folio->page);
> +			err = filler(file, folio);
>  		else
> -			err = mapping->a_ops->read_folio(data, folio);
> +			err = mapping->a_ops->read_folio(file, folio);

Wouldn't it just make sense to just pass mapping->a_ops->read_folio as
the filler here from the callers that currently pass NULL?

Otherwise this looks good:

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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 3/3] fs: Change the type of filler_t

[Matthew Wilcox]

On Tue, May 03, 2022 at 07:23:13AM -0700, Christoph Hellwig wrote:
> > @@ -3504,9 +3504,9 @@ static struct folio *do_read_cache_folio(struct address_space *mapping,
> >  
> >  filler:
> >  		if (filler)
> > -			err = filler(data, &folio->page);
> > +			err = filler(file, folio);
> >  		else
> > -			err = mapping->a_ops->read_folio(data, folio);
> > +			err = mapping->a_ops->read_folio(file, folio);
> 
> Wouldn't it just make sense to just pass mapping->a_ops->read_folio as
> the filler here from the callers that currently pass NULL?

Thanks for the review.  We're heading in the same direction; my
plan was to (in some subsequent merge window) convert all callers of
read_cache_page() to use read_mapping_folio() (by setting ->read_folio)
and then get rid of filler_t altogether.

Maybe there are some filesystems that can't do that, eg they need a
different ->read_folio() when called from read_cache_folio() to the
one they need when called for filemap_read(), but I bet they can all be
worked around.

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [Cluster-devel] [PATCH 3/3] fs: Change the type of filler_t

[Andreas Gruenbacher]

On Mon, May 2, 2022 at 7:58 AM Matthew Wilcox (Oracle)
<willy@infradead.org> wrote:
> By making filler_t the same as read_folio, we can use the same function
> for both in gfs2.  We can push the use of folios down one more level
> in jffs2 and nfs.  We also increase type safety for future users of the
> various read_cache_page() family of functions by forcing the parameter
> to be a pointer to struct file (or NULL).
>
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> ---
>  fs/gfs2/aops.c          | 29 +++++++++++------------------
>  fs/jffs2/file.c         |  9 ++++-----
>  fs/jffs2/gc.c           |  2 +-
>  fs/jffs2/os-linux.h     |  2 +-
>  fs/nfs/symlink.c        | 14 +++++++-------
>  include/linux/pagemap.h |  6 +++---
>  mm/filemap.c            | 40 ++++++++++++++++++++--------------------
>  7 files changed, 47 insertions(+), 55 deletions(-)
>
> diff --git a/fs/gfs2/aops.c b/fs/gfs2/aops.c
> index 340bf5d0e835..1016631bcbdc 100644
> --- a/fs/gfs2/aops.c
> +++ b/fs/gfs2/aops.c
> @@ -464,21 +464,24 @@ static int stuffed_readpage(struct gfs2_inode *ip, struct page *page)
>         return 0;
>  }
>
> -
> -static int __gfs2_readpage(void *file, struct page *page)
> +/**
> + * gfs2_read_folio - read a folio from a file
> + * @file: The file to read
> + * @folio: The folio in the file
> + */
> +static int gfs2_read_folio(struct file *file, struct folio *folio)
>  {
> -       struct folio *folio = page_folio(page);
> -       struct inode *inode = page->mapping->host;
> +       struct inode *inode = folio->mapping->host;
>         struct gfs2_inode *ip = GFS2_I(inode);
>         struct gfs2_sbd *sdp = GFS2_SB(inode);
>         int error;
>
>         if (!gfs2_is_jdata(ip) ||
> -           (i_blocksize(inode) == PAGE_SIZE && !page_has_buffers(page))) {
> +           (i_blocksize(inode) == PAGE_SIZE && !folio_buffers(folio))) {
>                 error = iomap_read_folio(folio, &gfs2_iomap_ops);
>         } else if (gfs2_is_stuffed(ip)) {
> -               error = stuffed_readpage(ip, page);
> -               unlock_page(page);
> +               error = stuffed_readpage(ip, &folio->page);
> +               folio_unlock(folio);
>         } else {
>                 error = mpage_read_folio(folio, gfs2_block_map);
>         }
> @@ -489,16 +492,6 @@ static int __gfs2_readpage(void *file, struct page *page)
>         return error;
>  }
>
> -/**
> - * gfs2_read_folio - read a folio from a file
> - * @file: The file to read
> - * @folio: The folio in the file
> - */
> -static int gfs2_read_folio(struct file *file, struct folio *folio)
> -{
> -       return __gfs2_readpage(file, &folio->page);
> -}
> -
>  /**
>   * gfs2_internal_read - read an internal file
>   * @ip: The gfs2 inode
> @@ -523,7 +516,7 @@ int gfs2_internal_read(struct gfs2_inode *ip, char *buf, loff_t *pos,
>                 amt = size - copied;
>                 if (offset + size > PAGE_SIZE)
>                         amt = PAGE_SIZE - offset;
> -               page = read_cache_page(mapping, index, __gfs2_readpage, NULL);
> +               page = read_cache_page(mapping, index, gfs2_read_folio, NULL);
>                 if (IS_ERR(page))
>                         return PTR_ERR(page);
>                 p = kmap_atomic(page);

Nice. For the gfs2 part:

Reviewed-by: Andreas Gruenbacher <agruenba@redhat.com>

Thanks,
Andreas

> diff --git a/fs/jffs2/file.c b/fs/jffs2/file.c
> index 492fb2da0403..ba86acbe12d3 100644
> --- a/fs/jffs2/file.c
> +++ b/fs/jffs2/file.c
> @@ -110,21 +110,20 @@ static int jffs2_do_readpage_nolock (struct inode *inode, struct page *pg)
>         return ret;
>  }
>
> -int jffs2_do_readpage_unlock(void *data, struct page *pg)
> +int __jffs2_read_folio(struct file *file, struct folio *folio)
>  {
> -       int ret = jffs2_do_readpage_nolock(pg->mapping->host, pg);
> -       unlock_page(pg);
> +       int ret = jffs2_do_readpage_nolock(folio->mapping->host, &folio->page);
> +       folio_unlock(folio);
>         return ret;
>  }
>
> -
>  static int jffs2_read_folio(struct file *file, struct folio *folio)
>  {
>         struct jffs2_inode_info *f = JFFS2_INODE_INFO(folio->mapping->host);
>         int ret;
>
>         mutex_lock(&f->sem);
> -       ret = jffs2_do_readpage_unlock(file, &folio->page);
> +       ret = __jffs2_read_folio(file, folio);
>         mutex_unlock(&f->sem);
>         return ret;
>  }
> diff --git a/fs/jffs2/gc.c b/fs/jffs2/gc.c
> index a53bac7569b6..5c6602f3c189 100644
> --- a/fs/jffs2/gc.c
> +++ b/fs/jffs2/gc.c
> @@ -1327,7 +1327,7 @@ static int jffs2_garbage_collect_dnode(struct jffs2_sb_info *c, struct jffs2_era
>          * trying to write out, read_cache_page() will not deadlock. */
>         mutex_unlock(&f->sem);
>         page = read_cache_page(inode->i_mapping, start >> PAGE_SHIFT,
> -                              jffs2_do_readpage_unlock, NULL);
> +                              __jffs2_read_folio, NULL);
>         if (IS_ERR(page)) {
>                 pr_warn("read_cache_page() returned error: %ld\n",
>                         PTR_ERR(page));
> diff --git a/fs/jffs2/os-linux.h b/fs/jffs2/os-linux.h
> index 173eccac691d..921d782583d6 100644
> --- a/fs/jffs2/os-linux.h
> +++ b/fs/jffs2/os-linux.h
> @@ -155,7 +155,7 @@ extern const struct file_operations jffs2_file_operations;
>  extern const struct inode_operations jffs2_file_inode_operations;
>  extern const struct address_space_operations jffs2_file_address_operations;
>  int jffs2_fsync(struct file *, loff_t, loff_t, int);
> -int jffs2_do_readpage_unlock(void *data, struct page *pg);
> +int __jffs2_read_folio(struct file *file, struct folio *folio);
>
>  /* ioctl.c */
>  long jffs2_ioctl(struct file *, unsigned int, unsigned long);
> diff --git a/fs/nfs/symlink.c b/fs/nfs/symlink.c
> index 8b53538bcc75..0e27a2e4e68b 100644
> --- a/fs/nfs/symlink.c
> +++ b/fs/nfs/symlink.c
> @@ -26,21 +26,21 @@
>   * and straight-forward than readdir caching.
>   */
>
> -static int nfs_symlink_filler(void *data, struct page *page)
> +static int nfs_symlink_filler(struct file *file, struct folio *folio)
>  {
> -       struct inode *inode = page->mapping->host;
> +       struct inode *inode = folio->mapping->host;
>         int error;
>
> -       error = NFS_PROTO(inode)->readlink(inode, page, 0, PAGE_SIZE);
> +       error = NFS_PROTO(inode)->readlink(inode, &folio->page, 0, PAGE_SIZE);
>         if (error < 0)
>                 goto error;
> -       SetPageUptodate(page);
> -       unlock_page(page);
> +       folio_mark_uptodate(folio);
> +       folio_unlock(folio);
>         return 0;
>
>  error:
> -       SetPageError(page);
> -       unlock_page(page);
> +       folio_set_error(folio);
> +       folio_unlock(folio);
>         return -EIO;
>  }
>
> diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h
> index b70192f56454..831b28dab01a 100644
> --- a/include/linux/pagemap.h
> +++ b/include/linux/pagemap.h
> @@ -492,7 +492,7 @@ static inline gfp_t readahead_gfp_mask(struct address_space *x)
>         return mapping_gfp_mask(x) | __GFP_NORETRY | __GFP_NOWARN;
>  }
>
> -typedef int filler_t(void *, struct page *);
> +typedef int filler_t(struct file *, struct folio *);
>
>  pgoff_t page_cache_next_miss(struct address_space *mapping,
>                              pgoff_t index, unsigned long max_scan);
> @@ -747,9 +747,9 @@ static inline struct page *grab_cache_page(struct address_space *mapping,
>  }
>
>  struct folio *read_cache_folio(struct address_space *, pgoff_t index,
> -               filler_t *filler, void *data);
> +               filler_t *filler, struct file *file);
>  struct page *read_cache_page(struct address_space *, pgoff_t index,
> -               filler_t *filler, void *data);
> +               filler_t *filler, struct file *file);
>  extern struct page * read_cache_page_gfp(struct address_space *mapping,
>                                 pgoff_t index, gfp_t gfp_mask);
>
> diff --git a/mm/filemap.c b/mm/filemap.c
> index 079f8cca7959..81a0ed08a82c 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -3483,7 +3483,7 @@ EXPORT_SYMBOL(generic_file_mmap);
>  EXPORT_SYMBOL(generic_file_readonly_mmap);
>
>  static struct folio *do_read_cache_folio(struct address_space *mapping,
> -               pgoff_t index, filler_t filler, void *data, gfp_t gfp)
> +               pgoff_t index, filler_t filler, struct file *file, gfp_t gfp)
>  {
>         struct folio *folio;
>         int err;
> @@ -3504,9 +3504,9 @@ static struct folio *do_read_cache_folio(struct address_space *mapping,
>
>  filler:
>                 if (filler)
> -                       err = filler(data, &folio->page);
> +                       err = filler(file, folio);
>                 else
> -                       err = mapping->a_ops->read_folio(data, folio);
> +                       err = mapping->a_ops->read_folio(file, folio);
>
>                 if (err < 0) {
>                         folio_put(folio);
> @@ -3557,44 +3557,44 @@ static struct folio *do_read_cache_folio(struct address_space *mapping,
>  }
>
>  /**
> - * read_cache_folio - read into page cache, fill it if needed
> - * @mapping:   the page's address_space
> - * @index:     the page index
> - * @filler:    function to perform the read
> - * @data:      first arg to filler(data, page) function, often left as NULL
> - *
> - * Read into the page cache. If a page already exists, and PageUptodate() is
> - * not set, try to fill the page and wait for it to become unlocked.
> + * read_cache_folio - Read into page cache, fill it if needed.
> + * @mapping: The address_space to read from.
> + * @index: The index to read.
> + * @filler: Function to perform the read, or NULL to use aops->read_folio().
> + * @file: Passed to filler function, may be NULL if not required.
>   *
> - * If the page does not get brought uptodate, return -EIO.
> + * Read one page into the page cache.  If it succeeds, the folio returned
> + * will contain @index, but it may not be the first page of the folio.
>   *
> - * The function expects mapping->invalidate_lock to be already held.
> + * If the filler function returns an error, it will be returned to the
> + * caller.
>   *
> - * Return: up to date page on success, ERR_PTR() on failure.
> + * Context: May sleep.  Expects mapping->invalidate_lock to be held.
> + * Return: An uptodate folio on success, ERR_PTR() on failure.
>   */
>  struct folio *read_cache_folio(struct address_space *mapping, pgoff_t index,
> -               filler_t filler, void *data)
> +               filler_t filler, struct file *file)
>  {
> -       return do_read_cache_folio(mapping, index, filler, data,
> +       return do_read_cache_folio(mapping, index, filler, file,
>                         mapping_gfp_mask(mapping));
>  }
>  EXPORT_SYMBOL(read_cache_folio);
>
>  static struct page *do_read_cache_page(struct address_space *mapping,
> -               pgoff_t index, filler_t *filler, void *data, gfp_t gfp)
> +               pgoff_t index, filler_t *filler, struct file *file, gfp_t gfp)
>  {
>         struct folio *folio;
>
> -       folio = do_read_cache_folio(mapping, index, filler, data, gfp);
> +       folio = do_read_cache_folio(mapping, index, filler, file, gfp);
>         if (IS_ERR(folio))
>                 return &folio->page;
>         return folio_file_page(folio, index);
>  }
>
>  struct page *read_cache_page(struct address_space *mapping,
> -                               pgoff_t index, filler_t *filler, void *data)
> +                       pgoff_t index, filler_t *filler, struct file *file)
>  {
> -       return do_read_cache_page(mapping, index, filler, data,
> +       return do_read_cache_page(mapping, index, filler, file,
>                         mapping_gfp_mask(mapping));
>  }
>  EXPORT_SYMBOL(read_cache_page);
> --
> 2.34.1
>


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/