| Message ID | 20201206064046.2921-3-laoar.shao@gmail.com (mailing list archive) |
|---|---|
| State | Deferred, archived |
| Headers | show |
| Series | avoid xfs transaction reservation recursion | expand |
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 <darrick.wong@oracle.com> > Cc: Matthew Wilcox (Oracle) <willy@infradead.org> > Cc: Christoph Hellwig <hch@lst.de> > Cc: Dave Chinner <david@fromorbit.com> > Cc: Michal Hocko <mhocko@kernel.org> > Cc: David Howells <dhowells@redhat.com> > Cc: Jeff Layton <jlayton@redhat.com> > Signed-off-by: Yafang Shao <laoar.shao@gmail.com> > --- > 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.
On Mon, Dec 7, 2020 at 5:04 AM Dave Chinner <david@fromorbit.com> wrote: > > 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 <darrick.wong@oracle.com> > > Cc: Matthew Wilcox (Oracle) <willy@infradead.org> > > Cc: Christoph Hellwig <hch@lst.de> > > Cc: Dave Chinner <david@fromorbit.com> > > Cc: Michal Hocko <mhocko@kernel.org> > > Cc: David Howells <dhowells@redhat.com> > > Cc: Jeff Layton <jlayton@redhat.com> > > Signed-off-by: Yafang Shao <laoar.shao@gmail.com> > > --- > > 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... > I will add the warning back in the next version. > > 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. > Right. I think we can remove this check. > > + > > 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? > Sure, I will use a wrapper. > > > > /* 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. > Sure. > > @@ -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. > Sure. > 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... > Good suggestion. Thanks.
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; - /* * Is this page beyond the end of the file? * diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c index 4304c6416fbb..1541ea5956fa 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 */ @@ -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); } diff --git a/fs/xfs/xfs_linux.h b/fs/xfs/xfs_linux.h index 5b7a1e201559..6ab0f8043c73 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 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); + 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; /* 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); @@ -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); } diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h index 084658946cc8..ceb530bf5c4b 100644 --- a/fs/xfs/xfs_trans.h +++ b/fs/xfs/xfs_trans.h @@ -268,4 +268,27 @@ xfs_trans_item_relog( return lip->li_ops->iop_relog(lip, tp); } +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_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__ */
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 <darrick.wong@oracle.com> Cc: Matthew Wilcox (Oracle) <willy@infradead.org> Cc: Christoph Hellwig <hch@lst.de> Cc: Dave Chinner <david@fromorbit.com> Cc: Michal Hocko <mhocko@kernel.org> Cc: David Howells <dhowells@redhat.com> Cc: Jeff Layton <jlayton@redhat.com> Signed-off-by: Yafang Shao <laoar.shao@gmail.com> --- 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(-)