Re: [Cluster-devel] [PATCH v6 2/7] fs: add infrastructure for multigrain timestamps
On Wed, 2023-08-02 at 21:35 +0200, Jan Kara wrote:
> On Tue 25-07-23 10:58:15, Jeff Layton wrote:
> > The VFS always uses coarse-grained timestamps when updating the ctime
> > and mtime after a change. This has the benefit of allowing filesystems
> > to optimize away a lot metadata updates, down to around 1 per jiffy,
> > even when a file is under heavy writes.
> >
> > Unfortunately, this has always been an issue when we're exporting via
> > NFSv3, which relies on timestamps to validate caches. A lot of changes
> > can happen in a jiffy, so timestamps aren't sufficient to help the
> > client decide to invalidate the cache. Even with NFSv4, a lot of
> > exported filesystems don't properly support a change attribute and are
> > subject to the same problems with timestamp granularity. Other
> > applications have similar issues with timestamps (e.g backup
> > applications).
> >
> > If we were to always use fine-grained timestamps, that would improve the
> > situation, but that becomes rather expensive, as the underlying
> > filesystem would have to log a lot more metadata updates.
> >
> > What we need is a way to only use fine-grained timestamps when they are
> > being actively queried.
> >
> > POSIX generally mandates that when the the mtime changes, the ctime must
> > also change. The kernel always stores normalized ctime values, so only
> > the first 30 bits of the tv_nsec field are ever used.
> >
> > Use the 31st bit of the ctime tv_nsec field to indicate that something
> > has queried the inode for the mtime or ctime. When this flag is set,
> > on the next mtime or ctime update, the kernel will fetch a fine-grained
> > timestamp instead of the usual coarse-grained one.
> >
> > Filesytems can opt into this behavior by setting the FS_MGTIME flag in
> > the fstype. Filesystems that don't set this flag will continue to use
> > coarse-grained timestamps.
> >
> > Later patches will convert individual filesystems to use the new
> > infrastructure.
> >
> > Signed-off-by: Jeff Layton
> > ---
> > fs/inode.c | 98
> > ++
> > fs/stat.c | 41 +--
> > include/linux/fs.h | 45 +++--
> > 3 files changed, 151 insertions(+), 33 deletions(-)
> >
> > diff --git a/fs/inode.c b/fs/inode.c
> > index d4ab92233062..369621e7faf5 100644
> > --- a/fs/inode.c
> > +++ b/fs/inode.c
> > @@ -1919,6 +1919,21 @@ int inode_update_time(struct inode *inode, struct
> > timespec64 *time, int flags)
> > }
> > EXPORT_SYMBOL(inode_update_time);
> >
> > +/**
> > + * current_coarse_time - Return FS time
> > + * @inode: inode.
> > + *
> > + * Return the current coarse-grained time truncated to the time
> > + * granularity supported by the fs.
> > + */
> > +static struct timespec64 current_coarse_time(struct inode *inode)
> > +{
> > + struct timespec64 now;
> > +
> > + ktime_get_coarse_real_ts64(&now);
> > + return timestamp_truncate(now, inode);
> > +}
> > +
> > /**
> > * atime_needs_update - update the access time
> > * @path: the &struct path to update
> > @@ -1952,7 +1967,7 @@ bool atime_needs_update(const struct path *path,
> > struct inode *inode)
> > if ((mnt->mnt_flags & MNT_NODIRATIME) && S_ISDIR(inode->i_mode))
> > return false;
> >
> > - now = current_time(inode);
> > + now = current_coarse_time(inode);
> >
> > if (!relatime_need_update(mnt, inode, now))
> > return false;
> > @@ -1986,7 +2001,7 @@ void touch_atime(const struct path *path)
> > * We may also fail on filesystems that have the ability to make parts
> > * of the fs read only, e.g. subvolumes in Btrfs.
> > */
> > - now = current_time(inode);
> > + now = current_coarse_time(inode);
> > inode_update_time(inode, &now, S_ATIME);
> > __mnt_drop_write(mnt);
> > skip_update:
>
> There are also calls in fs/smb/client/file.c:cifs_readpage_worker() and in
> fs/ocfs2/file.c:ocfs2_update_inode_atime() that should probably use
> current_coarse_time() to avoid needless querying of fine grained
> timestamps. But see below...
>
Technically, they already devolve to current_coarse_time anyway, but
changing them would allow them to skip the fstype flag check, but I like
your idea below better anyway.
> > @@ -2072,6 +2087,56 @@ int file_remove_privs(struct file *file)
> > }
> > EXPORT_SYMBOL(file_remove_privs);
> >
> > +/**
> > + * current_mgtime - Return FS time (possibly fine-grained)
> > + * @inode: inode.
> > + *
> > + * Return the current time truncated to the time granularity supported by
> > + * the fs, as suitable for a ctime/mtime change. If the ctime is flagged
> > + * as having been QUERIED, get a fine-grained timestamp.
> > + */
> > +static struct timespec64 current_mgtime(struct inode *inode)
> > +{
> > + struct timespec64 now;
> > + atomic_long_t *pnsec = (atomic_long_t *)&inode->__i_ctime.tv_nsec;
> > + long nsec = atomic_long_read(pnsec);
> > +
> > + if (
Re: [Cluster-devel] [PATCH v6 6/7] ext4: switch to multigrain timestamps
On Tue 25-07-23 10:58:19, Jeff Layton wrote:
> Enable multigrain timestamps, which should ensure that there is an
> apparent change to the timestamp whenever it has been written after
> being actively observed via getattr.
>
> For ext4, we only need to enable the FS_MGTIME flag.
>
> Signed-off-by: Jeff Layton
Looks good. Feel free to add:
Reviewed-by: Jan Kara
Honza
> ---
> fs/ext4/super.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index b54c70e1a74e..cb1ff47af156 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -7279,7 +7279,7 @@ static struct file_system_type ext4_fs_type = {
> .init_fs_context= ext4_init_fs_context,
> .parameters = ext4_param_specs,
> .kill_sb= kill_block_super,
> - .fs_flags = FS_REQUIRES_DEV | FS_ALLOW_IDMAP,
> + .fs_flags = FS_REQUIRES_DEV | FS_ALLOW_IDMAP | FS_MGTIME,
> };
> MODULE_ALIAS_FS("ext4");
>
>
> --
> 2.41.0
>
--
Jan Kara
SUSE Labs, CR
Re: [Cluster-devel] [PATCH v6 4/7] tmpfs: add support for multigrain timestamps
On Tue 25-07-23 10:58:17, Jeff Layton wrote:
> Enable multigrain timestamps, which should ensure that there is an
> apparent change to the timestamp whenever it has been written after
> being actively observed via getattr.
>
> tmpfs only requires the FS_MGTIME flag.
>
> Signed-off-by: Jeff Layton
Looks good. Feel free to add:
Reviewed-by: Jan Kara
Honza
> ---
> mm/shmem.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mm/shmem.c b/mm/shmem.c
> index 654d9a585820..b6019c905058 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -4264,7 +4264,7 @@ static struct file_system_type shmem_fs_type = {
> #endif
> .kill_sb= kill_litter_super,
> #ifdef CONFIG_SHMEM
> - .fs_flags = FS_USERNS_MOUNT | FS_ALLOW_IDMAP,
> + .fs_flags = FS_USERNS_MOUNT | FS_ALLOW_IDMAP | FS_MGTIME,
> #else
> .fs_flags = FS_USERNS_MOUNT,
> #endif
>
> --
> 2.41.0
>
--
Jan Kara
SUSE Labs, CR
Re: [Cluster-devel] [PATCH v6 2/7] fs: add infrastructure for multigrain timestamps
On Tue 25-07-23 10:58:15, Jeff Layton wrote:
> The VFS always uses coarse-grained timestamps when updating the ctime
> and mtime after a change. This has the benefit of allowing filesystems
> to optimize away a lot metadata updates, down to around 1 per jiffy,
> even when a file is under heavy writes.
>
> Unfortunately, this has always been an issue when we're exporting via
> NFSv3, which relies on timestamps to validate caches. A lot of changes
> can happen in a jiffy, so timestamps aren't sufficient to help the
> client decide to invalidate the cache. Even with NFSv4, a lot of
> exported filesystems don't properly support a change attribute and are
> subject to the same problems with timestamp granularity. Other
> applications have similar issues with timestamps (e.g backup
> applications).
>
> If we were to always use fine-grained timestamps, that would improve the
> situation, but that becomes rather expensive, as the underlying
> filesystem would have to log a lot more metadata updates.
>
> What we need is a way to only use fine-grained timestamps when they are
> being actively queried.
>
> POSIX generally mandates that when the the mtime changes, the ctime must
> also change. The kernel always stores normalized ctime values, so only
> the first 30 bits of the tv_nsec field are ever used.
>
> Use the 31st bit of the ctime tv_nsec field to indicate that something
> has queried the inode for the mtime or ctime. When this flag is set,
> on the next mtime or ctime update, the kernel will fetch a fine-grained
> timestamp instead of the usual coarse-grained one.
>
> Filesytems can opt into this behavior by setting the FS_MGTIME flag in
> the fstype. Filesystems that don't set this flag will continue to use
> coarse-grained timestamps.
>
> Later patches will convert individual filesystems to use the new
> infrastructure.
>
> Signed-off-by: Jeff Layton
> ---
> fs/inode.c | 98
> ++
> fs/stat.c | 41 +--
> include/linux/fs.h | 45 +++--
> 3 files changed, 151 insertions(+), 33 deletions(-)
>
> diff --git a/fs/inode.c b/fs/inode.c
> index d4ab92233062..369621e7faf5 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -1919,6 +1919,21 @@ int inode_update_time(struct inode *inode, struct
> timespec64 *time, int flags)
> }
> EXPORT_SYMBOL(inode_update_time);
>
> +/**
> + * current_coarse_time - Return FS time
> + * @inode: inode.
> + *
> + * Return the current coarse-grained time truncated to the time
> + * granularity supported by the fs.
> + */
> +static struct timespec64 current_coarse_time(struct inode *inode)
> +{
> + struct timespec64 now;
> +
> + ktime_get_coarse_real_ts64(&now);
> + return timestamp_truncate(now, inode);
> +}
> +
> /**
> * atime_needs_update - update the access time
> * @path: the &struct path to update
> @@ -1952,7 +1967,7 @@ bool atime_needs_update(const struct path *path, struct
> inode *inode)
> if ((mnt->mnt_flags & MNT_NODIRATIME) && S_ISDIR(inode->i_mode))
> return false;
>
> - now = current_time(inode);
> + now = current_coarse_time(inode);
>
> if (!relatime_need_update(mnt, inode, now))
> return false;
> @@ -1986,7 +2001,7 @@ void touch_atime(const struct path *path)
>* We may also fail on filesystems that have the ability to make parts
>* of the fs read only, e.g. subvolumes in Btrfs.
>*/
> - now = current_time(inode);
> + now = current_coarse_time(inode);
> inode_update_time(inode, &now, S_ATIME);
> __mnt_drop_write(mnt);
> skip_update:
There are also calls in fs/smb/client/file.c:cifs_readpage_worker() and in
fs/ocfs2/file.c:ocfs2_update_inode_atime() that should probably use
current_coarse_time() to avoid needless querying of fine grained
timestamps. But see below...
> @@ -2072,6 +2087,56 @@ int file_remove_privs(struct file *file)
> }
> EXPORT_SYMBOL(file_remove_privs);
>
> +/**
> + * current_mgtime - Return FS time (possibly fine-grained)
> + * @inode: inode.
> + *
> + * Return the current time truncated to the time granularity supported by
> + * the fs, as suitable for a ctime/mtime change. If the ctime is flagged
> + * as having been QUERIED, get a fine-grained timestamp.
> + */
> +static struct timespec64 current_mgtime(struct inode *inode)
> +{
> + struct timespec64 now;
> + atomic_long_t *pnsec = (atomic_long_t *)&inode->__i_ctime.tv_nsec;
> + long nsec = atomic_long_read(pnsec);
> +
> + if (nsec & I_CTIME_QUERIED) {
> + ktime_get_real_ts64(&now);
> + } else {
> + struct timespec64 ctime;
> +
> + ktime_get_coarse_real_ts64(&now);
> +
> + /*
> + * If we've recently fetched a fine-grained timestamp
> + * then the coarse-grained one may still be earlier than the
> + * existing one. Just keep the existing ctime if
Re: [Cluster-devel] [PATCH v6 5/7] xfs: switch to multigrain timestamps
On Wed, 2023-08-02 at 10:48 -0700, Darrick J. Wong wrote: > On Tue, Jul 25, 2023 at 10:58:18AM -0400, Jeff Layton wrote: > > Enable multigrain timestamps, which should ensure that there is an > > apparent change to the timestamp whenever it has been written after > > being actively observed via getattr. > > > > Also, anytime the mtime changes, the ctime must also change, and those > > are now the only two options for xfs_trans_ichgtime. Have that function > > unconditionally bump the ctime, and ASSERT that XFS_ICHGTIME_CHG is > > always set. > > > > Signed-off-by: Jeff Layton > > --- > > fs/xfs/libxfs/xfs_trans_inode.c | 6 +++--- > > fs/xfs/xfs_iops.c | 4 ++-- > > fs/xfs/xfs_super.c | 2 +- > > 3 files changed, 6 insertions(+), 6 deletions(-) > > > > diff --git a/fs/xfs/libxfs/xfs_trans_inode.c > > b/fs/xfs/libxfs/xfs_trans_inode.c > > index 6b2296ff248a..ad22656376d3 100644 > > --- a/fs/xfs/libxfs/xfs_trans_inode.c > > +++ b/fs/xfs/libxfs/xfs_trans_inode.c > > @@ -62,12 +62,12 @@ xfs_trans_ichgtime( > > ASSERT(tp); > > ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL)); > > > > - tv = current_time(inode); > > + /* If the mtime changes, then ctime must also change */ > > + ASSERT(flags & XFS_ICHGTIME_CHG); > > > > + tv = inode_set_ctime_current(inode); > > if (flags & XFS_ICHGTIME_MOD) > > inode->i_mtime = tv; > > - if (flags & XFS_ICHGTIME_CHG) > > - inode_set_ctime_to_ts(inode, tv); > > if (flags & XFS_ICHGTIME_CREATE) > > ip->i_crtime = tv; > > } > > diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c > > index 3a9363953ef2..3f89ef5a2820 100644 > > --- a/fs/xfs/xfs_iops.c > > +++ b/fs/xfs/xfs_iops.c > > @@ -573,10 +573,10 @@ xfs_vn_getattr( > > stat->gid = vfsgid_into_kgid(vfsgid); > > stat->ino = ip->i_ino; > > stat->atime = inode->i_atime; > > - stat->mtime = inode->i_mtime; > > - stat->ctime = inode_get_ctime(inode); > > stat->blocks = XFS_FSB_TO_BB(mp, ip->i_nblocks + ip->i_delayed_blks); > > > > + fill_mg_cmtime(request_mask, inode, stat); > > Huh. I would've thought @stat would come first since that's what we're > acting upon, but ... eh. :) > > If everyone else is ok with the fill_mg_cmtime signature, > Acked-by: Darrick J. Wong > > Good point. We can change the signature. I think xfs is the only caller outside of the generic vfs right now, and it'd be best to do it now. Christian, would you prefer that I send an updated series, or patches on top of vfs.ctime that can be folded in? -- Jeff Layton
Re: [Cluster-devel] [PATCH v6 5/7] xfs: switch to multigrain timestamps
On Tue, Jul 25, 2023 at 10:58:18AM -0400, Jeff Layton wrote:
> Enable multigrain timestamps, which should ensure that there is an
> apparent change to the timestamp whenever it has been written after
> being actively observed via getattr.
>
> Also, anytime the mtime changes, the ctime must also change, and those
> are now the only two options for xfs_trans_ichgtime. Have that function
> unconditionally bump the ctime, and ASSERT that XFS_ICHGTIME_CHG is
> always set.
>
> Signed-off-by: Jeff Layton
> ---
> fs/xfs/libxfs/xfs_trans_inode.c | 6 +++---
> fs/xfs/xfs_iops.c | 4 ++--
> fs/xfs/xfs_super.c | 2 +-
> 3 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_trans_inode.c b/fs/xfs/libxfs/xfs_trans_inode.c
> index 6b2296ff248a..ad22656376d3 100644
> --- a/fs/xfs/libxfs/xfs_trans_inode.c
> +++ b/fs/xfs/libxfs/xfs_trans_inode.c
> @@ -62,12 +62,12 @@ xfs_trans_ichgtime(
> ASSERT(tp);
> ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
>
> - tv = current_time(inode);
> + /* If the mtime changes, then ctime must also change */
> + ASSERT(flags & XFS_ICHGTIME_CHG);
>
> + tv = inode_set_ctime_current(inode);
> if (flags & XFS_ICHGTIME_MOD)
> inode->i_mtime = tv;
> - if (flags & XFS_ICHGTIME_CHG)
> - inode_set_ctime_to_ts(inode, tv);
> if (flags & XFS_ICHGTIME_CREATE)
> ip->i_crtime = tv;
> }
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index 3a9363953ef2..3f89ef5a2820 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -573,10 +573,10 @@ xfs_vn_getattr(
> stat->gid = vfsgid_into_kgid(vfsgid);
> stat->ino = ip->i_ino;
> stat->atime = inode->i_atime;
> - stat->mtime = inode->i_mtime;
> - stat->ctime = inode_get_ctime(inode);
> stat->blocks = XFS_FSB_TO_BB(mp, ip->i_nblocks + ip->i_delayed_blks);
>
> + fill_mg_cmtime(request_mask, inode, stat);
Huh. I would've thought @stat would come first since that's what we're
acting upon, but ... eh. :)
If everyone else is ok with the fill_mg_cmtime signature,
Acked-by: Darrick J. Wong
--D
> +
> if (xfs_has_v3inodes(mp)) {
> if (request_mask & STATX_BTIME) {
> stat->result_mask |= STATX_BTIME;
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index 818510243130..4b10edb2c972 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -2009,7 +2009,7 @@ static struct file_system_type xfs_fs_type = {
> .init_fs_context= xfs_init_fs_context,
> .parameters = xfs_fs_parameters,
> .kill_sb= kill_block_super,
> - .fs_flags = FS_REQUIRES_DEV | FS_ALLOW_IDMAP,
> + .fs_flags = FS_REQUIRES_DEV | FS_ALLOW_IDMAP | FS_MGTIME,
> };
> MODULE_ALIAS_FS("xfs");
>
>
> --
> 2.41.0
>
Re: [Cluster-devel] [PATCH v6 1/7] fs: pass the request_mask to generic_fillattr
On Tue 25-07-23 10:58:14, Jeff Layton wrote:
> generic_fillattr just fills in the entire stat struct indiscriminately
> today, copying data from the inode. There is at least one attribute
> (STATX_CHANGE_COOKIE) that can have side effects when it is reported,
> and we're looking at adding more with the addition of multigrain
> timestamps.
>
> Add a request_mask argument to generic_fillattr and have most callers
> just pass in the value that is passed to getattr. Have other callers
> (e.g. ksmbd) just pass in STATX_BASIC_STATS. Also move the setting of
> STATX_CHANGE_COOKIE into generic_fillattr.
>
> Signed-off-by: Jeff Layton
Looks good. Feel free to add:
Reviewed-by: Jan Kara
Honza
> ---
> fs/9p/vfs_inode.c | 4 ++--
> fs/9p/vfs_inode_dotl.c | 4 ++--
> fs/afs/inode.c | 2 +-
> fs/btrfs/inode.c| 2 +-
> fs/ceph/inode.c | 2 +-
> fs/coda/inode.c | 3 ++-
> fs/ecryptfs/inode.c | 5 +++--
> fs/erofs/inode.c| 2 +-
> fs/exfat/file.c | 2 +-
> fs/ext2/inode.c | 2 +-
> fs/ext4/inode.c | 2 +-
> fs/f2fs/file.c | 2 +-
> fs/fat/file.c | 2 +-
> fs/fuse/dir.c | 2 +-
> fs/gfs2/inode.c | 2 +-
> fs/hfsplus/inode.c | 2 +-
> fs/kernfs/inode.c | 2 +-
> fs/libfs.c | 4 ++--
> fs/minix/inode.c| 2 +-
> fs/nfs/inode.c | 2 +-
> fs/nfs/namespace.c | 3 ++-
> fs/ntfs3/file.c | 2 +-
> fs/ocfs2/file.c | 2 +-
> fs/orangefs/inode.c | 2 +-
> fs/proc/base.c | 4 ++--
> fs/proc/fd.c| 2 +-
> fs/proc/generic.c | 2 +-
> fs/proc/proc_net.c | 2 +-
> fs/proc/proc_sysctl.c | 2 +-
> fs/proc/root.c | 3 ++-
> fs/smb/client/inode.c | 2 +-
> fs/smb/server/smb2pdu.c | 22 +++---
> fs/smb/server/vfs.c | 3 ++-
> fs/stat.c | 18 ++
> fs/sysv/itree.c | 3 ++-
> fs/ubifs/dir.c | 2 +-
> fs/udf/symlink.c| 2 +-
> fs/vboxsf/utils.c | 2 +-
> include/linux/fs.h | 2 +-
> mm/shmem.c | 2 +-
> 40 files changed, 70 insertions(+), 62 deletions(-)
>
> diff --git a/fs/9p/vfs_inode.c b/fs/9p/vfs_inode.c
> index 16d85e6033a3..d24d1f20e922 100644
> --- a/fs/9p/vfs_inode.c
> +++ b/fs/9p/vfs_inode.c
> @@ -1016,7 +1016,7 @@ v9fs_vfs_getattr(struct mnt_idmap *idmap, const struct
> path *path,
> p9_debug(P9_DEBUG_VFS, "dentry: %p\n", dentry);
> v9ses = v9fs_dentry2v9ses(dentry);
> if (v9ses->cache & (CACHE_META|CACHE_LOOSE)) {
> - generic_fillattr(&nop_mnt_idmap, inode, stat);
> + generic_fillattr(&nop_mnt_idmap, request_mask, inode, stat);
> return 0;
> } else if (v9ses->cache & CACHE_WRITEBACK) {
> if (S_ISREG(inode->i_mode)) {
> @@ -1037,7 +1037,7 @@ v9fs_vfs_getattr(struct mnt_idmap *idmap, const struct
> path *path,
> return PTR_ERR(st);
>
> v9fs_stat2inode(st, d_inode(dentry), dentry->d_sb, 0);
> - generic_fillattr(&nop_mnt_idmap, d_inode(dentry), stat);
> + generic_fillattr(&nop_mnt_idmap, request_mask, d_inode(dentry), stat);
>
> p9stat_free(st);
> kfree(st);
> diff --git a/fs/9p/vfs_inode_dotl.c b/fs/9p/vfs_inode_dotl.c
> index 464ea73d1bf8..8e8d5d2a13d8 100644
> --- a/fs/9p/vfs_inode_dotl.c
> +++ b/fs/9p/vfs_inode_dotl.c
> @@ -451,7 +451,7 @@ v9fs_vfs_getattr_dotl(struct mnt_idmap *idmap,
> p9_debug(P9_DEBUG_VFS, "dentry: %p\n", dentry);
> v9ses = v9fs_dentry2v9ses(dentry);
> if (v9ses->cache & (CACHE_META|CACHE_LOOSE)) {
> - generic_fillattr(&nop_mnt_idmap, inode, stat);
> + generic_fillattr(&nop_mnt_idmap, request_mask, inode, stat);
> return 0;
> } else if (v9ses->cache) {
> if (S_ISREG(inode->i_mode)) {
> @@ -476,7 +476,7 @@ v9fs_vfs_getattr_dotl(struct mnt_idmap *idmap,
> return PTR_ERR(st);
>
> v9fs_stat2inode_dotl(st, d_inode(dentry), 0);
> - generic_fillattr(&nop_mnt_idmap, d_inode(dentry), stat);
> + generic_fillattr(&nop_mnt_idmap, request_mask, d_inode(dentry), stat);
> /* Change block size to what the server returned */
> stat->blksize = st->st_blksize;
>
> diff --git a/fs/afs/inode.c b/fs/afs/inode.c
> index 6b636f43f548..1c794a1896aa 100644
> --- a/fs/afs/inode.c
> +++ b/fs/afs/inode.c
> @@ -773,7 +773,7 @@ int afs_getattr(struct mnt_idmap *idmap, const struct
> path *path,
>
> do {
> read_seqbegin_or_lock(&vnode->cb_lock, &seq);
> - generic_fillattr(&nop_mnt_idmap, inode, stat);
> + generic_fillattr(&nop_mnt_idmap, request_mask, inode, stat);
> if (test_bit(AFS_VNODE_SILLY_DELETED, &vnode->flags) &&
> stat->nlink > 0)
> stat->nlink -= 1;
> diff --git a/fs/btrfs/in
Re: [Cluster-devel] [bug report] gfs2: Use mapping->gfp_mask for metadata inodes
On Wed, Aug 2, 2023 at 9:35 AM Dan Carpenter wrote:
> Hello Andreas Gruenbacher,
>
> The patch 8f18190e3173: "gfs2: Use mapping->gfp_mask for metadata
> inodes" from Jul 26, 2023 (linux-next), leads to the following Smatch
> static checker warning:
>
> fs/gfs2/inode.c:286 gfs2_lookup_simple()
> error: 'inode' dereferencing possible ERR_PTR()
>
> fs/gfs2/inode.c
> 268 struct inode *gfs2_lookup_simple(struct inode *dip, const char *name)
> 269 {
> 270 struct qstr qstr;
> 271 struct inode *inode;
> 272 gfs2_str2qstr(&qstr, name);
> 273 inode = gfs2_lookupi(dip, &qstr, 1);
> 274 /* gfs2_lookupi has inconsistent callers: vfs
> 275 * related routines expect NULL for no entry found,
> 276 * gfs2_lookup_simple callers expect ENOENT
> 277 * and do not check for NULL.
>
> This comment is ancient. I think how gfs2_lookupi() works is that if
> there is an error it returns an error code, but if the file does not
> exist it returns NULL. (This is just based on vague assumptions about
> how mixed error pointer/NULL functions work).
>
> 278 */
> 279 if (inode == NULL)
> 280 return ERR_PTR(-ENOENT);
> 281
> 282 /*
> 283 * Must not call back into the filesystem when allocating
> 284 * pages in the metadata inode's address space.
> 285 */
> --> 286 mapping_set_gfp_mask(inode->i_mapping, GFP_NOFS);
>
> error pointer dereference
Ah, right, thanks for pointing this out. I've pushed a fix.
Andreas
> 287
> 288 return inode;
> 289 }
>
> regards,
> dan carpenter
[Cluster-devel] [syzbot] Monthly gfs2 report (Aug 2023)
Hello gfs2 maintainers/developers, This is a 31-day syzbot report for the gfs2 subsystem. All related reports/information can be found at: https://syzkaller.appspot.com/upstream/s/gfs2 During the period, 3 new issues were detected and 0 were fixed. In total, 18 issues are still open and 18 have been fixed so far. Some of the still happening issues: Ref Crashes Repro Title <1> 3678Yes WARNING in folio_account_dirtied https://syzkaller.appspot.com/bug?extid=8d1d62bfb63d6a480be1 <2> 2371Yes WARNING in __folio_mark_dirty (2) https://syzkaller.appspot.com/bug?extid=e14d6cd6ec241f507ba7 <3> 501 Yes kernel BUG in gfs2_glock_nq (2) https://syzkaller.appspot.com/bug?extid=70f4e455dee59ab40c80 <4> 71 Yes INFO: task hung in gfs2_gl_hash_clear (3) https://syzkaller.appspot.com/bug?extid=ed7d0f71a89e28557a77 <5> 52 Yes WARNING in gfs2_check_blk_type https://syzkaller.appspot.com/bug?extid=092b28923eb79e0f3c41 <6> 3 Yes BUG: unable to handle kernel NULL pointer dereference in gfs2_rgrp_dump https://syzkaller.appspot.com/bug?extid=da0fc229cc1ff4bb2e6d <7> 3 Yes BUG: unable to handle kernel NULL pointer dereference in gfs2_rindex_update https://syzkaller.appspot.com/bug?extid=2b32df23ff6b5b307565 <8> 1 Yes BUG: sleeping function called from invalid context in gfs2_make_fs_ro https://syzkaller.appspot.com/bug?extid=60369f4775c014dd1804 --- This report is generated by a bot. It may contain errors. See https://goo.gl/tpsmEJ for more information about syzbot. syzbot engineers can be reached at syzkal...@googlegroups.com. To disable reminders for individual bugs, reply with the following command: #syz set no-reminders To change bug's subsystems, reply with: #syz set subsystems: new-subsystem You may send multiple commands in a single email message.
[Cluster-devel] [bug report] gfs2: Use mapping->gfp_mask for metadata inodes
Hello Andreas Gruenbacher,
The patch 8f18190e3173: "gfs2: Use mapping->gfp_mask for metadata
inodes" from Jul 26, 2023 (linux-next), leads to the following Smatch
static checker warning:
fs/gfs2/inode.c:286 gfs2_lookup_simple()
error: 'inode' dereferencing possible ERR_PTR()
fs/gfs2/inode.c
268 struct inode *gfs2_lookup_simple(struct inode *dip, const char *name)
269 {
270 struct qstr qstr;
271 struct inode *inode;
272 gfs2_str2qstr(&qstr, name);
273 inode = gfs2_lookupi(dip, &qstr, 1);
274 /* gfs2_lookupi has inconsistent callers: vfs
275 * related routines expect NULL for no entry found,
276 * gfs2_lookup_simple callers expect ENOENT
277 * and do not check for NULL.
This comment is ancient. I think how gfs2_lookupi() works is that if
there is an error it returns an error code, but if the file does not
exist it returns NULL. (This is just based on vague assumptions about
how mixed error pointer/NULL functions work).
278 */
279 if (inode == NULL)
280 return ERR_PTR(-ENOENT);
281
282 /*
283 * Must not call back into the filesystem when allocating
284 * pages in the metadata inode's address space.
285 */
--> 286 mapping_set_gfp_mask(inode->i_mapping, GFP_NOFS);
error pointer dereference
287
288 return inode;
289 }
regards,
dan carpenter
