Skip site navigation (1)Skip section navigation (2)
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>