Re: [PATCH 4/7] fuse: implement file attributes mask for statx

["Darrick J. Wong" <djwong@kernel.org>]

On Mon, Aug 18, 2025 at 01:01:55PM -0700, Darrick J. Wong wrote:
> On Mon, Aug 18, 2025 at 05:11:07PM +0200, Miklos Szeredi wrote:
> > On Fri, 18 Jul 2025 at 01:27, Darrick J. Wong <djwong@kernel.org> wrote:
> > >
> > > From: Darrick J. Wong <djwong@kernel.org>
> > >
> > > Actually copy the attributes/attributes_mask from userspace.
> > >
> > > Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
> > > ---
> > >  fs/fuse/dir.c |    2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> > >
> > > diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> > > index 45b4c3cc1396af..4d841869ba3d0a 100644
> > > --- a/fs/fuse/dir.c
> > > +++ b/fs/fuse/dir.c
> > > @@ -1285,6 +1285,8 @@ static int fuse_do_statx(struct mnt_idmap *idmap, struct inode *inode,
> > >                 stat->result_mask = sx->mask & (STATX_BASIC_STATS | STATX_BTIME);
> > >                 stat->btime.tv_sec = sx->btime.tv_sec;
> > >                 stat->btime.tv_nsec = min_t(u32, sx->btime.tv_nsec, NSEC_PER_SEC - 1);
> > > +               stat->attributes = sx->attributes;
> > > +               stat->attributes_mask = sx->attributes_mask;
> > 
> > fuse_update_get_attr() has a cached and an uncached branch and these
> > fields are only getting set in the uncached case.
> 
> Hrmm, do you want to cache all the various statx attributes in struct
> fuse_inode?  Or would you rather that the kernel always call the fuse
> server if any of the statx flags outside of (BASIC_STATS|BTIME) are set?

I should have said explicitly that attributes/attributes_mask need to be
cached because there's no separate STATX_ request flag for the bitfield.

However, the *new* fields that have been added since BASIC_STATS are the
subject of my ramblings below.

--D

> Right now the full version of kstat_from_fuse_statx contains:
> 
> 	if (sx->mask & STATX_BTIME) {
> 		stat->btime.tv_sec = sx->btime.tv_sec;
> 		stat->btime.tv_nsec = min_t(u32, sx->btime.tv_nsec, NSEC_PER_SEC - 1);
> 	}
> 
> 	if (sx->mask & STATX_DIOALIGN) {
> 		stat->dio_mem_align = sx->dio_mem_align;
> 		stat->dio_offset_align = sx->dio_offset_align;
> 	}
> 
> 	if (sx->mask & STATX_SUBVOL)
> 		stat->subvol = sx->subvol;
> 
> 	if (sx->mask & STATX_WRITE_ATOMIC) {
> 		stat->atomic_write_unit_min = sx->atomic_write_unit_min;
> 		stat->atomic_write_unit_max = sx->atomic_write_unit_max;
> 		stat->atomic_write_unit_max_opt = sx->atomic_write_unit_max_opt;
> 		stat->atomic_write_segments_max = sx->atomic_write_segments_max;
> 	}
> 
> 	if (sx->mask & STATX_DIO_READ_ALIGN)
> 		stat->dio_read_offset_align = sx->dio_read_offset_align;
> 
> In theory only specialty programs are going to be interested in directio
> or atomic writes, and only userspace nfs servers and backup programs are
> going to care about subvolumes, so I don't know if it's really worth the
> trouble to cache all that.
> 
> The dio/atomic fields are 7x u32, and the subvol id is u64.  That's 40
> bytes per inode, which is kind of a lot.
> 
> --D
> 
> > Thanks,
> > Miklos
> > 
>