Re: [PATCH v6 2/2] xfs: avoid transaction reservation recursion

[Yafang Shao <laoar.shao@gmail.com>]

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.

-- 
Thanks
Yafang