Message-ID: <20240807120659.y6cpxas5g3mze2rr@quack3>
Date: Wed, 7 Aug 2024 14:06:59 +0200
From: Jan Kara <jack@...e.cz>
To: Stephen Zhang <starzhangzsd@...il.com>
Cc: Jan Kara <jack@...e.cz>, tytso@....edu, jack@...e.com,
	linux-ext4@...r.kernel.org, linux-kernel@...r.kernel.org,
	zhangshida@...inos.cn, Baolin Liu <liubaolin@...inos.cn>
Subject: Re: [RFC PATCH] jbd2: fix a potential assertion failure due to
 improperly dirtied buffer

On Wed 07-08-24 16:10:50, Stephen Zhang wrote:
> Jan Kara <jack@...e.cz> 于2024年8月6日周二 21:40写道：
> > On Sat 20-07-24 14:23:56, zhangshida wrote:
> > > From: Shida Zhang <zhangshida@...inos.cn>
> > >
> > > On an old kernel version(4.19, ext3, journal=data, pagesize=64k),
> > > an assertion failure will occasionally be triggered by the line below:
> >
> > OK, just out of curiosity, why are you using data=journal mode? It doesn't
> > really get that much testing and the performance is quite bad...
> >
> 
> It is used by one of our customers. It's more like a historical issue:
> About 12 years ago, they used data=journal mode for the benefit of user
> data consistency brought by the mode.
> Time goes by, they attempted to change, say, maybe change it to ext4
> at least, but found it is no more stable than it was under ext3...
> And yeah, they decided to just leave the thing as it was and keep the system
> under that state until now...

I see, thanks for sharing. I was asking because we are mostly trying to
steer away people from using data=journal mode and deprecate it because it
adds a lot of complexity into the code without significant benefit.

> > > jbd2_journal_commit_transaction
> > > {
> > > ...
> > > J_ASSERT_BH(bh, !buffer_dirty(bh));
> > > /*
> > > * The buffer on BJ_Forget list and not jbddirty means
> > > ...
> > > }
> > >
> > > AFAIC, that's how the problem works:
> > > --------
> > > journal_unmap_buffer
> > > jbd2_journal_invalidatepage
> > > __ext4_journalled_invalidatepage
> > > ext4_journalled_invalidatepage
> > > do_invalidatepage
> > > truncate_inode_pages_range
> > > truncate_inode_pages
> > > truncate_pagecache
> > > ext4_setattr
> > > --------
> > >
> > > First try to truncate and invalidate the page.
> > > Sometimes the buffer and the page won't be freed immediately.
> > > the buffer will be sent to the BJ_Forget list of the currently
> > > committing transaction. Maybe the transaction knows when and how
> > > to free the buffer better.
> > > The buffer's states now: !jbd_dirty !mapped !dirty
> > >
> > > Then jbd2_journal_commit_transaction(）will try to iterate over the
> > > BJ_Forget list:
> > > --------
> > > jbd2_journal_commit_transaction()
> > >       while (commit_transaction->t_forget) {
> > >       ...
> > >       }
> > > --------
> > >
> > > At this exact moment, another write comes:
> > > --------
> > > mark_buffer_dirty
> > > __block_write_begin_int
> > > __block_write_begin
> > > ext4_write_begin
> > > --------
> > > it sees a unmapped new buffer, and marks it as dirty.
> >
> > This should not happen. When ext4_setattr() truncates the file, we do not
> > allow reallocating these blocks for other purposes until the transaction
> 
> ext4_setattr() will try to free it by adding it to the BJ_Forget list
> for further processing.
> Put it more clearly,
> when ext4_setattr() truncates the file, the buffer is not fully freed
> yet. It's half-freed.
> Furthermore,
> Because the buffer is half-freed, the reallocating thing won't need to happen.
> Now,
> under that scenario, can we redirty the half-freed buffer on the BJ_Forget list?
> The answer may be 'yes'.
> 
> redirty it by the following code:
> ext4_block_write_begin
>     if (!buffer_mapped(bh)) { // check 1
>          _ext4_get_block(inode, block, bh, 1);
>         (buffer_new(bh)) { // check 2
>              if (folio_test_uptodate(folio)) { // check 3
>                  mark_buffer_dirty(bh);

<snip>

I see, right. It is not that the block would get reused. It is just that
the buffer_head on the file's tail page gets reused and this causes issues.
In fact, the problem is with ext4_block_write_begin() (and
__block_write_begin_int()) that they call mark_buffer_dirty() on a
journalled buffer before calling jbd2_journal_get_write_access() (which
would remove the buffer from BJ_Forget list). This is what ultimately
confuses the commit code.

> For another proof, there is indeed a small window where the buffer could be
> seen dirty.
> Have a look at the code and comment in do_journal_get_write_access:
> ----------------
> int do_journal_get_write_access(handle_t *handle, struct inode *inode,
> struct buffer_head *bh)
> {
> ...
> /*
> * __block_write_begin() could have dirtied some buffers. Clean
> * the dirty bit as jbd2_journal_get_write_access() could complain
> * otherwise about fs integrity issues. Setting of the dirty bit
> * by __block_write_begin() isn't a real problem here as we clear
> * the bit before releasing a page lock and thus writeback cannot
> * ever write the buffer.
> */
> if (dirty)
> clear_buffer_dirty(bh); // clear the dirty immdiately in case some bad
> things happen

OK, it was even me adding that comment 14 years ago ;) I already forgot
about this nuance.

So I agree with your analysis now. But still don't like adding hacks to
jbd2 to acommodate for this oddity of data=journal mode. Since we already
have ext4_block_write_begin() implementation anyway, we should be able to
tweak it to do the right thing for data=journal mode inodes...

So we could replace uses of __block_write_begin() with
ext4_block_write_begin() and then call do_journal_get_write_access() in
ext4_block_write_begin() for inodes with journalled data after the buffer
is mapped with get_block().

>From the part:
                                if (folio_test_uptodate(folio)) {
                                        clear_buffer_new(bh);
                                        set_buffer_uptodate(bh);
                                        mark_buffer_dirty(bh);
                                        continue;
                                }

we can actually remove the clear_buffer_new() and mark_buffer_dirty() bits
because they will be done by block_commit_write() or
folio_zero_new_buffers() and they are superfluous and somewhat odd here
anyway.

And the call to folio_zero_new_buffers() from ext4_block_write_begin()
needs to call ext4_journalled_zero_new_buffers() for inodes where data is
journalled.

Will you try to implement this or should I look into it?

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

Powered by blists - more mailing lists