Date: Thu, 12 Mar 2015 10:24:59 +0800 From: Tiwei Bie <btw@mail.ustc.edu.cn> To: Mateusz Guzik <mjguzik@gmail.com> Cc: freebsd-hackers@freebsd.org, wca@freebsd.org Subject: Re: [PATCH] Finish the task 'Convert mountlist_mtx to rwlock' Message-ID: <20150312022459.GA42663@freebsd> In-Reply-To: <20150312010853.GC18699@dft-labs.eu> References: <1426079434-51568-1-git-send-email-btw@mail.ustc.edu.cn> <20150312010853.GC18699@dft-labs.eu>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, Mar 12, 2015 at 02:08:53AM +0100, Mateusz Guzik wrote:
> 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.
>
When naming these functions, I read cddl/compat/opensolaris/kern/opensolaris_vm.c
for reference. It also needs to use rwlock:
void
zfs_vmobject_wlock(vm_object_t object)
{
VM_OBJECT_WLOCK(object);
}
void
zfs_vmobject_wunlock(vm_object_t object)
{
VM_OBJECT_WUNLOCK(object);
}
> 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?
>
If we hide mountlist_lock behind some helpers, we will have to define
new callback function and argument type (when the callback function
needs more than one argument, I think these arguments have to be included
in a structure to keep the prototype of the callback function simple,
e.g. int (*func)(struct mount *mp, void *arg)), such as what we have to
do with zfsvfs_update_fromname. It isn't very simple and elegant. So I
prefer to provide some helpers to access mountlist_lock directly.
> > 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>
Tiwei Bie
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20150312022459.GA42663>