From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-16.7 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 19A04C07E95 for ; Wed, 14 Jul 2021 00:43:52 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id EA99860FED for ; Wed, 14 Jul 2021 00:43:51 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S237163AbhGNAql (ORCPT ); Tue, 13 Jul 2021 20:46:41 -0400 Received: from mail.kernel.org ([198.145.29.99]:33694 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S237113AbhGNAql (ORCPT ); Tue, 13 Jul 2021 20:46:41 -0400 Received: by mail.kernel.org (Postfix) with ESMTPSA id E11B660FED; Wed, 14 Jul 2021 00:43:50 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1626223431; bh=45aVF4oiqaStvIUS9oxCT34KIikAP4ygaukeXi2xJs8=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=UOpqvqNE7cROP4MQ0okdj8SlByIMGBba60I3i7+VaS17FgZXZowSZfpHqqXphO351 g2fQTHMOg+pkpgk8IGCg+1QAFxttM2OnhWKYZjdJVz9wy5UwBjFjulB5jRltTXgv7B HKrJq/xgsHXdXGqmGuOlwWy4dtYB7tCBb9Oa2BOwcV1vRUxiWiUG4gDf3yS0x/A0si 6k+8CLL7ABC4wFJMj843WRTQLtNcbZOYfY5ZwKFGwnEQS5z3Nez59mOBkWA5mYYxfX VVNkKZRgXmTl4nfsjIywQcVLPkl/f0vc6xp0ucJGVoxYDrYXy6vfBpr0loFnuV1Yru 23E95IK/YlQhQ== Date: Tue, 13 Jul 2021 17:43:50 -0700 From: "Darrick J. Wong" To: Dave Chinner Cc: xfs Subject: Re: [PATCH] xfs: keep the active perag reference between finish_one calls Message-ID: <20210714004350.GD22402@magnolia> References: <20210714000008.GC22402@magnolia> <20210714003125.GS664593@dread.disaster.area> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210714003125.GS664593@dread.disaster.area> Precedence: bulk List-ID: X-Mailing-List: linux-xfs@vger.kernel.org On Wed, Jul 14, 2021 at 10:31:25AM +1000, Dave Chinner wrote: > On Tue, Jul 13, 2021 at 05:00:08PM -0700, Darrick J. Wong wrote: > > From: Darrick J. Wong > > > > The refcount and rmap finish_one functions stash a btree cursor when > > there are multiple ->finish_one calls to be made in a single > > transaction. This mechanism is how we maintain the AGF lock between > > operations of a single intent item. Since ag btree cursors now need > > active references to perag structures, we must preserve the perag > > reference when we save the cursor. > > Hmmm. The cursor already carries it's own internal reference. So > this: > > > > > Signed-off-by: Darrick J. Wong > > --- > > fs/xfs/libxfs/xfs_refcount.c | 33 ++++++++++++++++++++------------- > > fs/xfs/libxfs/xfs_rmap.c | 8 +++++++- > > 2 files changed, 27 insertions(+), 14 deletions(-) > > > > diff --git a/fs/xfs/libxfs/xfs_refcount.c b/fs/xfs/libxfs/xfs_refcount.c > > index 860a0c9801ba..cfd98958d38c 100644 > > --- a/fs/xfs/libxfs/xfs_refcount.c > > +++ b/fs/xfs/libxfs/xfs_refcount.c > > @@ -1113,13 +1113,16 @@ xfs_refcount_finish_one_cleanup( > > int error) > > { > > struct xfs_buf *agbp; > > + struct xfs_perag *pag; > > > > if (rcur == NULL) > > return; > > agbp = rcur->bc_ag.agbp; > > + pag = rcur->bc_ag.pag; > > xfs_btree_del_cursor(rcur, error); > > if (error) > > xfs_trans_brelse(tp, agbp); > > + xfs_perag_put(pag); > > ... is just duplicating the reference the cursor already carries and > drops inside xfs_btree_del_cursor(). > > What problem is this actually fixing? LOL, I forgot that the btree cursor increments pag_ref on its own. Self-NAK for this patch then. --D > > > } > > > > /* > > @@ -1142,19 +1145,20 @@ xfs_refcount_finish_one( > > struct xfs_mount *mp = tp->t_mountp; > > struct xfs_btree_cur *rcur; > > struct xfs_buf *agbp = NULL; > > - int error = 0; > > + struct xfs_perag *pag; > > + unsigned long nr_ops = 0; > > + xfs_agnumber_t agno; > > xfs_agblock_t bno; > > xfs_agblock_t new_agbno; > > - unsigned long nr_ops = 0; > > int shape_changes = 0; > > - struct xfs_perag *pag; > > + int error = 0; > > > > - pag = xfs_perag_get(mp, XFS_FSB_TO_AGNO(mp, startblock)); > > + agno = XFS_FSB_TO_AGNO(mp, startblock); > > + pag = xfs_perag_get(mp, agno); > > bno = XFS_FSB_TO_AGBNO(mp, startblock); > > > > - trace_xfs_refcount_deferred(mp, XFS_FSB_TO_AGNO(mp, startblock), > > - type, XFS_FSB_TO_AGBNO(mp, startblock), > > - blockcount); > > + trace_xfs_refcount_deferred(mp, agno, type, > > + XFS_FSB_TO_AGBNO(mp, startblock), blockcount); > > > > if (XFS_TEST_ERROR(false, mp, XFS_ERRTAG_REFCOUNT_FINISH_ONE)) { > > error = -EIO; > > @@ -1174,14 +1178,16 @@ xfs_refcount_finish_one( > > *pcur = NULL; > > } > > if (rcur == NULL) { > > - error = xfs_alloc_read_agf(tp->t_mountp, tp, pag->pag_agno, > > + error = xfs_alloc_read_agf(mp, tp, agno, > > XFS_ALLOC_FLAG_FREEING, &agbp); > > Please don't revert these back to using a local variable. The next > step in cleaning up all these agf/agi read functions is to pass the > perag into them rather than the mount/agno pair.... > > > if (error) > > goto out_drop; > > > > + /* The cursor now owns the AGF buf and perag ref */ > > rcur = xfs_refcountbt_init_cursor(mp, tp, agbp, pag); > > rcur->bc_ag.refc.nr_ops = nr_ops; > > rcur->bc_ag.refc.shape_changes = shape_changes; > > + pag = NULL; > > The cursor takes it's own reference inside > xfs_refcountbt_init_cursor() that covers the perag for the life of > the cursor. THe local get/put covers the perag for this function, > and guarantees that the init_cursor() function can get it's own > reference without blocking because the perag already has active > references. > > Also, the cursor doesn't actually own the agbp at all. The active > reference to the agbp is actually carried by the transaction, not > the cursor, and if it is dirty when xfs_trans_brelse() is called, > then transaction reference is not dropped until > xfs_trans_commit()... > > Hence I think you're conflating "reference counted object" > with "cursor contains an object pointer" here, and as such the > statements about both objects are incorrect for different reasons... > > > } > > *pcur = rcur; > > > > @@ -1189,12 +1195,12 @@ xfs_refcount_finish_one( > > case XFS_REFCOUNT_INCREASE: > > error = xfs_refcount_adjust(rcur, bno, blockcount, &new_agbno, > > new_len, XFS_REFCOUNT_ADJUST_INCREASE, NULL); > > - *new_fsb = XFS_AGB_TO_FSB(mp, pag->pag_agno, new_agbno); > > + *new_fsb = XFS_AGB_TO_FSB(mp, agno, new_agbno); > > break; > > case XFS_REFCOUNT_DECREASE: > > error = xfs_refcount_adjust(rcur, bno, blockcount, &new_agbno, > > new_len, XFS_REFCOUNT_ADJUST_DECREASE, NULL); > > - *new_fsb = XFS_AGB_TO_FSB(mp, pag->pag_agno, new_agbno); > > + *new_fsb = XFS_AGB_TO_FSB(mp, agno, new_agbno); > > break; > > case XFS_REFCOUNT_ALLOC_COW: > > *new_fsb = startblock + blockcount; > > @@ -1211,10 +1217,11 @@ xfs_refcount_finish_one( > > error = -EFSCORRUPTED; > > } > > if (!error && *new_len > 0) > > - trace_xfs_refcount_finish_one_leftover(mp, pag->pag_agno, type, > > - bno, blockcount, new_agbno, *new_len); > > + trace_xfs_refcount_finish_one_leftover(mp, agno, type, bno, > > + blockcount, new_agbno, *new_len); > > out_drop: > > - xfs_perag_put(pag); > > + if (pag) > > + xfs_perag_put(pag); > > return error; > > Yup, this just smells wrong. The local get/put covers the reference > for the local function references, the reference gained in > _init_cursor ensures the perag is referenced for the life of the > cursor across multiple iterations (including duplicated child > cursors that also take their own references). > > Cheers, > > Dave. > -- > Dave Chinner > david@fromorbit.com