vfs/nfsd: clean up handling of the i_version counter
Subsystems (7)
Jeff Layton
maintainerv5 cover vfs/nfsd: clean up handling of the i_version counter
v5: don't try to expose i_version to userland (yet)
use getattr to get i_version in nfsd take inode lock when getting i_version
This set is a bit different from the earlier ones. In particular, it does not try to expose i_version to userland. STATX_INO_VERSION is added as a kernel-only symbol (for now), and the infrastructure is changed around to use it.
This allows us to fetch the i_version during the getattr operation, and eliminate the fetch_iversion export operation. This is a much more natual way to handle the i_version counter in nfsd and, as a bonus, this should allow nfsd to use ceph's i_version counter too. Huzzah!
The first two patches fix up ext4's i_version handling and enable it unconditionally. Those can go in independently of the rest, IMO. Ted, if you're ok with those and want to pick them up then that would be great.
The rest of the patches clean up the i_version handling, but should not change the userland API at all. This should make it very simple to expose i_version to userland in the future if we decide to do so. Maybe we should take these in via the nfsd tree? I'm not sure here.
The last patch should be considered an RFC. I think that that approach should prevent the potential race, without adding new work in the write codepath.
Jeff Layton (8):
iversion: clarify when the i_version counter must be updated
ext4: fix i_version handling in ext4
ext4: unconditionally enable the i_version counter
vfs: plumb i_version handling into struct kstat
nfs: report the inode version in getattr if requested
ceph: report the inode version in getattr if requested
nfsd: use the getattr operation to fetch i_version
nfsd: take inode_lock when querying for NFSv4 GETATTR
fs/ceph/inode.c | 14 +++++++++----- fs/ext4/inode.c | 15 +++++---------- fs/ext4/ioctl.c | 4 ++++ fs/ext4/move_extent.c | 6 ++++++ fs/ext4/super.c | 13 ++++--------- fs/ext4/xattr.c | 1 + fs/nfs/export.c | 7 ------- fs/nfs/inode.c | 7 +++++-- fs/nfsd/nfs4xdr.c | 17 ++++++++++++++++- fs/nfsd/nfsfh.c | 6 ++++++ fs/nfsd/nfsfh.h | 9 ++++----- fs/nfsd/vfs.h | 7 ++++++- fs/stat.c | 14 +++++++++++++- include/linux/exportfs.h | 1 - include/linux/iversion.h | 10 ++++++++-- include/linux/stat.h | 4 ++++ 16 files changed, 91 insertions(+), 44 deletions(-)
Signature · 1 line
2.37.3
Jeff Layton
maintainerPATCH v5 1/8 iversion: clarify when the i_version counter must be updated
The i_version field in the kernel has had different semantics over the decades, but NFSv4 has certain expectations. Update the comments in iversion.h to describe when the i_version must change.
--- include/linux/iversion.h | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
include/linux/iversion.h +8 -2
@@ -9,8 +9,14 @@* --------------------------- * The change attribute (i_version) is mandated by NFSv4 and is mostly for * knfsd, but is also used for other purposes (e.g. IMA). The i_version must -* appear different to observers if there was a change to the inode's data or -* metadata since it was last queried. +* appear larger to observers if there was an explicit change to the inode's +* data or metadata since it was last queried. +* +* An explicit change is one that would ordinarily result in a change to the +* inode status change time (aka ctime). i_version must appear to change, even +* if the ctime does not (since the whole point is to avoid missing updates due +* to timestamp granularity). If POSIX mandates that the ctime must change due +* to an operation, then the i_version counter must be incremented as well. * * Observers see the i_version as a 64-bit number that never decreases. If it * remains the same since it was last checked, then nothing has changed in the
Signature · 1 line
2.37.3
Jeff Layton
maintainerPATCH v5 2/8 ext4: fix i_version handling in ext4
In reply to Jeff Laytonext4 currently updates the i_version counter when the atime is updated during a read. This is less than ideal as it can cause unnecessary cache invalidations with NFSv4 and unnecessary remeasurements for IMA.
The increment in ext4_mark_iloc_dirty is also problematic since it can corrupt the i_version counter for ea_inodes. We aren't bumping the file times in ext4_mark_iloc_dirty, so changing the i_version there seems wrong, and is the cause of both problems.
Remove that callsite and add increments to the setattr, setxattr and ioctl codepaths, at the same times that we update the ctime. The i_version bump that already happens during timestamp updates should take care of the rest.
In ext4_move_extents, increment the i_version on both inodes, and also add in missing ctime updates.
--- fs/ext4/inode.c | 15 +++++---------- fs/ext4/ioctl.c | 8 ++++++++ fs/ext4/move_extent.c | 8 ++++++++ fs/ext4/xattr.c | 2 ++ 4 files changed, 23 insertions(+), 10 deletions(-)
fs/ext4/inode.c +5 -10
@@ -5342,6 +5342,7 @@ int ext4_setattr(struct user_namespace *mnt_userns, struct dentry *dentry,int error, rc =0 ;int orphan =0 ;const unsigned int ia_valid = attr->ia_valid; +bool inc_ivers =IS_I_VERSION (inode);if (unlikely (ext4_forced_shutdown (EXT4_SB (inode->i_sb))))return -EIO ;
@@ -5425,8 +5426,8 @@ int ext4_setattr(struct user_namespace *mnt_userns, struct dentry *dentry,return -EINVAL ; } -if (IS_I_VERSION (inode) && attr->ia_size != inode->i_size) -inode_inc_iversion (inode); +if (attr->ia_size == inode->i_size) + inc_ivers = false;if (shrink) {if (ext4_should_order_data (inode)) {
@@ -5528,6 +5529,8 @@ int ext4_setattr(struct user_namespace *mnt_userns, struct dentry *dentry, }if (!error) { +if (inc_ivers) +inode_inc_iversion (inode);setattr_copy (mnt_userns, inode, attr);mark_inode_dirty (inode); }
@@ -5731,14 +5734,6 @@ int ext4_mark_iloc_dirty(handle_t *handle, }ext4_fc_track_inode (handle, inode); -/* -* ea_inodes are using i_version for storing reference count, don't -* mess with it -*/ -if (IS_I_VERSION (inode) && - !(EXT4_I (inode)->i_flags &EXT4_EA_INODE_FL )) -inode_inc_iversion (inode); -/* the do_update_inode consumes one bh->b_count */ get_bh (iloc->bh);
fs/ext4/ioctl.c +8 -0
@@ -452,6 +452,8 @@ static long swap_inode_boot_loader(struct super_block *sb,swap_inode_data (inode, inode_bl); inode->i_ctime = inode_bl->i_ctime =current_time (inode); +if (IS_I_VERSION (inode)) +inode_inc_iversion (inode); inode->i_generation =prandom_u32 (); inode_bl->i_generation =prandom_u32 ();
@@ -665,6 +667,8 @@ static int ext4_ioctl_setflags(struct inode *inode,ext4_set_inode_flags (inode, false); inode->i_ctime =current_time (inode); +if (IS_I_VERSION (inode)) +inode_inc_iversion (inode); err =ext4_mark_iloc_dirty (handle, inode, &iloc); flags_err:
@@ -775,6 +779,8 @@ static int ext4_ioctl_setproject(struct inode *inode, __u32 projid)EXT4_I (inode)->i_projid = kprojid; inode->i_ctime =current_time (inode); +if (IS_I_VERSION (inode)) +inode_inc_iversion (inode); out_dirty: rc =ext4_mark_iloc_dirty (handle, inode, &iloc);if (!err)
@@ -1257,6 +1263,8 @@ static long __ext4_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) err =ext4_reserve_inode_write (handle, inode, &iloc);if (err ==0 ) { inode->i_ctime =current_time (inode); +if (IS_I_VERSION (inode)) +inode_inc_iversion (inode); inode->i_generation = generation; err =ext4_mark_iloc_dirty (handle, inode, &iloc); }
fs/ext4/move_extent.c +8 -0
@@ -6,6 +6,7 @@*/ #include <linux/fs.h> +#include <linux/iversion.h> #include <linux/quotaops.h> #include <linux/slab.h> #include <linux/sched/mm.h>
@@ -683,6 +684,13 @@ ext4_move_extents(struct file *o_filp, struct file *d_filp, __u64 orig_blk,break ; o_start += cur_len; d_start += cur_len; + + orig_inode->i_ctime =current_time (orig_inode); + donor_inode->i_ctime =current_time (donor_inode); +if (IS_I_VERSION (orig_inode)) +inode_inc_iversion (orig_inode); +if (IS_I_VERSION (donor_inode)) +inode_inc_iversion (donor_inode); } *moved_len = o_start - orig_blk;if (*moved_len > len)
fs/ext4/xattr.c +2 -0
@@ -2412,6 +2412,8 @@ ext4_xattr_set_handle(handle_t *handle, struct inode *inode, int name_index,if (!error) {ext4_xattr_update_super_block (handle, inode->i_sb); inode->i_ctime =current_time (inode); +if (IS_I_VERSION (inode)) +inode_inc_iversion (inode);if (!value) no_expand =0 ; error =ext4_mark_iloc_dirty (handle, inode, &is.iloc);
Signature · 1 line
2.37.3
Theodore Ts'o
maintainerRe: [PATCH v5 2/8] ext4: fix i_version handling in ext4
Quoted 14 lines · Jeff Layton
ext4 currently updates the
i_versioncounter when the atime is updated during a read. This is less than ideal as it can cause unnecessary cache invalidations with NFSv4 and unnecessary remeasurements for IMA.The increment in
ext4_mark_iloc_dirtyis also problematic since it can corrupt thei_versioncounter forea_inodes. We aren't bumping the file times inext4_mark_iloc_dirty, so changing thei_versionthere seems wrong, and is the cause of both problems.Remove that callsite and add increments to the setattr, setxattr and ioctl codepaths, at the same times that we update the ctime. The
i_versionbump that already happens during timestamp updates should take care of the rest.
Quoted 7 lines
In
ext4_move_extents, increment thei_versionon both inodes, and also add in missing ctime updates.Cc Lukas Czerner <[email protected]>Reviewed-by Jan Kara <[email protected]>Signed-off-by Jeff Layton <[email protected]>
Thanks, I've applied just this patch from this patch series. I made some minor adjustments since we've already enabled the i_version counter unconditionally via another patch series from Lukas.
- Ted
Jeff Layton
maintainerPATCH v5 3/8 ext4: unconditionally enable the i_version counter
In reply to Jeff LaytonThe original i_version implementation was pretty expensive, requiring a log flush on every change. Because of this, it was gated behind a mount option on ext4 (implemented via the MS_I_VERSION mountoption flag).
Commit ae5e165d855d (fs: new API for handling inode->i_version) made the i_version flag much less expensive, so there is no longer a performance penalty from enabling it. xfs and btrfs already enable it unconditionally when the on-disk format can support it.
Have ext4 ignore the SB_I_VERSION flag, and just enable it unconditionally. While we're in here, remove the handling of Opt_i_version as well since it's due for deprecation anyway.
Ideally, we'd couple this change with a way to disable the i_version counter (just in case), but the way the iversion mount option was implemented makes that difficult to do. We'd need to add a new mount option altogether or do something with tune2fs. That's probably best left to later patches if it turns out to be needed.
--- fs/ext4/inode.c | 2 +- fs/ext4/ioctl.c | 12 ++++-------- fs/ext4/move_extent.c | 6 ++---- fs/ext4/super.c | 13 ++++--------- fs/ext4/xattr.c | 3 +-- 5 files changed, 12 insertions(+), 24 deletions(-)
fs/ext4/inode.c +1 -1
@@ -5342,7 +5342,7 @@ int ext4_setattr(struct user_namespace *mnt_userns, struct dentry *dentry,int error, rc =0 ;int orphan =0 ;const unsigned int ia_valid = attr->ia_valid; -bool inc_ivers =IS_I_VERSION (inode); +bool inc_ivers = true;if (unlikely (ext4_forced_shutdown (EXT4_SB (inode->i_sb))))return -EIO ;
fs/ext4/ioctl.c +4 -8
@@ -452,8 +452,7 @@ static long swap_inode_boot_loader(struct super_block *sb,swap_inode_data (inode, inode_bl); inode->i_ctime = inode_bl->i_ctime =current_time (inode); -if (IS_I_VERSION (inode)) -inode_inc_iversion (inode); +inode_inc_iversion (inode); inode->i_generation =prandom_u32 (); inode_bl->i_generation =prandom_u32 ();
@@ -667,8 +666,7 @@ static int ext4_ioctl_setflags(struct inode *inode,ext4_set_inode_flags (inode, false); inode->i_ctime =current_time (inode); -if (IS_I_VERSION (inode)) -inode_inc_iversion (inode); +inode_inc_iversion (inode); err =ext4_mark_iloc_dirty (handle, inode, &iloc); flags_err:
@@ -779,8 +777,7 @@ static int ext4_ioctl_setproject(struct inode *inode, __u32 projid)EXT4_I (inode)->i_projid = kprojid; inode->i_ctime =current_time (inode); -if (IS_I_VERSION (inode)) -inode_inc_iversion (inode); +inode_inc_iversion (inode); out_dirty: rc =ext4_mark_iloc_dirty (handle, inode, &iloc);if (!err)
@@ -1263,8 +1260,7 @@ static long __ext4_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) err =ext4_reserve_inode_write (handle, inode, &iloc);if (err ==0 ) { inode->i_ctime =current_time (inode); -if (IS_I_VERSION (inode)) -inode_inc_iversion (inode); +inode_inc_iversion (inode); inode->i_generation = generation; err =ext4_mark_iloc_dirty (handle, inode, &iloc); }
fs/ext4/move_extent.c +2 -4
@@ -687,10 +687,8 @@ ext4_move_extents(struct file *o_filp, struct file *d_filp, __u64 orig_blk, orig_inode->i_ctime =current_time (orig_inode); donor_inode->i_ctime =current_time (donor_inode); -if (IS_I_VERSION (orig_inode)) -inode_inc_iversion (orig_inode); -if (IS_I_VERSION (donor_inode)) -inode_inc_iversion (donor_inode); +inode_inc_iversion (orig_inode); +inode_inc_iversion (donor_inode); } *moved_len = o_start - orig_blk;if (*moved_len > len)
fs/ext4/super.c +4 -9
@@ -1585,7 +1585,7 @@ enum { Opt_inlinecrypt, Opt_usrjquota, Opt_grpjquota, Opt_quota, Opt_noquota, Opt_barrier, Opt_nobarrier, Opt_err, - Opt_usrquota, Opt_grpquota, Opt_prjquota, Opt_i_version, + Opt_usrquota, Opt_grpquota, Opt_prjquota, Opt_dax, Opt_dax_always, Opt_dax_inode, Opt_dax_never, Opt_stripe, Opt_delalloc, Opt_nodelalloc, Opt_warn_on_error, Opt_nowarn_on_error, Opt_mblk_io_submit, Opt_debug_want_extra_isize,
@@ -1694,7 +1694,6 @@ static const struct fs_parameter_spec ext4_param_specs[] = {fsparam_flag ("barrier" , Opt_barrier),fsparam_u32 ("barrier" , Opt_barrier),fsparam_flag ("nobarrier" , Opt_nobarrier), -fsparam_flag ("i_version" , Opt_i_version),fsparam_flag ("dax" , Opt_dax),fsparam_enum ("dax" , Opt_dax_type, ext4_param_dax), fsparam_u32 ("stripe", Opt_stripe),
@@ -2140,11 +2139,6 @@ static int ext4_parse_param(struct fs_context *fc, struct fs_parameter *param)case Opt_abort:ctx_set_mount_flag (ctx,EXT4_MF_FS_ABORTED );return 0 ; -case Opt_i_version: -ext4_msg (NULL ,KERN_WARNING , deprecated_msg, param->key,"5.20" ); -ext4_msg (NULL ,KERN_WARNING ,"Use iversion instead\n" ); -ctx_set_flags (ctx,SB_I_VERSION ); -return 0 ;case Opt_inlinecrypt:#ifdef CONFIG_FS_ENCRYPTION_INLINE_CRYPT ctx_set_flags (ctx,SB_INLINECRYPT );
@@ -2970,8 +2964,6 @@ static int _ext4_show_options(struct seq_file *seq, struct super_block *sb,SEQ_OPTS_PRINT ("min_batch_time=%u" , sbi->s_min_batch_time);if (nodefs || sbi->s_max_batch_time !=EXT4_DEF_MAX_BATCH_TIME )SEQ_OPTS_PRINT ("max_batch_time=%u" , sbi->s_max_batch_time); -if (sb->s_flags &SB_I_VERSION ) -SEQ_OPTS_PUTS ("i_version" );if (nodefs || sbi->s_stripe)SEQ_OPTS_PRINT ("stripe=%lu" , sbi->s_stripe); if (nodefs || EXT4_MOUNT_DATA_FLAGS &
@@ -4640,6 +4632,9 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb) sb->s_flags = (sb->s_flags & ~SB_POSIXACL ) | (test_opt (sb,POSIX_ACL ) ?SB_POSIXACL :0 ); +/* i_version is always enabled now */ + sb->s_flags |=SB_I_VERSION ; + if (le32_to_cpu(es->s_rev_level) == EXT4_GOOD_OLD_REV && (ext4_has_compat_features(sb) || ext4_has_ro_compat_features(sb) ||
fs/ext4/xattr.c +1 -2
@@ -2412,8 +2412,7 @@ ext4_xattr_set_handle(handle_t *handle, struct inode *inode, int name_index,if (!error) {ext4_xattr_update_super_block (handle, inode->i_sb); inode->i_ctime =current_time (inode); -if (IS_I_VERSION (inode)) -inode_inc_iversion (inode); +inode_inc_iversion (inode);if (!value) no_expand =0 ; error =ext4_mark_iloc_dirty (handle, inode, &is.iloc);
Signature · 1 line
2.37.3
Jeff Layton
maintainerPATCH v5 4/8 vfs: plumb i_version handling into struct kstat
In reply to Jeff LaytonFrom: Jeff Layton <[email protected]>
The NFS server has a lot of special handling for different types of change attribute access, depending on what sort of inode we have. In most cases, it's doing a getattr anyway and then fetching that value after the fact.
Rather that do that, add a new STATX_INO_VERSION flag that is a kernel-only symbol (for now). If requested and getattr can implement it, it can fill out this field. For IS_I_VERSION inodes, add a generic implementation in vfs_getattr_nosec. Take care to mask STATX_INO_VERSION off in requests from userland and in the result mask.
Eventually if we decide to make this available to userland, we can just designate a field for it in struct statx, and move the STATX_INO_VERSION definition to the uapi header.
--- fs/stat.c | 14 +++++++++++++- include/linux/stat.h | 4 ++++ 2 files changed, 17 insertions(+), 1 deletion(-)
fs/stat.c +13 -1
@@ -17,6 +17,7 @@#include <linux/syscalls.h> #include <linux/pagemap.h> #include <linux/compat.h> +#include <linux/iversion.h> #include <linux/uaccess.h> #include <asm/unistd.h>
@@ -118,6 +119,11 @@ int vfs_getattr_nosec(const struct path *path, struct kstat *stat, stat->attributes_mask |= (STATX_ATTR_AUTOMOUNT |STATX_ATTR_DAX ); +if ((request_mask &STATX_INO_VERSION ) &&IS_I_VERSION (inode)) { + stat->result_mask |=STATX_INO_VERSION ; + stat->ino_version =inode_query_iversion (inode); + } + mnt_userns =mnt_user_ns (path->mnt);if (inode->i_op->getattr) return inode->i_op->getattr(mnt_userns, path, stat,
@@ -587,7 +593,8 @@ cp_statx(const struct kstat *stat, struct statx __user *buffer)memset (&tmp,0 ,sizeof (tmp)); - tmp.stx_mask = stat->result_mask; +/* STATX_INO_VERSION is kernel-only for now */ + tmp.stx_mask = stat->result_mask & ~STATX_INO_VERSION ; tmp.stx_blksize = stat->blksize; tmp.stx_attributes = stat->attributes; tmp.stx_nlink = stat->nlink;
@@ -626,6 +633,11 @@ int do_statx(int dfd, struct filename *filename, unsigned int flags,if ((flags &AT_STATX_SYNC_TYPE ) ==AT_STATX_SYNC_TYPE )return -EINVAL ; +/* STATX_INO_VERSION is kernel-only for now. Ignore requests +* from userland. +*/ + mask &= ~STATX_INO_VERSION ; + error =vfs_statx (dfd, filename, flags, &stat, mask);if (error)return error;
include/linux/stat.h +4 -0
@@ -50,6 +50,10 @@ struct kstat {struct timespec64 btime;/* File creation time */ u64 blocks;u64 mnt_id; + u64 ino_version; }; +/* This definition is internal to the kernel for now. Mainly used by nfsd */ +#define STATX_INO_VERSION 0x40000000U/* Want/got stx_change_attr */ +#endif
Signature · 1 line
2.37.3
Jeff Layton
maintainerPATCH v5 5/8 nfs: report the inode version in getattr if requested
In reply to Jeff LaytonAllow NFS to report the i_version in getattr requests. Since the cost to fetch it is relatively cheap, do it unconditionally and just set the flag if it looks like it's valid.
--- fs/nfs/inode.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
fs/nfs/inode.c +5 -2
@@ -830,6 +830,8 @@ static u32 nfs_get_valid_attrmask(struct inode *inode) reply_mask |=STATX_UID |STATX_GID ;if (!(cache_validity &NFS_INO_INVALID_BLOCKS )) reply_mask |=STATX_BLOCKS ; +if (!(cache_validity &NFS_INO_INVALID_CHANGE )) + reply_mask |=STATX_INO_VERSION ;return reply_mask; }
@@ -848,7 +850,7 @@ int nfs_getattr(struct user_namespace *mnt_userns, const struct path *path, request_mask &=STATX_TYPE |STATX_MODE |STATX_NLINK |STATX_UID |STATX_GID |STATX_ATIME |STATX_MTIME |STATX_CTIME | -STATX_INO |STATX_SIZE |STATX_BLOCKS ; +STATX_INO |STATX_SIZE |STATX_BLOCKS |STATX_INO_VERSION ;if ((query_flags &AT_STATX_DONT_SYNC ) && !force_sync) {if (readdirplus_enabled)
@@ -877,7 +879,7 @@ int nfs_getattr(struct user_namespace *mnt_userns, const struct path *path,/* Is the user requesting attributes that might need revalidation? */ if (!(request_mask & (STATX_MODE |STATX_NLINK |STATX_ATIME |STATX_CTIME |STATX_MTIME |STATX_UID |STATX_GID | -STATX_SIZE |STATX_BLOCKS ))) +STATX_SIZE |STATX_BLOCKS |STATX_INO_VERSION ))) goto out_no_revalidate;/* Check whether the cached attributes are stale */
@@ -915,6 +917,7 @@ int nfs_getattr(struct user_namespace *mnt_userns, const struct path *path,generic_fillattr (&init_user_ns, inode, stat); stat->ino =nfs_compat_user_ino64 (NFS_FILEID (inode)); + stat->ino_version =inode_peek_iversion_raw (inode);if (S_ISDIR (inode->i_mode)) stat->blksize =NFS_SERVER (inode)->dtsize; out:
Signature · 1 line
2.37.3
Jeff Layton
maintainerPATCH v5 6/8 ceph: report the inode version in getattr if requested
In reply to Jeff LaytonWhen getattr requests the STX_INO_VERSION, request the full gamut of caps (similarly to how ctime is handled). When the change attribute seems to be valid, return it in the ino_version field and set the flag in the reply mask.
--- fs/ceph/inode.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-)
fs/ceph/inode.c +9 -5
@@ -2415,10 +2415,10 @@ static int statx_to_caps(u32 want, umode_t mode) {int mask =0 ; -if (want & (STATX_MODE |STATX_UID |STATX_GID |STATX_CTIME |STATX_BTIME )) +if (want & (STATX_MODE |STATX_UID |STATX_GID |STATX_CTIME |STATX_BTIME |STATX_INO_VERSION )) mask |=CEPH_CAP_AUTH_SHARED ; -if (want & (STATX_NLINK |STATX_CTIME )) { +if (want & (STATX_NLINK |STATX_CTIME |STATX_INO_VERSION )) {/* * The link count for directories depends on inode->i_subdirs, * and that is only updated when Fs caps are held.
@@ -2429,11 +2429,10 @@ static int statx_to_caps(u32 want, umode_t mode) mask |=CEPH_CAP_LINK_SHARED ; } -if (want & (STATX_ATIME |STATX_MTIME |STATX_CTIME |STATX_SIZE | -STATX_BLOCKS )) +if (want & (STATX_ATIME |STATX_MTIME |STATX_CTIME |STATX_SIZE |STATX_BLOCKS |STATX_INO_VERSION )) mask |=CEPH_CAP_FILE_SHARED ; -if (want & (STATX_CTIME )) +if (want & (STATX_CTIME |STATX_INO_VERSION )) mask |=CEPH_CAP_XATTR_SHARED ;return mask;
@@ -2475,6 +2474,11 @@ int ceph_getattr(struct user_namespace *mnt_userns, const struct path *path, valid_mask |=STATX_BTIME ; } +if (request_mask &STATX_INO_VERSION ) { + stat->ino_version =inode_peek_iversion_raw (inode); + valid_mask |=STATX_INO_VERSION ; + } +if (ceph_snap (inode) ==CEPH_NOSNAP ) stat->dev = inode->i_sb->s_dev;else
Signature · 1 line
2.37.3
Jeff Layton
maintainerPATCH v5 7/8 nfsd: use the getattr operation to fetch i_version
In reply to Jeff LaytonNow that we can call into vfs_getattr to get the i_version field, use that facility to fetch it instead of doing it in nfsd4_change_attribute.
Set the STATX_INO_VERSION (and BTIME) bits in the request when we're dealing with a v4 request. Then, instead of looking at IS_I_VERSION when generating the change attr, look at the result mask and only use it if STATX_INO_VERSION is set.
With this change, we can drop the fetch_iversion export operation as well.
--- fs/nfs/export.c | 7 ------- fs/nfsd/nfs4xdr.c | 4 +++- fs/nfsd/nfsfh.c | 6 ++++++ fs/nfsd/nfsfh.h | 9 ++++----- fs/nfsd/vfs.h | 7 ++++++- include/linux/exportfs.h | 1 - 6 files changed, 19 insertions(+), 15 deletions(-)
fs/nfs/export.c +0 -7
@@ -145,17 +145,10 @@ nfs_get_parent(struct dentry *dentry)return parent; } -static u64 nfs_fetch_iversion (struct inode *inode) -{ -nfs_revalidate_inode (inode,NFS_INO_INVALID_CHANGE ); -return inode_peek_iversion_raw (inode); -} -const struct export_operations nfs_export_ops = { .encode_fh = nfs_encode_fh, .fh_to_dentry = nfs_fh_to_dentry, .get_parent = nfs_get_parent, - .fetch_iversion = nfs_fetch_iversion, .flags =EXPORT_OP_NOWCC |EXPORT_OP_NOSUBTREECHK |EXPORT_OP_CLOSE_BEFORE_UNLINK |EXPORT_OP_REMOTE_FS |EXPORT_OP_NOATOMIC_ATTR ,
fs/nfsd/nfs4xdr.c +3 -1
@@ -2872,7 +2872,9 @@ nfsd4_encode_fattr(struct xdr_stream *xdr, struct svc_fh *fhp, goto out; } - err =vfs_getattr (&path, &stat,STATX_BASIC_STATS ,AT_STATX_SYNC_AS_STAT ); + err =vfs_getattr (&path, &stat, +STATX_BASIC_STATS |STATX_BTIME |STATX_INO_VERSION , +AT_STATX_SYNC_AS_STAT );if (err) goto out_nfserr;if (!(stat.result_mask &STATX_BTIME ))
fs/nfsd/nfsfh.c +6 -0
@@ -634,6 +634,10 @@ void fh_fill_pre_attrs(struct svc_fh *fhp) stat.mtime = inode->i_mtime; stat.ctime = inode->i_ctime; stat.size = inode->i_size; +if (v4 &&IS_I_VERSION (inode)) { + stat.ino_version =inode_query_iversion (inode); + stat.result_mask |=STATX_INO_VERSION ; + } }if (v4) fhp->fh_pre_change =nfsd4_change_attribute (&stat, inode);
@@ -665,6 +669,8 @@ void fh_fill_post_attrs(struct svc_fh *fhp)if (err) { fhp->fh_post_saved = false; fhp->fh_post_attr.ctime = inode->i_ctime; +if (v4 &&IS_I_VERSION (inode)) + fhp->fh_post_attr.ino_version =inode_query_iversion (inode); }else fhp->fh_post_saved = true;if (v4)
fs/nfsd/nfsfh.h +4 -5
@@ -305,18 +305,17 @@ static inline void fh_clear_pre_post_attrs(struct svc_fh *fhp)static inline u64 nfsd4_change_attribute (struct kstat *stat,struct inode *inode) { -if (inode->i_sb->s_export_op->fetch_iversion) -return inode->i_sb->s_export_op->fetch_iversion (inode); -else if (IS_I_VERSION (inode)) { +if (stat->result_mask &STATX_INO_VERSION ) {u64 chattr; chattr = stat->ctime.tv_sec; chattr <<=30 ; chattr += stat->ctime.tv_nsec; - chattr +=inode_query_iversion (inode); + chattr += stat->ino_version;return chattr; - }else + }else {return time_to_chattr (&stat->ctime); + } }extern void fh_fill_pre_attrs (struct svc_fh *fhp);
fs/nfsd/vfs.h +6 -1
@@ -168,9 +168,14 @@ static inline void fh_drop_write(struct svc_fh *fh)static inline __be32 fh_getattr (const struct svc_fh *fh,struct kstat *stat) { +u32 request_mask =STATX_BASIC_STATS ;struct path p = {.mnt = fh->fh_export->ex_path.mnt, .dentry = fh->fh_dentry}; -return nfserrno (vfs_getattr (&p, stat,STATX_BASIC_STATS , + +if (fh->fh_maxsize ==NFS4_FHSIZE ) + request_mask |= (STATX_BTIME |STATX_INO_VERSION ); + +return nfserrno (vfs_getattr (&p, stat, request_mask,AT_STATX_SYNC_AS_STAT )); }
include/linux/exportfs.h +0 -1
@@ -213,7 +213,6 @@ struct export_operations { bool write, u32 *device_generation);int (*commit_blocks)(struct inode *inode,struct iomap *iomaps,int nr_iomaps,struct iattr *iattr); -u64 (*fetch_iversion)(struct inode *);#define EXPORT_OP_NOWCC (0x1)/* don't collect v3 wcc data */ #define EXPORT_OP_NOSUBTREECHK (0x2)/* no subtree checking */ #define EXPORT_OP_CLOSE_BEFORE_UNLINK (0x4)/* close files before unlink */
Signature · 1 line
2.37.3
Jeff Layton
maintainerRFC PATCH v5 8/8 nfsd: take inode_lock when querying for NFSv4 GETATTR
In reply to Jeff LaytonThe i_version counter for regular files is updated in update_time, and that's usually done before copying the data to the pagecache. It's possible that a reader and writer could race like this:
reader writer ------ ------ i_version++ read getattr update page cache
If that happens then the reader may associate the i_version value with the wrong inode state.
All of the existing filesystems that implement i_version take the i_rwsem in their write_iter operations before incrementing it. Take the inode_lock when issuing a getattr for NFSv4 attributes to prevent the above race.
--- fs/nfsd/nfs4xdr.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)
fs/nfsd/nfs4xdr.c +13 -0
@@ -2872,9 +2872,22 @@ nfsd4_encode_fattr(struct xdr_stream *xdr, struct svc_fh *fhp, goto out; } +/* +* The inode lock is needed here to ensure that there is not a +* write to the inode in progress that might change the size, +* or an in-progress directory morphing operation for directory +* inodes. +* +* READ and GETATTR are not guaranteed to be atomic, even when in +* the same compound, but we do try to present attributes in the +* GETATTR reply as representing a single point in time. +*/ +inode_lock (d_inode (dentry)); err =vfs_getattr (&path, &stat,STATX_BASIC_STATS |STATX_BTIME |STATX_INO_VERSION ,AT_STATX_SYNC_AS_STAT ); +inode_unlock (d_inode (dentry)); +if (err) goto out_nfserr;if (!(stat.result_mask &STATX_BTIME ))
Signature · 1 line
2.37.3