* [PATCH v8 0/2] avoid xfs transaction reservation recursion @ 2020-10-09 12:51 Yafang Shao 2020-10-09 12:51 ` [PATCH v8 1/2] mm: Add become_kswapd and restore_kswapd Yafang Shao 2020-10-09 12:51 ` [PATCH v8 2/2] xfs: avoid transaction reservation recursion Yafang Shao 0 siblings, 2 replies; 8+ messages in thread From: Yafang Shao @ 2020-10-09 12:51 UTC (permalink / raw) To: david, hch, darrick.wong, willy, mhocko, akpm Cc: linux-xfs, linux-fsdevel, linux-mm, Yafang Shao This patchset avoids transaction reservation recursion by reintroducing the discarded PF_FSTRANS in a new way, suggested by Dave. In this new implementation, four new helpers are introduced, which are xfs_trans_context_{set, clear, update, active}, suggested by Dave. And re-using the task->journal_info to indicates whehter the task is in fstrans or not, suggested by Willy Patch #1 is picked from Willy's patchset "Overhaul memalloc_no*"[1] [1] https://lore.kernel.org/linux-mm/20200625113122.7540-1-willy@infradead.org/ v8: - check xfs_trans_context_active() in xfs_vm_writepage(s), per Dave. v7: - check fstrans recursion for XFS only, by introducing a new member in struct writeback_control. v6: - add Michal's ack and comment in patch #1. v5: - pick one of Willy's patch - introduce four new helpers, per Dave v4: - retitle from "xfs: introduce task->in_fstrans for transaction reservation recursion protection" - reuse current->journal_info, per Willy Matthew Wilcox (Oracle) (1): mm: Add become_kswapd and restore_kswapd Yafang Shao (1): xfs: avoid transaction reservation recursion fs/iomap/buffered-io.c | 7 ------- fs/xfs/libxfs/xfs_btree.c | 14 ++++++++------ fs/xfs/xfs_aops.c | 23 +++++++++++++++++++++-- fs/xfs/xfs_linux.h | 4 ---- fs/xfs/xfs_trans.c | 19 +++++++++---------- fs/xfs/xfs_trans.h | 30 ++++++++++++++++++++++++++++++ include/linux/sched/mm.h | 23 +++++++++++++++++++++++ mm/vmscan.c | 16 +--------------- 8 files changed, 92 insertions(+), 44 deletions(-) -- 2.17.1 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v8 1/2] mm: Add become_kswapd and restore_kswapd 2020-10-09 12:51 [PATCH v8 0/2] avoid xfs transaction reservation recursion Yafang Shao @ 2020-10-09 12:51 ` Yafang Shao 2020-10-09 15:53 ` Darrick J. Wong 2020-10-15 8:30 ` Christoph Hellwig 2020-10-09 12:51 ` [PATCH v8 2/2] xfs: avoid transaction reservation recursion Yafang Shao 1 sibling, 2 replies; 8+ messages in thread From: Yafang Shao @ 2020-10-09 12:51 UTC (permalink / raw) To: david, hch, darrick.wong, willy, mhocko, akpm Cc: linux-xfs, linux-fsdevel, linux-mm, Yafang Shao From: "Matthew Wilcox (Oracle)" <willy@infradead.org> Since XFS needs to pretend to be kswapd in some of its worker threads, create methods to save & restore kswapd state. Don't bother restoring kswapd state in kswapd -- the only time we reach this code is when we're exiting and the task_struct is about to be destroyed anyway. Cc: Dave Chinner <david@fromorbit.com> Cc: Christoph Hellwig <hch@infradead.org> Cc: Darrick J. Wong <darrick.wong@oracle.com> Cc: Matthew Wilcox <willy@infradead.org> Acked-by: Michal Hocko <mhocko@suse.com> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> Signed-off-by: Yafang Shao <laoar.shao@gmail.com> --- fs/xfs/libxfs/xfs_btree.c | 14 ++++++++------ include/linux/sched/mm.h | 23 +++++++++++++++++++++++ mm/vmscan.c | 16 +--------------- 3 files changed, 32 insertions(+), 21 deletions(-) diff --git a/fs/xfs/libxfs/xfs_btree.c b/fs/xfs/libxfs/xfs_btree.c index 2d25bab68764..a04a44238aab 100644 --- a/fs/xfs/libxfs/xfs_btree.c +++ b/fs/xfs/libxfs/xfs_btree.c @@ -2813,8 +2813,9 @@ xfs_btree_split_worker( { struct xfs_btree_split_args *args = container_of(work, struct xfs_btree_split_args, work); + bool is_kswapd = args->kswapd; unsigned long pflags; - unsigned long new_pflags = PF_MEMALLOC_NOFS; + int memalloc_nofs; /* * we are in a transaction context here, but may also be doing work @@ -2822,16 +2823,17 @@ xfs_btree_split_worker( * temporarily to ensure that we don't block waiting for memory reclaim * in any way. */ - if (args->kswapd) - new_pflags |= PF_MEMALLOC | PF_SWAPWRITE | PF_KSWAPD; - - current_set_flags_nested(&pflags, new_pflags); + if (is_kswapd) + pflags = become_kswapd(); + memalloc_nofs = memalloc_nofs_save(); args->result = __xfs_btree_split(args->cur, args->level, args->ptrp, args->key, args->curp, args->stat); complete(args->done); - current_restore_flags_nested(&pflags, new_pflags); + memalloc_nofs_restore(memalloc_nofs); + if (is_kswapd) + restore_kswapd(pflags); } /* diff --git a/include/linux/sched/mm.h b/include/linux/sched/mm.h index f889e332912f..b38fdcb977a4 100644 --- a/include/linux/sched/mm.h +++ b/include/linux/sched/mm.h @@ -303,6 +303,29 @@ static inline void memalloc_nocma_restore(unsigned int flags) } #endif +/* + * Tell the memory management code that this thread is working on behalf + * of background memory reclaim (like kswapd). That means that it will + * get access to memory reserves should it need to allocate memory in + * order to make forward progress. With this great power comes great + * responsibility to not exhaust those reserves. + */ +#define KSWAPD_PF_FLAGS (PF_MEMALLOC | PF_SWAPWRITE | PF_KSWAPD) + +static inline unsigned long become_kswapd(void) +{ + unsigned long flags = current->flags & KSWAPD_PF_FLAGS; + + current->flags |= KSWAPD_PF_FLAGS; + + return flags; +} + +static inline void restore_kswapd(unsigned long flags) +{ + current->flags &= ~(flags ^ KSWAPD_PF_FLAGS); +} + #ifdef CONFIG_MEMCG /** * memalloc_use_memcg - Starts the remote memcg charging scope. diff --git a/mm/vmscan.c b/mm/vmscan.c index 466fc3144fff..eb6f6e8103c1 100644 --- a/mm/vmscan.c +++ b/mm/vmscan.c @@ -3867,19 +3867,7 @@ static int kswapd(void *p) if (!cpumask_empty(cpumask)) set_cpus_allowed_ptr(tsk, cpumask); - /* - * Tell the memory management that we're a "memory allocator", - * and that if we need more memory we should get access to it - * regardless (see "__alloc_pages()"). "kswapd" should - * never get caught in the normal page freeing logic. - * - * (Kswapd normally doesn't need memory anyway, but sometimes - * you need a small amount of memory in order to be able to - * page out something else, and this flag essentially protects - * us from recursively trying to free more memory as we're - * trying to free the first piece of memory in the first place). - */ - tsk->flags |= PF_MEMALLOC | PF_SWAPWRITE | PF_KSWAPD; + become_kswapd(); set_freezable(); WRITE_ONCE(pgdat->kswapd_order, 0); @@ -3929,8 +3917,6 @@ static int kswapd(void *p) goto kswapd_try_sleep; } - tsk->flags &= ~(PF_MEMALLOC | PF_SWAPWRITE | PF_KSWAPD); - return 0; } -- 2.17.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v8 1/2] mm: Add become_kswapd and restore_kswapd 2020-10-09 12:51 ` [PATCH v8 1/2] mm: Add become_kswapd and restore_kswapd Yafang Shao @ 2020-10-09 15:53 ` Darrick J. Wong 2020-10-15 8:30 ` Christoph Hellwig 1 sibling, 0 replies; 8+ messages in thread From: Darrick J. Wong @ 2020-10-09 15:53 UTC (permalink / raw) To: Yafang Shao Cc: david, hch, willy, mhocko, akpm, linux-xfs, linux-fsdevel, linux-mm On Fri, Oct 09, 2020 at 08:51:26PM +0800, Yafang Shao wrote: > From: "Matthew Wilcox (Oracle)" <willy@infradead.org> > > Since XFS needs to pretend to be kswapd in some of its worker threads, > create methods to save & restore kswapd state. Don't bother restoring > kswapd state in kswapd -- the only time we reach this code is when we're > exiting and the task_struct is about to be destroyed anyway. > > Cc: Dave Chinner <david@fromorbit.com> > Cc: Christoph Hellwig <hch@infradead.org> > Cc: Darrick J. Wong <darrick.wong@oracle.com> > Cc: Matthew Wilcox <willy@infradead.org> > Acked-by: Michal Hocko <mhocko@suse.com> > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> > Signed-off-by: Yafang Shao <laoar.shao@gmail.com> > --- > fs/xfs/libxfs/xfs_btree.c | 14 ++++++++------ > include/linux/sched/mm.h | 23 +++++++++++++++++++++++ > mm/vmscan.c | 16 +--------------- > 3 files changed, 32 insertions(+), 21 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_btree.c b/fs/xfs/libxfs/xfs_btree.c > index 2d25bab68764..a04a44238aab 100644 > --- a/fs/xfs/libxfs/xfs_btree.c > +++ b/fs/xfs/libxfs/xfs_btree.c > @@ -2813,8 +2813,9 @@ xfs_btree_split_worker( > { > struct xfs_btree_split_args *args = container_of(work, > struct xfs_btree_split_args, work); > + bool is_kswapd = args->kswapd; > unsigned long pflags; > - unsigned long new_pflags = PF_MEMALLOC_NOFS; > + int memalloc_nofs; > > /* > * we are in a transaction context here, but may also be doing work > @@ -2822,16 +2823,17 @@ xfs_btree_split_worker( > * temporarily to ensure that we don't block waiting for memory reclaim > * in any way. > */ > - if (args->kswapd) > - new_pflags |= PF_MEMALLOC | PF_SWAPWRITE | PF_KSWAPD; > - > - current_set_flags_nested(&pflags, new_pflags); > + if (is_kswapd) > + pflags = become_kswapd(); > + memalloc_nofs = memalloc_nofs_save(); > > args->result = __xfs_btree_split(args->cur, args->level, args->ptrp, > args->key, args->curp, args->stat); > complete(args->done); > > - current_restore_flags_nested(&pflags, new_pflags); > + memalloc_nofs_restore(memalloc_nofs); > + if (is_kswapd) > + restore_kswapd(pflags); It's kind of a pity that these two APIs both touch current->flags but there isn't a clean way to only use one flags variable to pop all of these things at the end. :/ Oh well, at least the end result is more compact but still communicates what we're doing with all these PF_ mods. Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> --D > } > > /* > diff --git a/include/linux/sched/mm.h b/include/linux/sched/mm.h > index f889e332912f..b38fdcb977a4 100644 > --- a/include/linux/sched/mm.h > +++ b/include/linux/sched/mm.h > @@ -303,6 +303,29 @@ static inline void memalloc_nocma_restore(unsigned int flags) > } > #endif > > +/* > + * Tell the memory management code that this thread is working on behalf > + * of background memory reclaim (like kswapd). That means that it will > + * get access to memory reserves should it need to allocate memory in > + * order to make forward progress. With this great power comes great > + * responsibility to not exhaust those reserves. > + */ > +#define KSWAPD_PF_FLAGS (PF_MEMALLOC | PF_SWAPWRITE | PF_KSWAPD) > + > +static inline unsigned long become_kswapd(void) > +{ > + unsigned long flags = current->flags & KSWAPD_PF_FLAGS; > + > + current->flags |= KSWAPD_PF_FLAGS; > + > + return flags; > +} > + > +static inline void restore_kswapd(unsigned long flags) > +{ > + current->flags &= ~(flags ^ KSWAPD_PF_FLAGS); > +} > + > #ifdef CONFIG_MEMCG > /** > * memalloc_use_memcg - Starts the remote memcg charging scope. > diff --git a/mm/vmscan.c b/mm/vmscan.c > index 466fc3144fff..eb6f6e8103c1 100644 > --- a/mm/vmscan.c > +++ b/mm/vmscan.c > @@ -3867,19 +3867,7 @@ static int kswapd(void *p) > if (!cpumask_empty(cpumask)) > set_cpus_allowed_ptr(tsk, cpumask); > > - /* > - * Tell the memory management that we're a "memory allocator", > - * and that if we need more memory we should get access to it > - * regardless (see "__alloc_pages()"). "kswapd" should > - * never get caught in the normal page freeing logic. > - * > - * (Kswapd normally doesn't need memory anyway, but sometimes > - * you need a small amount of memory in order to be able to > - * page out something else, and this flag essentially protects > - * us from recursively trying to free more memory as we're > - * trying to free the first piece of memory in the first place). > - */ > - tsk->flags |= PF_MEMALLOC | PF_SWAPWRITE | PF_KSWAPD; > + become_kswapd(); > set_freezable(); > > WRITE_ONCE(pgdat->kswapd_order, 0); > @@ -3929,8 +3917,6 @@ static int kswapd(void *p) > goto kswapd_try_sleep; > } > > - tsk->flags &= ~(PF_MEMALLOC | PF_SWAPWRITE | PF_KSWAPD); > - > return 0; > } > > -- > 2.17.1 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v8 1/2] mm: Add become_kswapd and restore_kswapd 2020-10-09 12:51 ` [PATCH v8 1/2] mm: Add become_kswapd and restore_kswapd Yafang Shao 2020-10-09 15:53 ` Darrick J. Wong @ 2020-10-15 8:30 ` Christoph Hellwig 1 sibling, 0 replies; 8+ messages in thread From: Christoph Hellwig @ 2020-10-15 8:30 UTC (permalink / raw) To: Yafang Shao Cc: david, hch, darrick.wong, willy, mhocko, akpm, linux-xfs, linux-fsdevel, linux-mm Looks good, Reviewed-by: Christoph Hellwig <hch@lst.de> ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v8 2/2] xfs: avoid transaction reservation recursion 2020-10-09 12:51 [PATCH v8 0/2] avoid xfs transaction reservation recursion Yafang Shao 2020-10-09 12:51 ` [PATCH v8 1/2] mm: Add become_kswapd and restore_kswapd Yafang Shao @ 2020-10-09 12:51 ` Yafang Shao 2020-10-09 13:35 ` Matthew Wilcox ` (2 more replies) 1 sibling, 3 replies; 8+ messages in thread From: Yafang Shao @ 2020-10-09 12:51 UTC (permalink / raw) To: david, hch, darrick.wong, willy, mhocko, akpm Cc: linux-xfs, linux-fsdevel, linux-mm, Yafang Shao 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 [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 <david@fromorbit.com> Cc: Christoph Hellwig <hch@infradead.org> Cc: Michal Hocko <mhocko@kernel.org> Cc: Darrick J. Wong <darrick.wong@oracle.com> Cc: Matthew Wilcox <willy@infradead.org> 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 | 19 +++++++++---------- fs/xfs/xfs_trans.h | 30 ++++++++++++++++++++++++++++++ 5 files changed, 60 insertions(+), 23 deletions(-) diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c index bcfc288dba3f..3dc57a38bf0b 100644 --- a/fs/iomap/buffered-io.c +++ b/fs/iomap/buffered-io.c @@ -1498,13 +1498,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 b35611882ff9..af7270f5f8a9 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,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); } @@ -575,6 +586,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 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__ */ -- 2.17.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v8 2/2] xfs: avoid transaction reservation recursion 2020-10-09 12:51 ` [PATCH v8 2/2] xfs: avoid transaction reservation recursion Yafang Shao @ 2020-10-09 13:35 ` Matthew Wilcox 2020-10-09 16:00 ` Darrick J. Wong 2020-10-15 8:31 ` Christoph Hellwig 2 siblings, 0 replies; 8+ messages in thread From: Matthew Wilcox @ 2020-10-09 13:35 UTC (permalink / raw) To: Yafang Shao Cc: david, hch, darrick.wong, mhocko, akpm, linux-xfs, linux-fsdevel, linux-mm On Fri, Oct 09, 2020 at 08:51:27PM +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. Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v8 2/2] xfs: avoid transaction reservation recursion 2020-10-09 12:51 ` [PATCH v8 2/2] xfs: avoid transaction reservation recursion Yafang Shao 2020-10-09 13:35 ` Matthew Wilcox @ 2020-10-09 16:00 ` Darrick J. Wong 2020-10-15 8:31 ` Christoph Hellwig 2 siblings, 0 replies; 8+ messages in thread From: Darrick J. Wong @ 2020-10-09 16:00 UTC (permalink / raw) To: Yafang Shao Cc: david, hch, willy, mhocko, akpm, linux-xfs, linux-fsdevel, linux-mm On Fri, Oct 09, 2020 at 08:51:27PM +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, 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 > > [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 <david@fromorbit.com> > Cc: Christoph Hellwig <hch@infradead.org> > Cc: Michal Hocko <mhocko@kernel.org> > Cc: Darrick J. Wong <darrick.wong@oracle.com> > Cc: Matthew Wilcox <willy@infradead.org> > Signed-off-by: Yafang Shao <laoar.shao@gmail.com> Well, now that I've killed off (AFAIK) the last nested transaction in XFS I guess it's time to do this... Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> --D > --- > fs/iomap/buffered-io.c | 7 ------- > fs/xfs/xfs_aops.c | 23 +++++++++++++++++++++-- > fs/xfs/xfs_linux.h | 4 ---- > fs/xfs/xfs_trans.c | 19 +++++++++---------- > fs/xfs/xfs_trans.h | 30 ++++++++++++++++++++++++++++++ > 5 files changed, 60 insertions(+), 23 deletions(-) > > diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c > index bcfc288dba3f..3dc57a38bf0b 100644 > --- a/fs/iomap/buffered-io.c > +++ b/fs/iomap/buffered-io.c > @@ -1498,13 +1498,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 b35611882ff9..af7270f5f8a9 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,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); > } > > @@ -575,6 +586,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 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__ */ > -- > 2.17.1 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v8 2/2] xfs: avoid transaction reservation recursion 2020-10-09 12:51 ` [PATCH v8 2/2] xfs: avoid transaction reservation recursion Yafang Shao 2020-10-09 13:35 ` Matthew Wilcox 2020-10-09 16:00 ` Darrick J. Wong @ 2020-10-15 8:31 ` Christoph Hellwig 2 siblings, 0 replies; 8+ messages in thread From: Christoph Hellwig @ 2020-10-15 8:31 UTC (permalink / raw) To: Yafang Shao Cc: david, hch, darrick.wong, willy, mhocko, akpm, linux-xfs, linux-fsdevel, linux-mm Looks good, Reviewed-by: Christoph Hellwig <hch@lst.de> ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-10-15 8:31 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-10-09 12:51 [PATCH v8 0/2] avoid xfs transaction reservation recursion Yafang Shao 2020-10-09 12:51 ` [PATCH v8 1/2] mm: Add become_kswapd and restore_kswapd Yafang Shao 2020-10-09 15:53 ` Darrick J. Wong 2020-10-15 8:30 ` Christoph Hellwig 2020-10-09 12:51 ` [PATCH v8 2/2] xfs: avoid transaction reservation recursion Yafang Shao 2020-10-09 13:35 ` Matthew Wilcox 2020-10-09 16:00 ` Darrick J. Wong 2020-10-15 8:31 ` Christoph Hellwig
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).