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

[Yafang Shao <laoar.shao@xxxxxxxxx>]

On Tue, Aug 25, 2020 at 1:32 PM Dave Chinner <david@xxxxxxxxxxxxx> 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@xxxxxxxxx> 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;


-- 
Thanks
Yafang