Received-SPF: pass (google.com: best guess record for domain of linux-ext4-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Mauricio Faria de Oliveira <mfo@canonical.com>
To:     "Theodore Y . Ts'o" <tytso@mit.edu>
Cc:     dann frazier <dann.frazier@canonical.com>,
        linux-ext4@vger.kernel.org
Subject: Re: Re: ext4 fsck vs. kernel recovery policy
Date:   Mon, 27 Jan 2020 08:34:27 -0300
Message-Id: <20200127113427.20214-1-mfo@canonical.com>
In-Reply-To: <20190830012236.GC10779@mit.edu>
References: <20190830012236.GC10779@mit.edu>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Sender: linux-ext4-owner@vger.kernel.org
Precedence: bulk

On Thu, 29 Aug 2019 21:22:36 -0400, Theodore Y. Ts'o wrote:
> On Thu, Aug 29, 2019 at 04:53:48PM -0600, dann frazier wrote:
>> JBD2: Invalid checksum recovering data block 517634 in log
>> 
>> So is it correct to say that the checksum errors were identifying
>> filesystem correctness issues, and therefore e2fsck was needed to
>> correct them?
> 
> That's correct.  More precisely, checksum errors for journal blocks
> are presumed to mean that file system might be corrupt, so a full
> e2fsck check was needed to make sure the file system was consistent.
> 
> > You're probably right - this issue is very easy to reproduce w/
> > data=journal,journal_checksum. I was never able to reproduce it
> > otherwise.
> 
> I've looked at the data block numbers that you've reported, and they
> come from a journald file.  The problem is with data=journal +
> journal_checksum + mmap.  Unfortunately, we don't handle that
> combination correctly at the moment.
> 
> The fix is going to have to involve fixing __ext4_journalled_writepage()
> to call set_page_writeback() before it unlocks the page, adding a list of
> pages under data=journalled writeback which is attached to the
> transaction handle, have the jbd2 commit hook call end_page_writeback()
> on all of these pages, and then in the places where ext4 calls
> wait_for_stable_page() or grab_cache_page_write_begin(),
> we need to add:
> 
> 	if (ext4_should_journal_data(inode))
> 		wait_on_page_writeback(page);
> 

If I understood and wrote things correctly, this can hit a deadlock between

1) jbd2_journal_commit_transaction() -- waiting for t_updates to drop (i.e.,
   ext4_journal_stop() to be called), blocking commit / end_page_writeback().

and

2) ext4_write_begin() -- waiting in wait_on_page_writeback() in between the
   calls to ext4_journal_start()/stop(), blocking t_updates from dropping.

I worked around it moving wait_on_page_writeback() before ext4_journal_start(),
but wonder if this is really expected and the algorithm needs tuning/changes,
or not expected and thus an implementation error. (First time with ext4 code.)

P.S.: sorry to bother again about it (I already wrote/pinged on [1, 2]) but
this does seem interesting and shows an issue we're interested in fixing up.

Thanks again for your help/suggestions on this.

[1] https://lore.kernel.org/linux-ext4/20191221202630.30718-1-mfo@canonical.com/
[2] https://lore.kernel.org/linux-ext4/20191221202630.30718-2-mfo@canonical.com/

cheers,
Mauricio

> It's all relatively straightforward except for the part where we have to
> attach a list of pages to the currently running transaction.  That
> will require adding  some plumbing into the jbd2 layer.
> 
> Dann, any interest in trying to code this fix?
> 
>       	  	      	     	     	  - Ted