Re: [RFC PATCH 2/2] jbd2: reset fast commit offset only after fs cleanup is done

[Luis Henriques]

On Thu 23 May 2024 09:44:34 AM +02, Jan Kara wrote;

> On Wed 22-05-24 14:36:20, Luis Henriques wrote:
>> On Wed 22 May 2024 12:45:00 PM +02, Jan Kara wrote;
>> 
>> > On Tue 21-05-24 16:45:35, Luis Henriques (SUSE) wrote:
>> >> When doing a journal commit, the fast journal offset (journal->j_fc_off) is
>> >> set to zero too early in the process.  Since ext4 filesystem calls function
>> >> jbd2_fc_release_bufs() in its j_fc_cleanup_callback (ext4_fc_cleanup()),
>> >> that call will be a no-op exactly because the offset is zero.
>> >> 
>> >> Move the fast commit offset further down in the journal commit code, until
>> >> it's mostly done, immediately before clearing the on-going commit flags.
>> >> 
>> >> Signed-off-by: Luis Henriques (SUSE) <luis.henriques@xxxxxxxxx>
>> >
>> > Did you see any particular failure because of this? Because AFAICS the
>> > buffers cleaned up by jbd2_fc_release_bufs() are only allocated during fast
>> > commit (from ext4_fc_reserve_space()). And the code in
>> > jbd2_journal_commit_transaction() is making sure fast commit isn't running
>> > before we set journal->j_fc_off to 0.
>> 
>> No, I did not see any failure caused by this, this patch is simply based
>> on my understanding of the code after spending some time reviewing it.
>> 
>> The problem I saw was that jbd2_journal_commit_transaction() will run the
>> clean-up callbacks, which includes ext4_fc_cleanup().  One of the first
>> things that this callback will do is to call jbd2_fc_release_bufs().
>> Because journal->j_fc_off is zero, this call is useless:
>> 
>> 	j_fc_off = journal->j_fc_off;
>> 
>> 	for (i = j_fc_off - 1; i >= 0; i--) {
>> 		[...]
>> 	}
>> 
>> (It's even a bit odd to start the loop with 'i = -1'...)
>> 
>> So the question is whether this call is actually useful at all.  Maybe the
>> thing to do is to simply remove the call to jbd2_fc_release_bufs()?  (And
>> in that case, remove the function too, as this is the only call site.)
>
> What is I guess confusing for you (and somewhat for me as well) is that
> journal->j_fc_cleanup_callback() gets called from __jbd2_fc_end_commit()
> *and* from jbd2_journal_commit_transaction(). I agree the
> jbd2_fc_release_bufs() is useless for the call from
> jbd2_journal_commit_transaction(), it is however needed for the call from
> __jbd2_fc_end_commit(). There are however other bits - namely the
> s_fc_dentry_q and s_fc_q list handling that need to happen both for normal
> and fast commit...

Oops!  I totally missed that second callback execution.  Yeah, it does
make sense now, of course.  Sorry for the noise and thank you for looking
into it.  I'll go back and focus on reworking on the other patch (and also
look into Harshad's patchset).

Cheers,
-- 
Luis