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=-12.8 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS 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 DB543C433FE for ; Sun, 13 Dec 2020 09:09:41 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 4B94C2312C for ; Sun, 13 Dec 2020 09:09:41 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 4B94C2312C Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=gmail.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 90D846B0036; Sun, 13 Dec 2020 04:09:40 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 89ABF6B005C; Sun, 13 Dec 2020 04:09:40 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 761446B005D; Sun, 13 Dec 2020 04:09:40 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0068.hostedemail.com [216.40.44.68]) by kanga.kvack.org (Postfix) with ESMTP id 5D5646B0036 for ; Sun, 13 Dec 2020 04:09:40 -0500 (EST) Received: from smtpin21.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay01.hostedemail.com (Postfix) with ESMTP id 1F638180AD837 for ; Sun, 13 Dec 2020 09:09:40 +0000 (UTC) X-FDA: 77587686120.21.cry46_5707e5b27411 Received: from filter.hostedemail.com (10.5.16.251.rfc1918.com [10.5.16.251]) by smtpin21.hostedemail.com (Postfix) with ESMTP id E5DD4180442DE for ; Sun, 13 Dec 2020 09:09:39 +0000 (UTC) X-HE-Tag: cry46_5707e5b27411 X-Filterd-Recvd-Size: 8188 Received: from mail-il1-f195.google.com (mail-il1-f195.google.com [209.85.166.195]) by imf24.hostedemail.com (Postfix) with ESMTP for ; Sun, 13 Dec 2020 09:09:39 +0000 (UTC) Received: by mail-il1-f195.google.com with SMTP id 75so952459ilv.13 for ; Sun, 13 Dec 2020 01:09:39 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=D7GTvYEsVIVwjvV23D7RG5wUssdQLZojxIkxkNlOkWI=; b=SPCtjOSGrOXBOb7FXLg34p2igyWU588Jy1ITL3uDxg3KV9j9hqegwxfkW0PMA6cx/V EOIY59QB7x7/mp9zDB3fIYNm11dir6pHU/4ExH2vXTUGRd00cXGfTzPxzChX4Qv4yd5v xiIf3SCDIEbRRDnyfjhmXzSHdwK73M3q/6Aq28OeqAkDqKi3lGjNUviM7naD4H8dfA5m sO9n7Wj6Uwr9uIgS7bzSu2sstoR4o/whHfcY+rwcirUgxtAcdL9JArIIGwmtes1AGVKF yJ3o0HMnr4p9QSg2B/iMtWAAE0sHf4p7GDyv+C3lTz4hy+xxfeoj4sNT16ILLJa9PGKh YmiA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=D7GTvYEsVIVwjvV23D7RG5wUssdQLZojxIkxkNlOkWI=; b=kIEGdc4fC1haQKeUg8LPlxog7ZFyZr1THr1ndaZdPQdVo9HdKcn6Ifzzo4JeCwnMHN DvX5y/mTrTKiKjd8JiPAs+nQGhhLcX53Le0CBshuokEpR2SIcavHvnR67PK3gK13SZeR MbKk/XKyVRxnZCe9nkfd0taZ+gx77Ysj81OkQO39v0fnH06D23/PPwSdbgWb5SZ0S9Ik NeI9H3/UG2UrXpcw4JKtuYqnlPdFkzRXMWoIXq/7jdoFOelXdujYSKZgGkUvymjxuZMS /bLOvB5HFqSPOHVsMHAKBfAiNYznF4kxC8IBcWe5jrG3pSzbJU2ayL7APE0wBfrAzhOb BN+g== X-Gm-Message-State: AOAM532Ml39JheCGnQ9VchotXOUYGzw6vITb/9UJ5vsUEfxgLZ3aG4oT 6sxejg77mamu4jKEpQOCqN8Xkf7Y2pJiKVdTCfw= X-Google-Smtp-Source: ABdhPJxSjfFeaIvKMeUtKyBFgh0BXATRF6yhOG6j7VgiMcuP7Alm48/FakLeqW9J+sdvHIt3tJBWychg6p4AMtF5CBw= X-Received: by 2002:a05:6e02:68f:: with SMTP id o15mr27943384ils.93.1607850578855; Sun, 13 Dec 2020 01:09:38 -0800 (PST) MIME-Version: 1.0 References: <20201209131146.67289-1-laoar.shao@gmail.com> <20201209131146.67289-4-laoar.shao@gmail.com> <20201209195235.GN1943235@magnolia> In-Reply-To: <20201209195235.GN1943235@magnolia> From: Yafang Shao Date: Sun, 13 Dec 2020 17:09:02 +0800 Message-ID: Subject: Re: [PATCH v12 3/4] xfs: refactor the usage around xfs_trans_context_{set,clear} To: "Darrick J. Wong" Cc: Matthew Wilcox , Dave Chinner , Christoph Hellwig , Michal Hocko , Andrew Morton , David Howells , jlayton@redhat.com, linux-fsdevel@vger.kernel.org, linux-cachefs@redhat.com, linux-xfs@vger.kernel.org, Linux MM , Christoph Hellwig Content-Type: text/plain; charset="UTF-8" X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Thu, Dec 10, 2020 at 3:52 AM Darrick J. Wong wrote: > > On Wed, Dec 09, 2020 at 09:11:45PM +0800, Yafang Shao wrote: > > The xfs_trans context should be active after it is allocated, and > > deactive when it is freed. > > > > So these two helpers are refactored as, > > - xfs_trans_context_set() > > Used in xfs_trans_alloc() > > - xfs_trans_context_clear() > > Used in xfs_trans_free() > > > > This patch is based on Darrick's work to fix the issue in xfs/141 in the > > earlier version. [1] > > > > 1. https://lore.kernel.org/linux-xfs/20201104001649.GN7123@magnolia > > > > Cc: Darrick J. Wong > > Cc: Matthew Wilcox (Oracle) > > Cc: Christoph Hellwig > > Cc: Dave Chinner > > Signed-off-by: Yafang Shao > > --- > > fs/xfs/xfs_trans.c | 28 +++++++++++++++------------- > > 1 file changed, 15 insertions(+), 13 deletions(-) > > > > diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c > > index 11d390f0d3f2..4f4645329bb2 100644 > > --- a/fs/xfs/xfs_trans.c > > +++ b/fs/xfs/xfs_trans.c > > @@ -67,6 +67,17 @@ xfs_trans_free( > > xfs_extent_busy_sort(&tp->t_busy); > > xfs_extent_busy_clear(tp->t_mountp, &tp->t_busy, false); > > > > + > > + /* Detach the transaction from this thread. */ > > + ASSERT(current->journal_info != NULL); > > + /* > > + * The PF_MEMALLOC_NOFS is bound to the transaction itself instead > > + * of the reservation, so we need to check if tp is still the > > + * current transaction before clearing the flag. > > + */ > > + if (current->journal_info == tp) > > Um, you don't start setting journal_info until the next patch, so this > means that someone who lands on this commit with git bisect will have a > xfs with broken logic. > > Because this is the patch that changes where we set and restore NOFS > context, I think you have to introduce xfs_trans_context_swap here, > and not in the next patch. > Thanks for the review. I will change it in the next version. > I also think the _swap routine has to move the old NOFS state to the > new transaction's t_pflags, Sure > and then set NOFS in the old transaction's > t_pflags so that when we clear the context on the old transaction we > don't actually change the thread's NOFS state. > Both thread's NOFS state and thead's journal_info state can't be changed in that case, right ? So should it better be, __xfs_trans_commit(tp, regrant) xfs_trans_free(tp, regrant) if (!regrant). // don't clear the xfs_trans_context if regrant is true. xfs_trans_context_clear() > --D > > > + xfs_trans_context_clear(tp); > > + > > trace_xfs_trans_free(tp, _RET_IP_); > > if (!(tp->t_flags & XFS_TRANS_NO_WRITECOUNT)) > > sb_end_intwrite(tp->t_mountp->m_super); > > @@ -153,9 +164,6 @@ xfs_trans_reserve( > > int error = 0; > > bool rsvd = (tp->t_flags & XFS_TRANS_RESERVE) != 0; > > > > - /* Mark this thread as being in a transaction */ > > - xfs_trans_context_set(tp); > > - > > /* > > * Attempt to reserve the needed disk blocks by decrementing > > * the number needed from the number available. This will > > @@ -163,10 +171,9 @@ xfs_trans_reserve( > > */ > > if (blocks > 0) { > > error = xfs_mod_fdblocks(mp, -((int64_t)blocks), rsvd); > > - if (error != 0) { > > - xfs_trans_context_clear(tp); > > + if (error != 0) > > return -ENOSPC; > > - } > > + > > tp->t_blk_res += blocks; > > } > > > > @@ -241,8 +248,6 @@ xfs_trans_reserve( > > tp->t_blk_res = 0; > > } > > > > - xfs_trans_context_clear(tp); > > - > > return error; > > } > > > > @@ -284,6 +289,8 @@ xfs_trans_alloc( > > INIT_LIST_HEAD(&tp->t_dfops); > > tp->t_firstblock = NULLFSBLOCK; > > > > + /* Mark this thread as being in a transaction */ > > + xfs_trans_context_set(tp); > > error = xfs_trans_reserve(tp, resp, blocks, rtextents); > > if (error) { > > xfs_trans_cancel(tp); > > @@ -878,7 +885,6 @@ __xfs_trans_commit( > > > > xfs_log_commit_cil(mp, tp, &commit_lsn, regrant); > > > > - xfs_trans_context_clear(tp); > > xfs_trans_free(tp); > > > > /* > > @@ -911,7 +917,6 @@ __xfs_trans_commit( > > tp->t_ticket = NULL; > > } > > > > - xfs_trans_context_clear(tp); > > xfs_trans_free_items(tp, !!error); > > xfs_trans_free(tp); > > > > @@ -971,9 +976,6 @@ xfs_trans_cancel( > > tp->t_ticket = NULL; > > } > > > > - /* mark this thread as no longer being in a transaction */ > > - xfs_trans_context_clear(tp); > > - > > xfs_trans_free_items(tp, dirty); > > xfs_trans_free(tp); > > } > > -- > > 2.18.4 > > -- Thanks Yafang