[PATCH 0/1] fuse: fix direct io folio offset and length calculation

[PATCH 0/1] fuse: fix direct io folio offset and length calculation

[Joanne Koong]

As Malte noted in [1], there is an issue with commit 3b97c3652d91 ("fuse:
convert direct io to use folios"). This commit mistakenly assumed that all
folios encountered in fuse are one page size, but this is not true for the
direct io case.

This problem was found when running bcachefs as the rootfs on an Arch VM, and
installing FreeCAD with "flatpak install flathub org.freecad.FreeCAD".

Before this fix, the checksum was corrupted and installation fails:
 error: Failed to install org.kde.Platform: Error pulling from repo:
 While pulling runtime/org.kde.Platform/x86_64/6.7 from remote flathub:
 fsck content object
 886fd60617b81e81475db5e62beda5846d3e85fe77562eae536d2dd2a7af5b33:
 Corrupted file object; checksum
 expected='886fd60617b81e81475db5e62beda5846d3e85fe77562eae536d2dd2a7af5b33'
 actual='67f5a60d19f7a65e1ee272d455fed138b864be73399816ad18fa71319614a418'

After this fix, the installation succeeds.

A test case will be added for this (eg userspace opting into huge pages and
using O_DIRECT) as well, in a separate patchset.

[1] https://lore.kernel.org/linux-fsdevel/p3iss6hssbvtdutnwmuddvdadubrhfkdoosgmbewvo674f7f3y@cwnwffjqltzw/

Joanne Koong (1):
  fuse: fix direct io folio offset and length calculation

 fs/fuse/file.c | 28 ++++++++++++++++------------
 1 file changed, 16 insertions(+), 12 deletions(-)

-- 
2.43.5

[PATCH 1/1] fuse: fix direct io folio offset and length calculation

[Joanne Koong]

For the direct io case, the pages from userspace may be part of a huge
folio, even if all folios in the page cache for fuse are small.

Fix the logic for calculating the offset and length of the folio for
the direct io case, which currently incorrectly assumes that all folios
encountered are one page size.

Fixes: 3b97c3652d91 ("fuse: convert direct io to use folios")
Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
---
 fs/fuse/file.c | 28 ++++++++++++++++------------
 1 file changed, 16 insertions(+), 12 deletions(-)

diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index 88d0946b5bc9..15b08d6a5739 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -1557,18 +1557,22 @@ static int fuse_get_user_pages(struct fuse_args_pages *ap, struct iov_iter *ii,
 
 		nbytes += ret;
 
-		ret += start;
-		/* Currently, all folios in FUSE are one page */
-		nfolios = DIV_ROUND_UP(ret, PAGE_SIZE);
-
-		ap->descs[ap->num_folios].offset = start;
-		fuse_folio_descs_length_init(ap->descs, ap->num_folios, nfolios);
-		for (i = 0; i < nfolios; i++)
-			ap->folios[i + ap->num_folios] = page_folio(pages[i]);
-
-		ap->num_folios += nfolios;
-		ap->descs[ap->num_folios - 1].length -=
-			(PAGE_SIZE - ret) & (PAGE_SIZE - 1);
+		nfolios = DIV_ROUND_UP(ret + start, PAGE_SIZE);
+
+		for (i = 0; i < nfolios; i++) {
+			struct folio *folio = page_folio(pages[i]);
+			unsigned int offset = start +
+				(folio_page_idx(folio, pages[i]) << PAGE_SHIFT);
+			unsigned int len = min_t(unsigned int, ret, PAGE_SIZE - start);
+
+			ap->descs[ap->num_folios].offset = offset;
+			ap->descs[ap->num_folios].length = len;
+			ap->folios[ap->num_folios] = folio;
+			start = 0;
+			ret -= len;
+			ap->num_folios++;
+		}
+
 		nr_pages += nfolios;
 	}
 	kfree(pages);
-- 
2.43.5

Re: [PATCH 1/1] fuse: fix direct io folio offset and length calculation

[Bernd Schubert]

On 12/11/24 21:55, Joanne Koong wrote:
> For the direct io case, the pages from userspace may be part of a huge
> folio, even if all folios in the page cache for fuse are small.
> 
> Fix the logic for calculating the offset and length of the folio for
> the direct io case, which currently incorrectly assumes that all folios
> encountered are one page size.
> 
> Fixes: 3b97c3652d91 ("fuse: convert direct io to use folios")
> Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> ---
>  fs/fuse/file.c | 28 ++++++++++++++++------------
>  1 file changed, 16 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> index 88d0946b5bc9..15b08d6a5739 100644
> --- a/fs/fuse/file.c
> +++ b/fs/fuse/file.c
> @@ -1557,18 +1557,22 @@ static int fuse_get_user_pages(struct fuse_args_pages *ap, struct iov_iter *ii,
>  
>  		nbytes += ret;
>  
> -		ret += start;
> -		/* Currently, all folios in FUSE are one page */
> -		nfolios = DIV_ROUND_UP(ret, PAGE_SIZE);
> -
> -		ap->descs[ap->num_folios].offset = start;
> -		fuse_folio_descs_length_init(ap->descs, ap->num_folios, nfolios);
> -		for (i = 0; i < nfolios; i++)
> -			ap->folios[i + ap->num_folios] = page_folio(pages[i]);
> -
> -		ap->num_folios += nfolios;
> -		ap->descs[ap->num_folios - 1].length -=
> -			(PAGE_SIZE - ret) & (PAGE_SIZE - 1);
> +		nfolios = DIV_ROUND_UP(ret + start, PAGE_SIZE);
> +
> +		for (i = 0; i < nfolios; i++) {
> +			struct folio *folio = page_folio(pages[i]);
> +			unsigned int offset = start +
> +				(folio_page_idx(folio, pages[i]) << PAGE_SHIFT);
> +			unsigned int len = min_t(unsigned int, ret, PAGE_SIZE - start);
> +
> +			ap->descs[ap->num_folios].offset = offset;
> +			ap->descs[ap->num_folios].length = len;
> +			ap->folios[ap->num_folios] = folio;
> +			start = 0;
> +			ret -= len;
> +			ap->num_folios++;
> +		}
> +
>  		nr_pages += nfolios;
>  	}
>  	kfree(pages);


I had already looked at that yesterday. Had even created a
6.12 branch to remove the rather confusing 
ap->descs[ap->num_pages - 1].length
line and replaced it with

-               ap->descs[ap->num_pages - 1].length -=
-                       (PAGE_SIZE - ret) & (PAGE_SIZE - 1);
+               ap->descs[ap->num_pages - 1].length = offset_in_page(ret);


Anyway, thanks for the quick fix! Looks good to me. 

Reviewed-by: Bernd Schubert <bschubert@ddn.com>

Re: [PATCH 1/1] fuse: fix direct io folio offset and length calculation

[Jingbo Xu]

On 12/12/24 4:55 AM, Joanne Koong wrote:
> For the direct io case, the pages from userspace may be part of a huge
> folio, even if all folios in the page cache for fuse are small.
> 
> Fix the logic for calculating the offset and length of the folio for
> the direct io case, which currently incorrectly assumes that all folios
> encountered are one page size.
> 
> Fixes: 3b97c3652d91 ("fuse: convert direct io to use folios")
> Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> ---
>  fs/fuse/file.c | 28 ++++++++++++++++------------
>  1 file changed, 16 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> index 88d0946b5bc9..15b08d6a5739 100644
> --- a/fs/fuse/file.c
> +++ b/fs/fuse/file.c
> @@ -1557,18 +1557,22 @@ static int fuse_get_user_pages(struct fuse_args_pages *ap, struct iov_iter *ii,
>  
>  		nbytes += ret;
>  
> -		ret += start;
> -		/* Currently, all folios in FUSE are one page */
> -		nfolios = DIV_ROUND_UP(ret, PAGE_SIZE);
> -
> -		ap->descs[ap->num_folios].offset = start;
> -		fuse_folio_descs_length_init(ap->descs, ap->num_folios, nfolios);
> -		for (i = 0; i < nfolios; i++)
> -			ap->folios[i + ap->num_folios] = page_folio(pages[i]);
> -
> -		ap->num_folios += nfolios;
> -		ap->descs[ap->num_folios - 1].length -=
> -			(PAGE_SIZE - ret) & (PAGE_SIZE - 1);
> +		nfolios = DIV_ROUND_UP(ret + start, PAGE_SIZE);
> +
> +		for (i = 0; i < nfolios; i++) {
> +			struct folio *folio = page_folio(pages[i]);
> +			unsigned int offset = start +
> +				(folio_page_idx(folio, pages[i]) << PAGE_SHIFT);
> +			unsigned int len = min_t(unsigned int, ret, PAGE_SIZE - start);
> +
> +			ap->descs[ap->num_folios].offset = offset;
> +			ap->descs[ap->num_folios].length = len;
> +			ap->folios[ap->num_folios] = folio;
> +			start = 0;
> +			ret -= len;
> +			ap->num_folios++;
> +		}
> +
>  		nr_pages += nfolios;
>  	}
>  	kfree(pages);

LGTM.

Reviewed-by: Jingbo Xu <jefflexu@linux.alibaba.com>

-- 
Thanks,
Jingbo

Re: [PATCH 1/1] fuse: fix direct io folio offset and length calculation

[Miklos Szeredi]

On Wed, 11 Dec 2024 at 21:58, Joanne Koong <joannelkoong@gmail.com> wrote:
>
> For the direct io case, the pages from userspace may be part of a huge
> folio, even if all folios in the page cache for fuse are small.
>
> Fix the logic for calculating the offset and length of the folio for
> the direct io case, which currently incorrectly assumes that all folios
> encountered are one page size.
>
> Fixes: 3b97c3652d91 ("fuse: convert direct io to use folios")
> Signed-off-by: Joanne Koong <joannelkoong@gmail.com>

Applied to fuse.git#for-next.

Thanks,
Miklos