Date: Thu, 30 Jul 2015 00:24:04 -0700 From: "K. Macy" <kmacy@freebsd.org> To: Andriy Gapon <avg@freebsd.org> Cc: "src-committers@freebsd.org" <src-committers@freebsd.org>, "svn-src-all@freebsd.org" <svn-src-all@freebsd.org>, "svn-src-head@freebsd.org" <svn-src-head@freebsd.org>, Xin Li <delphij@delphij.net> Subject: Re: svn commit: r285021 - head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs Message-ID: <CAHM0Q_OcLqT_VuxmL4tFrDyEhAEprm_ivP8j_mrow6ZG1FLcsg@mail.gmail.com> In-Reply-To: <201507020832.t628W3WJ002944@repo.freebsd.org> References: <201507020832.t628W3WJ002944@repo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
Just FYI this change introduces a deadlock with with the
spa_namespace_lock. Mount will be holding this lock while trying to
acquire the spa_namespace_lock. zfskern on the other hand holds the
spa_namespace_lock when calling zfs_freebsd_access which in turn
tries to acquire the teardown lock.
static int
zfs_access(vnode_t *vp, int mode, int flag, cred_t *cr,
caller_context_t *ct)
{
znode_t *zp = VTOZ(vp);
zfsvfs_t *zfsvfs = zp->z_zfsvfs;
int error;
ZFS_ENTER(zfsvfs);
where:
/* Called on entry to each ZFS vnode and vfs operation */
#define ZFS_ENTER(zfsvfs) \
{ \
rrm_enter_read(&(zfsvfs)->z_teardown_lock, FTAG); \
if ((zfsvfs)->z_unmounted) { \
ZFS_EXIT(zfsvfs); \
return (EIO); \
} \
I think this makes a pretty strong case for the need to educate
WITNESS about rrm locks.
Cheers.
-K
On Thu, Jul 2, 2015 at 1:32 AM, Andriy Gapon <avg@freebsd.org> wrote:
> Author: avg
> Date: Thu Jul 2 08:32:02 2015
> New Revision: 285021
> URL: https://svnweb.freebsd.org/changeset/base/285021
>
> Log:
> zfs_mount(MS_REMOUNT): protect zfs_(un)register_callbacks calls
>
> We now take z_teardown_lock as a writer to ensure that there is no I/O
> while the filesystem state is in a flux. Also, zfs_suspend_fs() ->
> zfsvfs_teardown() call zfs_unregister_callbacks() and zfs_resume_fs() ->
> zfsvfs_setup() call zfs_unregister_callbacks(). Previously there was no
> synchronization between those calls and the calls in the re-mounting
> case. That could lead to concurrent execution and a crash.
>
> PR: 180060
> Differential Revision: https://reviews.freebsd.org/D2865
> Suggested by: mahrens
> Reviewed by: delphij, pho, mahrens, will
> MFC after: 13 days
> Sponsored by: ClusterHQ
>
> Modified:
> head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_vfsops.c
>
> Modified: head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_vfsops.c
> ==============================================================================
> --- head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_vfsops.c Thu Jul 2 08:25:45 2015 (r285020)
> +++ head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_vfsops.c Thu Jul 2 08:32:02 2015 (r285021)
> @@ -1717,9 +1717,19 @@ zfs_mount(vfs_t *vfsp)
> * according to those options set in the current VFS options.
> */
> if (vfsp->vfs_flag & MS_REMOUNT) {
> - /* refresh mount options */
> - zfs_unregister_callbacks(vfsp->vfs_data);
> + zfsvfs_t *zfsvfs = vfsp->vfs_data;
> +
> + /*
> + * Refresh mount options with z_teardown_lock blocking I/O while
> + * the filesystem is in an inconsistent state.
> + * The lock also serializes this code with filesystem
> + * manipulations between entry to zfs_suspend_fs() and return
> + * from zfs_resume_fs().
> + */
> + rrm_enter(&zfsvfs->z_teardown_lock, RW_WRITER, FTAG);
> + zfs_unregister_callbacks(zfsvfs);
> error = zfs_register_callbacks(vfsp);
> + rrm_exit(&zfsvfs->z_teardown_lock, FTAG);
> goto out;
> }
>
> _______________________________________________
> svn-src-head@freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/svn-src-head
> To unsubscribe, send any mail to "svn-src-head-unsubscribe@freebsd.org"
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAHM0Q_OcLqT_VuxmL4tFrDyEhAEprm_ivP8j_mrow6ZG1FLcsg>