| Message ID | 20201217011157.92549-4-laoar.shao@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xfs: avoid transaction reservation recursion | expand |
On Thu, Dec 17, 2020 at 09:11:56AM +0800, Yafang Shao wrote: > The xfs_trans context should be active after it is allocated, and > deactive when it is freed. > The rolling transaction should be specially considered, because in the > case when we clear the old transaction the thread's NOFS state shouldn't > be changed, as a result we have to set NOFS in the old transaction's > t_pflags in xfs_trans_context_swap(). > > So these helpers are refactored as, > - xfs_trans_context_set() > Used in xfs_trans_alloc() > - xfs_trans_context_clear() > Used in xfs_trans_free() > > And a new helper is instroduced to handle the rolling transaction, > - xfs_trans_context_swap() > Used in rolling transaction > > This patch is based on Darrick's work to fix the issue in xfs/141 in the > earlier version. [1] > > 1. https://lore.kernel.org/linux-xfs/20201104001649.GN7123@magnolia As I said in my last comments, this change of logic is not necessary. All we need to do is transfer the NOFS state to the new transactions and *remove it from the old one*. IOWs, all this patch should do is: > @@ -119,7 +123,9 @@ 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; > + > + /* Associate the new transaction with this thread. */ > + xfs_trans_context_swap(tp, ntp); > > /* move deferred ops over to the new tp */ > xfs_defer_move(ntp, tp); This, and > diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h > index 44b11c64a15e..12380eaaf7ce 100644 > --- a/fs/xfs/xfs_trans.h > +++ b/fs/xfs/xfs_trans.h > @@ -280,4 +280,17 @@ xfs_trans_context_clear(struct xfs_trans *tp) > memalloc_nofs_restore(tp->t_pflags); > } > > +static inline void > +xfs_trans_context_swap(struct xfs_trans *tp, struct xfs_trans *ntp) > +{ introduce this wrapper. > + ntp->t_pflags = tp->t_pflags; > + /* > + * For the rolling transaction, we have to set NOFS in the old > + * transaction's t_pflags so that when we clear the context on > + * the old transaction we don't actually change the thread's NOFS > + * state. > + */ > + tp->t_pflags = current->flags | PF_MEMALLOC_NOFS; > +} But not with this implementation. Think for a minute, please. All we want to do is avoid clearing the nofs state when we call xfs_trans_context_clear(tp) if the state has been handed to another transaction. Your current implementation hands the state to ntp, but *then leaves it on tp* as well. So then you hack a PF_MEMALLOC_NOFS flag into tp->t_pflags so that it doesn't clear that flag (abusing the masking done during clearing). That's just nasty because it relies on internal memalloc_nofs_restore() details for correct functionality. The obvious solution: we've moved the saved process state to a different context, so it is no longer needed for the current transaction we are about to commit. So How about just clearing the saved state from the original transaction when swappingi like so: static inline void xfs_trans_context_swap(struct xfs_trans *tp, struct xfs_trans *ntp) { ntp->t_pflags = tp->t_pflags; tp->t_flags = 0; } And now, when we go to clear the transaction context, we can simply do this: static inline void xfs_trans_context_clear(struct xfs_trans *tp) { if (tp->t_pflags) memalloc_nofs_restore(tp->t_pflags); } and the problem is solved. The NOFS state will follow the active transaction and not be reset until the entire transaction chain is completed. In the next patch you can go and introduce current->journal_info into just the wrapper functions, maintaining the same overall logic. -Dave.
On Fri, Dec 18, 2020 at 09:15:09AM +1100, Dave Chinner wrote: > On Thu, Dec 17, 2020 at 09:11:56AM +0800, Yafang Shao wrote: > > The xfs_trans context should be active after it is allocated, and > > deactive when it is freed. > > The rolling transaction should be specially considered, because in the > > case when we clear the old transaction the thread's NOFS state shouldn't > > be changed, as a result we have to set NOFS in the old transaction's > > t_pflags in xfs_trans_context_swap(). > > > > So these helpers are refactored as, > > - xfs_trans_context_set() > > Used in xfs_trans_alloc() > > - xfs_trans_context_clear() > > Used in xfs_trans_free() > > > > And a new helper is instroduced to handle the rolling transaction, > > - xfs_trans_context_swap() > > Used in rolling transaction > > > > This patch is based on Darrick's work to fix the issue in xfs/141 in the > > earlier version. [1] > > > > 1. https://lore.kernel.org/linux-xfs/20201104001649.GN7123@magnolia > > As I said in my last comments, this change of logic is not > necessary. All we need to do is transfer the NOFS state to the new > transactions and *remove it from the old one*. > > IOWs, all this patch should do is: > > > @@ -119,7 +123,9 @@ 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; > > + > > + /* Associate the new transaction with this thread. */ > > + xfs_trans_context_swap(tp, ntp); > > > > /* move deferred ops over to the new tp */ > > xfs_defer_move(ntp, tp); > > This, and > > > diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h > > index 44b11c64a15e..12380eaaf7ce 100644 > > --- a/fs/xfs/xfs_trans.h > > +++ b/fs/xfs/xfs_trans.h > > @@ -280,4 +280,17 @@ xfs_trans_context_clear(struct xfs_trans *tp) > > memalloc_nofs_restore(tp->t_pflags); > > } > > > > +static inline void > > +xfs_trans_context_swap(struct xfs_trans *tp, struct xfs_trans *ntp) > > +{ > > introduce this wrapper. > > > + ntp->t_pflags = tp->t_pflags; > > + /* > > + * For the rolling transaction, we have to set NOFS in the old > > + * transaction's t_pflags so that when we clear the context on > > + * the old transaction we don't actually change the thread's NOFS > > + * state. > > + */ > > + tp->t_pflags = current->flags | PF_MEMALLOC_NOFS; > > +} > > But not with this implementation. > > Think for a minute, please. All we want to do is avoid clearing > the nofs state when we call xfs_trans_context_clear(tp) if the state > has been handed to another transaction. > > Your current implementation hands the state to ntp, but *then leaves > it on tp* as well. So then you hack a PF_MEMALLOC_NOFS flag into > tp->t_pflags so that it doesn't clear that flag (abusing the masking > done during clearing). That's just nasty because it relies on > internal memalloc_nofs_restore() details for correct functionality. > > The obvious solution: we've moved the saved process state to a > different context, so it is no longer needed for the current > transaction we are about to commit. So How about just clearing the > saved state from the original transaction when swappingi like so: > > static inline void > xfs_trans_context_swap(struct xfs_trans *tp, struct xfs_trans *ntp) > { > ntp->t_pflags = tp->t_pflags; > tp->t_flags = 0; > } > > And now, when we go to clear the transaction context, we can simply > do this: > > static inline void > xfs_trans_context_clear(struct xfs_trans *tp) > { > if (tp->t_pflags) > memalloc_nofs_restore(tp->t_pflags); > } > > and the problem is solved. The NOFS state will follow the active > transaction and not be reset until the entire transaction chain is > completed. Er... correct me if I'm wrong, but I thought t_pflags stores the old state of current->flags from before we call xfs_trans_alloc? So if we call into xfs_trans_alloc with current->flags==0 and we commit the transaction having not rolled, we won't unset the _NOFS state, and exit back to userspace with _NOFS set. I think the logic is correct here -- we transfer the old pflags value from @tp to @ntp, which effectively puts @ntp in charge of restoring the old pflags value; and then we set tp->t_pflags to current->flags, which means that @tp will "restore" the current pflags value into pflags, which is a nop. --D > In the next patch you can go and introduce current->journal_info > into just the wrapper functions, maintaining the same overall > logic. > > -Dave. > -- > Dave Chinner > david@fromorbit.com
On Thu, Dec 17, 2020 at 03:06:27PM -0800, Darrick J. Wong wrote: > On Fri, Dec 18, 2020 at 09:15:09AM +1100, Dave Chinner wrote: > > The obvious solution: we've moved the saved process state to a > > different context, so it is no longer needed for the current > > transaction we are about to commit. So How about just clearing the > > saved state from the original transaction when swappingi like so: > > > > static inline void > > xfs_trans_context_swap(struct xfs_trans *tp, struct xfs_trans *ntp) > > { > > ntp->t_pflags = tp->t_pflags; > > tp->t_flags = 0; > > } > > > > And now, when we go to clear the transaction context, we can simply > > do this: > > > > static inline void > > xfs_trans_context_clear(struct xfs_trans *tp) > > { > > if (tp->t_pflags) > > memalloc_nofs_restore(tp->t_pflags); > > } > > > > and the problem is solved. The NOFS state will follow the active > > transaction and not be reset until the entire transaction chain is > > completed. > > Er... correct me if I'm wrong, but I thought t_pflags stores the old > state of current->flags from before we call xfs_trans_alloc? So if we > call into xfs_trans_alloc with current->flags==0 and we commit the > transaction having not rolled, we won't unset the _NOFS state, and exit > back to userspace with _NOFS set. Minor thinko. tp->t_pflags only stores a single bit of information w.r.t PF_MEMALLOC_NOFS state, not the entire set of current task flags: whether it should be cleared or not on a call to memalloc_nofs_restore(). See memalloc_nofs_save(): static inline unsigned int memalloc_nofs_save(void) { unsigned int flags = current->flags & PF_MEMALLOC_NOFS; current->flags |= PF_MEMALLOC_NOFS; return flags; } So, yeah, I got the t_pflags logic the wrong way around here - zero means clear it, non-zero means don't clear it. IOWs, swap: ntp->t_pflags = tp->t_pflags; tp->t_flags = -1; and clear: if (!tp->t_flags) memalloc_nofs_restore(tp->t_pflags); This should only be temporary code, anyway. The next patch should change this state clearing check to use current->journal_info, then we aren't dependent on process flags state at all. > I think the logic is correct here -- we transfer the old pflags value > from @tp to @ntp, which effectively puts @ntp in charge of restoring the > old pflags value; and then we set tp->t_pflags to current->flags, which > means that @tp will "restore" the current pflags value into pflags, which > is a nop. That's pretty much what the current logic guarantees. Once the wrappers provide this same guarantee, we can greatly simply the the transaction context state management (i.e. set on alloc, swap on dup, clear on free). And thread handoffs can just use clear/set appropriately. Cheers, Dave.
On Fri, Dec 18, 2020 at 6:15 AM Dave Chinner <david@fromorbit.com> wrote: > > On Thu, Dec 17, 2020 at 09:11:56AM +0800, Yafang Shao wrote: > > The xfs_trans context should be active after it is allocated, and > > deactive when it is freed. > > The rolling transaction should be specially considered, because in the > > case when we clear the old transaction the thread's NOFS state shouldn't > > be changed, as a result we have to set NOFS in the old transaction's > > t_pflags in xfs_trans_context_swap(). > > > > So these helpers are refactored as, > > - xfs_trans_context_set() > > Used in xfs_trans_alloc() > > - xfs_trans_context_clear() > > Used in xfs_trans_free() > > > > And a new helper is instroduced to handle the rolling transaction, > > - xfs_trans_context_swap() > > Used in rolling transaction > > > > This patch is based on Darrick's work to fix the issue in xfs/141 in the > > earlier version. [1] > > > > 1. https://lore.kernel.org/linux-xfs/20201104001649.GN7123@magnolia > > As I said in my last comments, this change of logic is not > necessary. All we need to do is transfer the NOFS state to the new > transactions and *remove it from the old one*. > Thanks for the explanation, I will change it. > IOWs, all this patch should do is: > > > @@ -119,7 +123,9 @@ 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; > > + > > + /* Associate the new transaction with this thread. */ > > + xfs_trans_context_swap(tp, ntp); > > > > /* move deferred ops over to the new tp */ > > xfs_defer_move(ntp, tp); > > This, and > > > diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h > > index 44b11c64a15e..12380eaaf7ce 100644 > > --- a/fs/xfs/xfs_trans.h > > +++ b/fs/xfs/xfs_trans.h > > @@ -280,4 +280,17 @@ xfs_trans_context_clear(struct xfs_trans *tp) > > memalloc_nofs_restore(tp->t_pflags); > > } > > > > +static inline void > > +xfs_trans_context_swap(struct xfs_trans *tp, struct xfs_trans *ntp) > > +{ > > introduce this wrapper. > > > + ntp->t_pflags = tp->t_pflags; > > + /* > > + * For the rolling transaction, we have to set NOFS in the old > > + * transaction's t_pflags so that when we clear the context on > > + * the old transaction we don't actually change the thread's NOFS > > + * state. > > + */ > > + tp->t_pflags = current->flags | PF_MEMALLOC_NOFS; > > +} > > But not with this implementation. > > Think for a minute, please. All we want to do is avoid clearing > the nofs state when we call xfs_trans_context_clear(tp) if the state > has been handed to another transaction. > > Your current implementation hands the state to ntp, but *then leaves > it on tp* as well. So then you hack a PF_MEMALLOC_NOFS flag into > tp->t_pflags so that it doesn't clear that flag (abusing the masking > done during clearing). That's just nasty because it relies on > internal memalloc_nofs_restore() details for correct functionality. > > The obvious solution: we've moved the saved process state to a > different context, so it is no longer needed for the current > transaction we are about to commit. So How about just clearing the > saved state from the original transaction when swappingi like so: > > static inline void > xfs_trans_context_swap(struct xfs_trans *tp, struct xfs_trans *ntp) > { > ntp->t_pflags = tp->t_pflags; > tp->t_flags = 0; > } > > And now, when we go to clear the transaction context, we can simply > do this: > > static inline void > xfs_trans_context_clear(struct xfs_trans *tp) > { > if (tp->t_pflags) > memalloc_nofs_restore(tp->t_pflags); > } > > and the problem is solved. The NOFS state will follow the active > transaction and not be reset until the entire transaction chain is > completed. > > In the next patch you can go and introduce current->journal_info > into just the wrapper functions, maintaining the same overall > logic. > > -Dave. > -- > Dave Chinner > david@fromorbit.com
On Fri, Dec 18, 2020 at 8:07 AM Dave Chinner <david@fromorbit.com> wrote: > > On Thu, Dec 17, 2020 at 03:06:27PM -0800, Darrick J. Wong wrote: > > On Fri, Dec 18, 2020 at 09:15:09AM +1100, Dave Chinner wrote: > > > The obvious solution: we've moved the saved process state to a > > > different context, so it is no longer needed for the current > > > transaction we are about to commit. So How about just clearing the > > > saved state from the original transaction when swappingi like so: > > > > > > static inline void > > > xfs_trans_context_swap(struct xfs_trans *tp, struct xfs_trans *ntp) > > > { > > > ntp->t_pflags = tp->t_pflags; > > > tp->t_flags = 0; > > > } > > > > > > And now, when we go to clear the transaction context, we can simply > > > do this: > > > > > > static inline void > > > xfs_trans_context_clear(struct xfs_trans *tp) > > > { > > > if (tp->t_pflags) > > > memalloc_nofs_restore(tp->t_pflags); > > > } > > > > > > and the problem is solved. The NOFS state will follow the active > > > transaction and not be reset until the entire transaction chain is > > > completed. > > > > Er... correct me if I'm wrong, but I thought t_pflags stores the old > > state of current->flags from before we call xfs_trans_alloc? So if we > > call into xfs_trans_alloc with current->flags==0 and we commit the > > transaction having not rolled, we won't unset the _NOFS state, and exit > > back to userspace with _NOFS set. > > Minor thinko. > > tp->t_pflags only stores a single bit of information w.r.t > PF_MEMALLOC_NOFS state, not the entire set of current task flags: > whether it should be cleared or not on a call to > memalloc_nofs_restore(). See memalloc_nofs_save(): > > static inline unsigned int memalloc_nofs_save(void) > { > unsigned int flags = current->flags & PF_MEMALLOC_NOFS; > current->flags |= PF_MEMALLOC_NOFS; > return flags; > } > > So, yeah, I got the t_pflags logic the wrong way around here - zero > means clear it, non-zero means don't clear it. IOWs, swap: > > ntp->t_pflags = tp->t_pflags; > tp->t_flags = -1; > > and clear: > > if (!tp->t_flags) > memalloc_nofs_restore(tp->t_pflags); > > This should only be temporary code, anyway. The next patch should > change this state clearing check to use current->journal_info, then > we aren't dependent on process flags state at all. > > > I think the logic is correct here -- we transfer the old pflags value > > from @tp to @ntp, which effectively puts @ntp in charge of restoring the > > old pflags value; and then we set tp->t_pflags to current->flags, which > > means that @tp will "restore" the current pflags value into pflags, which > > is a nop. > > That's pretty much what the current logic guarantees. Once the > wrappers provide this same guarantee, we can greatly simply the the > transaction context state management (i.e. set on alloc, swap on > dup, clear on free). And thread handoffs can just use clear/set > appropriately. > Make sense to me.
diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c index 11d390f0d3f2..aa213c3e2408 100644 --- a/fs/xfs/xfs_trans.c +++ b/fs/xfs/xfs_trans.c @@ -67,6 +67,10 @@ 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. */ + 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 +123,9 @@ 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; + + /* Associate the new transaction with this thread. */ + xfs_trans_context_swap(tp, ntp); /* move deferred ops over to the new tp */ xfs_defer_move(ntp, tp); @@ -153,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 */ - xfs_trans_context_set(tp); - /* * Attempt to reserve the needed disk blocks by decrementing * the number needed from the number available. This will @@ -163,10 +166,9 @@ xfs_trans_reserve( */ if (blocks > 0) { error = xfs_mod_fdblocks(mp, -((int64_t)blocks), rsvd); - if (error != 0) { - xfs_trans_context_clear(tp); + if (error != 0) return -ENOSPC; - } + tp->t_blk_res += blocks; } @@ -241,8 +243,6 @@ xfs_trans_reserve( tp->t_blk_res = 0; } - xfs_trans_context_clear(tp); - return error; } @@ -284,6 +284,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 +880,6 @@ __xfs_trans_commit( xfs_log_commit_cil(mp, tp, &commit_lsn, regrant); - xfs_trans_context_clear(tp); xfs_trans_free(tp); /* @@ -911,7 +912,6 @@ __xfs_trans_commit( tp->t_ticket = NULL; } - xfs_trans_context_clear(tp); xfs_trans_free_items(tp, !!error); xfs_trans_free(tp); @@ -971,9 +971,6 @@ xfs_trans_cancel( tp->t_ticket = NULL; } - /* mark this thread as no longer being in a transaction */ - xfs_trans_context_clear(tp); - 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 44b11c64a15e..12380eaaf7ce 100644 --- a/fs/xfs/xfs_trans.h +++ b/fs/xfs/xfs_trans.h @@ -280,4 +280,17 @@ xfs_trans_context_clear(struct xfs_trans *tp) memalloc_nofs_restore(tp->t_pflags); } +static inline void +xfs_trans_context_swap(struct xfs_trans *tp, struct xfs_trans *ntp) +{ + ntp->t_pflags = tp->t_pflags; + /* + * For the rolling transaction, we have to set NOFS in the old + * transaction's t_pflags so that when we clear the context on + * the old transaction we don't actually change the thread's NOFS + * state. + */ + tp->t_pflags = current->flags | PF_MEMALLOC_NOFS; +} + #endif /* __XFS_TRANS_H__ */
The xfs_trans context should be active after it is allocated, and deactive when it is freed. The rolling transaction should be specially considered, because in the case when we clear the old transaction the thread's NOFS state shouldn't be changed, as a result we have to set NOFS in the old transaction's t_pflags in xfs_trans_context_swap(). So these helpers are refactored as, - xfs_trans_context_set() Used in xfs_trans_alloc() - xfs_trans_context_clear() Used in xfs_trans_free() And a new helper is instroduced to handle the rolling transaction, - xfs_trans_context_swap() Used in rolling transaction This patch is based on Darrick's work to fix the issue in xfs/141 in the earlier version. [1] 1. 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> Signed-off-by: Yafang Shao <laoar.shao@gmail.com> --- fs/xfs/xfs_trans.c | 25 +++++++++++-------------- fs/xfs/xfs_trans.h | 13 +++++++++++++ 2 files changed, 24 insertions(+), 14 deletions(-)