[PATCH 1/2] fuse: Fix fuse_copy_folio() size assignation

[PATCH 1/2] fuse: Fix fuse_copy_folio() size assignation

[Joanne Koong]

Only call folio_size() in fuse_copy_folio() after checking that the
folio is not null.

Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
Fixes: f008a4390bde (“fuse: support copying large folios”)

---
This was pointed out by Dan in this bug report:
https://lore.kernel.org/linux-fsdevel/aDCbR9VpB3ojnM7q@stanley.mountain/T/#u

It'd be great if this patch could be folded into the original f008a4390bde
commit in the for-next tree.

Thanks,
Joanne
---
 fs/fuse/dev.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
index fa038327f7a7..e80cd8f2c049 100644
--- a/fs/fuse/dev.c
+++ b/fs/fuse/dev.c
@@ -1098,10 +1098,13 @@ static int fuse_copy_folio(struct fuse_copy_state *cs, struct folio **foliop,
 {
 	int err;
 	struct folio *folio = *foliop;
-	size_t size = folio_size(folio);
+	size_t size;
 
-	if (folio && zeroing && count < size)
-		folio_zero_range(folio, 0, size);
+	if (folio) {
+		size = folio_size(folio);
+		if (zeroing && count < size)
+			folio_zero_range(folio, 0, size);
+	}
 
 	while (count) {
 		if (cs->write && cs->pipebufs && folio) {
@@ -1118,7 +1121,7 @@ static int fuse_copy_folio(struct fuse_copy_state *cs, struct folio **foliop,
 			}
 		} else if (!cs->len) {
 			if (cs->move_folios && folio &&
-			    offset == 0 && count == folio_size(folio)) {
+			    offset == 0 && count == size) {
 				err = fuse_try_move_folio(cs, foliop);
 				if (err <= 0)
 					return err;
-- 
2.47.1

[PATCH 2/2] fuse: clean up null folio check in fuse_copy_folio()

[Joanne Koong]

In fuse_copy_folio(), the folio in *foliop will never be null.
fuse_copy_folio() is called from two places, fuse_copy_folios() and
fuse_notify_store(). In fuse_copy_folios(), the folio will never be null
since ap->num_folios always reflects how many folios are stored in the
ap->folios[] array. In fuse_notify_store(), the folio will never be null
since there's already a check for filemap_grab_folio() returning a null
folio.

Add a WARN_ON for a null folio, which allows us to simplify the logic
inside fuse_copy_folio() that otherwise checks against a null folio.

Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
---
 fs/fuse/dev.c | 50 +++++++++++++++++++++++++-------------------------
 1 file changed, 25 insertions(+), 25 deletions(-)

diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
index e80cd8f2c049..54f42a92733b 100644
--- a/fs/fuse/dev.c
+++ b/fs/fuse/dev.c
@@ -1100,14 +1100,18 @@ static int fuse_copy_folio(struct fuse_copy_state *cs, struct folio **foliop,
 	struct folio *folio = *foliop;
 	size_t size;
 
-	if (folio) {
-		size = folio_size(folio);
-		if (zeroing && count < size)
-			folio_zero_range(folio, 0, size);
-	}
+	if (WARN_ON(!folio))
+		return 0;
+
+	size = folio_size(folio);
+	if (zeroing && count < size)
+		folio_zero_range(folio, 0, size);
 
 	while (count) {
-		if (cs->write && cs->pipebufs && folio) {
+		void *mapaddr, *buf;
+		unsigned int copy, bytes_copied;
+
+		if (cs->write && cs->pipebufs) {
 			/*
 			 * Can't control lifetime of pipe buffers, so always
 			 * copy user pages.
@@ -1120,8 +1124,7 @@ static int fuse_copy_folio(struct fuse_copy_state *cs, struct folio **foliop,
 				return fuse_ref_folio(cs, folio, offset, count);
 			}
 		} else if (!cs->len) {
-			if (cs->move_folios && folio &&
-			    offset == 0 && count == size) {
+			if (cs->move_folios && offset == 0 && count == size) {
 				err = fuse_try_move_folio(cs, foliop);
 				if (err <= 0)
 					return err;
@@ -1131,23 +1134,20 @@ static int fuse_copy_folio(struct fuse_copy_state *cs, struct folio **foliop,
 					return err;
 			}
 		}
-		if (folio) {
-			void *mapaddr = kmap_local_folio(folio, offset);
-			void *buf = mapaddr;
-			unsigned int copy = count;
-			unsigned int bytes_copied;
-
-			if (folio_test_highmem(folio) && count > PAGE_SIZE - offset_in_page(offset))
-				copy = PAGE_SIZE - offset_in_page(offset);
-
-			bytes_copied = fuse_copy_do(cs, &buf, &copy);
-			kunmap_local(mapaddr);
-			offset += bytes_copied;
-			count -= bytes_copied;
-		} else
-			offset += fuse_copy_do(cs, NULL, &count);
-	}
-	if (folio && !cs->write)
+
+		mapaddr = kmap_local_folio(folio, offset);
+		buf = mapaddr;
+		copy = count;
+
+		if (folio_test_highmem(folio) && count > PAGE_SIZE - offset_in_page(offset))
+			copy = PAGE_SIZE - offset_in_page(offset);
+
+		bytes_copied = fuse_copy_do(cs, &buf, &copy);
+		kunmap_local(mapaddr);
+		offset += bytes_copied;
+		count -= bytes_copied;
+	}
+	if (!cs->write)
 		flush_dcache_folio(folio);
 	return 0;
 }
-- 
2.47.1

Re: [PATCH 2/2] fuse: clean up null folio check in fuse_copy_folio()

[Miklos Szeredi]

On Fri, 23 May 2025 at 20:18, Joanne Koong <joannelkoong@gmail.com> wrote:
>
> In fuse_copy_folio(), the folio in *foliop will never be null.
> fuse_copy_folio() is called from two places, fuse_copy_folios() and
> fuse_notify_store(). In fuse_copy_folios(), the folio will never be null
> since ap->num_folios always reflects how many folios are stored in the
> ap->folios[] array.

Hmm, well, did you verify that none of the callers leave any holes?
ISTR there was a reason to put the NULL check in there, I just don't
remember what that reason was.

Thanks,
Miklos

Re: [PATCH 2/2] fuse: clean up null folio check in fuse_copy_folio()

[Joanne Koong]

On Tue, Jul 1, 2025 at 10:34 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Fri, 23 May 2025 at 20:18, Joanne Koong <joannelkoong@gmail.com> wrote:
> >
> > In fuse_copy_folio(), the folio in *foliop will never be null.
> > fuse_copy_folio() is called from two places, fuse_copy_folios() and
> > fuse_notify_store(). In fuse_copy_folios(), the folio will never be null
> > since ap->num_folios always reflects how many folios are stored in the
> > ap->folios[] array.
>
> Hmm, well, did you verify that none of the callers leave any holes?
> ISTR there was a reason to put the NULL check in there, I just don't
> remember what that reason was.

I audited the places where ap->num_folios gets set or incremented and
didn't see any place where there wasn't also an
ap->folios[ap->num_folios] assignment preceding it.

I'm fine with dropping this patch if you would rather the NULL check
be left in there.

Thanks,
Joanne

>
> Thanks,
> Miklos