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 14:16:00 +0100
From:   Jan Kara <jack@suse.cz>
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: <20220318131600.iv7ct2m4o52plkhl@quack3.lan>
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 17-03-22 21:16:20, Matthew Wilcox wrote:
> On Thu, Mar 17, 2022 at 12:26:35PM -0700, Linus Torvalds wrote:
> > 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.

I agree with Dave that 'keep_towrite' thing is kind of self-inflicted
damage on the ext4 side (we need to write out some blocks underlying the
page but cannot write all from the transaction commit code, so we need to
keep xarray tags intact so that data integrity sync cannot miss the page).
Also it is no longer needed in the current default ext4 setup. But if you
have blocksize < pagesize and mount the fs with 'dioreadlock,data=ordered'
mount options, the hack is still needed AFAIK and we don't have a
reasonable way around it.

								Honza

-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR