Messages in this thread Patch in this message | | | Date | Thu, 13 Jan 2022 12:27:49 +0100 | From | Jan Kara <> | Subject | Re: [PATCH 6/6] jbd2: No need to use t_handle_lock in jbd2_journal_wait_updates |
| |
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@linux.ibm.com>
This patch looks good. Feel free to add:
Reviewed-by: Jan Kara <jack@suse.cz>
Actually looking at it, t_handle_lock seems to be very much unused. I agree 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.
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@suse.com> SUSE Labs, CR
| |