Re: [RFC PATCH] jbd2: fix a potential assertion failure due to improperly dirtied buffer

[Jan Kara <jack@suse.cz>]

On Thu 08-08-24 11:05:26, Stephen Zhang wrote:
> Jan Kara <jack@suse.cz> 于2024年8月7日周三 20:07写道：
> > 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?
> >
> 
> Yeah, Thank you for giving me the opportunity to work on something truly
> meaningful. All I can do until now is some small cleanups. And doing cleanups
> all the time is annoyable to the maintainers and frustrating to me. I
> will try my best.
> 
> So basically, we should:
> 1.Trace the user data dirting in ext4_block_write_begin().
> 2.Replace the uncontrollable __block_write_begin with ext4_block_write_begin().
> 3.Remove some superfluous things.

Yes. In the first patch, I'd convert all uses of __block_write_begin() to
ext4_block_write_begin(). In the second patch I'd replace
folio_zero_new_buffers() with ext4_journalled_zero_new_buffers() if inode
has journalled data (with explanation to avoid unexpected dirtying). In the
third patch I'd remove the clear_buffer_new() and mark_buffer_dirty()
mentioned above with explanation that either folio_zero_new_buffers() or
block_commit_write() take care of dirtying the buffer properly. Thanks for
working on this!

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