Skip to content

Commit

Permalink
zfs_enter rework
Browse files Browse the repository at this point in the history 
Replace ZFS_ENTER and ZFS_VERIFY_ZP, which have hidden returns, with
functions that return error code. The reason we want to do this is
because hidden returns are not obvious and had caused some missing fail
path unwinding.

This patch changes the common, linux, and freebsd parts. Also fixes
fail path unwinding in zfs_fsync, zpl_fsync, zpl_xattr_{list,get,set}, and
zfs_lookup().

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Ryan Moeller <[email protected]>
Signed-off-by: Chunwei Chen <[email protected]>
Closes openzfs#13831
  • Loading branch information
@tuxoko
tuxoko authored Sep 16, 2022
1 parent b24d1c7 commit 768eace
Show file tree
Hide file tree
Showing 15 changed files with 591 additions and 486 deletions.
35 changes: 15 additions & 20 deletions include/os/freebsd/zfs/sys/zfs_znode_impl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -121,29 +121,24 @@ typedef struct zfs_soft_state {
#define zn_rlimit_fsize(zp, uio) \
vn_rlimit_fsize(ZTOV(zp), GET_UIO_STRUCT(uio), zfs_uio_td(uio))

#define ZFS_ENTER_ERROR(zfsvfs, error) do { \
ZFS_TEARDOWN_ENTER_READ((zfsvfs), FTAG); \
if (__predict_false((zfsvfs)->z_unmounted)) { \
ZFS_TEARDOWN_EXIT_READ(zfsvfs, FTAG); \
return (error); \
} \
} while (0)

/* Called on entry to each ZFS vnode and vfs operation */
#define ZFS_ENTER(zfsvfs) ZFS_ENTER_ERROR(zfsvfs, EIO)
static inline int
zfs_enter(zfsvfs_t *zfsvfs, const char *tag)
{
ZFS_TEARDOWN_ENTER_READ(zfsvfs, tag);
if (__predict_false((zfsvfs)->z_unmounted)) {
ZFS_TEARDOWN_EXIT_READ(zfsvfs, tag);
return (SET_ERROR(EIO));
}
return (0);
}

/* Must be called before exiting the vop */
#define ZFS_EXIT(zfsvfs) ZFS_TEARDOWN_EXIT_READ(zfsvfs, FTAG)

#define ZFS_VERIFY_ZP_ERROR(zp, error) do { \
if (__predict_false((zp)->z_sa_hdl == NULL)) { \
ZFS_EXIT((zp)->z_zfsvfs); \
return (error); \
} \
} while (0)

/* Verifies the znode is valid */
#define ZFS_VERIFY_ZP(zp) ZFS_VERIFY_ZP_ERROR(zp, EIO)
static inline void
zfs_exit(zfsvfs_t *zfsvfs, const char *tag)
{
ZFS_TEARDOWN_EXIT_READ(zfsvfs, tag);
}

/*
* Macros for dealing with dmu_buf_hold
Expand Down
60 changes: 31 additions & 29 deletions include/os/linux/zfs/sys/zfs_znode_impl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -84,39 +84,41 @@ extern "C" {
#define zrele(zp) iput(ZTOI((zp)))

/* Called on entry to each ZFS inode and vfs operation. */
#define ZFS_ENTER_ERROR(zfsvfs, error) \
do { \
ZFS_TEARDOWN_ENTER_READ(zfsvfs, FTAG); \
if (unlikely((zfsvfs)->z_unmounted)) { \
ZFS_TEARDOWN_EXIT_READ(zfsvfs, FTAG); \
return (error); \
} \
} while (0)
#define ZFS_ENTER(zfsvfs) ZFS_ENTER_ERROR(zfsvfs, EIO)
#define ZPL_ENTER(zfsvfs) ZFS_ENTER_ERROR(zfsvfs, -EIO)
static inline int
zfs_enter(zfsvfs_t *zfsvfs, const char *tag)
{
ZFS_TEARDOWN_ENTER_READ(zfsvfs, tag);
if (unlikely(zfsvfs->z_unmounted)) {
ZFS_TEARDOWN_EXIT_READ(zfsvfs, tag);
return (SET_ERROR(EIO));
}
return (0);
}

/* Must be called before exiting the operation. */
#define ZFS_EXIT(zfsvfs) \
do { \
zfs_exit_fs(zfsvfs); \
ZFS_TEARDOWN_EXIT_READ(zfsvfs, FTAG); \
} while (0)
static inline void
zfs_exit(zfsvfs_t *zfsvfs, const char *tag)
{
zfs_exit_fs(zfsvfs);
ZFS_TEARDOWN_EXIT_READ(zfsvfs, tag);
}

#define ZPL_EXIT(zfsvfs) \
do { \
rrm_exit(&(zfsvfs)->z_teardown_lock, FTAG); \
} while (0)
static inline int
zpl_enter(zfsvfs_t *zfsvfs, const char *tag)
{
return (-zfs_enter(zfsvfs, tag));
}

/* Verifies the znode is valid. */
#define ZFS_VERIFY_ZP_ERROR(zp, error) \
do { \
if (unlikely((zp)->z_sa_hdl == NULL)) { \
ZFS_EXIT(ZTOZSB(zp)); \
return (error); \
} \
} while (0)
#define ZFS_VERIFY_ZP(zp) ZFS_VERIFY_ZP_ERROR(zp, EIO)
#define ZPL_VERIFY_ZP(zp) ZFS_VERIFY_ZP_ERROR(zp, -EIO)
static inline void
zpl_exit(zfsvfs_t *zfsvfs, const char *tag)
{
ZFS_TEARDOWN_EXIT_READ(zfsvfs, tag);
}

/* zfs_verify_zp and zfs_enter_verify_zp are defined in zfs_znode.h */
#define zpl_verify_zp(zp) (-zfs_verify_zp(zp))
#define zpl_enter_verify_zp(zfsvfs, zp, tag) \
(-zfs_enter_verify_zp(zfsvfs, zp, tag))

/*
* Macros for dealing with dmu_buf_hold
Expand Down
23 changes: 23 additions & 0 deletions include/sys/zfs_znode.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,29 @@ typedef struct znode {
ZNODE_OS_FIELDS;
} znode_t;

/* Verifies the znode is valid. */
static inline int
zfs_verify_zp(znode_t *zp)
{
if (unlikely(zp->z_sa_hdl == NULL))
return (SET_ERROR(EIO));
return (0);
}

/* zfs_enter and zfs_verify_zp together */
static inline int
zfs_enter_verify_zp(zfsvfs_t *zfsvfs, znode_t *zp, const char *tag)
{
int error;
if ((error = zfs_enter(zfsvfs, tag)) != 0)
return (error);
if ((error = zfs_verify_zp(zp)) != 0) {
zfs_exit(zfsvfs, tag);
return (error);
}
return (0);
}

typedef struct znode_hold {
uint64_t zh_obj; /* object id */
kmutex_t zh_lock; /* lock serializing object access */
Expand Down
14 changes: 8 additions & 6 deletions module/os/freebsd/zfs/zfs_ctldir.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1053,7 +1053,8 @@ zfsctl_snapdir_readdir(struct vop_readdir_args *ap)
return (error);
}

ZFS_ENTER(zfsvfs);
if ((error = zfs_enter(zfsvfs, FTAG)) != 0)
return (error);
for (;;) {
uint64_t cookie;
uint64_t id;
Expand All @@ -1070,7 +1071,7 @@ zfsctl_snapdir_readdir(struct vop_readdir_args *ap)
*eofp = 1;
error = 0;
}
ZFS_EXIT(zfsvfs);
zfs_exit(zfsvfs, FTAG);
return (error);
}

Expand All @@ -1083,7 +1084,7 @@ zfsctl_snapdir_readdir(struct vop_readdir_args *ap)
if (error != 0) {
if (error == ENAMETOOLONG)
error = 0;
ZFS_EXIT(zfsvfs);
zfs_exit(zfsvfs, FTAG);
return (SET_ERROR(error));
}
zfs_uio_setoffset(&uio, cookie + dots_offset);
Expand All @@ -1101,7 +1102,8 @@ zfsctl_snapdir_getattr(struct vop_getattr_args *ap)
uint64_t snap_count;
int err;

ZFS_ENTER(zfsvfs);
if ((err = zfs_enter(zfsvfs, FTAG)) != 0)
return (err);
ds = dmu_objset_ds(zfsvfs->z_os);
zfsctl_common_getattr(vp, vap);
vap->va_ctime = dmu_objset_snap_cmtime(zfsvfs->z_os);
Expand All @@ -1111,14 +1113,14 @@ zfsctl_snapdir_getattr(struct vop_getattr_args *ap)
err = zap_count(dmu_objset_pool(ds->ds_objset)->dp_meta_objset,
dsl_dataset_phys(ds)->ds_snapnames_zapobj, &snap_count);
if (err != 0) {
ZFS_EXIT(zfsvfs);
zfs_exit(zfsvfs, FTAG);
return (err);
}
vap->va_nlink += snap_count;
}
vap->va_size = vap->va_nlink;

ZFS_EXIT(zfsvfs);
zfs_exit(zfsvfs, FTAG);
return (0);
}

Expand Down
48 changes: 28 additions & 20 deletions module/os/freebsd/zfs/zfs_vfsops.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -286,7 +286,8 @@ zfs_quotactl(vfs_t *vfsp, int cmds, uid_t id, void *arg)
cmd = cmds >> SUBCMDSHIFT;
type = cmds & SUBCMDMASK;

ZFS_ENTER(zfsvfs);
if ((error = zfs_enter(zfsvfs, FTAG)) != 0)
return (error);
if (id == -1) {
switch (type) {
case USRQUOTA:
Expand Down Expand Up @@ -385,7 +386,7 @@ zfs_quotactl(vfs_t *vfsp, int cmds, uid_t id, void *arg)
break;
}
done:
ZFS_EXIT(zfsvfs);
zfs_exit(zfsvfs, FTAG);
return (error);
}

Expand Down Expand Up @@ -426,22 +427,23 @@ zfs_sync(vfs_t *vfsp, int waitfor)
if (error != 0)
return (error);

ZFS_ENTER(zfsvfs);
if ((error = zfs_enter(zfsvfs, FTAG)) != 0)
return (error);
dp = dmu_objset_pool(zfsvfs->z_os);

/*
* If the system is shutting down, then skip any
* filesystems which may exist on a suspended pool.
*/
if (rebooting && spa_suspended(dp->dp_spa)) {
ZFS_EXIT(zfsvfs);
zfs_exit(zfsvfs, FTAG);
return (0);
}

if (zfsvfs->z_log != NULL)
zil_commit(zfsvfs->z_log, 0);

ZFS_EXIT(zfsvfs);
zfs_exit(zfsvfs, FTAG);
} else {
/*
* Sync all ZFS filesystems. This is what happens when you
Expand Down Expand Up @@ -1408,10 +1410,12 @@ zfs_statfs(vfs_t *vfsp, struct statfs *statp)
{
zfsvfs_t *zfsvfs = vfsp->vfs_data;
uint64_t refdbytes, availbytes, usedobjs, availobjs;
int error;

statp->f_version = STATFS_VERSION;

ZFS_ENTER(zfsvfs);
if ((error = zfs_enter(zfsvfs, FTAG)) != 0)
return (error);

dmu_objset_space(zfsvfs->z_os,
&refdbytes, &availbytes, &usedobjs, &availobjs);
Expand Down Expand Up @@ -1458,7 +1462,7 @@ zfs_statfs(vfs_t *vfsp, struct statfs *statp)

statp->f_namemax = MAXNAMELEN - 1;

ZFS_EXIT(zfsvfs);
zfs_exit(zfsvfs, FTAG);
return (0);
}

Expand All @@ -1469,13 +1473,14 @@ zfs_root(vfs_t *vfsp, int flags, vnode_t **vpp)
znode_t *rootzp;
int error;

ZFS_ENTER(zfsvfs);
if ((error = zfs_enter(zfsvfs, FTAG)) != 0)
return (error);

error = zfs_zget(zfsvfs, zfsvfs->z_root, &rootzp);
if (error == 0)
*vpp = ZTOV(rootzp);

ZFS_EXIT(zfsvfs);
zfs_exit(zfsvfs, FTAG);

if (error == 0) {
error = vn_lock(*vpp, flags);
Expand Down Expand Up @@ -1712,15 +1717,16 @@ zfs_vget(vfs_t *vfsp, ino_t ino, int flags, vnode_t **vpp)
(zfsvfs->z_shares_dir != 0 && ino == zfsvfs->z_shares_dir))
return (EOPNOTSUPP);

ZFS_ENTER(zfsvfs);
if ((err = zfs_enter(zfsvfs, FTAG)) != 0)
return (err);
err = zfs_zget(zfsvfs, ino, &zp);
if (err == 0 && zp->z_unlinked) {
vrele(ZTOV(zp));
err = EINVAL;
}
if (err == 0)
*vpp = ZTOV(zp);
ZFS_EXIT(zfsvfs);
zfs_exit(zfsvfs, FTAG);
if (err == 0) {
err = vn_lock(*vpp, flags);
if (err != 0)
Expand Down Expand Up @@ -1774,7 +1780,8 @@ zfs_fhtovp(vfs_t *vfsp, fid_t *fidp, int flags, vnode_t **vpp)

*vpp = NULL;

ZFS_ENTER(zfsvfs);
if ((err = zfs_enter(zfsvfs, FTAG)) != 0)
return (err);

/*
* On FreeBSD we can get snapshot's mount point or its parent file
Expand All @@ -1790,12 +1797,13 @@ zfs_fhtovp(vfs_t *vfsp, fid_t *fidp, int flags, vnode_t **vpp)
for (i = 0; i < sizeof (zlfid->zf_setgen); i++)
setgen |= ((uint64_t)zlfid->zf_setgen[i]) << (8 * i);

ZFS_EXIT(zfsvfs);
zfs_exit(zfsvfs, FTAG);

err = zfsctl_lookup_objset(vfsp, objsetid, &zfsvfs);
if (err)
return (SET_ERROR(EINVAL));
ZFS_ENTER(zfsvfs);
if ((err = zfs_enter(zfsvfs, FTAG)) != 0)
return (err);
}

if (fidp->fid_len == SHORT_FID_LEN || fidp->fid_len == LONG_FID_LEN) {
Expand All @@ -1807,7 +1815,7 @@ zfs_fhtovp(vfs_t *vfsp, fid_t *fidp, int flags, vnode_t **vpp)
for (i = 0; i < sizeof (zfid->zf_gen); i++)
fid_gen |= ((uint64_t)zfid->zf_gen[i]) << (8 * i);
} else {
ZFS_EXIT(zfsvfs);
zfs_exit(zfsvfs, FTAG);
return (SET_ERROR(EINVAL));
}

Expand All @@ -1825,7 +1833,7 @@ zfs_fhtovp(vfs_t *vfsp, fid_t *fidp, int flags, vnode_t **vpp)
if ((fid_gen == 0 &&
(object == ZFSCTL_INO_ROOT || object == ZFSCTL_INO_SNAPDIR)) ||
(zfsvfs->z_shares_dir != 0 && object == zfsvfs->z_shares_dir)) {
ZFS_EXIT(zfsvfs);
zfs_exit(zfsvfs, FTAG);
VERIFY0(zfsctl_root(zfsvfs, LK_SHARED, &dvp));
if (object == ZFSCTL_INO_SNAPDIR) {
cn.cn_nameptr = "snapshot";
Expand Down Expand Up @@ -1860,7 +1868,7 @@ zfs_fhtovp(vfs_t *vfsp, fid_t *fidp, int flags, vnode_t **vpp)
(u_longlong_t)fid_gen,
(u_longlong_t)gen_mask);
if ((err = zfs_zget(zfsvfs, object, &zp))) {
ZFS_EXIT(zfsvfs);
zfs_exit(zfsvfs, FTAG);
return (err);
}
(void) sa_lookup(zp->z_sa_hdl, SA_ZPL_GEN(zfsvfs), &zp_gen,
Expand All @@ -1872,12 +1880,12 @@ zfs_fhtovp(vfs_t *vfsp, fid_t *fidp, int flags, vnode_t **vpp)
dprintf("znode gen (%llu) != fid gen (%llu)\n",
(u_longlong_t)zp_gen, (u_longlong_t)fid_gen);
vrele(ZTOV(zp));
ZFS_EXIT(zfsvfs);
zfs_exit(zfsvfs, FTAG);
return (SET_ERROR(EINVAL));
}

*vpp = ZTOV(zp);
ZFS_EXIT(zfsvfs);
zfs_exit(zfsvfs, FTAG);
err = vn_lock(*vpp, flags);
if (err == 0)
vnode_create_vobject(*vpp, zp->z_size, curthread);
Expand Down Expand Up @@ -1945,7 +1953,7 @@ zfs_resume_fs(zfsvfs_t *zfsvfs, dsl_dataset_t *ds)
/*
* Attempt to re-establish all the active znodes with
* their dbufs. If a zfs_rezget() fails, then we'll let
* any potential callers discover that via ZFS_ENTER_VERIFY_VP
* any potential callers discover that via zfs_enter_verify_zp
* when they try to use their znode.
*/
mutex_enter(&zfsvfs->z_znodes_lock);
Expand Down
Loading

0 comments on commit 768eace

Please sign in to comment.