Re: [PATCH 1/4] xfs: don't nest icloglock inside ic_callback_lock

On Wed, Jun 23, 2021 at 08:42:47AM +1000, Dave Chinner wrote:

On Tue, Jun 22, 2021 at 08:38:56AM -0400, Brian Foster wrote:

quoted

On Tue, Jun 22, 2021 at 02:06:01PM +1000, Dave Chinner wrote:

quoted

From: Dave Chinner <redacted>

It's completely unnecessary because callbacks are added to iclogs
without holding the icloglock, hence no amount of ordering between
the icloglock and ic_callback_lock will order the removal of
callbacks from the iclog.

Signed-off-by: Dave Chinner <redacted>
---
 fs/xfs/xfs_log.c | 18 ++++--------------
 1 file changed, 4 insertions(+), 14 deletions(-)

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index e93cac6b5378..bb4390942275 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c

@@ -2773,11 +2773,8 @@ static void
 xlog_state_do_iclog_callbacks(
 	struct xlog		*log,
 	struct xlog_in_core	*iclog)
-		__releases(&log->l_icloglock)
-		__acquires(&log->l_icloglock)
 {
 	trace_xlog_iclog_callbacks_start(iclog, _RET_IP_);
-	spin_unlock(&log->l_icloglock);
 	spin_lock(&iclog->ic_callback_lock);
 	while (!list_empty(&iclog->ic_callbacks)) {
 		LIST_HEAD(tmp);

@@ -2789,12 +2786,6 @@ xlog_state_do_iclog_callbacks(
 		spin_lock(&iclog->ic_callback_lock);
 	}
 
-	/*
-	 * Pick up the icloglock while still holding the callback lock so we
-	 * serialise against anyone trying to add more callbacks to this iclog
-	 * now we've finished processing.
-	 */

This makes sense wrt to the current locking, but I'd like to better
understand what's being removed. When would we add callbacks to an iclog
that's made it to this stage (i.e., already completed I/O)? Is this some
historical case or attempt at defensive logic?

This was done in 2008. It's very likely that, at the time, nobody
(including me) understood the iclog state machine well enough to
determine if we could race with adding iclogs at this time. Maybe
they did race and this was a bandaid over, say, a shutdown race condition.
But, more likely, it was just defensive to try to prevent callbacks
from being added before the iclog was marked ACTIVE again...

Really, though, nobody is going to be able to tell you why the code
was written like this in the first place because even the author
doesn't remember...

Ok, just wanted to be sure there wasn't some context I was missing. The
patch seems fine to me:

Reviewed-by: Brian Foster <redacted>

-Dave.
-- 
Dave Chinner
david@fromorbit.com