Re: [fsdax xfs] Regression panic at inode_switch_wbs_work_fn

[Roman Gushchin <guro@fb.com>]

On Sun, Jul 18, 2021 at 12:26:00AM +0100, Matthew Wilcox wrote:
> On Sat, Jul 17, 2021 at 03:08:28PM -0700, Roman Gushchin wrote:
> > On Sat, Jul 17, 2021 at 10:17:13AM -0700, Darrick J. Wong wrote:
> > > On Fri, Jul 16, 2021 at 01:13:05PM -0700, Roman Gushchin wrote:
> > > > On Fri, Jul 16, 2021 at 01:57:55PM +0800, Murphy Zhou wrote:
> > > > > Hi,
> > > > > 
> > > > > On Fri, Jul 16, 2021 at 12:07 AM Roman Gushchin <guro@fb.com> wrote:
> > > > > >
> > > > > > On Thu, Jul 15, 2021 at 06:10:22PM +0800, Murphy Zhou wrote:
> > > > > > > Hi,
> > > > > > >
> > > > > > > #Looping generic/270 of xfstests[1] on pmem ramdisk with
> > > > > > > mount option:  -o dax=always
> > > > > > > mkfs.xfs option: -f -b size=4096 -m reflink=0
> > > > > > > can hit this panic now.
> > > > > > >
> > > > > > > #It's not reproducible on ext4.
> > > > > > > #It's not reproducible without dax=always.
> > > > > >
> > > > > > Hi Murphy!
> > > > > >
> > > > > > Thank you for the report!
> > > > > >
> > > > > > Can you, please, check if the following patch fixes the problem?
> > > > > 
> > > > > No. Still the same panic.
> > > > 
> > > > Hm, can you, please, double check this? It seems that the patch fixes the
> > > > problem for others (of course, it can be a different problem).
> > > > CCed you on the proper patch, just sent to the list.
> > > > 
> > > > Otherwise, can you, please, say on which line of code the panic happens?
> > > > (using addr2line utility, for example)
> > > 
> > > I experience the same problem that Murphy does, and I tracked it down
> > > to this chunk of inode_do_switch_wbs:
> > > 
> > > 	/*
> > > 	 * Count and transfer stats.  Note that PAGECACHE_TAG_DIRTY points
> > > 	 * to possibly dirty pages while PAGECACHE_TAG_WRITEBACK points to
> > > 	 * pages actually under writeback.
> > > 	 */
> > > 	xas_for_each_marked(&xas, page, ULONG_MAX, PAGECACHE_TAG_DIRTY) {
> > > here >>>>>>>>>> if (PageDirty(page)) {
> > > 			dec_wb_stat(old_wb, WB_RECLAIMABLE);
> > > 			inc_wb_stat(new_wb, WB_RECLAIMABLE);
> > > 		}
> > > 	}
> > > 
> > > I suspect that "page" is really a pfn to a pmem mapping and not a real
> > > struct page.
> > 
> > Good catch! Now it's clear that it's a different issue.
> > 
> > I think as now the best option is to ignore dax inodes completely.
> > Can you, please, confirm, that the following patch solves the problem?
> > 
> > Thanks!
> > 
> > --
> > 
> > diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
> > index 06d04a74ab6c..4c3370548982 100644
> > --- a/fs/fs-writeback.c
> > +++ b/fs/fs-writeback.c
> > @@ -521,6 +521,9 @@ static bool inode_prepare_wbs_switch(struct inode *inode,
> >          */
> >         smp_mb();
> >  
> > +       if (IS_DAX(inode))
> > +               return false;
> > +
> >         /* while holding I_WB_SWITCH, no one else can update the association */
> >         spin_lock(&inode->i_lock);
> >         if (!(inode->i_sb->s_flags & SB_ACTIVE) ||
> 
> That should work, but wouldn't it be better to test that at the top of
> inode_switch_wbs()?  Or even earlier?
> 

Hm, inode_switch_wbs() is not called from the cleanup path.
The cleanup path works like this:
  cleanup_offline_cgwbs_workfn()
    cleanup_offline_cgwb()
      inode_prepare_wbs_switch()
      inode_switch_wbs_work_fn()

While the generic switching path:
  inode_switch_wbs()
    inode_prepare_wbs_switch()
    inode_switch_wbs_work_fn()

Thanks!