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=-13.8 required=3.0 tests=BAYES_00, 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 CFC4BC4361B for ; Sun, 6 Dec 2020 21:04:55 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 369E822E03 for ; Sun, 6 Dec 2020 21:04:54 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 369E822E03 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=fromorbit.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 6D72B8D0002; Sun, 6 Dec 2020 16:04:54 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 660CD8D0001; Sun, 6 Dec 2020 16:04:54 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 48B538D0002; Sun, 6 Dec 2020 16:04:54 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0147.hostedemail.com [216.40.44.147]) by kanga.kvack.org (Postfix) with ESMTP id 29CBF8D0001 for ; Sun, 6 Dec 2020 16:04:54 -0500 (EST) Received: from smtpin02.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay05.hostedemail.com (Postfix) with ESMTP id D78B7181AEF07 for ; Sun, 6 Dec 2020 21:04:53 +0000 (UTC) X-FDA: 77564086866.02.suit04_130cdaa273d8 Received: from filter.hostedemail.com (10.5.16.251.rfc1918.com [10.5.16.251]) by smtpin02.hostedemail.com (Postfix) with ESMTP id B45B910097AA0 for ; Sun, 6 Dec 2020 21:04:53 +0000 (UTC) X-HE-Tag: suit04_130cdaa273d8 X-Filterd-Recvd-Size: 10451 Received: from mail108.syd.optusnet.com.au (mail108.syd.optusnet.com.au [211.29.132.59]) by imf20.hostedemail.com (Postfix) with ESMTP for ; Sun, 6 Dec 2020 21:04:52 +0000 (UTC) Received: from dread.disaster.area (pa49-179-6-140.pa.nsw.optusnet.com.au [49.179.6.140]) by mail108.syd.optusnet.com.au (Postfix) with ESMTPS id F08A11AC887; Mon, 7 Dec 2020 08:04:48 +1100 (AEDT) Received: from dave by dread.disaster.area with local (Exim 4.92.3) (envelope-from ) id 1km1D3-001Dtb-0V; Mon, 07 Dec 2020 08:04:45 +1100 Date: Mon, 7 Dec 2020 08:04:45 +1100 From: Dave Chinner To: Yafang Shao Cc: darrick.wong@oracle.com, willy@infradead.org, hch@infradead.org, mhocko@kernel.org, 000akpm@linux-foundation.org, dhowells@redhat.com, jlayton@redhat.com, linux-fsdevel@vger.kernel.org, linux-cachefs@redhat.com, linux-xfs@vger.kernel.org, linux-mm@kvack.org, Christoph Hellwig Subject: Re: [PATCH v9 2/2] xfs: avoid transaction reservation recursion Message-ID: <20201206210445.GI3913616@dread.disaster.area> References: <20201206064046.2921-1-laoar.shao@gmail.com> <20201206064046.2921-3-laoar.shao@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20201206064046.2921-3-laoar.shao@gmail.com> X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.3 cv=YKPhNiOx c=1 sm=1 tr=0 cx=a_idp_d a=uDU3YIYVKEaHT0eX+MXYOQ==:117 a=uDU3YIYVKEaHT0eX+MXYOQ==:17 a=kj9zAlcOel0A:10 a=zTNgK-yGK50A:10 a=VwQbUJbxAAAA:8 a=yPCof4ZbAAAA:8 a=JfrnYn6hAAAA:8 a=7-415B0cAAAA:8 a=20KFwNOVAAAA:8 a=pGLkceISAAAA:8 a=FYxHPdz51nehZZpLQW4A:9 a=CjuIK1q_8ugA:10 a=AjGcO6oz07-iQ99wixmX:22 a=1CNFftbPRP8L7MoqJWF3:22 a=biEYGPWJfzWAr4FL6Ov7:22 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 Sun, Dec 06, 2020 at 02:40:46PM +0800, Yafang Shao wrote: > PF_FSTRANS which is used to avoid transaction reservation recursion, is > dropped since commit 9070733b4efa ("xfs: abstract PF_FSTRANS to > PF_MEMALLOC_NOFS") and commit 7dea19f9ee63 ("mm: introduce > memalloc_nofs_{save,restore} API") and replaced by PF_MEMALLOC_NOFS which > means to avoid filesystem reclaim recursion. That change is subtle. > Let's take the exmple of the check of WARN_ON_ONCE(current->flags & > PF_MEMALLOC_NOFS)) to explain why this abstraction from PF_FSTRANS to > PF_MEMALLOC_NOFS is not proper. > Below comment is quoted from Dave, > > It wasn't for memory allocation recursion protection in XFS - it was for > > transaction reservation recursion protection by something trying to flush > > data pages while holding a transaction reservation. Doing > > this could deadlock the journal because the existing reservation > > could prevent the nested reservation for being able to reserve space > > in the journal and that is a self-deadlock vector. > > IOWs, this check is not protecting against memory reclaim recursion > > bugs at all (that's the previous check [1]). This check is > > protecting against the filesystem calling writepages directly from a > > context where it can self-deadlock. > > So what we are seeing here is that the PF_FSTRANS -> > > PF_MEMALLOC_NOFS abstraction lost all the actual useful information > > about what type of error this check was protecting against. > > As a result, we should reintroduce PF_FSTRANS. As current->journal_info > isn't used in XFS, we can reuse it to indicate whehter the task is in > fstrans or not, Per Willy. To achieve that, some new helpers are introduce > in this patch, per Dave: > - xfs_trans_context_set() > Used in xfs_trans_alloc() > - xfs_trans_context_clear() > Used in xfs_trans_commit() and xfs_trans_cancel() > - xfs_trans_context_active() > To check whehter current is in fs transcation or not > > Darrick helped fix the error occurred in xfs/141.[2] > > No obvious error occurred when I run xfstests in my test machine. > > [1]. Below check is to avoid memory reclaim recursion. > if (WARN_ON_ONCE((current->flags & (PF_MEMALLOC|PF_KSWAPD)) == > PF_MEMALLOC)) > goto redirty; > > [2]. https://lore.kernel.org/linux-xfs/20201104001649.GN7123@magnolia/ > > Cc: Darrick J. Wong > Cc: Matthew Wilcox (Oracle) > Cc: Christoph Hellwig > Cc: Dave Chinner > Cc: Michal Hocko > Cc: David Howells > Cc: Jeff Layton > Signed-off-by: Yafang Shao > --- > fs/iomap/buffered-io.c | 7 ------- > fs/xfs/xfs_aops.c | 23 +++++++++++++++++++++-- > fs/xfs/xfs_linux.h | 4 ---- > fs/xfs/xfs_trans.c | 25 +++++++++++++------------ > fs/xfs/xfs_trans.h | 23 +++++++++++++++++++++++ > 5 files changed, 57 insertions(+), 25 deletions(-) > > diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c > index 10cc7979ce38..3c53fa6ce64d 100644 > --- a/fs/iomap/buffered-io.c > +++ b/fs/iomap/buffered-io.c > @@ -1458,13 +1458,6 @@ iomap_do_writepage(struct page *page, struct writeback_control *wbc, void *data) > PF_MEMALLOC)) > goto redirty; > > - /* > - * Given that we do not allow direct reclaim to call us, we should > - * never be called in a recursive filesystem reclaim context. > - */ > - if (WARN_ON_ONCE(current->flags & PF_MEMALLOC_NOFS)) > - goto redirty; > - So this would have given us a warning if something went wrong... > @@ -568,6 +569,16 @@ xfs_vm_writepage( > { > struct xfs_writepage_ctx wpc = { }; > > + /* > + * Given that we do not allow direct reclaim to call us, we should > + * never be called while in a filesystem transaction. > + */ > + if (xfs_trans_context_active()) { > + redirty_page_for_writepage(wbc, page); > + unlock_page(page); > + return 0; > + } > + > return iomap_writepage(page, wbc, &wpc.ctx, &xfs_writeback_ops); > } > > @@ -579,6 +590,14 @@ xfs_vm_writepages( > struct xfs_writepage_ctx wpc = { }; > > xfs_iflags_clear(XFS_I(mapping->host), XFS_ITRUNCATED); > + > + /* > + * Given that we do not allow direct reclaim to call us, we should > + * never be called while in a filesystem transaction. > + */ > + if (xfs_trans_context_active()) > + return 0; > + > return iomap_writepages(mapping, wbc, &wpc.ctx, &xfs_writeback_ops); > } But neither of these will trigger a warning at all, so we won't know that there's a bug in the code at all. Given this is primarily a "we have a bug in the code" deadlock avoidance check, we really need the noisy warnings if these fire... > diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c > index c94e71f741b6..09ae5c181299 100644 > --- a/fs/xfs/xfs_trans.c > +++ b/fs/xfs/xfs_trans.c > @@ -67,6 +67,11 @@ 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); > + if (current->journal_info == tp) > + xfs_trans_context_clear(tp); >From the context of this patch and the code it is replacing, I have no idea why this condition could occur, so this needs a comment explaining when current->journal_info is not equal to the transaction we are freeing. > + > trace_xfs_trans_free(tp, _RET_IP_); > if (!(tp->t_flags & XFS_TRANS_NO_WRITECOUNT)) > sb_end_intwrite(tp->t_mountp->m_super); > @@ -119,7 +124,11 @@ 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; > + > + /* Associate the new transaction with this thread. */ > + ASSERT(current->journal_info == tp); > ntp->t_pflags = tp->t_pflags; > + current->journal_info = ntp; Why is this open coded and not in a helper like all the current->journal_info manipulations? Something like xfs_trans_context_swap(tp, ntp) with a comment explaining that it is used to transfer the transaction context when rolling a permanent transaction? > > /* move deferred ops over to the new tp */ > xfs_defer_move(ntp, tp); > @@ -153,8 +162,6 @@ xfs_trans_reserve( > int error = 0; > bool rsvd = (tp->t_flags & XFS_TRANS_RESERVE) != 0; > > - /* Mark this thread as being in a transaction */ > - current_set_flags_nested(&tp->t_pflags, PF_MEMALLOC_NOFS); > > /* > * Attempt to reserve the needed disk blocks by decrementing > @@ -163,10 +170,8 @@ xfs_trans_reserve( > */ > if (blocks > 0) { > error = xfs_mod_fdblocks(mp, -((int64_t)blocks), rsvd); > - if (error != 0) { > - current_restore_flags_nested(&tp->t_pflags, PF_MEMALLOC_NOFS); > + if (error != 0) > return -ENOSPC; > - } > tp->t_blk_res += blocks; > } > > @@ -241,8 +246,6 @@ xfs_trans_reserve( > tp->t_blk_res = 0; > } > > - current_restore_flags_nested(&tp->t_pflags, PF_MEMALLOC_NOFS); > - > return error; > } > > @@ -284,6 +287,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); This refactoring should probably be a separate patch, done first. > @@ -878,7 +883,6 @@ __xfs_trans_commit( > > xfs_log_commit_cil(mp, tp, &commit_lsn, regrant); > > - current_restore_flags_nested(&tp->t_pflags, PF_MEMALLOC_NOFS); > xfs_trans_free(tp); > > /* > @@ -910,7 +914,7 @@ __xfs_trans_commit( > xfs_log_ticket_ungrant(mp->m_log, tp->t_ticket); > tp->t_ticket = NULL; > } > - current_restore_flags_nested(&tp->t_pflags, PF_MEMALLOC_NOFS); > + > xfs_trans_free_items(tp, !!error); > xfs_trans_free(tp); > > @@ -970,9 +974,6 @@ xfs_trans_cancel( > tp->t_ticket = NULL; > } > > - /* mark this thread as no longer being in a transaction */ > - current_restore_flags_nested(&tp->t_pflags, PF_MEMALLOC_NOFS); > - > xfs_trans_free_items(tp, dirty); > xfs_trans_free(tp); > } And moving current_restore_flags_nested() into xfs_trans_free() should also probably be a separate patch. That way this patch is simply changing all the flags manipulations to use the new wrappers, and not a mix of refactoring and API rework... Cheers, Dave. -- Dave Chinner david@fromorbit.com