Skip site navigation (1)Skip section navigation (2)
Date:      Sat, 19 Jul 2014 11:42:36 +0100 (BST)
From:      Robert Watson <rwatson@FreeBSD.org>
To:        Xin LI <delphij@FreeBSD.org>
Cc:        svn-src-all@freebsd.org, src-committers@freebsd.org, svn-src-vendor@freebsd.org
Subject:   Re: svn commit: r268852 - in vendor-sys/illumos/dist/uts/common/fs/zfs: . sys
Message-ID:  <alpine.BSF.2.11.1407191141550.65204@fledge.watson.org>
In-Reply-To: <201407181809.s6II9KkB002810@svn.freebsd.org>
References:  <201407181809.s6II9KkB002810@svn.freebsd.org>

next in thread | previous in thread | raw e-mail | index | archive | help
On Fri, 18 Jul 2014, Xin LI wrote:

> Log:
>  5008 lock contention (rrw_exit) while running a read only load
>  Reviewed by: Matthew Ahrens <matthew.ahrens@delphix.com>
>  Reviewed by: George Wilson <george.wilson@delphix.com>
>  Reviewed by: Alex Reece <alex.reece@delphix.com>
>  Reviewed by: Christopher Siden <christopher.siden@delphix.com>
>  Reviewed by: Richard Yao <ryao@gentoo.org>
>  Reviewed by: Saso Kiselkov <skiselkov.ml@gmail.com>
>  Approved by: Garrett D'Amore <garrett@damore.org>
>
>  illumos/illumos-gate@c9030f6c93613fe30ee0c16f92b96da7816ac052

Is there an opportunity to use our own read-mostly lock implementation here? 
It should be substantially more scalable, has integration with WITNESS, proper 
priority propagation, etc.

Robert


>
> Modified:
>  vendor-sys/illumos/dist/uts/common/fs/zfs/rrwlock.c
>  vendor-sys/illumos/dist/uts/common/fs/zfs/sys/rrwlock.h
>  vendor-sys/illumos/dist/uts/common/fs/zfs/sys/zfs_vfsops.h
>  vendor-sys/illumos/dist/uts/common/fs/zfs/sys/zfs_znode.h
>  vendor-sys/illumos/dist/uts/common/fs/zfs/zfs_ioctl.c
>  vendor-sys/illumos/dist/uts/common/fs/zfs/zfs_vfsops.c
>  vendor-sys/illumos/dist/uts/common/fs/zfs/zfs_znode.c
>
> Modified: vendor-sys/illumos/dist/uts/common/fs/zfs/rrwlock.c
> ==============================================================================
> --- vendor-sys/illumos/dist/uts/common/fs/zfs/rrwlock.c	Fri Jul 18 18:05:09 2014	(r268851)
> +++ vendor-sys/illumos/dist/uts/common/fs/zfs/rrwlock.c	Fri Jul 18 18:09:20 2014	(r268852)
> @@ -286,3 +286,91 @@ rrw_tsd_destroy(void *arg)
> 		    (void *)curthread, (void *)rn->rn_rrl);
> 	}
> }
> +
> +/*
> + * A reader-mostly lock implementation, tuning above reader-writer locks
> + * for hightly parallel read acquisitions, while pessimizing writes.
> + *
> + * The idea is to split single busy lock into array of locks, so that
> + * each reader can lock only one of them for read, depending on result
> + * of simple hash function.  That proportionally reduces lock congestion.
> + * Writer same time has to sequentially aquire write on all the locks.
> + * That makes write aquisition proportionally slower, but in places where
> + * it is used (filesystem unmount) performance is not critical.
> + *
> + * All the functions below are direct wrappers around functions above.
> + */
> +void
> +rrm_init(rrmlock_t *rrl, boolean_t track_all)
> +{
> +	int i;
> +
> +	for (i = 0; i < RRM_NUM_LOCKS; i++)
> +		rrw_init(&rrl->locks[i], track_all);
> +}
> +
> +void
> +rrm_destroy(rrmlock_t *rrl)
> +{
> +	int i;
> +
> +	for (i = 0; i < RRM_NUM_LOCKS; i++)
> +		rrw_destroy(&rrl->locks[i]);
> +}
> +
> +void
> +rrm_enter(rrmlock_t *rrl, krw_t rw, void *tag)
> +{
> +	if (rw == RW_READER)
> +		rrm_enter_read(rrl, tag);
> +	else
> +		rrm_enter_write(rrl);
> +}
> +
> +/*
> + * This maps the current thread to a specific lock.  Note that the lock
> + * must be released by the same thread that acquired it.  We do this
> + * mapping by taking the thread pointer mod a prime number.  We examine
> + * only the low 32 bits of the thread pointer, because 32-bit division
> + * is faster than 64-bit division, and the high 32 bits have little
> + * entropy anyway.
> + */
> +#define	RRM_TD_LOCK()	(((uint32_t)(uintptr_t)(curthread)) % RRM_NUM_LOCKS)
> +
> +void
> +rrm_enter_read(rrmlock_t *rrl, void *tag)
> +{
> +	rrw_enter_read(&rrl->locks[RRM_TD_LOCK()], tag);
> +}
> +
> +void
> +rrm_enter_write(rrmlock_t *rrl)
> +{
> +	int i;
> +
> +	for (i = 0; i < RRM_NUM_LOCKS; i++)
> +		rrw_enter_write(&rrl->locks[i]);
> +}
> +
> +void
> +rrm_exit(rrmlock_t *rrl, void *tag)
> +{
> +	int i;
> +
> +	if (rrl->locks[0].rr_writer == curthread) {
> +		for (i = 0; i < RRM_NUM_LOCKS; i++)
> +			rrw_exit(&rrl->locks[i], tag);
> +	} else {
> +		rrw_exit(&rrl->locks[RRM_TD_LOCK()], tag);
> +	}
> +}
> +
> +boolean_t
> +rrm_held(rrmlock_t *rrl, krw_t rw)
> +{
> +	if (rw == RW_WRITER) {
> +		return (rrw_held(&rrl->locks[0], rw));
> +	} else {
> +		return (rrw_held(&rrl->locks[RRM_TD_LOCK()], rw));
> +	}
> +}
>
> Modified: vendor-sys/illumos/dist/uts/common/fs/zfs/sys/rrwlock.h
> ==============================================================================
> --- vendor-sys/illumos/dist/uts/common/fs/zfs/sys/rrwlock.h	Fri Jul 18 18:05:09 2014	(r268851)
> +++ vendor-sys/illumos/dist/uts/common/fs/zfs/sys/rrwlock.h	Fri Jul 18 18:09:20 2014	(r268852)
> @@ -80,6 +80,31 @@ void rrw_tsd_destroy(void *arg);
> #define	RRW_LOCK_HELD(x) \
> 	(rrw_held(x, RW_WRITER) || rrw_held(x, RW_READER))
>
> +/*
> + * A reader-mostly lock implementation, tuning above reader-writer locks
> + * for hightly parallel read acquisitions, pessimizing write acquisitions.
> + *
> + * This should be a prime number.  See comment in rrwlock.c near
> + * RRM_TD_LOCK() for details.
> + */
> +#define	RRM_NUM_LOCKS		17
> +typedef struct rrmlock {
> +	rrwlock_t	locks[RRM_NUM_LOCKS];
> +} rrmlock_t;
> +
> +void rrm_init(rrmlock_t *rrl, boolean_t track_all);
> +void rrm_destroy(rrmlock_t *rrl);
> +void rrm_enter(rrmlock_t *rrl, krw_t rw, void *tag);
> +void rrm_enter_read(rrmlock_t *rrl, void *tag);
> +void rrm_enter_write(rrmlock_t *rrl);
> +void rrm_exit(rrmlock_t *rrl, void *tag);
> +boolean_t rrm_held(rrmlock_t *rrl, krw_t rw);
> +
> +#define	RRM_READ_HELD(x)	rrm_held(x, RW_READER)
> +#define	RRM_WRITE_HELD(x)	rrm_held(x, RW_WRITER)
> +#define	RRM_LOCK_HELD(x) \
> +	(rrm_held(x, RW_WRITER) || rrm_held(x, RW_READER))
> +
> #ifdef	__cplusplus
> }
> #endif
>
> Modified: vendor-sys/illumos/dist/uts/common/fs/zfs/sys/zfs_vfsops.h
> ==============================================================================
> --- vendor-sys/illumos/dist/uts/common/fs/zfs/sys/zfs_vfsops.h	Fri Jul 18 18:05:09 2014	(r268851)
> +++ vendor-sys/illumos/dist/uts/common/fs/zfs/sys/zfs_vfsops.h	Fri Jul 18 18:09:20 2014	(r268852)
> @@ -64,7 +64,7 @@ struct zfsvfs {
> 	int		z_norm;		/* normalization flags */
> 	boolean_t	z_atime;	/* enable atimes mount option */
> 	boolean_t	z_unmounted;	/* unmounted */
> -	rrwlock_t	z_teardown_lock;
> +	rrmlock_t	z_teardown_lock;
> 	krwlock_t	z_teardown_inactive_lock;
> 	list_t		z_all_znodes;	/* all vnodes in the fs */
> 	kmutex_t	z_znodes_lock;	/* lock for z_all_znodes */
>
> Modified: vendor-sys/illumos/dist/uts/common/fs/zfs/sys/zfs_znode.h
> ==============================================================================
> --- vendor-sys/illumos/dist/uts/common/fs/zfs/sys/zfs_znode.h	Fri Jul 18 18:05:09 2014	(r268851)
> +++ vendor-sys/illumos/dist/uts/common/fs/zfs/sys/zfs_znode.h	Fri Jul 18 18:09:20 2014	(r268852)
> @@ -238,7 +238,7 @@ typedef struct znode {
> /* Called on entry to each ZFS vnode and vfs operation  */
> #define	ZFS_ENTER(zfsvfs) \
> 	{ \
> -		rrw_enter_read(&(zfsvfs)->z_teardown_lock, FTAG); \
> +		rrm_enter_read(&(zfsvfs)->z_teardown_lock, FTAG); \
> 		if ((zfsvfs)->z_unmounted) { \
> 			ZFS_EXIT(zfsvfs); \
> 			return (EIO); \
> @@ -246,7 +246,7 @@ typedef struct znode {
> 	}
>
> /* Must be called before exiting the vop */
> -#define	ZFS_EXIT(zfsvfs) rrw_exit(&(zfsvfs)->z_teardown_lock, FTAG)
> +#define	ZFS_EXIT(zfsvfs) rrm_exit(&(zfsvfs)->z_teardown_lock, FTAG)
>
> /* Verifies the znode is valid */
> #define	ZFS_VERIFY_ZP(zp) \
>
> Modified: vendor-sys/illumos/dist/uts/common/fs/zfs/zfs_ioctl.c
> ==============================================================================
> --- vendor-sys/illumos/dist/uts/common/fs/zfs/zfs_ioctl.c	Fri Jul 18 18:05:09 2014	(r268851)
> +++ vendor-sys/illumos/dist/uts/common/fs/zfs/zfs_ioctl.c	Fri Jul 18 18:09:20 2014	(r268852)
> @@ -1420,7 +1420,7 @@ zfsvfs_hold(const char *name, void *tag,
> 	if (getzfsvfs(name, zfvp) != 0)
> 		error = zfsvfs_create(name, zfvp);
> 	if (error == 0) {
> -		rrw_enter(&(*zfvp)->z_teardown_lock, (writer) ? RW_WRITER :
> +		rrm_enter(&(*zfvp)->z_teardown_lock, (writer) ? RW_WRITER :
> 		    RW_READER, tag);
> 		if ((*zfvp)->z_unmounted) {
> 			/*
> @@ -1428,7 +1428,7 @@ zfsvfs_hold(const char *name, void *tag,
> 			 * thread should be just about to disassociate the
> 			 * objset from the zfsvfs.
> 			 */
> -			rrw_exit(&(*zfvp)->z_teardown_lock, tag);
> +			rrm_exit(&(*zfvp)->z_teardown_lock, tag);
> 			return (SET_ERROR(EBUSY));
> 		}
> 	}
> @@ -1438,7 +1438,7 @@ zfsvfs_hold(const char *name, void *tag,
> static void
> zfsvfs_rele(zfsvfs_t *zfsvfs, void *tag)
> {
> -	rrw_exit(&zfsvfs->z_teardown_lock, tag);
> +	rrm_exit(&zfsvfs->z_teardown_lock, tag);
>
> 	if (zfsvfs->z_vfs) {
> 		VFS_RELE(zfsvfs->z_vfs);
>
> Modified: vendor-sys/illumos/dist/uts/common/fs/zfs/zfs_vfsops.c
> ==============================================================================
> --- vendor-sys/illumos/dist/uts/common/fs/zfs/zfs_vfsops.c	Fri Jul 18 18:05:09 2014	(r268851)
> +++ vendor-sys/illumos/dist/uts/common/fs/zfs/zfs_vfsops.c	Fri Jul 18 18:09:20 2014	(r268852)
> @@ -1004,7 +1004,7 @@ zfsvfs_create(const char *osname, zfsvfs
> 	mutex_init(&zfsvfs->z_lock, NULL, MUTEX_DEFAULT, NULL);
> 	list_create(&zfsvfs->z_all_znodes, sizeof (znode_t),
> 	    offsetof(znode_t, z_link_node));
> -	rrw_init(&zfsvfs->z_teardown_lock, B_FALSE);
> +	rrm_init(&zfsvfs->z_teardown_lock, B_FALSE);
> 	rw_init(&zfsvfs->z_teardown_inactive_lock, NULL, RW_DEFAULT, NULL);
> 	rw_init(&zfsvfs->z_fuid_lock, NULL, RW_DEFAULT, NULL);
> 	for (i = 0; i != ZFS_OBJ_MTX_SZ; i++)
> @@ -1119,7 +1119,7 @@ zfsvfs_free(zfsvfs_t *zfsvfs)
> 	mutex_destroy(&zfsvfs->z_znodes_lock);
> 	mutex_destroy(&zfsvfs->z_lock);
> 	list_destroy(&zfsvfs->z_all_znodes);
> -	rrw_destroy(&zfsvfs->z_teardown_lock);
> +	rrm_destroy(&zfsvfs->z_teardown_lock);
> 	rw_destroy(&zfsvfs->z_teardown_inactive_lock);
> 	rw_destroy(&zfsvfs->z_fuid_lock);
> 	for (i = 0; i != ZFS_OBJ_MTX_SZ; i++)
> @@ -1784,7 +1784,7 @@ zfsvfs_teardown(zfsvfs_t *zfsvfs, boolea
> {
> 	znode_t	*zp;
>
> -	rrw_enter(&zfsvfs->z_teardown_lock, RW_WRITER, FTAG);
> +	rrm_enter(&zfsvfs->z_teardown_lock, RW_WRITER, FTAG);
>
> 	if (!unmounting) {
> 		/*
> @@ -1814,7 +1814,7 @@ zfsvfs_teardown(zfsvfs_t *zfsvfs, boolea
> 	 */
> 	if (!unmounting && (zfsvfs->z_unmounted || zfsvfs->z_os == NULL)) {
> 		rw_exit(&zfsvfs->z_teardown_inactive_lock);
> -		rrw_exit(&zfsvfs->z_teardown_lock, FTAG);
> +		rrm_exit(&zfsvfs->z_teardown_lock, FTAG);
> 		return (SET_ERROR(EIO));
> 	}
>
> @@ -1841,7 +1841,7 @@ zfsvfs_teardown(zfsvfs_t *zfsvfs, boolea
> 	 */
> 	if (unmounting) {
> 		zfsvfs->z_unmounted = B_TRUE;
> -		rrw_exit(&zfsvfs->z_teardown_lock, FTAG);
> +		rrm_exit(&zfsvfs->z_teardown_lock, FTAG);
> 		rw_exit(&zfsvfs->z_teardown_inactive_lock);
> 	}
>
> @@ -2073,7 +2073,7 @@ zfs_resume_fs(zfsvfs_t *zfsvfs, const ch
> 	znode_t *zp;
> 	uint64_t sa_obj = 0;
>
> -	ASSERT(RRW_WRITE_HELD(&zfsvfs->z_teardown_lock));
> +	ASSERT(RRM_WRITE_HELD(&zfsvfs->z_teardown_lock));
> 	ASSERT(RW_WRITE_HELD(&zfsvfs->z_teardown_inactive_lock));
>
> 	/*
> @@ -2129,7 +2129,7 @@ zfs_resume_fs(zfsvfs_t *zfsvfs, const ch
> bail:
> 	/* release the VOPs */
> 	rw_exit(&zfsvfs->z_teardown_inactive_lock);
> -	rrw_exit(&zfsvfs->z_teardown_lock, FTAG);
> +	rrm_exit(&zfsvfs->z_teardown_lock, FTAG);
>
> 	if (err) {
> 		/*
>
> Modified: vendor-sys/illumos/dist/uts/common/fs/zfs/zfs_znode.c
> ==============================================================================
> --- vendor-sys/illumos/dist/uts/common/fs/zfs/zfs_znode.c	Fri Jul 18 18:05:09 2014	(r268851)
> +++ vendor-sys/illumos/dist/uts/common/fs/zfs/zfs_znode.c	Fri Jul 18 18:09:20 2014	(r268852)
> @@ -276,7 +276,7 @@ zfs_znode_move(void *buf, void *newbuf,
> 	 * can safely ensure that the filesystem is not and will not be
> 	 * unmounted. The next statement is equivalent to ZFS_ENTER().
> 	 */
> -	rrw_enter(&zfsvfs->z_teardown_lock, RW_READER, FTAG);
> +	rrm_enter(&zfsvfs->z_teardown_lock, RW_READER, FTAG);
> 	if (zfsvfs->z_unmounted) {
> 		ZFS_EXIT(zfsvfs);
> 		rw_exit(&zfsvfs_lock);
>
>



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?alpine.BSF.2.11.1407191141550.65204>