Date: Thu, 12 Mar 2015 02:08:53 +0100 From: Mateusz Guzik <mjguzik@gmail.com> To: Tiwei Bie <btw@mail.ustc.edu.cn> Cc: freebsd-hackers@freebsd.org, wca@freebsd.org Subject: Re: [PATCH] Finish the task 'Convert mountlist_mtx to rwlock' Message-ID: <20150312010853.GC18699@dft-labs.eu> In-Reply-To: <1426079434-51568-1-git-send-email-btw@mail.ustc.edu.cn> References: <1426079434-51568-1-git-send-email-btw@mail.ustc.edu.cn>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, Mar 11, 2015 at 09:10:34PM +0800, Tiwei Bie wrote:
> Hi, Mateusz!
>
> I have finished the task: Convert mountlist_mtx to rwlock [1].
>
> sys/rwlock.h and cddl/compat/opensolaris/sys/rwlock.h have defined
> the same symbols, such as rw_init(), rw_destroy(). So they could not
> be included in the same file. And the latter has been indirectly
> included in cddl/compat/opensolaris/kern/opensolaris_vfs.c and
> cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_ioctl.c
> which needs to use mountlist_lock. So I implemented the following
> functions to access mountlist_lock in these files:
>
> void zfs_mountlist_wlock(void);
> void zfs_mountlist_wunlock(void);
> void zfs_mountlist_rlock(void);
> void zfs_mountlist_runlock(void);
>
(cc-ed one of our zfs guys)
Oops, completely forgot about header conflict.
First off, if providing such functions, they should be prefixed with
freebsd_ or something similar.
Ideally we would somewhow fix namespace problems, but this may not be
that easy.
I think this is a nice opportunity to hide mountlist_lock behind some
helpers.
I left all usage samples below.
We can fix mount_snapshot no problem by providing a dedicated function
to insert into mountlist.
I'm somewhat torn with zfs_get_vfs and zfsvfs_update_fromname. We could
provide a helper function which takes a function pointer and calls it
for each entry in the list. Somewhat crappy but should work fine.
Comments?
> diff --git a/sys/cddl/compat/opensolaris/kern/opensolaris_vfs.c b/sys/cddl/compat/opensolaris/kern/opensolaris_vfs.c
> index a2532f8..045aa80 100644
> --- a/sys/cddl/compat/opensolaris/kern/opensolaris_vfs.c
> +++ b/sys/cddl/compat/opensolaris/kern/opensolaris_vfs.c
> @@ -222,9 +222,9 @@ mount_snapshot(kthread_t *td, vnode_t **vpp, const char *fstype, char *fspath,
>
> vp->v_mountedhere = mp;
> /* Put the new filesystem on the mount list. */
> - mtx_lock(&mountlist_mtx);
> + zfs_mountlist_wlock();
> TAILQ_INSERT_TAIL(&mountlist, mp, mnt_list);
> - mtx_unlock(&mountlist_mtx);
> + zfs_mountlist_wunlock();
> vfs_event_signal(NULL, VQ_MOUNT, 0);
> if (VFS_ROOT(mp, LK_EXCLUSIVE, &mvp))
> panic("mount: lost mount");
> diff --git a/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_ioctl.c b/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_ioctl.c
> index a829b06..4d86892 100644
> --- a/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_ioctl.c
> +++ b/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_ioctl.c
> @@ -3015,14 +3015,14 @@ zfs_get_vfs(const char *resource)
> {
> vfs_t *vfsp;
>
> - mtx_lock(&mountlist_mtx);
> + zfs_mountlist_rlock();
> TAILQ_FOREACH(vfsp, &mountlist, mnt_list) {
> if (strcmp(refstr_value(vfsp->vfs_resource), resource) == 0) {
> VFS_HOLD(vfsp);
> break;
> }
> }
> - mtx_unlock(&mountlist_mtx);
> + zfs_mountlist_runlock();
> return (vfsp);
> }
>
> diff --git a/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_vfsops.c b/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_vfsops.c
> index 415db9e..9b29323 100644
> --- a/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_vfsops.c
> +++ b/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_vfsops.c
> @@ -2537,7 +2537,7 @@ zfsvfs_update_fromname(const char *oldname, const char *newname)
>
> oldlen = strlen(oldname);
>
> - mtx_lock(&mountlist_mtx);
> + zfs_mountlist_rlock();
> TAILQ_FOREACH(mp, &mountlist, mnt_list) {
> fromname = mp->mnt_stat.f_mntfromname;
> if (strcmp(fromname, oldname) == 0) {
> @@ -2554,6 +2554,6 @@ zfsvfs_update_fromname(const char *oldname, const char *newname)
> continue;
> }
> }
> - mtx_unlock(&mountlist_mtx);
> + zfs_mountlist_runlock();
> }
> #endif
--
Mateusz Guzik <mjguzik gmail.com>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20150312010853.GC18699>