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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [eCos]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux