Skip site navigation (1)Skip section navigation (2) 
Date:      Tue, 25 Apr 2023 16:03:27 GMT
From:      Mateusz Guzik <mjg@FreeBSD.org>
To:        src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org
Subject:   git: b0695c6abe3f - main - zfs: Revert "Fix data race between zil_commit() and zil_suspend()"
Message-ID:  <202304251603.33PG3R7J095585@gitrepo.freebsd.org>
next in thread | raw e-mail | index | archive | help 
The branch main has been updated by mjg:

URL: https://cgit.FreeBSD.org/src/commit/?id=b0695c6abe3f33a4188bcbcbf214823cd86f54d6

commit b0695c6abe3f33a4188bcbcbf214823cd86f54d6
Author:     Mateusz Guzik <mjg@FreeBSD.org>
AuthorDate: 2023-04-25 16:01:22 +0000
Commit:     Mateusz Guzik <mjg@FreeBSD.org>
CommitDate: 2023-04-25 16:01:22 +0000

    zfs: Revert "Fix data race between zil_commit() and zil_suspend()"
    
    This reverts commit 4c856fb333ac57d9b4a6ddd44407fd022a702f00.
    
    To quote a pending upstream PR:
    This reverts commit 4c856fb to resolve a newly introduced deadlock which
    in practice is more disruptive that the issue this commit intended to
    address.
    
    Causes deadlocks described in https://github.com/openzfs/zfs/issues/14775
    
    Sponsored by:   Rubicon Communications, LLC ("Netgate")
---
 sys/contrib/openzfs/include/sys/zil_impl.h |  1 -
 sys/contrib/openzfs/module/zfs/zil.c       | 27 ---------------------------
 2 files changed, 28 deletions(-)

diff --git a/sys/contrib/openzfs/include/sys/zil_impl.h b/sys/contrib/openzfs/include/sys/zil_impl.h
index f44a01afee5c..bb85bf6d1eb1 100644
--- a/sys/contrib/openzfs/include/sys/zil_impl.h
+++ b/sys/contrib/openzfs/include/sys/zil_impl.h
@@ -183,7 +183,6 @@ struct zilog {
 	uint64_t	zl_destroy_txg;	/* txg of last zil_destroy() */
 	uint64_t	zl_replayed_seq[TXG_SIZE]; /* last replayed rec seq */
 	uint64_t	zl_replaying_seq; /* current replay seq number */
-	krwlock_t	zl_suspend_lock;	/* protects suspend count */
 	uint32_t	zl_suspend;	/* log suspend count */
 	kcondvar_t	zl_cv_suspend;	/* log suspend completion */
 	uint8_t		zl_suspending;	/* log is currently suspending */
diff --git a/sys/contrib/openzfs/module/zfs/zil.c b/sys/contrib/openzfs/module/zfs/zil.c
index eb26e4b32998..d1631c2ac9db 100644
--- a/sys/contrib/openzfs/module/zfs/zil.c
+++ b/sys/contrib/openzfs/module/zfs/zil.c
@@ -3317,21 +3317,6 @@ zil_commit(zilog_t *zilog, uint64_t foid)
 		return;
 	}
 
-	/*
-	 * The ->zl_suspend_lock rwlock ensures that all in-flight
-	 * zil_commit() operations finish before suspension begins and that
-	 * no more begin. Without it, it is possible for the scheduler to
-	 * preempt us right after the zilog->zl_suspend suspend check, run
-	 * another thread that runs zil_suspend() and after the other thread
-	 * has finished its call to zil_commit_impl(), resume this thread while
-	 * zil is suspended. This can trigger an assertion failure in
-	 * VERIFY(list_is_empty(&lwb->lwb_itxs)). If it is held, it means that
-	 * `zil_suspend()` is executing in another thread, so we go to
-	 * txg_wait_synced().
-	 */
-	if (!rw_tryenter(&zilog->zl_suspend_lock, RW_READER))
-		goto wait;
-
 	/*
 	 * If the ZIL is suspended, we don't want to dirty it by calling
 	 * zil_commit_itx_assign() below, nor can we write out
@@ -3340,14 +3325,11 @@ zil_commit(zilog_t *zilog, uint64_t foid)
 	 * semantics, and avoid calling those functions altogether.
 	 */
 	if (zilog->zl_suspend > 0) {
-		rw_exit(&zilog->zl_suspend_lock);
-wait:
 		txg_wait_synced(zilog->zl_dmu_pool, 0);
 		return;
 	}
 
 	zil_commit_impl(zilog, foid);
-	rw_exit(&zilog->zl_suspend_lock);
 }
 
 void
@@ -3612,8 +3594,6 @@ zil_alloc(objset_t *os, zil_header_t *zh_phys)
 	cv_init(&zilog->zl_cv_suspend, NULL, CV_DEFAULT, NULL);
 	cv_init(&zilog->zl_lwb_io_cv, NULL, CV_DEFAULT, NULL);
 
-	rw_init(&zilog->zl_suspend_lock, NULL, RW_DEFAULT, NULL);
-
 	return (zilog);
 }
 
@@ -3653,8 +3633,6 @@ zil_free(zilog_t *zilog)
 	cv_destroy(&zilog->zl_cv_suspend);
 	cv_destroy(&zilog->zl_lwb_io_cv);
 
-	rw_destroy(&zilog->zl_suspend_lock);
-
 	kmem_free(zilog, sizeof (zilog_t));
 }
 
@@ -3782,14 +3760,11 @@ zil_suspend(const char *osname, void **cookiep)
 		return (error);
 	zilog = dmu_objset_zil(os);
 
-	rw_enter(&zilog->zl_suspend_lock, RW_WRITER);
-
 	mutex_enter(&zilog->zl_lock);
 	zh = zilog->zl_header;
 
 	if (zh->zh_flags & ZIL_REPLAY_NEEDED) {		/* unplayed log */
 		mutex_exit(&zilog->zl_lock);
-		rw_exit(&zilog->zl_suspend_lock);
 		dmu_objset_rele(os, suspend_tag);
 		return (SET_ERROR(EBUSY));
 	}
@@ -3803,7 +3778,6 @@ zil_suspend(const char *osname, void **cookiep)
 	if (cookiep == NULL && !zilog->zl_suspending &&
 	    (zilog->zl_suspend > 0 || BP_IS_HOLE(&zh->zh_log))) {
 		mutex_exit(&zilog->zl_lock);
-		rw_exit(&zilog->zl_suspend_lock);
 		dmu_objset_rele(os, suspend_tag);
 		return (0);
 	}
@@ -3812,7 +3786,6 @@ zil_suspend(const char *osname, void **cookiep)
 	dsl_pool_rele(dmu_objset_pool(os), suspend_tag);
 
 	zilog->zl_suspend++;
-	rw_exit(&zilog->zl_suspend_lock);
 
 	if (zilog->zl_suspend > 1) {
 		/*

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