xfs: use current->journal_info for detecting transaction recursion

Because the iomap code using PF_MEMALLOC_NOFS to detect transaction
recursion in XFS is just wrong. Remove it from the iomap code and
replace it with XFS specific internal checks using
current->journal_info instead.

[djwong: This change also realigns the lifetime of NOFS flag changes to
match the incore transaction, instead of the inconsistent scheme we have
now.]

Fixes: 9070733b4efa ("xfs: abstract PF_FSTRANS to PF_MEMALLOC_NOFS")
Signed-off-by: Dave Chinner <dchinner@redhat.com>
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>

diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
index 16a1e82e3aeb0dc3dc9b629397b4652e1aa8c5ad..fcd4a0d71fc1af2fd91d320a5c6837a9ac336022 100644 (file)
--- 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/libxfs/xfs_btree.c b/fs/xfs/libxfs/xfs_btree.c
index b56ff451adcee8960b14bf2917a74f2026f5960f..5b6fcb9b44e28b8dec2b51cc2c5373901ba46e1a 100644 (file)
--- a/fs/xfs/libxfs/xfs_btree.c
+++ b/fs/xfs/libxfs/xfs_btree.c
@@ -2805,7 +2805,7 @@ xfs_btree_split_worker(
        struct xfs_btree_split_args     *args = container_of(work,
                                                struct xfs_btree_split_args, work);
        unsigned long           pflags;
-       unsigned long           new_pflags = PF_MEMALLOC_NOFS;
+       unsigned long           new_pflags = 0;
 
        /*
         * we are in a transaction context here, but may also be doing work
@@ -2817,12 +2817,20 @@ xfs_btree_split_worker(
                new_pflags |= PF_MEMALLOC | PF_SWAPWRITE | PF_KSWAPD;
 
        current_set_flags_nested(&pflags, new_pflags);
+       xfs_trans_set_context(args->cur->bc_tp);
 
        args->result = __xfs_btree_split(args->cur, args->level, args->ptrp,
                                         args->key, args->curp, args->stat);
-       complete(args->done);
 
+       xfs_trans_clear_context(args->cur->bc_tp);
        current_restore_flags_nested(&pflags, new_pflags);
+
+       /*
+        * Do not access args after complete() has run here. We don't own args
+        * and the owner may run and free args before we return here.
+        */
+       complete(args->done);
+
 }
 
 /*

diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c
index 4304c6416fbbc1e30b77d655bd3678c2815c5df9..b4186d666157e5bed63eac073b2adf140a1260ec 100644 (file)
--- a/fs/xfs/xfs_aops.c
+++ b/fs/xfs/xfs_aops.c
@@ -62,7 +62,7 @@ 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_clear_context(tp);
        return 0;
 }
 
@@ -125,7 +125,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_set_context(tp);
        __sb_writers_acquired(VFS_I(ip)->i_sb, SB_FREEZE_FS);
 
        /* we abort the update if there was an IO error */
@@ -568,6 +568,12 @@ xfs_vm_writepage(
 {
        struct xfs_writepage_ctx wpc = { };
 
+       if (WARN_ON_ONCE(current->journal_info)) {
+               redirty_page_for_writepage(wbc, page);
+               unlock_page(page);
+               return 0;
+       }
+
        return iomap_writepage(page, wbc, &wpc.ctx, &xfs_writeback_ops);
 }
 
@@ -578,6 +584,13 @@ xfs_vm_writepages(
 {
        struct xfs_writepage_ctx wpc = { };
 
+       /*
+        * Writing back data in a transaction context can result in recursive
+        * transactions. This is bad, so issue a warning and get out of here.
+        */
+       if (WARN_ON_ONCE(current->journal_info))
+               return 0;
+
        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_trans.c b/fs/xfs/xfs_trans.c
index 377f3961d7ed4075fb3e6ab1e6410bd7d9700d8d..b22a09e9daeefd84ad7bd30e049bcad5884e43b7 100644 (file)
--- a/fs/xfs/xfs_trans.c
+++ b/fs/xfs/xfs_trans.c
@@ -72,6 +72,7 @@ xfs_trans_free(
        xfs_extent_busy_clear(tp->t_mountp, &tp->t_busy, false);
 
        trace_xfs_trans_free(tp, _RET_IP_);
+       xfs_trans_clear_context(tp);
        if (!(tp->t_flags & XFS_TRANS_NO_WRITECOUNT))
                sb_end_intwrite(tp->t_mountp->m_super);
        xfs_trans_free_dqinfo(tp);
@@ -123,7 +124,8 @@ 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;
+
+       xfs_trans_switch_context(tp, ntp);
 
        /* move deferred ops over to the new tp */
        xfs_defer_move(ntp, tp);
@@ -157,9 +159,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
         * the number needed from the number available.  This will
@@ -167,10 +166,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;
        }
 
@@ -244,9 +241,6 @@ undo_blocks:
                xfs_mod_fdblocks(mp, (int64_t)blocks, rsvd);
                tp->t_blk_res = 0;
        }
-
-       current_restore_flags_nested(&tp->t_pflags, PF_MEMALLOC_NOFS);
-
        return error;
 }
 
@@ -272,6 +266,7 @@ retry:
        tp = kmem_cache_zalloc(xfs_trans_zone, GFP_KERNEL | __GFP_NOFAIL);
        if (!(flags & XFS_TRANS_NO_WRITECOUNT))
                sb_start_intwrite(mp->m_super);
+       xfs_trans_set_context(tp);
 
        /*
         * Zero-reservation ("empty") transactions can't modify anything, so
@@ -900,7 +895,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);
 
        /*
@@ -932,7 +926,6 @@ out_unreserve:
                        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);
 
@@ -992,9 +985,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 8b03fbfe9a1bda3984951ed068dea827130c81c6..9dd745cf77c9cdd140500c76f16ebecc0ff9b58b 100644 (file)
--- a/fs/xfs/xfs_trans.h
+++ b/fs/xfs/xfs_trans.h
@@ -281,4 +281,34 @@ int xfs_trans_alloc_ichange(struct xfs_inode *ip, struct xfs_dquot *udqp,
                struct xfs_dquot *gdqp, struct xfs_dquot *pdqp, bool force,
                struct xfs_trans **tpp);
 
+static inline void
+xfs_trans_set_context(
+       struct xfs_trans        *tp)
+{
+       ASSERT(current->journal_info == NULL);
+       tp->t_pflags = memalloc_nofs_save();
+       current->journal_info = tp;
+}
+
+static inline void
+xfs_trans_clear_context(
+       struct xfs_trans        *tp)
+{
+       if (current->journal_info == tp) {
+               memalloc_nofs_restore(tp->t_pflags);
+               current->journal_info = NULL;
+       }
+}
+
+static inline void
+xfs_trans_switch_context(
+       struct xfs_trans        *old_tp,
+       struct xfs_trans        *new_tp)
+{
+       ASSERT(current->journal_info == old_tp);
+       new_tp->t_pflags = old_tp->t_pflags;
+       old_tp->t_pflags = 0;
+       current->journal_info = new_tp;
+}
+
 #endif /* __XFS_TRANS_H__ */