| Message ID | 20200824014234.7109-3-laoar.shao@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | avoid xfs transaction reservation recursion | expand |
On Mon, 24 Aug 2020 09:42:34 +0800 Yafang Shao <laoar.shao@gmail.com> wrote: > --- a/include/linux/iomap.h > +++ b/include/linux/iomap.h > @@ -271,4 +271,11 @@ int iomap_swapfile_activate(struct swap_info_struct *sis, > # define iomap_swapfile_activate(sis, swapfile, pagespan, ops) (-EIO) > #endif /* CONFIG_SWAP */ > > +/* Use the journal_info to indicate current is in a transaction */ > +static inline bool > +fstrans_context_active(void) > +{ > + return current->journal_info != NULL; > +} Why choose iomap.h for this?
On Mon, Aug 24, 2020 at 01:09:25PM -0700, Andrew Morton wrote: > On Mon, 24 Aug 2020 09:42:34 +0800 Yafang Shao <laoar.shao@gmail.com> wrote: > > > --- a/include/linux/iomap.h > > +++ b/include/linux/iomap.h > > @@ -271,4 +271,11 @@ int iomap_swapfile_activate(struct swap_info_struct *sis, > > # define iomap_swapfile_activate(sis, swapfile, pagespan, ops) (-EIO) > > #endif /* CONFIG_SWAP */ > > > > +/* Use the journal_info to indicate current is in a transaction */ > > +static inline bool > > +fstrans_context_active(void) > > +{ > > + return current->journal_info != NULL; > > +} > > Why choose iomap.h for this? Because it gets used in iomap/buffered-io.c I don't think this is necessarily a useful abstraction, to be honest. I'd just open-code 'if (current->journal_info)' or !current->journal_info, whichever way round the code is: fs/btrfs/delalloc-space.c: if (current->journal_info) fs/ceph/xattr.c: if (current->journal_info) { fs/gfs2/bmap.c: if (current->journal_info) { fs/jbd2/transaction.c: if (WARN_ON(current->journal_info)) { fs/reiserfs/super.c: if (!current->journal_info) { (to pluck a few examples from existing filesystems)
On Tue, Aug 25, 2020 at 4:56 AM Matthew Wilcox <willy@infradead.org> wrote: > > On Mon, Aug 24, 2020 at 01:09:25PM -0700, Andrew Morton wrote: > > On Mon, 24 Aug 2020 09:42:34 +0800 Yafang Shao <laoar.shao@gmail.com> wrote: > > > > > --- a/include/linux/iomap.h > > > +++ b/include/linux/iomap.h > > > @@ -271,4 +271,11 @@ int iomap_swapfile_activate(struct swap_info_struct *sis, > > > # define iomap_swapfile_activate(sis, swapfile, pagespan, ops) (-EIO) > > > #endif /* CONFIG_SWAP */ > > > > > > +/* Use the journal_info to indicate current is in a transaction */ > > > +static inline bool > > > +fstrans_context_active(void) > > > +{ > > > + return current->journal_info != NULL; > > > +} > > > > Why choose iomap.h for this? > > Because it gets used in iomap/buffered-io.c > > I don't think this is necessarily a useful abstraction, to be honest. > I'd just open-code 'if (current->journal_info)' or !current->journal_info, > whichever way round the code is: > > fs/btrfs/delalloc-space.c: if (current->journal_info) > fs/ceph/xattr.c: if (current->journal_info) { > fs/gfs2/bmap.c: if (current->journal_info) { > fs/jbd2/transaction.c: if (WARN_ON(current->journal_info)) { > fs/reiserfs/super.c: if (!current->journal_info) { > > (to pluck a few examples from existing filesystems) Make sense to me. I will update it in the next version.
On Mon, Aug 24, 2020 at 09:56:47PM +0100, Matthew Wilcox wrote: > On Mon, Aug 24, 2020 at 01:09:25PM -0700, Andrew Morton wrote: > > On Mon, 24 Aug 2020 09:42:34 +0800 Yafang Shao <laoar.shao@gmail.com> wrote: > > > > > --- a/include/linux/iomap.h > > > +++ b/include/linux/iomap.h > > > @@ -271,4 +271,11 @@ int iomap_swapfile_activate(struct swap_info_struct *sis, > > > # define iomap_swapfile_activate(sis, swapfile, pagespan, ops) (-EIO) > > > #endif /* CONFIG_SWAP */ > > > > > > +/* Use the journal_info to indicate current is in a transaction */ > > > +static inline bool > > > +fstrans_context_active(void) > > > +{ > > > + return current->journal_info != NULL; > > > +} > > > > Why choose iomap.h for this? > > Because it gets used in iomap/buffered-io.c > > I don't think this is necessarily a useful abstraction, to be honest. > I'd just open-code 'if (current->journal_info)' or !current->journal_info, > whichever way round the code is: > > fs/btrfs/delalloc-space.c: if (current->journal_info) > fs/ceph/xattr.c: if (current->journal_info) { > fs/gfs2/bmap.c: if (current->journal_info) { > fs/jbd2/transaction.c: if (WARN_ON(current->journal_info)) { > fs/reiserfs/super.c: if (!current->journal_info) { /me wonders idly if any of the other filesystems that use current->journal_info can have an active transaction while calling ->writepages... .... and if so, whether this patchset has taken the wrong path in trying to use current->journal_info for XFS to re-implement this warning..... .... so we'll have to remove or rework this yet again when other filesystems are converted to use iomap.... /me suspects the btrfs_write_and_wait_transaction() is a path where this can actually happen... -Dave.
On Tue, Aug 25, 2020 at 1:32 PM Dave Chinner <david@fromorbit.com> wrote: > > On Mon, Aug 24, 2020 at 09:56:47PM +0100, Matthew Wilcox wrote: > > On Mon, Aug 24, 2020 at 01:09:25PM -0700, Andrew Morton wrote: > > > On Mon, 24 Aug 2020 09:42:34 +0800 Yafang Shao <laoar.shao@gmail.com> wrote: > > > > > > > --- a/include/linux/iomap.h > > > > +++ b/include/linux/iomap.h > > > > @@ -271,4 +271,11 @@ int iomap_swapfile_activate(struct swap_info_struct *sis, > > > > # define iomap_swapfile_activate(sis, swapfile, pagespan, ops) (-EIO) > > > > #endif /* CONFIG_SWAP */ > > > > > > > > +/* Use the journal_info to indicate current is in a transaction */ > > > > +static inline bool > > > > +fstrans_context_active(void) > > > > +{ > > > > + return current->journal_info != NULL; > > > > +} > > > > > > Why choose iomap.h for this? > > > > Because it gets used in iomap/buffered-io.c > > > > I don't think this is necessarily a useful abstraction, to be honest. > > I'd just open-code 'if (current->journal_info)' or !current->journal_info, > > whichever way round the code is: > > > > fs/btrfs/delalloc-space.c: if (current->journal_info) > > fs/ceph/xattr.c: if (current->journal_info) { > > fs/gfs2/bmap.c: if (current->journal_info) { > > fs/jbd2/transaction.c: if (WARN_ON(current->journal_info)) { > > fs/reiserfs/super.c: if (!current->journal_info) { > > /me wonders idly if any of the other filesystems that use > current->journal_info can have an active transaction while calling > ->writepages... > > .... and if so, whether this patchset has taken the wrong path in > trying to use current->journal_info for XFS to re-implement this > warning..... > > .... so we'll have to remove or rework this yet again when other > filesystems are converted to use iomap.... > > /me suspects the btrfs_write_and_wait_transaction() is a path where > this can actually happen... > How about adding a flag in struct writeback_control ? struct writeback_control { ... unsigned fstrans_check:1; /* Whether to check the current is in fstrans */ }; Then we can set it in xfs_vm_writepage(s), for example, xfs_vm_writepage { wbc->fstrans_check = 1; // set it for XFS only. return iomap_writepage(page, wbc, &wpc.ctx, &xfs_writeback_ops); } And then we check this flag in iomap_do_writepage(): iomap_do_writepage if (WARN_ON_ONCE(wbc->fstrans_check && current->journal_info)) goto redirty;
On Tue, Aug 25, 2020 at 02:22:08PM +0800, Yafang Shao wrote: > On Tue, Aug 25, 2020 at 1:32 PM Dave Chinner <david@fromorbit.com> wrote: > > > > On Mon, Aug 24, 2020 at 09:56:47PM +0100, Matthew Wilcox wrote: > > > On Mon, Aug 24, 2020 at 01:09:25PM -0700, Andrew Morton wrote: > > > > On Mon, 24 Aug 2020 09:42:34 +0800 Yafang Shao <laoar.shao@gmail.com> wrote: > > > > > > > > > --- a/include/linux/iomap.h > > > > > +++ b/include/linux/iomap.h > > > > > @@ -271,4 +271,11 @@ int iomap_swapfile_activate(struct swap_info_struct *sis, > > > > > # define iomap_swapfile_activate(sis, swapfile, pagespan, ops) (-EIO) > > > > > #endif /* CONFIG_SWAP */ > > > > > > > > > > +/* Use the journal_info to indicate current is in a transaction */ > > > > > +static inline bool > > > > > +fstrans_context_active(void) > > > > > +{ > > > > > + return current->journal_info != NULL; > > > > > +} > > > > > > > > Why choose iomap.h for this? > > > > > > Because it gets used in iomap/buffered-io.c > > > > > > I don't think this is necessarily a useful abstraction, to be honest. > > > I'd just open-code 'if (current->journal_info)' or !current->journal_info, > > > whichever way round the code is: > > > > > > fs/btrfs/delalloc-space.c: if (current->journal_info) > > > fs/ceph/xattr.c: if (current->journal_info) { > > > fs/gfs2/bmap.c: if (current->journal_info) { > > > fs/jbd2/transaction.c: if (WARN_ON(current->journal_info)) { > > > fs/reiserfs/super.c: if (!current->journal_info) { > > > > /me wonders idly if any of the other filesystems that use > > current->journal_info can have an active transaction while calling > > ->writepages... > > > > .... and if so, whether this patchset has taken the wrong path in > > trying to use current->journal_info for XFS to re-implement this > > warning..... > > > > .... so we'll have to remove or rework this yet again when other > > filesystems are converted to use iomap.... > > > > /me suspects the btrfs_write_and_wait_transaction() is a path where > > this can actually happen... > > > > How about adding a flag in struct writeback_control ? > struct writeback_control { > ... > unsigned fstrans_check:1; /* Whether to check the current is in fstrans */ > }; > > Then we can set it in xfs_vm_writepage(s), for example, > > xfs_vm_writepage > { > wbc->fstrans_check = 1; // set it for XFS only. > return iomap_writepage(page, wbc, &wpc.ctx, &xfs_writeback_ops); > } Yeah, but if we are doing that then I think we should just remove the check completely from iomap_writepage() and move it up into xfs_vm_writepage() and xfs_vm_writepages(). Cheers, Dave.
On Wed, Aug 26, 2020 at 6:47 AM Dave Chinner <david@fromorbit.com> wrote: > > On Tue, Aug 25, 2020 at 02:22:08PM +0800, Yafang Shao wrote: > > On Tue, Aug 25, 2020 at 1:32 PM Dave Chinner <david@fromorbit.com> wrote: > > > > > > On Mon, Aug 24, 2020 at 09:56:47PM +0100, Matthew Wilcox wrote: > > > > On Mon, Aug 24, 2020 at 01:09:25PM -0700, Andrew Morton wrote: > > > > > On Mon, 24 Aug 2020 09:42:34 +0800 Yafang Shao <laoar.shao@gmail.com> wrote: > > > > > > > > > > > --- a/include/linux/iomap.h > > > > > > +++ b/include/linux/iomap.h > > > > > > @@ -271,4 +271,11 @@ int iomap_swapfile_activate(struct swap_info_struct *sis, > > > > > > # define iomap_swapfile_activate(sis, swapfile, pagespan, ops) (-EIO) > > > > > > #endif /* CONFIG_SWAP */ > > > > > > > > > > > > +/* Use the journal_info to indicate current is in a transaction */ > > > > > > +static inline bool > > > > > > +fstrans_context_active(void) > > > > > > +{ > > > > > > + return current->journal_info != NULL; > > > > > > +} > > > > > > > > > > Why choose iomap.h for this? > > > > > > > > Because it gets used in iomap/buffered-io.c > > > > > > > > I don't think this is necessarily a useful abstraction, to be honest. > > > > I'd just open-code 'if (current->journal_info)' or !current->journal_info, > > > > whichever way round the code is: > > > > > > > > fs/btrfs/delalloc-space.c: if (current->journal_info) > > > > fs/ceph/xattr.c: if (current->journal_info) { > > > > fs/gfs2/bmap.c: if (current->journal_info) { > > > > fs/jbd2/transaction.c: if (WARN_ON(current->journal_info)) { > > > > fs/reiserfs/super.c: if (!current->journal_info) { > > > > > > /me wonders idly if any of the other filesystems that use > > > current->journal_info can have an active transaction while calling > > > ->writepages... > > > > > > .... and if so, whether this patchset has taken the wrong path in > > > trying to use current->journal_info for XFS to re-implement this > > > warning..... > > > > > > .... so we'll have to remove or rework this yet again when other > > > filesystems are converted to use iomap.... > > > > > > /me suspects the btrfs_write_and_wait_transaction() is a path where > > > this can actually happen... > > > > > > > How about adding a flag in struct writeback_control ? > > struct writeback_control { > > ... > > unsigned fstrans_check:1; /* Whether to check the current is in fstrans */ > > }; > > > > Then we can set it in xfs_vm_writepage(s), for example, > > > > xfs_vm_writepage > > { > > wbc->fstrans_check = 1; // set it for XFS only. > > return iomap_writepage(page, wbc, &wpc.ctx, &xfs_writeback_ops); > > } > > Yeah, but if we are doing that then I think we should just remove > the check completely from iomap_writepage() and move it up into > xfs_vm_writepage() and xfs_vm_writepages(). > Sure.
diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c index bcfc288dba3f..8043224ec079 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(fstrans_context_active())) goto redirty; /* diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c index b35611882ff9..83e0a1840221 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 */ 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..895f560229d6 100644 --- a/fs/xfs/xfs_trans.h +++ b/fs/xfs/xfs_trans.h @@ -243,4 +243,27 @@ 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); +} + #endif /* __XFS_TRANS_H__ */ diff --git a/include/linux/iomap.h b/include/linux/iomap.h index 4d1d3c3469e9..54194dd6009d 100644 --- a/include/linux/iomap.h +++ b/include/linux/iomap.h @@ -271,4 +271,11 @@ int iomap_swapfile_activate(struct swap_info_struct *sis, # define iomap_swapfile_activate(sis, swapfile, pagespan, ops) (-EIO) #endif /* CONFIG_SWAP */ +/* Use the journal_info to indicate current is in a transaction */ +static inline bool +fstrans_context_active(void) +{ + return current->journal_info != NULL; +} + #endif /* LINUX_IOMAP_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, 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() - fstrans_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 | 4 ++-- fs/xfs/xfs_aops.c | 5 +++-- fs/xfs/xfs_linux.h | 4 ---- fs/xfs/xfs_trans.c | 19 +++++++++---------- fs/xfs/xfs_trans.h | 23 +++++++++++++++++++++++ include/linux/iomap.h | 7 +++++++ 6 files changed, 44 insertions(+), 18 deletions(-)