[PATCH v2] fuse: keep inode->i_blkbits constant

[PATCH v2] fuse: keep inode->i_blkbits constant

[Joanne Koong]

With fuse now using iomap for writeback handling, inode blkbits changes
are problematic because iomap relies on inode->i_blkbits for its
internal bitmap logic. Currently we change inode->i_blkbits in fuse to
match the attr->blksize value passed in by the server.

This commit keeps inode->i_blkbits constant in fuse. Any attr->blksize
values passed in by the server will not update inode->i_blkbits. The
client-side behavior for stat is unaffected, stat will still reflect the
blocksize passed in by the server.

Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
Fixes: ef7e7cbb32 ("fuse: use iomap for writeback")
---
Changelog:
v1: https://lore.kernel.org/linux-fsdevel/20250804210743.1239373-1-joannelkoong@gmail.com/#t
v1 -> v2:
  * Remove warning and keep stat() behavior unchanged (Miklos)
  * Dropped 2nd patch
---
 fs/fuse/inode.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index bfe8d8af46f3..33632c32ba6c 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -285,11 +285,6 @@ void fuse_change_attributes_common(struct inode *inode, struct fuse_attr *attr,
 		}
 	}
 
-	if (attr->blksize != 0)
-		inode->i_blkbits = ilog2(attr->blksize);
-	else
-		inode->i_blkbits = inode->i_sb->s_blocksize_bits;
-
 	/*
 	 * Don't set the sticky bit in i_mode, unless we want the VFS
 	 * to check permissions.  This prevents failures due to the
-- 
2.47.3

Re: [PATCH v2] fuse: keep inode->i_blkbits constant

[Christian Brauner]

On Thu, 07 Aug 2025 10:50:15 -0700, Joanne Koong wrote:
> With fuse now using iomap for writeback handling, inode blkbits changes
> are problematic because iomap relies on inode->i_blkbits for its
> internal bitmap logic. Currently we change inode->i_blkbits in fuse to
> match the attr->blksize value passed in by the server.
> 
> This commit keeps inode->i_blkbits constant in fuse. Any attr->blksize
> values passed in by the server will not update inode->i_blkbits. The
> client-side behavior for stat is unaffected, stat will still reflect the
> blocksize passed in by the server.
> 
> [...]

Applied to the vfs.fixes branch of the vfs/vfs.git tree.
Patches in the vfs.fixes branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs.fixes

[1/1] fuse: keep inode->i_blkbits constant
      https://git.kernel.org/vfs/vfs/c/0110b6d5545d

Re: [PATCH v2] fuse: keep inode->i_blkbits constant

[Miklos Szeredi]

On Thu, 7 Aug 2025 at 19:51, Joanne Koong <joannelkoong@gmail.com> wrote:
>
> With fuse now using iomap for writeback handling, inode blkbits changes
> are problematic because iomap relies on inode->i_blkbits for its
> internal bitmap logic. Currently we change inode->i_blkbits in fuse to
> match the attr->blksize value passed in by the server.
>
> This commit keeps inode->i_blkbits constant in fuse. Any attr->blksize
> values passed in by the server will not update inode->i_blkbits. The
> client-side behavior for stat is unaffected, stat will still reflect the
> blocksize passed in by the server.

Not quite.  You also need to save ilog2(attr->blksize) in
fi->orig_i_blkbits and restore it after calling generic_fillattr() in
fuse_update_get_attr() just like it's done for i_mode and i_ino.

Thanks,
Miklos

Re: [PATCH v2] fuse: keep inode->i_blkbits constant

[Joanne Koong]

On Tue, Aug 12, 2025 at 1:27 AM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Thu, 7 Aug 2025 at 19:51, Joanne Koong <joannelkoong@gmail.com> wrote:
> >
> > With fuse now using iomap for writeback handling, inode blkbits changes
> > are problematic because iomap relies on inode->i_blkbits for its
> > internal bitmap logic. Currently we change inode->i_blkbits in fuse to
> > match the attr->blksize value passed in by the server.
> >
> > This commit keeps inode->i_blkbits constant in fuse. Any attr->blksize
> > values passed in by the server will not update inode->i_blkbits. The
> > client-side behavior for stat is unaffected, stat will still reflect the
> > blocksize passed in by the server.
>
> Not quite.  You also need to save ilog2(attr->blksize) in
> fi->orig_i_blkbits and restore it after calling generic_fillattr() in
> fuse_update_get_attr() just like it's done for i_mode and i_ino.
>

Oh I see, thanks. I'll resubmit this. And will do the same logic for
the fuseblk i_blkbits patch I submitted yesterday.

Thanks,
Joanne

> Thanks,
> Miklos

Re: [PATCH v2] fuse: keep inode->i_blkbits constant

[Darrick J. Wong]

On Tue, Aug 12, 2025 at 10:27:41AM +0200, Miklos Szeredi wrote:
> On Thu, 7 Aug 2025 at 19:51, Joanne Koong <joannelkoong@gmail.com> wrote:
> >
> > With fuse now using iomap for writeback handling, inode blkbits changes
> > are problematic because iomap relies on inode->i_blkbits for its
> > internal bitmap logic. Currently we change inode->i_blkbits in fuse to
> > match the attr->blksize value passed in by the server.
> >
> > This commit keeps inode->i_blkbits constant in fuse. Any attr->blksize
> > values passed in by the server will not update inode->i_blkbits. The
> > client-side behavior for stat is unaffected, stat will still reflect the
> > blocksize passed in by the server.
> 
> Not quite.  You also need to save ilog2(attr->blksize) in
> fi->orig_i_blkbits and restore it after calling generic_fillattr() in
> fuse_update_get_attr() just like it's done for i_mode and i_ino.

Why is that?  Is the goal here that fstat() should always return the
most recent blocksize the fuse server reported, even if the pagecache
accounting still uses the blocksize first reported when the kernel
created the incore struct inode?

--D

Re: [PATCH v2] fuse: keep inode->i_blkbits constant

[Joanne Koong]

On Tue, Aug 12, 2025 at 12:59 PM Darrick J. Wong <djwong@kernel.org> wrote:
>
> On Tue, Aug 12, 2025 at 10:27:41AM +0200, Miklos Szeredi wrote:
> > On Thu, 7 Aug 2025 at 19:51, Joanne Koong <joannelkoong@gmail.com> wrote:
> > >
> > > With fuse now using iomap for writeback handling, inode blkbits changes
> > > are problematic because iomap relies on inode->i_blkbits for its
> > > internal bitmap logic. Currently we change inode->i_blkbits in fuse to
> > > match the attr->blksize value passed in by the server.
> > >
> > > This commit keeps inode->i_blkbits constant in fuse. Any attr->blksize
> > > values passed in by the server will not update inode->i_blkbits. The
> > > client-side behavior for stat is unaffected, stat will still reflect the
> > > blocksize passed in by the server.
> >
> > Not quite.  You also need to save ilog2(attr->blksize) in
> > fi->orig_i_blkbits and restore it after calling generic_fillattr() in
> > fuse_update_get_attr() just like it's done for i_mode and i_ino.
>
> Why is that?  Is the goal here that fstat() should always return the
> most recent blocksize the fuse server reported, even if the pagecache
> accounting still uses the blocksize first reported when the kernel
> created the incore struct inode?

My understanding is that it's because in that path it uses cached stat
values instead of fetching with another statx call to the server so it
has to reflect the blocksize the server previously set. It took me a
while to realize that the blocksize the server reports to the client
is unrelated to whatever blocksize the kernel internally uses for the
inode since the kernel doesn't do any block i/o for fuse; the commit
message in commit 0e9663ee452ff ("fuse: add blksize field to
fuse_attr") says the blocksize attribute is if "the filesystem might
want to give a hint to the app about the optimal I/O size".


Thanks,
Joanne

>
> --D

Re: [PATCH v2] fuse: keep inode->i_blkbits constant

[Miklos Szeredi]

On Tue, 12 Aug 2025 at 22:44, Joanne Koong <joannelkoong@gmail.com> wrote:

> My understanding is that it's because in that path it uses cached stat
> values instead of fetching with another statx call to the server so it
> has to reflect the blocksize the server previously set. It took me a
> while to realize that the blocksize the server reports to the client
> is unrelated to whatever blocksize the kernel internally uses for the
> inode since the kernel doesn't do any block i/o for fuse; the commit
> message in commit 0e9663ee452ff ("fuse: add blksize field to
> fuse_attr") says the blocksize attribute is if "the filesystem might
> want to give a hint to the app about the optimal I/O size".

Right, that's what POSIX says:

  st_blksize A file system-specific preferred I/O block size for
                       this object. In some file system types, this may
                       vary from file to file.

Thanks,
Miklos

Re: [PATCH v2] fuse: keep inode->i_blkbits constant

[Darrick J. Wong]

On Wed, Aug 13, 2025 at 10:24:36AM +0200, Miklos Szeredi wrote:
> On Tue, 12 Aug 2025 at 22:44, Joanne Koong <joannelkoong@gmail.com> wrote:
> 
> > My understanding is that it's because in that path it uses cached stat
> > values instead of fetching with another statx call to the server so it
> > has to reflect the blocksize the server previously set. It took me a
> > while to realize that the blocksize the server reports to the client
> > is unrelated to whatever blocksize the kernel internally uses for the
> > inode since the kernel doesn't do any block i/o for fuse; the commit
> > message in commit 0e9663ee452ff ("fuse: add blksize field to
> > fuse_attr") says the blocksize attribute is if "the filesystem might
> > want to give a hint to the app about the optimal I/O size".
> 
> Right, that's what POSIX says:
> 
>   st_blksize A file system-specific preferred I/O block size for
>                        this object. In some file system types, this may
>                        vary from file to file.

Ahahaha yes, I forgot about that.  Regular filesystems can set i_blkbits
to whatever granularity they want and it persists until eviction, and if
they want to trick userspace they set st_blksize to something else.
Kind of like how XFS rounds that up to PAGE_SIZE for 1k fsblock
filesystems.

Now I see what Joanne meant about it taking a while to wrap her head
around i_blkbits vs. st_blksize.  Thanks for pointing that out. :)

--D

> Thanks,
> Miklos
>