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@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);
> }
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.
--
Dave Chinner
david@xxxxxxxxxxxxx
