From owner-freebsd-hackers@FreeBSD.ORG Thu Mar 12 01:09:03 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 3FFF2444 for ; Thu, 12 Mar 2015 01:09:03 +0000 (UTC) Received: from mail-wg0-x22f.google.com (mail-wg0-x22f.google.com [IPv6:2a00:1450:400c:c00::22f]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id BD951AAB for ; Thu, 12 Mar 2015 01:08:59 +0000 (UTC) Received: by wggx13 with SMTP id x13so12950293wgg.12; Wed, 11 Mar 2015 18:08:58 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=date:from:to:cc:subject:message-id:mail-followup-to:references :mime-version:content-type:content-disposition:in-reply-to :user-agent; bh=iXd9h/B/dJjQeOObpxLvLkDMvScDv1ZxKi6A/zDNy5k=; b=ES69gJZ+MQL6O5DAZ2Yy2yHRX+aGHDUgILdHDCk3FW0TiXtEZy8NbqkNB0EUou5Slr eOGNxUpy7uQ6YDtoYrbTxdydXpdrUk0Hz5AJ4h1FiHwdRLfJpD/PkkhdHbZyAPbutgOj I7Fmz3piwvnl3c04CfGuPq2ghgQOxdyqE2JnewfHDrNDnZAZawiuPbc/7aUdUg5/5K7x 9UhKUc6VH20r2GObIkg6UyBdqp5dlVsuXcCFds1kglin/isKicB00GXNWeEtDwKIMzm7 GUgIcZ1CJlrbEtX/RypL5V4RQWdbqUm7SuSXXZTGfJuYWe7exlJEbY20R0H1isYX7baF gchg== X-Received: by 10.194.48.12 with SMTP id h12mr84588520wjn.74.1426122538253; Wed, 11 Mar 2015 18:08:58 -0700 (PDT) Received: from dft-labs.eu (n1x0n-1-pt.tunnel.tserv5.lon1.ipv6.he.net. [2001:470:1f08:1f7::2]) by mx.google.com with ESMTPSA id hd10sm26369090wib.7.2015.03.11.18.08.56 (version=TLSv1.2 cipher=RC4-SHA bits=128/128); Wed, 11 Mar 2015 18:08:57 -0700 (PDT) Date: Thu, 12 Mar 2015 02:08:53 +0100 From: Mateusz Guzik To: Tiwei Bie Subject: Re: [PATCH] Finish the task 'Convert mountlist_mtx to rwlock' Message-ID: <20150312010853.GC18699@dft-labs.eu> Mail-Followup-To: Mateusz Guzik , Tiwei Bie , freebsd-hackers@freebsd.org, wca@freebsd.org References: <1426079434-51568-1-git-send-email-btw@mail.ustc.edu.cn> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <1426079434-51568-1-git-send-email-btw@mail.ustc.edu.cn> User-Agent: Mutt/1.5.21 (2010-09-15) 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 01:09:03 -0000 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