From patchwork Thu Aug 27 01:34:44 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Yafang Shao X-Patchwork-Id: 11739809 Return-Path: Received: from mail.kernel.org (pdx-korg-mail-1.web.codeaurora.org [172.30.200.123]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id D8C571751 for ; Thu, 27 Aug 2020 01:35:16 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 95B1D2087C for ; Thu, 27 Aug 2020 01:35:16 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="fkoa0php" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 95B1D2087C 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 C5FE66B0006; Wed, 26 Aug 2020 21:35:15 -0400 (EDT) Delivered-To: linux-mm-outgoing@kvack.org Received: by kanga.kvack.org (Postfix, from userid 40) id C10B36B0007; Wed, 26 Aug 2020 21:35:15 -0400 (EDT) X-Original-To: int-list-linux-mm@kvack.org X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id B02C48D0001; Wed, 26 Aug 2020 21:35:15 -0400 (EDT) X-Original-To: linux-mm@kvack.org X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0203.hostedemail.com [216.40.44.203]) by kanga.kvack.org (Postfix) with ESMTP id 9A8A26B0006 for ; Wed, 26 Aug 2020 21:35:15 -0400 (EDT) Received: from smtpin30.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay04.hostedemail.com (Postfix) with ESMTP id 631B91E11 for ; Thu, 27 Aug 2020 01:35:15 +0000 (UTC) X-FDA: 77194630590.30.seed49_5d13b9d27069 Received: from filter.hostedemail.com (10.5.16.251.rfc1918.com [10.5.16.251]) by smtpin30.hostedemail.com (Postfix) with ESMTP id 26428180B3AA7 for ; Thu, 27 Aug 2020 01:35:15 +0000 (UTC) X-Spam-Summary: 1,0,0,0f67c50623cec804,d41d8cd98f00b204,laoar.shao@gmail.com,,RULES_HIT:1:2:41:69:355:379:541:800:960:966:973:981:988:989:1260:1345:1359:1437:1605:1730:1747:1777:1792:2196:2198:2199:2200:2393:2559:2562:2892:2895:2897:3138:3139:3140:3141:3142:3664:3865:3866:3867:3868:3870:3871:3872:3874:4052:4250:4321:4385:4605:5007:6117:6119:6261:6653:7514:7903:8660:9010:9413:9592:10004:11026:11232:11233:11473:11658:11914:12043:12048:12291:12295:12296:12297:12438:12517:12519:12555:12683:12895:12986:13148:13230:14096:14394:14687:21080:21433:21444:21451:21611:21627:21666:21740:21809:21939:21972:21990:30034:30054:30064:30069:30070:30075,0,RBL:209.85.222.196:@gmail.com:.lbl8.mailshell.net-62.18.0.100 66.100.201.100;04yfizsm44di55xoadf8pmc8opbnwopb78tur1o9fps9ban1t5b8whifyr1pqrj.dj38ghzats44gx1h1uc1ux19jscje418w4per1ow754er3mc79os9ierpkg4i36.c-lbl8.mailshell.net-223.238.255.100,CacheIP:none,Bayesian:0.5,0.5,0.5,Netcheck:none,DomainCache:0,MSF:not bulk,SPF:fp,MSBL:0,DNSBL:none,Custo m_rules: X-HE-Tag: seed49_5d13b9d27069 X-Filterd-Recvd-Size: 12064 Received: from mail-qk1-f196.google.com (mail-qk1-f196.google.com [209.85.222.196]) by imf37.hostedemail.com (Postfix) with ESMTP for ; Thu, 27 Aug 2020 01:35:14 +0000 (UTC) Received: by mail-qk1-f196.google.com with SMTP id b14so4566703qkn.4 for ; Wed, 26 Aug 2020 18:35:14 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:to:cc:subject:date:message-id:in-reply-to:references; bh=S3mmr61aWECXok7VQ3kTB9s/VYemdPO0T7uhxid0NO0=; b=fkoa0phpc8sjFc9xsYTwjspDweCn1jXC65Gazqek6/dY9sL56oxZiw4CsETIpVXd82 Xn/CzYB2D2puI+3jglfkw0z6NfbxkJLGsvXHndinot7MFcg/fahOo49JewvgnETa4ubG 0w2iXhj4b8auC1ZVYgb4jeky4QlEGBTeeBb+rDGk2Hhk5BjhWRq3Qm7XzjZ38JeWrTM7 C4Nyku9kuerlI9t8N0gwsNPn65n2Lp00sY9+edtEXNWWfafIz1AAfbAL+0KvWGZfkFQS o9lUN3EUcmQbS212GCFvnQ8VkZX1wkpjWNcfg6ImSxKRBVXaumH4nDZa7jatIgmDo7Ui xJSA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references; bh=S3mmr61aWECXok7VQ3kTB9s/VYemdPO0T7uhxid0NO0=; b=WjA+R3S0crLA7VGiN0XnbTIx0gXNPCp4uXlOP6veSOT4Tms/usQSwK7kQZNkSLEE7n mlXXZouPytH6K8pmlJDH1rLdAcIx1Z3tHEvPfb8UAsEyVyDOvMIMIfZ191EBf8/QD2OL 2MUwCOc3/DhSDLgbR5Wu7UNbSlguMo39ajexBMmgLV78NfdEnOngTMQW4ywGyDHjDzW5 wJUPesk0aNz5rs/L0gx67iLHDXZqy/RXaGBK85ownmeQW5dqiE4MZv4Hr265l0TL87GR qRVQHtqZeFs95qJNEV5TShZI2m2MTdKKOxYbjCgrF2bT3R4IjuZdQyljxdSK3+11Innf Gd8w== X-Gm-Message-State: AOAM532U6RS3P0UvmEzn6D+NbIYVZbg4fSPh5ZbYDMcvkGkpHWBcLBSy SHuStx7tPa/P3nzgVPFwiyo= X-Google-Smtp-Source: ABdhPJxXPQqo9BPMTzBOGiK3uX4KyYanh8BvY02+bIZnOK71340bWLqI6riwFkf2OhWzEo5sbxZ9lQ== X-Received: by 2002:a05:620a:1122:: with SMTP id p2mr17491254qkk.45.1598492113964; Wed, 26 Aug 2020 18:35:13 -0700 (PDT) Received: from localhost.localdomain ([50.236.19.102]) by smtp.gmail.com with ESMTPSA id i18sm631846qtv.39.2020.08.26.18.35.09 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Wed, 26 Aug 2020 18:35:13 -0700 (PDT) From: Yafang Shao To: david@fromorbit.com, hch@infradead.org, darrick.wong@oracle.com, willy@infradead.org, mhocko@kernel.org, akpm@linux-foundation.org Cc: linux-xfs@vger.kernel.org, linux-fsdevel@vger.kernel.org, linux-mm@kvack.org, Yafang Shao Subject: [PATCH v7 2/2] xfs: avoid transaction reservation recursion Date: Thu, 27 Aug 2020 09:34:44 +0800 Message-Id: <20200827013444.24270-3-laoar.shao@gmail.com> X-Mailer: git-send-email 2.17.2 (Apple Git-113) In-Reply-To: <20200827013444.24270-1-laoar.shao@gmail.com> References: <20200827013444.24270-1-laoar.shao@gmail.com> X-Rspamd-Queue-Id: 26428180B3AA7 X-Spamd-Result: default: False [0.00 / 100.00] X-Rspamd-Server: rspam01 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: 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, four 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_update() Used in xfs_trans_roll() - xfs_trans_context_active() To check whehter current is in fs transcation or not This recursion check works for XFS only currently, so a new member is introduced in struct writeback_control. [1]. Below check is to avoid memory reclaim recursion. if (WARN_ON_ONCE((current->flags & (PF_MEMALLOC|PF_KSWAPD)) == PF_MEMALLOC)) goto redirty; Cc: Dave Chinner Cc: Christoph Hellwig Cc: Michal Hocko Cc: Darrick J. Wong Cc: Matthew Wilcox Signed-off-by: Yafang Shao --- fs/iomap/buffered-io.c | 4 ++-- fs/xfs/xfs_aops.c | 11 +++++++++-- fs/xfs/xfs_linux.h | 4 ---- fs/xfs/xfs_trans.c | 19 +++++++++---------- fs/xfs/xfs_trans.h | 30 ++++++++++++++++++++++++++++++ include/linux/writeback.h | 3 +++ 6 files changed, 53 insertions(+), 18 deletions(-) diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c index bcfc288dba3f..12e0fc4cb825 100644 --- a/fs/iomap/buffered-io.c +++ b/fs/iomap/buffered-io.c @@ -1500,9 +1500,9 @@ iomap_do_writepage(struct page *page, struct writeback_control *wbc, void *data) /* * Given that we do not allow direct reclaim to call us, we should - * never be called in a recursive filesystem reclaim context. + * never be called while in a filesystem transaction. */ - if (WARN_ON_ONCE(current->flags & PF_MEMALLOC_NOFS)) + if (WARN_ON_ONCE(wbc->fstrans_recursion)) goto redirty; /* diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c index b35611882ff9..9e2eedc9d208 100644 --- a/fs/xfs/xfs_aops.c +++ b/fs/xfs/xfs_aops.c @@ -62,7 +62,8 @@ xfs_setfilesize_trans_alloc( * We hand off the transaction to the completion thread now, so * clear the flag here. */ - current_restore_flags_nested(&tp->t_pflags, PF_MEMALLOC_NOFS); + xfs_trans_context_clear(tp); + return 0; } @@ -125,7 +126,7 @@ xfs_setfilesize_ioend( * thus we need to mark ourselves as being in a transaction manually. * Similarly for freeze protection. */ - current_set_flags_nested(&tp->t_pflags, PF_MEMALLOC_NOFS); + xfs_trans_context_set(tp); __sb_writers_acquired(VFS_I(ip)->i_sb, SB_FREEZE_FS); /* we abort the update if there was an IO error */ @@ -564,6 +565,9 @@ xfs_vm_writepage( { struct xfs_writepage_ctx wpc = { }; + if (xfs_trans_context_active()) + wbc->fstrans_recursion = 1; + return iomap_writepage(page, wbc, &wpc.ctx, &xfs_writeback_ops); } @@ -574,6 +578,9 @@ xfs_vm_writepages( { struct xfs_writepage_ctx wpc = { }; + if (xfs_trans_context_active()) + wbc->fstrans_recursion = 1; + xfs_iflags_clear(XFS_I(mapping->host), XFS_ITRUNCATED); return iomap_writepages(mapping, wbc, &wpc.ctx, &xfs_writeback_ops); } diff --git a/fs/xfs/xfs_linux.h b/fs/xfs/xfs_linux.h index ab737fed7b12..8a4f6db77e33 100644 --- a/fs/xfs/xfs_linux.h +++ b/fs/xfs/xfs_linux.h @@ -102,10 +102,6 @@ typedef __u32 xfs_nlink_t; #define xfs_cowb_secs xfs_params.cowb_timer.val #define current_cpu() (raw_smp_processor_id()) -#define current_set_flags_nested(sp, f) \ - (*(sp) = current->flags, current->flags |= (f)) -#define current_restore_flags_nested(sp, f) \ - (current->flags = ((current->flags & ~(f)) | (*(sp) & (f)))) #define NBBY 8 /* number of bits per byte */ diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c index ed72867b1a19..5f3a4ff51b3c 100644 --- a/fs/xfs/xfs_trans.c +++ b/fs/xfs/xfs_trans.c @@ -153,8 +153,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 +161,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 +237,6 @@ xfs_trans_reserve( tp->t_blk_res = 0; } - current_restore_flags_nested(&tp->t_pflags, PF_MEMALLOC_NOFS); - return error; } @@ -284,6 +278,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 +874,8 @@ __xfs_trans_commit( xfs_log_commit_cil(mp, tp, &commit_lsn, regrant); - current_restore_flags_nested(&tp->t_pflags, PF_MEMALLOC_NOFS); + if (!regrant) + xfs_trans_context_clear(tp); xfs_trans_free(tp); /* @@ -910,7 +907,8 @@ __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_context_clear(tp); xfs_trans_free_items(tp, !!error); xfs_trans_free(tp); @@ -971,7 +969,7 @@ xfs_trans_cancel( } /* mark this thread as no longer being in a transaction */ - current_restore_flags_nested(&tp->t_pflags, PF_MEMALLOC_NOFS); + xfs_trans_context_clear(tp); xfs_trans_free_items(tp, dirty); xfs_trans_free(tp); @@ -1013,6 +1011,7 @@ xfs_trans_roll( if (error) return error; + xfs_trans_context_update(trans, *tpp); /* * Reserve space in the log for the next transaction. * This also pushes items in the "AIL", the list of logged items, diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h index b752501818d2..f84b563438f6 100644 --- a/fs/xfs/xfs_trans.h +++ b/fs/xfs/xfs_trans.h @@ -243,4 +243,34 @@ void xfs_trans_buf_copy_type(struct xfs_buf *dst_bp, extern kmem_zone_t *xfs_trans_zone; +static inline void +xfs_trans_context_set(struct xfs_trans *tp) +{ + ASSERT(!current->journal_info); + current->journal_info = tp; + tp->t_pflags = memalloc_nofs_save(); +} + +static inline void +xfs_trans_context_update(struct xfs_trans *old, struct xfs_trans *new) +{ + ASSERT(current->journal_info == old); + current->journal_info = new; +} + +static inline void +xfs_trans_context_clear(struct xfs_trans *tp) +{ + ASSERT(current->journal_info == tp); + current->journal_info = NULL; + memalloc_nofs_restore(tp->t_pflags); +} + +static inline bool +xfs_trans_context_active(void) +{ + /* Use journal_info to indicate current is in a transaction */ + return current->journal_info != NULL; +} + #endif /* __XFS_TRANS_H__ */ diff --git a/include/linux/writeback.h b/include/linux/writeback.h index 8e5c5bb16e2d..33f6fb901a78 100644 --- a/include/linux/writeback.h +++ b/include/linux/writeback.h @@ -80,6 +80,9 @@ struct writeback_control { unsigned punt_to_cgroup:1; /* cgrp punting, see __REQ_CGROUP_PUNT */ + /* To avoid filesystem transaction reservation recursion */ + unsigned fstrans_recursion:1; + #ifdef CONFIG_CGROUP_WRITEBACK struct bdi_writeback *wb; /* wb this writeback is issued under */ struct inode *inode; /* inode being written out */