Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 18 Mar 2022 09:52:16 +1100
From:   Dave Chinner <david@fromorbit.com>
To:     Matthew Wilcox <willy@infradead.org>
Cc:     Linus Torvalds <torvalds@linux-foundation.org>,
        Brian Foster <bfoster@redhat.com>,
        Linux-MM <linux-mm@kvack.org>,
        linux-fsdevel <linux-fsdevel@vger.kernel.org>,
        linux-xfs <linux-xfs@vger.kernel.org>,
        Hugh Dickins <hughd@google.com>,
        Namjae Jeon <namjae.jeon@samsung.com>,
        Ashish Sangwan <a.sangwan@samsung.com>,
        Theodore Ts'o <tytso@mit.edu>, Jan Kara <jack@suse.cz>,
        linux-ext4@vger.kernel.org
Subject: Re: writeback completion soft lockup BUG in folio_wake_bit()
Message-ID: <20220317225216.GB1544202@dread.disaster.area>
References: <YjDj3lvlNJK/IPiU@bfoster>
 <YjJPu/3tYnuKK888@casper.infradead.org>
 <CAHk-=wgPTWoXCa=JembExs8Y7fw7YUi9XR0zn1xaxWLSXBN_vg@mail.gmail.com>
 <YjNN5SzHELGig+U4@casper.infradead.org>
 <CAHk-=wiZvOpaP0DVyqOnspFqpXRaT6q53=gnA2psxnf5dbt7bw@mail.gmail.com>
 <YjOlJL7xwktKoLFN@casper.infradead.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YjOlJL7xwktKoLFN@casper.infradead.org>
Precedence: bulk

On Thu, Mar 17, 2022 at 09:16:20PM +0000, Matthew Wilcox wrote:
> On Thu, Mar 17, 2022 at 12:26:35PM -0700, Linus Torvalds wrote:
> > On Thu, Mar 17, 2022 at 8:04 AM Matthew Wilcox <willy@infradead.org> wrote:
> > >
> > > So how about we do something like this:
> > >
> > >  - Make folio_start_writeback() and set_page_writeback() return void,
> > >    fixing up AFS and NFS.
> > >  - Add a folio_wait_start_writeback() to use in the VFS
> > >  - Remove the calls to set_page_writeback() in the filesystems
> > 
> > That sounds lovely, but it does worry me a bit. Not just the odd
> > 'keepwrite' thing, but also the whole ordering between the folio bit
> > and the tagging bits. Does the ordering possibly matter?
> 
> I wouldn't change the ordering of setting the xarray bits and the
> writeback flag; they'd just be set a little earlier.  It'd all be done
> while the page was still locked.  But you're right, there's lots of
> subtle interactions here.
> 
> > That whole "xyz_writeback_keepwrite()" thing seems odd. It's used in
> > only one place (the folio version isn't used at all):
> > 
> >   ext4_writepage():
> > 
> >      ext4_walk_page_buffers() fails:
> >                 redirty_page_for_writepage(wbc, page);
> >                 keep_towrite = true;
> >       ext4_bio_write_page().
> > 
> > which just looks odd. Why does it even try to continue to do the
> > writepage when the page buffer thing has failed?
> > 
> > In the regular write path (ie ext4_write_begin()), a
> > ext4_walk_page_buffers() failure is fatal or causes a retry). Why is
> > ext4_writepage() any different? Particularly since it wants to keep
> > the page dirty, then trying to do the writeback just seems wrong.
> > 
> > So this code is all a bit odd, I suspect there are decades of "people
> > continued to do what they historically did" changes, and it is all
> > worrisome.
> 
> I found the commit: 1c8349a17137 ("ext4: fix data integrity sync in
> ordered mode").  Fortunately, we have a documented test for this,
> generic/127, so we'll know if we've broken it.

Looks like a footgun. ext4 needs the keepwrite stuff for block size <
page size, in the case where a page has both written and
delalloc/unwritten buffers on it. In that case ext4_writepage tries
to write just the written blocks and leave the dealloc/unwritten
buffers alone because it can't do allocation in ->writepage context.

I say footgun, because the nested ->writepage call that needs
keepwrite comes a from journal stop context in the high level
->writepages context that is doing allocation that will allow the
entire page to be written. i.e. it seems a bit silly to be
triggering partial page writeback that skips delalloc/unwritten
extents, but then needs special awareness of higher level IO that is
in progress that is currently doing the allocation that will allow
all the delalloc/unwritten extents on the page to also be written
back...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com