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.7 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,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 6F170C4361B for ; Tue, 15 Dec 2020 00:42:48 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id DCAC5224D2 for ; Tue, 15 Dec 2020 00:42:47 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org DCAC5224D2 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 ED2F06B0036; Mon, 14 Dec 2020 19:42:46 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id E82C46B005D; Mon, 14 Dec 2020 19:42:46 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id D98D26B0068; Mon, 14 Dec 2020 19:42:46 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0037.hostedemail.com [216.40.44.37]) by kanga.kvack.org (Postfix) with ESMTP id C38BB6B0036 for ; Mon, 14 Dec 2020 19:42:46 -0500 (EST) Received: from smtpin08.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay02.hostedemail.com (Postfix) with ESMTP id 92F213638 for ; Tue, 15 Dec 2020 00:42:46 +0000 (UTC) X-FDA: 77593666332.08.smell05_360c51c2741f Received: from filter.hostedemail.com (10.5.16.251.rfc1918.com [10.5.16.251]) by smtpin08.hostedemail.com (Postfix) with ESMTP id 6B2FE1819E62A for ; Tue, 15 Dec 2020 00:42:46 +0000 (UTC) X-HE-Tag: smell05_360c51c2741f X-Filterd-Recvd-Size: 7371 Received: from mail-il1-f194.google.com (mail-il1-f194.google.com [209.85.166.194]) by imf23.hostedemail.com (Postfix) with ESMTP for ; Tue, 15 Dec 2020 00:42:45 +0000 (UTC) Received: by mail-il1-f194.google.com with SMTP id q1so17649184ilt.6 for ; Mon, 14 Dec 2020 16:42:45 -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=AQ8sWYcc6++GKAUXS48FF0Tzgm+rRka2SqEuWOPEWKY=; b=a7EUEJTmS4Z2hWBbqkFKAJPVhs2fShTmmSbgtLqEecX4a3vy8EeaAVhvFPoScC2NKe OYH2bnMpZzgIRmGE0qVgxB8KYIMPkzt5hGwkIxzWhFNMERhKWKeprMFZ1zOMNS5ADeQY CS1Pd6dxpY4oytDY83HyWgrxuqxOzRVvUEkXzonznn5iPVkes6axrw6BjY43rEqx4gQ1 BMm1v7htdRt9LtLA21zhpaQrZ669TslYlPs73BIhT0miccmfHyEJrLzPrplzg4eOYNu5 RVR72C3nxnNiGpX6Rd8gY+Z+7az3ULXWYtUI6TPNm95yOM2HcadsTB6fhVXHf218VmKW PwJA== 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=AQ8sWYcc6++GKAUXS48FF0Tzgm+rRka2SqEuWOPEWKY=; b=i/a5rNPt/6sexyD67v4DV8iXhZy4SXUn9+eiiomE5f6GOoVZGjaataQK8MIgZEogrb Bd6JT4xwMS06urEvs824tgrLfXf1RIY7Z9amykDCH98y9QDWQwrMSY8v9ryu7IcegUHM 6IagFVXWw6hUYF2llWzvmeKAzyT+QHfAyBgkIzXH5NYYToqwqhGGveXz6j401+t8SnS8 uQjHToyg2teNlLb27+8NWicdi/f4znSPBA6aIEscIWhHuOH7Sj/hqGVddyjb4r1xw9Nq oYhEcuZ72UJ6Pf/D0GyvRDoXnNqb4jCopQ5dwtCKyqazTBQBCgUGjs4Z2cR8HS+jxnNE /tug== X-Gm-Message-State: AOAM532gmDzNWy7tD2uCfAHHtvTCZT/cwpchZScAWupCYY371jV1V9i4 ZQVXxyQ3HcXXPaqWqE/IeSnk0TU/iH/IAA1rMNw= X-Google-Smtp-Source: ABdhPJzEyU9JNu0fnNhUilOe+9j6k4b6rE292AN0DfQ1OORKBk0RqCWZOtCSN7nTK5XdJ0tiHr7sd+RqSKkpVM+935M= X-Received: by 2002:a05:6e02:68f:: with SMTP id o15mr39228993ils.93.1607992965458; Mon, 14 Dec 2020 16:42:45 -0800 (PST) MIME-Version: 1.0 References: <20201209131146.67289-1-laoar.shao@gmail.com> <20201209131146.67289-4-laoar.shao@gmail.com> <20201209195235.GN1943235@magnolia> <20201214210833.GE632069@dread.disaster.area> In-Reply-To: <20201214210833.GE632069@dread.disaster.area> From: Yafang Shao Date: Tue, 15 Dec 2020 08:42:08 +0800 Message-ID: Subject: Re: [PATCH v12 3/4] xfs: refactor the usage around xfs_trans_context_{set,clear} To: Dave Chinner Cc: "Darrick J. Wong" , Matthew Wilcox , 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 Tue, Dec 15, 2020 at 5:08 AM Dave Chinner wrote: > > On Sun, Dec 13, 2020 at 05:09:02PM +0800, Yafang Shao wrote: > > 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() > > No. You are trying to make this way more complex than it needs to be. > The logic in the core XFS code is *already correct* and all we need > to do is move that logic to wrapper functions, then slightly modify > the implementation inside the wrapper functions. > Thanks for the explanation. > > That is, xfs_trans_context_clear() should end up like this: > Agreed. > static inline void > xfs_trans_context_clear(struct xfs_trans *tp) > { > /* > * If xfs_trans_context_swap() handed the NOFS context to a > * new transaction we do not clear the context here. > */ > if (current->journal_info != tp) current->journal_info hasn't been used in patch #3, that will make patch #3 a little more complex. We have to do some workaround in patch #3. I will think about it. > return; > current->journal_info = NULL; > memalloc_nofs_restore(tp->t_pflags); > } > > -Dave. > -- > Dave Chinner > david@fromorbit.com -- Thanks Yafang