Skip to content

Commit

Permalink
nfsd: use the getattr operation to fetch i_version
Browse files Browse the repository at this point in the history
Now 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.

Neil also pointed out recently that IS_I_VERSION directory operations
are always logged, and so we only need to mitigate the rollback problem
on regular files. Also, we don't need to factor in the ctime when
reexporting NFS or Ceph.

Set the STATX_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_VERSION is set. With this change, we can drop the fetch_iversion
export operation as well.

Move nfsd4_change_attribute into nfsfh.c, and change it to only factor
in the ctime if it's a regular file and the fs doesn't advertise
STATX_ATTR_VERSION_MONOTONIC.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
  • Loading branch information
jtlayton authored and intel-lab-lkp committed Sep 30, 2022
1 parent a90f271 commit 73d7c5b
Show file tree
Hide file tree
Showing 6 changed files with 50 additions and 38 deletions.
7 changes: 0 additions & 7 deletions fs/nfs/export.c
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
4 changes: 3 additions & 1 deletion fs/nfsd/nfs4xdr.c
Original file line number Diff line number Diff line change
Expand Up @@ -2906,7 +2906,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_VERSION,
AT_STATX_SYNC_AS_STAT);
if (err)
goto out_nfserr;
if (!(stat.result_mask & STATX_BTIME))
Expand Down
40 changes: 40 additions & 0 deletions fs/nfsd/nfsfh.c
Original file line number Diff line number Diff line change
Expand Up @@ -628,6 +628,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.version = inode_query_iversion(inode);
stat.result_mask |= STATX_VERSION;
}
}
if (v4)
fhp->fh_pre_change = nfsd4_change_attribute(&stat, inode);
Expand Down Expand Up @@ -659,6 +663,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.version = inode_query_iversion(inode);
} else
fhp->fh_post_saved = true;
if (v4)
Expand Down Expand Up @@ -748,3 +754,37 @@ enum fsid_source fsid_source(const struct svc_fh *fhp)
return FSIDSOURCE_UUID;
return FSIDSOURCE_DEV;
}

/*
* We could use i_version alone as the change attribute. However, i_version
* can go backwards on a regular file after an unclean shutdown. On its own
* that doesn't necessarily cause a problem, but if i_version goes backwards
* and then is incremented again it could reuse a value that was previously
* used before boot, and a client who queried the two values might incorrectly
* assume nothing changed.
*
* By using both ctime and the i_version counter we guarantee that as long as
* time doesn't go backwards we never reuse an old value. If the filesystem
* advertises STATX_ATTR_VERSION_MONOTONIC, then this mitigation is not needed.
*
* We only need to do this for regular files as well. For directories, we
* assume that the new change attr is always logged to stable storage in some
* fashion before the results can be seen.
*/
u64 nfsd4_change_attribute(struct kstat *stat, struct inode *inode)
{
u64 chattr;

if (stat->result_mask & STATX_VERSION) {
chattr = stat->version;

if (S_ISREG(inode->i_mode) &&
!(stat->attributes & STATX_ATTR_VERSION_MONOTONIC)) {
chattr += (u64)stat->ctime.tv_sec << 30;
chattr += stat->ctime.tv_nsec;
}
} else {
chattr = time_to_chattr(&stat->ctime);
}
return chattr;
}
29 changes: 1 addition & 28 deletions fs/nfsd/nfsfh.h
Original file line number Diff line number Diff line change
Expand Up @@ -291,34 +291,7 @@ static inline void fh_clear_pre_post_attrs(struct svc_fh *fhp)
fhp->fh_pre_saved = false;
}

/*
* We could use i_version alone as the change attribute. However,
* i_version can go backwards after a reboot. On its own that doesn't
* necessarily cause a problem, but if i_version goes backwards and then
* is incremented again it could reuse a value that was previously used
* before boot, and a client who queried the two values might
* incorrectly assume nothing changed.
*
* By using both ctime and the i_version counter we guarantee that as
* long as time doesn't go backwards we never reuse an old value.
*/
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)) {
u64 chattr;

chattr = stat->ctime.tv_sec;
chattr <<= 30;
chattr += stat->ctime.tv_nsec;
chattr += inode_query_iversion(inode);
return chattr;
} else
return time_to_chattr(&stat->ctime);
}

u64 nfsd4_change_attribute(struct kstat *stat, struct inode *inode);
extern void fh_fill_pre_attrs(struct svc_fh *fhp);
extern void fh_fill_post_attrs(struct svc_fh *fhp);
extern void fh_fill_both_attrs(struct svc_fh *fhp);
Expand Down
7 changes: 6 additions & 1 deletion fs/nfsd/vfs.h
Original file line number Diff line number Diff line change
Expand Up @@ -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_VERSION);

return nfserrno(vfs_getattr(&p, stat, request_mask,
AT_STATX_SYNC_AS_STAT));
}

Expand Down
1 change: 0 additions & 1 deletion include/linux/exportfs.h
Original file line number Diff line number Diff line change
Expand Up @@ -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 */
Expand Down

0 comments on commit 73d7c5b

Please sign in to comment.