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=-7.7 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS,URIBL_BLOCKED autolearn=no 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 C58CDC2D0E4 for ; Sat, 19 Dec 2020 00:28:40 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 542E123BA9 for ; Sat, 19 Dec 2020 00:28:40 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 542E123BA9 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 EE6BE6B0068; Fri, 18 Dec 2020 19:28:39 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id E1FE66B006C; Fri, 18 Dec 2020 19:28:39 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id CE6476B006E; Fri, 18 Dec 2020 19:28:39 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0064.hostedemail.com [216.40.44.64]) by kanga.kvack.org (Postfix) with ESMTP id A3F256B0068 for ; Fri, 18 Dec 2020 19:28:39 -0500 (EST) Received: from smtpin09.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay01.hostedemail.com (Postfix) with ESMTP id 67F061801D519 for ; Sat, 19 Dec 2020 00:28:39 +0000 (UTC) X-FDA: 77608145958.09.taste13_020e45d27441 Received: from filter.hostedemail.com (10.5.16.251.rfc1918.com [10.5.16.251]) by smtpin09.hostedemail.com (Postfix) with ESMTP id 5682818036D48 for ; Sat, 19 Dec 2020 00:28:39 +0000 (UTC) X-HE-Tag: taste13_020e45d27441 X-Filterd-Recvd-Size: 7239 Received: from mail-il1-f182.google.com (mail-il1-f182.google.com [209.85.166.182]) by imf25.hostedemail.com (Postfix) with ESMTP for ; Sat, 19 Dec 2020 00:28:38 +0000 (UTC) Received: by mail-il1-f182.google.com with SMTP id q5so3783385ilc.10 for ; Fri, 18 Dec 2020 16:28:38 -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=jPVM+h1yU+rYtYYm/wlc4NEIQ0PzwsacEydpO06K9XE=; b=kTZ3m3xvfqnp9olK7BWcZoiF8s6BlP1qTGMLUdT+kfIF5NEFTompVw3BrK8ewB1xVj Rj1ys2nHKp8Z3ndWgBi1pOysfG1C1ny8hl8Xoo1JWfTA55QaerWD6Ctfyds6uFkaobmU +92DpR8JuhGgWaloC7F7qIddTvnw21bXBllcSYHt/vWsFQFPNdlQHDTmGe+klTnxM8KB QmpfZkODK6Q1kB+SQY4z/xXojN3STKg73hDhBuVBH2LLR9MEodWFI1ezaN8wPBmYTs4v xQKJ+/sYHW3dajEAB3SA74PRpuFZlnPMliNoQVow/0GG91tmsBy/yP8c2EgSRjPIy8V/ tI0Q== 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=jPVM+h1yU+rYtYYm/wlc4NEIQ0PzwsacEydpO06K9XE=; b=SRPu07YgXWeqkYBqbY6YeUoBWjbQEXSKnkxzwC2um42g8vv8SwEs7sOiYzZj/B9Y2E T8diZu9ofaGHZIbu+lM721fKpkmJDfyClNOoJyQjHGZN73J06x/yIqXoXWP1h9qICqUP JaCGhcVZ3xBysZumBpUPyzBAMQCiGPq/MCvEOTWKTjYAuSThqNnpxkgIzOzqihpbHIso NnSAwrSdu1izsUNN3kdMyfyX+4aAFv0+qwBRggEdkfL4GWxTHwkJin/MEqr8vg42RSb+ thd9XE/UPsGpKk36DkO03qJ/qFAKBOSddu53MzBupgYsI2g5I3drY/eJy1tN2rxwGwPE jwsA== X-Gm-Message-State: AOAM530MSCSX10EtQxOTWgjkV1TdGW0AdNGdaz3GEksL3lLPEn/5eZmO TsCyFyZKEE3ycX/EykHXaQKezxW3kblVyF+Mtdw= X-Google-Smtp-Source: ABdhPJz+KJh7dGrxqQVVQCtCBTrQGXNchHDSEESjWTprd7z6kNilqN06QJJfnHMx9qKQkDTMmhKgdvbax3MdOwj737w= X-Received: by 2002:a92:489b:: with SMTP id j27mr6630232ilg.168.1608337718154; Fri, 18 Dec 2020 16:28:38 -0800 (PST) MIME-Version: 1.0 References: <20201217011157.92549-1-laoar.shao@gmail.com> <20201217011157.92549-4-laoar.shao@gmail.com> <20201217221509.GQ632069@dread.disaster.area> In-Reply-To: <20201217221509.GQ632069@dread.disaster.area> From: Yafang Shao Date: Sat, 19 Dec 2020 08:28:02 +0800 Message-ID: Subject: Re: [PATCH v13 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 Fri, Dec 18, 2020 at 6:15 AM Dave Chinner wrote: > > On Thu, Dec 17, 2020 at 09:11:56AM +0800, Yafang Shao wrote: > > The xfs_trans context should be active after it is allocated, and > > deactive when it is freed. > > The rolling transaction should be specially considered, because in the > > case when we clear the old transaction the thread's NOFS state shouldn't > > be changed, as a result we have to set NOFS in the old transaction's > > t_pflags in xfs_trans_context_swap(). > > > > So these helpers are refactored as, > > - xfs_trans_context_set() > > Used in xfs_trans_alloc() > > - xfs_trans_context_clear() > > Used in xfs_trans_free() > > > > And a new helper is instroduced to handle the rolling transaction, > > - xfs_trans_context_swap() > > Used in rolling transaction > > > > 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 > > As I said in my last comments, this change of logic is not > necessary. All we need to do is transfer the NOFS state to the new > transactions and *remove it from the old one*. > Thanks for the explanation, I will change it. > IOWs, all this patch should do is: > > > @@ -119,7 +123,9 @@ xfs_trans_dup( > > > > ntp->t_rtx_res = tp->t_rtx_res - tp->t_rtx_res_used; > > tp->t_rtx_res = tp->t_rtx_res_used; > > - ntp->t_pflags = tp->t_pflags; > > + > > + /* Associate the new transaction with this thread. */ > > + xfs_trans_context_swap(tp, ntp); > > > > /* move deferred ops over to the new tp */ > > xfs_defer_move(ntp, tp); > > This, and > > > diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h > > index 44b11c64a15e..12380eaaf7ce 100644 > > --- a/fs/xfs/xfs_trans.h > > +++ b/fs/xfs/xfs_trans.h > > @@ -280,4 +280,17 @@ xfs_trans_context_clear(struct xfs_trans *tp) > > memalloc_nofs_restore(tp->t_pflags); > > } > > > > +static inline void > > +xfs_trans_context_swap(struct xfs_trans *tp, struct xfs_trans *ntp) > > +{ > > introduce this wrapper. > > > + ntp->t_pflags = tp->t_pflags; > > + /* > > + * For the rolling transaction, we have to 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. > > + */ > > + tp->t_pflags = current->flags | PF_MEMALLOC_NOFS; > > +} > > But not with this implementation. > > Think for a minute, please. All we want to do is avoid clearing > the nofs state when we call xfs_trans_context_clear(tp) if the state > has been handed to another transaction. > > Your current implementation hands the state to ntp, but *then leaves > it on tp* as well. So then you hack a PF_MEMALLOC_NOFS flag into > tp->t_pflags so that it doesn't clear that flag (abusing the masking > done during clearing). That's just nasty because it relies on > internal memalloc_nofs_restore() details for correct functionality. > > The obvious solution: we've moved the saved process state to a > different context, so it is no longer needed for the current > transaction we are about to commit. So How about just clearing the > saved state from the original transaction when swappingi like so: > > static inline void > xfs_trans_context_swap(struct xfs_trans *tp, struct xfs_trans *ntp) > { > ntp->t_pflags = tp->t_pflags; > tp->t_flags = 0; > } > > And now, when we go to clear the transaction context, we can simply > do this: > > static inline void > xfs_trans_context_clear(struct xfs_trans *tp) > { > if (tp->t_pflags) > memalloc_nofs_restore(tp->t_pflags); > } > > and the problem is solved. The NOFS state will follow the active > transaction and not be reset until the entire transaction chain is > completed. > > In the next patch you can go and introduce current->journal_info > into just the wrapper functions, maintaining the same overall > logic. > > -Dave. > -- > Dave Chinner > david@fromorbit.com -- Thanks Yafang