On Tue, Dec 15, 2020 at 5:08 AM Dave Chinner <david@xxxxxxxxxxxxx> wrote: > > On Sun, Dec 13, 2020 at 05:09:02PM +0800, Yafang Shao wrote: > > On Thu, Dec 10, 2020 at 3:52 AM Darrick J. Wong <darrick.wong@xxxxxxxxxx> wrote: > > > > > > On Wed, Dec 09, 2020 at 09:11:45PM +0800, Yafang Shao wrote: > > > > The xfs_trans context should be active after it is allocated, and > > > > deactive when it is freed. > > > > > > > > So these two helpers are refactored as, > > > > - xfs_trans_context_set() > > > > Used in xfs_trans_alloc() > > > > - xfs_trans_context_clear() > > > > Used in xfs_trans_free() > > > > > > > > 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@xxxxxxxxxx> > > > > Cc: Matthew Wilcox (Oracle) <willy@xxxxxxxxxxxxx> > > > > Cc: Christoph Hellwig <hch@xxxxxx> > > > > Cc: Dave Chinner <david@xxxxxxxxxxxxx> > > > > Signed-off-by: Yafang Shao <laoar.shao@xxxxxxxxx> > > > > --- > > > > fs/xfs/xfs_trans.c | 28 +++++++++++++++------------- > > > > 1 file changed, 15 insertions(+), 13 deletions(-) > > > > > > > > diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c > > > > index 11d390f0d3f2..4f4645329bb2 100644 > > > > --- a/fs/xfs/xfs_trans.c > > > > +++ b/fs/xfs/xfs_trans.c > > > > @@ -67,6 +67,17 @@ 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); > > > > + /* > > > > + * The PF_MEMALLOC_NOFS is bound to the transaction itself instead > > > > + * of the reservation, so we need to check if tp is still the > > > > + * current transaction before clearing the flag. > > > > + */ > > > > + if (current->journal_info == tp) > > > > > > Um, you don't start setting journal_info until the next patch, so this > > > means that someone who lands on this commit with git bisect will have a > > > xfs with broken logic. > > > > > > Because this is the patch that changes where we set and restore NOFS > > > context, I think you have to introduce xfs_trans_context_swap here, > > > and not in the next patch. > > > > > > > Thanks for the review. I will change it in the next version. > > > > > I also think the _swap routine has to move the old NOFS state to the > > > new transaction's t_pflags, > > > > Sure > > > > > and then 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. > > > > > > > Both thread's NOFS state and thead's journal_info state can't be > > changed in that case, right ? > > So should it better be, > > > > __xfs_trans_commit(tp, regrant) > > xfs_trans_free(tp, regrant) > > if (!regrant). // don't clear the xfs_trans_context if > > regrant is true. > > xfs_trans_context_clear() > > No. You are trying to make this way more complex than it needs to be. > The logic in the core XFS code is *already correct* and all we need > to do is move that logic to wrapper functions, then slightly modify > the implementation inside the wrapper functions. > Thanks for the explanation. > > That is, xfs_trans_context_clear() should end up like this: > Agreed. > static inline void > xfs_trans_context_clear(struct xfs_trans *tp) > { > /* > * If xfs_trans_context_swap() handed the NOFS context to a > * new transaction we do not clear the context here. > */ > if (current->journal_info != tp) current->journal_info hasn't been used in patch #3, that will make patch #3 a little more complex. We have to do some workaround in patch #3. I will think about it. > return; > current->journal_info = NULL; > memalloc_nofs_restore(tp->t_pflags); > } > > -Dave. > -- > Dave Chinner > david@xxxxxxxxxxxxx -- Thanks Yafang
