[PATCH] Finish the task 'Convert mountlist_mtx to rwlock'

[Tiwei Bie]

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