lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <87wmnk3o6n.fsf@brahms.olymp>
Date: Thu, 23 May 2024 09:52:32 +0100
From: Luis Henriques <luis.henriques@...ux.dev>
To: Jan Kara <jack@...e.cz>
Cc: Luis Henriques <luis.henriques@...ux.dev>,  Theodore Ts'o
 <tytso@....edu>,  Andreas Dilger <adilger@...ger.ca>,  Jan Kara
 <jack@...e.com>,  linux-ext4@...r.kernel.org,
  linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH 2/2] jbd2: reset fast commit offset only after fs
 cleanup is done

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@...ux.dev>
>> >
>> > 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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ