[PATCH 0/7] Convert UDF to folios

[PATCH 0/7] Convert UDF to folios

[Matthew Wilcox (Oracle)]

I'm not making any attempt here to support large folios.  This is just to
remove uses of the page-based APIs.  Most of these places are for inline
data or symlinks, so it wouldn't be appropriate to use large folios
(unless we want to support bs>PS, which seems to be permitted by UDF,
although not widely supported).

Matthew Wilcox (Oracle) (7):
  udf: Convert udf_symlink_filler() to use a folio
  udf: Convert udf_write_begin() to use a folio
  udf: Convert udf_expand_file_adinicb() to use a folio
  udf: Convert udf_adinicb_readpage() to udf_adinicb_read_folio()
  udf: Convert udf_symlink_getattr() to use a folio
  udf: Convert udf_page_mkwrite() to use a folio
  udf: Use a folio in udf_write_end()

 fs/udf/file.c    | 20 +++++++--------
 fs/udf/inode.c   | 65 ++++++++++++++++++++++++------------------------
 fs/udf/symlink.c | 34 +++++++++----------------
 3 files changed, 54 insertions(+), 65 deletions(-)

-- 
2.43.0

[PATCH 1/7] udf: Convert udf_symlink_filler() to use a folio

[Matthew Wilcox (Oracle)]

Remove the conversion to struct page and use folio APIs throughout.

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

diff --git a/fs/udf/symlink.c b/fs/udf/symlink.c
index f7eaf7b14594..0105e7e2ba3d 100644
--- a/fs/udf/symlink.c
+++ b/fs/udf/symlink.c
@@ -99,18 +99,17 @@ static int udf_pc_to_char(struct super_block *sb, unsigned char *from,
 
 static int udf_symlink_filler(struct file *file, struct folio *folio)
 {
-	struct page *page = &folio->page;
-	struct inode *inode = page->mapping->host;
+	struct inode *inode = folio->mapping->host;
 	struct buffer_head *bh = NULL;
 	unsigned char *symlink;
 	int err = 0;
-	unsigned char *p = page_address(page);
+	unsigned char *p = folio_address(folio);
 	struct udf_inode_info *iinfo = UDF_I(inode);
 
 	/* We don't support symlinks longer than one block */
 	if (inode->i_size > inode->i_sb->s_blocksize) {
 		err = -ENAMETOOLONG;
-		goto out_unlock;
+		goto out;
 	}
 
 	if (iinfo->i_alloc_type == ICBTAG_FLAG_AD_IN_ICB) {
@@ -120,24 +119,15 @@ static int udf_symlink_filler(struct file *file, struct folio *folio)
 		if (!bh) {
 			if (!err)
 				err = -EFSCORRUPTED;
-			goto out_err;
+			goto out;
 		}
 		symlink = bh->b_data;
 	}
 
 	err = udf_pc_to_char(inode->i_sb, symlink, inode->i_size, p, PAGE_SIZE);
 	brelse(bh);
-	if (err)
-		goto out_err;
-
-	SetPageUptodate(page);
-	unlock_page(page);
-	return 0;
-
-out_err:
-	SetPageError(page);
-out_unlock:
-	unlock_page(page);
+out:
+	folio_end_read(folio, err == 0);
 	return err;
 }
 
-- 
2.43.0

[PATCH 2/7] udf: Convert udf_write_begin() to use a folio

[Matthew Wilcox (Oracle)]

Use the folio APIs throughout instead of the deprecated page APIs.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 fs/udf/inode.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/fs/udf/inode.c b/fs/udf/inode.c
index 2f831a3a91af..5146b9d7aba3 100644
--- a/fs/udf/inode.c
+++ b/fs/udf/inode.c
@@ -254,7 +254,7 @@ static int udf_write_begin(struct file *file, struct address_space *mapping,
 			   struct page **pagep, void **fsdata)
 {
 	struct udf_inode_info *iinfo = UDF_I(file_inode(file));
-	struct page *page;
+	struct folio *folio;
 	int ret;
 
 	if (iinfo->i_alloc_type != ICBTAG_FLAG_AD_IN_ICB) {
@@ -266,12 +266,13 @@ static int udf_write_begin(struct file *file, struct address_space *mapping,
 	}
 	if (WARN_ON_ONCE(pos >= PAGE_SIZE))
 		return -EIO;
-	page = grab_cache_page_write_begin(mapping, 0);
-	if (!page)
-		return -ENOMEM;
-	*pagep = page;
-	if (!PageUptodate(page))
-		udf_adinicb_readpage(page);
+	folio = __filemap_get_folio(mapping, 0, FGP_WRITEBEGIN,
+			mapping_gfp_mask(mapping));
+	if (IS_ERR(folio))
+		return PTR_ERR(folio);
+	*pagep = &folio->page;
+	if (!folio_test_uptodate(folio))
+		udf_adinicb_readpage(&folio->page);
 	return 0;
 }
 
-- 
2.43.0

[PATCH 3/7] udf: Convert udf_expand_file_adinicb() to use a folio

[Matthew Wilcox (Oracle)]

Use the folio APIs throughout this function.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 fs/udf/inode.c | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

diff --git a/fs/udf/inode.c b/fs/udf/inode.c
index 5146b9d7aba3..59215494e6f6 100644
--- a/fs/udf/inode.c
+++ b/fs/udf/inode.c
@@ -342,7 +342,7 @@ const struct address_space_operations udf_aops = {
  */
 int udf_expand_file_adinicb(struct inode *inode)
 {
-	struct page *page;
+	struct folio *folio;
 	struct udf_inode_info *iinfo = UDF_I(inode);
 	int err;
 
@@ -358,12 +358,13 @@ int udf_expand_file_adinicb(struct inode *inode)
 		return 0;
 	}
 
-	page = find_or_create_page(inode->i_mapping, 0, GFP_KERNEL);
-	if (!page)
-		return -ENOMEM;
+	folio = __filemap_get_folio(inode->i_mapping, 0,
+			FGP_LOCK | FGP_ACCESSED | FGP_CREAT, GFP_KERNEL);
+	if (IS_ERR(folio))
+		return PTR_ERR(folio);
 
-	if (!PageUptodate(page))
-		udf_adinicb_readpage(page);
+	if (!folio_test_uptodate(folio))
+		udf_adinicb_readpage(&folio->page);
 	down_write(&iinfo->i_data_sem);
 	memset(iinfo->i_data + iinfo->i_lenEAttr, 0x00,
 	       iinfo->i_lenAlloc);
@@ -372,22 +373,22 @@ int udf_expand_file_adinicb(struct inode *inode)
 		iinfo->i_alloc_type = ICBTAG_FLAG_AD_SHORT;
 	else
 		iinfo->i_alloc_type = ICBTAG_FLAG_AD_LONG;
-	set_page_dirty(page);
-	unlock_page(page);
+	folio_mark_dirty(folio);
+	folio_unlock(folio);
 	up_write(&iinfo->i_data_sem);
 	err = filemap_fdatawrite(inode->i_mapping);
 	if (err) {
 		/* Restore everything back so that we don't lose data... */
-		lock_page(page);
+		folio_lock(folio);
 		down_write(&iinfo->i_data_sem);
-		memcpy_to_page(page, 0, iinfo->i_data + iinfo->i_lenEAttr,
-			       inode->i_size);
-		unlock_page(page);
+		memcpy_from_folio(iinfo->i_data + iinfo->i_lenEAttr,
+				folio, 0, inode->i_size);
+		folio_unlock(folio);
 		iinfo->i_alloc_type = ICBTAG_FLAG_AD_IN_ICB;
 		iinfo->i_lenAlloc = inode->i_size;
 		up_write(&iinfo->i_data_sem);
 	}
-	put_page(page);
+	folio_put(folio);
 	mark_inode_dirty(inode);
 
 	return err;
-- 
2.43.0

[PATCH 4/7] udf: Convert udf_adinicb_readpage() to udf_adinicb_read_folio()

[Matthew Wilcox (Oracle)]

Now that all three callers have a folio, convert this function to
take a folio, and use the folio APIs.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 fs/udf/inode.c | 19 +++++++------------
 1 file changed, 7 insertions(+), 12 deletions(-)

diff --git a/fs/udf/inode.c b/fs/udf/inode.c
index 59215494e6f6..d34562156522 100644
--- a/fs/udf/inode.c
+++ b/fs/udf/inode.c
@@ -208,19 +208,14 @@ static int udf_writepages(struct address_space *mapping,
 	return write_cache_pages(mapping, wbc, udf_adinicb_writepage, NULL);
 }
 
-static void udf_adinicb_readpage(struct page *page)
+static void udf_adinicb_read_folio(struct folio *folio)
 {
-	struct inode *inode = page->mapping->host;
-	char *kaddr;
+	struct inode *inode = folio->mapping->host;
 	struct udf_inode_info *iinfo = UDF_I(inode);
 	loff_t isize = i_size_read(inode);
 
-	kaddr = kmap_local_page(page);
-	memcpy(kaddr, iinfo->i_data + iinfo->i_lenEAttr, isize);
-	memset(kaddr + isize, 0, PAGE_SIZE - isize);
-	flush_dcache_page(page);
-	SetPageUptodate(page);
-	kunmap_local(kaddr);
+	folio_fill_tail(folio, 0, iinfo->i_data + iinfo->i_lenEAttr, isize);
+	folio_mark_uptodate(folio);
 }
 
 static int udf_read_folio(struct file *file, struct folio *folio)
@@ -228,7 +223,7 @@ static int udf_read_folio(struct file *file, struct folio *folio)
 	struct udf_inode_info *iinfo = UDF_I(file_inode(file));
 
 	if (iinfo->i_alloc_type == ICBTAG_FLAG_AD_IN_ICB) {
-		udf_adinicb_readpage(&folio->page);
+		udf_adinicb_read_folio(folio);
 		folio_unlock(folio);
 		return 0;
 	}
@@ -272,7 +267,7 @@ static int udf_write_begin(struct file *file, struct address_space *mapping,
 		return PTR_ERR(folio);
 	*pagep = &folio->page;
 	if (!folio_test_uptodate(folio))
-		udf_adinicb_readpage(&folio->page);
+		udf_adinicb_read_folio(folio);
 	return 0;
 }
 
@@ -364,7 +359,7 @@ int udf_expand_file_adinicb(struct inode *inode)
 		return PTR_ERR(folio);
 
 	if (!folio_test_uptodate(folio))
-		udf_adinicb_readpage(&folio->page);
+		udf_adinicb_read_folio(folio);
 	down_write(&iinfo->i_data_sem);
 	memset(iinfo->i_data + iinfo->i_lenEAttr, 0x00,
 	       iinfo->i_lenAlloc);
-- 
2.43.0

[PATCH 5/7] udf: Convert udf_symlink_getattr() to use a folio

[Matthew Wilcox (Oracle)]

We're getting this from the page cache, so it's definitely a folio.
Saves a call to compound_head() hidden in put_page().

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

diff --git a/fs/udf/symlink.c b/fs/udf/symlink.c
index 0105e7e2ba3d..fe03745d09b1 100644
--- a/fs/udf/symlink.c
+++ b/fs/udf/symlink.c
@@ -137,12 +137,12 @@ static int udf_symlink_getattr(struct mnt_idmap *idmap,
 {
 	struct dentry *dentry = path->dentry;
 	struct inode *inode = d_backing_inode(dentry);
-	struct page *page;
+	struct folio *folio;
 
 	generic_fillattr(&nop_mnt_idmap, request_mask, inode, stat);
-	page = read_mapping_page(inode->i_mapping, 0, NULL);
-	if (IS_ERR(page))
-		return PTR_ERR(page);
+	folio = read_mapping_folio(inode->i_mapping, 0, NULL);
+	if (IS_ERR(folio))
+		return PTR_ERR(folio);
 	/*
 	 * UDF uses non-trivial encoding of symlinks so i_size does not match
 	 * number of characters reported by readlink(2) which apparently some
@@ -152,8 +152,8 @@ static int udf_symlink_getattr(struct mnt_idmap *idmap,
 	 * let's report the length of string returned by readlink(2) for
 	 * st_size.
 	 */
-	stat->size = strlen(page_address(page));
-	put_page(page);
+	stat->size = strlen(folio_address(folio));
+	folio_put(folio);
 
 	return 0;
 }
-- 
2.43.0

[PATCH 6/7] udf: Convert udf_page_mkwrite() to use a folio

[Matthew Wilcox (Oracle)]

Convert the vm_fault page to a folio, then use it throughout.
Replaces five calls to compound_head() with one.

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

diff --git a/fs/udf/file.c b/fs/udf/file.c
index 0ceac4b5937c..97c59585208c 100644
--- a/fs/udf/file.c
+++ b/fs/udf/file.c
@@ -39,7 +39,7 @@ static vm_fault_t udf_page_mkwrite(struct vm_fault *vmf)
 	struct vm_area_struct *vma = vmf->vma;
 	struct inode *inode = file_inode(vma->vm_file);
 	struct address_space *mapping = inode->i_mapping;
-	struct page *page = vmf->page;
+	struct folio *folio = page_folio(vmf->page);
 	loff_t size;
 	unsigned int end;
 	vm_fault_t ret = VM_FAULT_LOCKED;
@@ -48,31 +48,31 @@ static vm_fault_t udf_page_mkwrite(struct vm_fault *vmf)
 	sb_start_pagefault(inode->i_sb);
 	file_update_time(vma->vm_file);
 	filemap_invalidate_lock_shared(mapping);
-	lock_page(page);
+	folio_lock(folio);
 	size = i_size_read(inode);
-	if (page->mapping != inode->i_mapping || page_offset(page) >= size) {
-		unlock_page(page);
+	if (folio->mapping != inode->i_mapping || folio_pos(folio) >= size) {
+		folio_unlock(folio);
 		ret = VM_FAULT_NOPAGE;
 		goto out_unlock;
 	}
 	/* Space is already allocated for in-ICB file */
 	if (UDF_I(inode)->i_alloc_type == ICBTAG_FLAG_AD_IN_ICB)
 		goto out_dirty;
-	if (page->index == size >> PAGE_SHIFT)
+	if (folio->index == size >> PAGE_SHIFT)
 		end = size & ~PAGE_MASK;
 	else
 		end = PAGE_SIZE;
-	err = __block_write_begin(page, 0, end, udf_get_block);
+	err = __block_write_begin(&folio->page, 0, end, udf_get_block);
 	if (err) {
-		unlock_page(page);
+		folio_unlock(folio);
 		ret = vmf_fs_error(err);
 		goto out_unlock;
 	}
 
-	block_commit_write(page, 0, end);
+	block_commit_write(&folio->page, 0, end);
 out_dirty:
-	set_page_dirty(page);
-	wait_for_stable_page(page);
+	folio_mark_dirty(folio);
+	folio_wait_stable(folio);
 out_unlock:
 	filemap_invalidate_unlock_shared(mapping);
 	sb_end_pagefault(inode->i_sb);
-- 
2.43.0

[PATCH 7/7] udf: Use a folio in udf_write_end()

[Matthew Wilcox (Oracle)]

Convert the page to a folio and use the folio APIs.  Replaces three
calls to compound_head() with one.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 fs/udf/inode.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/fs/udf/inode.c b/fs/udf/inode.c
index d34562156522..2fb21c5ffccf 100644
--- a/fs/udf/inode.c
+++ b/fs/udf/inode.c
@@ -276,17 +276,19 @@ static int udf_write_end(struct file *file, struct address_space *mapping,
 			 struct page *page, void *fsdata)
 {
 	struct inode *inode = file_inode(file);
+	struct folio *folio;
 	loff_t last_pos;
 
 	if (UDF_I(inode)->i_alloc_type != ICBTAG_FLAG_AD_IN_ICB)
 		return generic_write_end(file, mapping, pos, len, copied, page,
 					 fsdata);
+	folio = page_folio(page);
 	last_pos = pos + copied;
 	if (last_pos > inode->i_size)
 		i_size_write(inode, last_pos);
-	set_page_dirty(page);
-	unlock_page(page);
-	put_page(page);
+	folio_mark_dirty(folio);
+	folio_unlock(folio);
+	folio_put(folio);
 
 	return copied;
 }
-- 
2.43.0

Re: [PATCH 1/7] udf: Convert udf_symlink_filler() to use a folio

[Jan Kara]

On Wed 17-04-24 16:04:07, Matthew Wilcox (Oracle) wrote:
> Remove the conversion to struct page and use folio APIs throughout.
> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>

Looks good. I've just noticed this removes the SetPageError(). Grepping for
a while we seem to set/clear that in quite some places in the filesystems
but nobody is reading it these days (to be fair jfs has one test and btrfs
also one)? And similarly with folio_test_error... I have a recollection
this was actually used in the past but maybe you've removed it as part of
folio overhaul? Anyway, either something should start using the error bit
or we can drop the dead code and free up a page flags bit. Yay.

								Honza

> ---
>  fs/udf/symlink.c | 22 ++++++----------------
>  1 file changed, 6 insertions(+), 16 deletions(-)
> 
> diff --git a/fs/udf/symlink.c b/fs/udf/symlink.c
> index f7eaf7b14594..0105e7e2ba3d 100644
> --- a/fs/udf/symlink.c
> +++ b/fs/udf/symlink.c
> @@ -99,18 +99,17 @@ static int udf_pc_to_char(struct super_block *sb, unsigned char *from,
>  
>  static int udf_symlink_filler(struct file *file, struct folio *folio)
>  {
> -	struct page *page = &folio->page;
> -	struct inode *inode = page->mapping->host;
> +	struct inode *inode = folio->mapping->host;
>  	struct buffer_head *bh = NULL;
>  	unsigned char *symlink;
>  	int err = 0;
> -	unsigned char *p = page_address(page);
> +	unsigned char *p = folio_address(folio);
>  	struct udf_inode_info *iinfo = UDF_I(inode);
>  
>  	/* We don't support symlinks longer than one block */
>  	if (inode->i_size > inode->i_sb->s_blocksize) {
>  		err = -ENAMETOOLONG;
> -		goto out_unlock;
> +		goto out;
>  	}
>  
>  	if (iinfo->i_alloc_type == ICBTAG_FLAG_AD_IN_ICB) {
> @@ -120,24 +119,15 @@ static int udf_symlink_filler(struct file *file, struct folio *folio)
>  		if (!bh) {
>  			if (!err)
>  				err = -EFSCORRUPTED;
> -			goto out_err;
> +			goto out;
>  		}
>  		symlink = bh->b_data;
>  	}
>  
>  	err = udf_pc_to_char(inode->i_sb, symlink, inode->i_size, p, PAGE_SIZE);
>  	brelse(bh);
> -	if (err)
> -		goto out_err;
> -
> -	SetPageUptodate(page);
> -	unlock_page(page);
> -	return 0;
> -
> -out_err:
> -	SetPageError(page);
> -out_unlock:
> -	unlock_page(page);
> +out:
> +	folio_end_read(folio, err == 0);
>  	return err;
>  }
>  
> -- 
> 2.43.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 3/7] udf: Convert udf_expand_file_adinicb() to use a folio

[Jan Kara]

On Wed 17-04-24 16:04:09, Matthew Wilcox (Oracle) wrote:
> Use the folio APIs throughout this function.
> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>

Looks good.

>  	up_write(&iinfo->i_data_sem);
>  	err = filemap_fdatawrite(inode->i_mapping);
>  	if (err) {
>  		/* Restore everything back so that we don't lose data... */
> -		lock_page(page);
> +		folio_lock(folio);
>  		down_write(&iinfo->i_data_sem);
> -		memcpy_to_page(page, 0, iinfo->i_data + iinfo->i_lenEAttr,
> -			       inode->i_size);
> -		unlock_page(page);
> +		memcpy_from_folio(iinfo->i_data + iinfo->i_lenEAttr,
> +				folio, 0, inode->i_size);
> +		folio_unlock(folio);

So this actually silently fixes a bug on the error recovery path where we
could be loosing old data in case of ENOSPC. I'll add:

Fixes: 1eeceaec794e ("udf: Convert udf_expand_file_adinicb() to avoid kmap_atomic()")

and some commentary on commit.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 4/7] udf: Convert udf_adinicb_readpage() to udf_adinicb_read_folio()

[Jan Kara]

On Wed 17-04-24 16:04:10, Matthew Wilcox (Oracle) wrote:
> Now that all three callers have a folio, convert this function to
> take a folio, and use the folio APIs.
> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>

One comment below:

> -static void udf_adinicb_readpage(struct page *page)
> +static void udf_adinicb_read_folio(struct folio *folio)
>  {
> -	struct inode *inode = page->mapping->host;
> -	char *kaddr;
> +	struct inode *inode = folio->mapping->host;
>  	struct udf_inode_info *iinfo = UDF_I(inode);
>  	loff_t isize = i_size_read(inode);
>  
> -	kaddr = kmap_local_page(page);
> -	memcpy(kaddr, iinfo->i_data + iinfo->i_lenEAttr, isize);
> -	memset(kaddr + isize, 0, PAGE_SIZE - isize);
> -	flush_dcache_page(page);

So where did the flush_dcache_page() call go? AFAIU we should be calling
flush_dcache_folio(folio) here, shouldn't we?

> -	SetPageUptodate(page);
> -	kunmap_local(kaddr);
> +	folio_fill_tail(folio, 0, iinfo->i_data + iinfo->i_lenEAttr, isize);
> +	folio_mark_uptodate(folio);
>  }

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 0/7] Convert UDF to folios

[Jan Kara]

On Wed 17-04-24 16:04:06, Matthew Wilcox (Oracle) wrote:
> I'm not making any attempt here to support large folios.  This is just to
> remove uses of the page-based APIs.  Most of these places are for inline
> data or symlinks, so it wouldn't be appropriate to use large folios
> (unless we want to support bs>PS, which seems to be permitted by UDF,
> although not widely supported).

Thanks for the conversion. Overall it looks good and I can fixup the minor
stuff I've found on commit, just I'd like to get a confirmation regarding
the flush_dcache_folio() thing...

								Honza

> Matthew Wilcox (Oracle) (7):
>   udf: Convert udf_symlink_filler() to use a folio
>   udf: Convert udf_write_begin() to use a folio
>   udf: Convert udf_expand_file_adinicb() to use a folio
>   udf: Convert udf_adinicb_readpage() to udf_adinicb_read_folio()
>   udf: Convert udf_symlink_getattr() to use a folio
>   udf: Convert udf_page_mkwrite() to use a folio
>   udf: Use a folio in udf_write_end()
> 
>  fs/udf/file.c    | 20 +++++++--------
>  fs/udf/inode.c   | 65 ++++++++++++++++++++++++------------------------
>  fs/udf/symlink.c | 34 +++++++++----------------
>  3 files changed, 54 insertions(+), 65 deletions(-)
> 
> -- 
> 2.43.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 1/7] udf: Convert udf_symlink_filler() to use a folio

[Matthew Wilcox]

On Thu, Apr 18, 2024 at 12:37:34PM +0200, Jan Kara wrote:
> On Wed 17-04-24 16:04:07, Matthew Wilcox (Oracle) wrote:
> > Remove the conversion to struct page and use folio APIs throughout.
> > 
> > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> 
> Looks good. I've just noticed this removes the SetPageError(). Grepping for
> a while we seem to set/clear that in quite some places in the filesystems
> but nobody is reading it these days (to be fair jfs has one test and btrfs
> also one)? And similarly with folio_test_error... I have a recollection
> this was actually used in the past but maybe you've removed it as part of
> folio overhaul? Anyway, either something should start using the error bit
> or we can drop the dead code and free up a page flags bit. Yay.

Right, the VFS never checks PageError nor folio_test_error.  It's purely
a filesystem-internal-use flag at this point.  I think buffer.c used
to test it, but only locally, so I turned it into a local bool.  So all
the places in filesystems which set/clear it can be removed ... except
for the filesystems which check it.

I'd love to reclaim that flag, I just need to figure out how to remove
the few remaining places that check it.  The btrfs usage is awful because
PageError was _supposed_ to be used for read errors, but they're using
it for writeback errors.  And they're using that flag on the bdev's page
cache, not even their own page cache.  It's also buggy for machines with
PAGE_SIZE > 4kB; it's just that writeback errors are rare, so they get
away with it.  I had a go at fixing it once, but failed.

JFS is more straightforward; I think I can use a bit in struct metapage
as a replacement read error flag.  I should probably have another go at
fixing these two and then I can reclaim PG_error.

Re: [PATCH 4/7] udf: Convert udf_adinicb_readpage() to udf_adinicb_read_folio()

[Matthew Wilcox]

On Thu, Apr 18, 2024 at 12:50:00PM +0200, Jan Kara wrote:
> > -	kaddr = kmap_local_page(page);
> > -	memcpy(kaddr, iinfo->i_data + iinfo->i_lenEAttr, isize);
> > -	memset(kaddr + isize, 0, PAGE_SIZE - isize);
> > -	flush_dcache_page(page);
> 
> So where did the flush_dcache_page() call go? AFAIU we should be calling
> flush_dcache_folio(folio) here, shouldn't we?
> 
> > -	SetPageUptodate(page);
> > -	kunmap_local(kaddr);
> > +	folio_fill_tail(folio, 0, iinfo->i_data + iinfo->i_lenEAttr, isize);
> > +	folio_mark_uptodate(folio);
> >  }

It's inside folio_zero_tail(), called from folio_fill_tail().

Re: [PATCH 4/7] udf: Convert udf_adinicb_readpage() to udf_adinicb_read_folio()

[Jan Kara]

On Thu 18-04-24 13:29:20, Matthew Wilcox wrote:
> On Thu, Apr 18, 2024 at 12:50:00PM +0200, Jan Kara wrote:
> > > -	kaddr = kmap_local_page(page);
> > > -	memcpy(kaddr, iinfo->i_data + iinfo->i_lenEAttr, isize);
> > > -	memset(kaddr + isize, 0, PAGE_SIZE - isize);
> > > -	flush_dcache_page(page);
> > 
> > So where did the flush_dcache_page() call go? AFAIU we should be calling
> > flush_dcache_folio(folio) here, shouldn't we?
> > 
> > > -	SetPageUptodate(page);
> > > -	kunmap_local(kaddr);
> > > +	folio_fill_tail(folio, 0, iinfo->i_data + iinfo->i_lenEAttr, isize);
> > > +	folio_mark_uptodate(folio);
> > >  }
> 
> It's inside folio_zero_tail(), called from folio_fill_tail().

Ah, missed that. Well hidden ;) Thanks for clarification.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 0/7] Convert UDF to folios

[Jan Kara]

On Thu 18-04-24 12:53:23, Jan Kara wrote:
> On Wed 17-04-24 16:04:06, Matthew Wilcox (Oracle) wrote:
> > I'm not making any attempt here to support large folios.  This is just to
> > remove uses of the page-based APIs.  Most of these places are for inline
> > data or symlinks, so it wouldn't be appropriate to use large folios
> > (unless we want to support bs>PS, which seems to be permitted by UDF,
> > although not widely supported).
> 
> Thanks for the conversion. Overall it looks good and I can fixup the minor
> stuff I've found on commit, just I'd like to get a confirmation regarding
> the flush_dcache_folio() thing...

OK, I've merged the patches into my tree.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR