On Wed 07-08-24 16:10:50, Stephen Zhang wrote:
> Jan Kara <jack@xxxxxxx> 于2024年8月6日周二 21:40写道：
> > On Sat 20-07-24 14:23:56, zhangshida wrote:
> > > From: Shida Zhang <zhangshida@xxxxxxxxxx>
> > >
> > > 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().
