linux-kernel - Re: [PATCH 6/6] jbd2: No need to use t_handle_lock in jbd2_journal_wait

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] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 13 Jan 2022 18:08:42 +0530
From:   Ritesh Harjani <riteshh@...ux.ibm.com>
To:     Jan Kara <jack@...e.cz>
Cc:     linux-ext4@...r.kernel.org, linux-fsdevel@...r.kernel.org,
        linux-kernel@...r.kernel.org, Jan Kara <jack@...e.com>,
        Andreas Dilger <adilger.kernel@...ger.ca>, tytso@....edu,
        Eric Whitney <enwlinux@...il.com>
Subject: Re: [PATCH 6/6] jbd2: No need to use t_handle_lock in
 jbd2_journal_wait_updates

On 22/01/13 12:27PM, Jan Kara wrote:
> On Thu 13-01-22 08:56:29, Ritesh Harjani wrote:
> > Since jbd2_journal_wait_updates() uses waitq based on t_updates atomic_t
> > variable. So from code review it looks like we don't need to use
> > t_handle_lock spinlock for checking t_updates value.
> > Hence this patch gets rid of the spinlock protection in
> > jbd2_journal_wait_updates()
> >
> > Signed-off-by: Ritesh Harjani <riteshh@...ux.ibm.com>
>
> This patch looks good. Feel free to add:
>
> Reviewed-by: Jan Kara <jack@...e.cz>
>
> Actually looking at it, t_handle_lock seems to be very much unused. I agree

I too had this thought in mind. Thanks for taking a deeper look into it :)

>
> we don't need it when waiting for outstanding handles but the only
> remaining uses are:
>
> 1) jbd2_journal_extend() where it is not needed either - we use
> atomic_add_return() to manipulate t_outstanding_credits and hold
> j_state_lock for reading which provides us enough exclusion.
>
> 2) update_t_max_wait() - this is the only valid use of t_handle_lock but we
> can just switch it to cmpxchg loop with a bit of care. Something like:
>
> 	unsigned long old;
>
> 	ts = jbd2_time_diff(ts, transaction->t_start);
> 	old = transaction->t_max_wait;
> 	while (old < ts)
> 		old = cmpxchg(&transaction->t_max_wait, old, ts);
>
> So perhaps you can add two more patches to remove other t_handle_lock uses
> and drop it completely.

Thanks for providing the details Jan :)
Agree with jbd2_journal_extend(). I did looked a bit around t_max_wait and
I agree that something like above could work. I will spend some more time around
that code and will submit those changes together in v2.

-ritesh

>
> 								Honza
>
> > ---
> >  include/linux/jbd2.h | 4 ----
> >  1 file changed, 4 deletions(-)
> >
> > diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
> > index 34b051aa9009..9bef47622b9d 100644
> > --- a/include/linux/jbd2.h
> > +++ b/include/linux/jbd2.h
> > @@ -1768,22 +1768,18 @@ static inline void jbd2_journal_wait_updates(journal_t *journal)
> >  	if (!commit_transaction)
> >  		return;
> >
> > -	spin_lock(&commit_transaction->t_handle_lock);
> >  	while (atomic_read(&commit_transaction->t_updates)) {
> >  		DEFINE_WAIT(wait);
> >
> >  		prepare_to_wait(&journal->j_wait_updates, &wait,
> >  					TASK_UNINTERRUPTIBLE);
> >  		if (atomic_read(&commit_transaction->t_updates)) {
> > -			spin_unlock(&commit_transaction->t_handle_lock);
> >  			write_unlock(&journal->j_state_lock);
> >  			schedule();
> >  			write_lock(&journal->j_state_lock);
> > -			spin_lock(&commit_transaction->t_handle_lock);
> >  		}
> >  		finish_wait(&journal->j_wait_updates, &wait);
> >  	}
> > -	spin_unlock(&commit_transaction->t_handle_lock);
> >  }
> >
> >  /*
> > --
> > 2.31.1
> >
> --
> Jan Kara <jack@...e.com>
> SUSE Labs, CR