From owner-freebsd-hackers@FreeBSD.ORG Thu Mar 12 02:25:17 2015 Return-Path: Delivered-To: freebsd-hackers@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 04A575A4 for ; Thu, 12 Mar 2015 02:25:17 +0000 (UTC) Received: from ustc.edu.cn (email6.ustc.edu.cn [IPv6:2001:da8:d800::8]) by mx1.freebsd.org (Postfix) with ESMTP id E53E727D for ; Thu, 12 Mar 2015 02:25:14 +0000 (UTC) Received: from freebsd (unknown [58.211.218.74]) by newmailweb.ustc.edu.cn (Coremail) with SMTP id LkAmygBXORb++ABV_NuIAw--.47100S2; Thu, 12 Mar 2015 10:25:09 +0800 (CST) Date: Thu, 12 Mar 2015 10:24:59 +0800 From: Tiwei Bie To: Mateusz Guzik Subject: Re: [PATCH] Finish the task 'Convert mountlist_mtx to rwlock' Message-ID: <20150312022459.GA42663@freebsd> References: <1426079434-51568-1-git-send-email-btw@mail.ustc.edu.cn> <20150312010853.GC18699@dft-labs.eu> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20150312010853.GC18699@dft-labs.eu> User-Agent: Mutt/1.5.23 (2014-03-12) X-CM-TRANSID: LkAmygBXORb++ABV_NuIAw--.47100S2 X-Coremail-Antispam: 1UD129KBjvJXoWxAFWxZw1DZF43Aw18GrW7Jwb_yoW7JryDpF n0yF4jkF4kAr4DJr47t3Z5ua48Zr4v9r1UGw4kWry7Z398Xw1IqF48ta9xCay5urs29a93 Z3W8urn8CanayaUanT9S1TB71UUUUUUqnTZGkaVYY2UrUUUUjbIjqfuFe4nvWSU5nxnvy2 9KBjDU0xBIdaVrnRJUUUk0b7Iv0xC_Kw4lb4IE77IF4wAFF20E14v26r1j6r4UM7CY07I2 0VC2zVCF04k26cxKx2IYs7xG6rWj6s0DM7CIcVAFz4kK6r1j6r18M28lY4IEw2IIxxk0rw A2F7IY1VAKz4vEj48ve4kI8wA2z4x0Y4vE2Ix0cI8IcVAFwI0_tr0E3s1l84ACjcxK6xII jxv20xvEc7CjxVAFwI0_Gr1j6F4UJwA2z4x0Y4vEx4A2jsIE14v26rxl6s0DM28EF7xvwV C2z280aVCY1x0267AKxVW0oVCq3wAS0I0E0xvYzxvE52x082IY62kv0487Mc02F40EFcxC 0VAKzVAqx4xG6I80ewAv7VC0I7IYx2IY67AKxVWUAVWUtwAv7VC2z280aVAFwI0_Gr0_Cr 1lOx8S6xCaFVCjc4AY6r1j6r4UM4x0Y48IcVAKI48JMxkIecxEwVAFwVW8JwCF04k20xvY 0x0EwIxGrwCFx2IqxVCFs4IE7xkEbVWUJVW8JwC20s026c02F40E14v26r1j6r18MI8I3I 0E7480Y4vE14v26r106r1rMI8E67AF67kF1VAFwI0_Jrv_JF1lIxkGc2Ij64vIr41lIxAI cVC0I7IYx2IY67AKxVWUJVWUCwCI42IY6xIIjxv20xvEc7CjxVAFwI0_Jr0_Gr1lIxAIcV CF04k26cxKx2IYs7xG6rW3Jr0E3s1lIxAIcVC2z280aVAFwI0_Jr0_Gr1lIxAIcVC2z280 aVCY1x0267AKxVWUJVW8JbIYCTnIWIevJa73UjIFyTuYvjxUyrMaDUUUU X-CM-SenderInfo: xewzqzxdloh3xvwfhvlgxou0/1tbiAQUFAVQhl-bDMwAZs9 Cc: freebsd-hackers@freebsd.org, wca@freebsd.org X-BeenThere: freebsd-hackers@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: Technical Discussions relating to FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 12 Mar 2015 02:25:17 -0000 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 Tiwei Bie