[6/6] jbd2: No need to use t_handle_lock in jbd2_journal_wait_updates

Commit Message

Ritesh Harjani Jan. 13, 2022, 3:26 a.m. UTC

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 <[e-mail]>
---
 include/linux/jbd2.h | 4 ----
 1 file changed, 4 deletions(-)

Comments

Jan Kara Jan. 13, 2022, 11:27 a.m. UTC | #1

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 <[e-mail]>

This patch looks good. Feel free to add:

Reviewed-by: Jan Kara <[e-mail]>

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
>

Ritesh Harjani Jan. 13, 2022, 12:38 p.m. UTC | #2

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 <[e-mail]>
>
> This patch looks good. Feel free to add:
>
> Reviewed-by: Jan Kara <[e-mail]>
>
> 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 <[e-mail]>
> SUSE Labs, CR

Ritesh Harjani Jan. 17, 2022, 12:55 p.m. UTC | #3

On 22/01/13 06:08PM, Ritesh Harjani wrote:
> 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 <[e-mail]>
> >
> > This patch looks good. Feel free to add:
> >
> > Reviewed-by: Jan Kara <[e-mail]>
> >
> > Actually looking at it, t_handle_lock seems to be very much unused. I agree

Thanks Jan for your help in this.
I have dropped this patch from v2 in order to discuss few more things and I felt
killing t_handle_lock completely can be sent in a seperate patch series.


>
> 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.

I looked into jbd2_journal_extend and yes, we don't need t_handle_lock
for updating transaction->t_outstanding_credits, since it already happens with
atomic API calls.

Now I do see we update handle->h_**_credits in that function.
But I think this is per process (based on task_struct, current->journal_info)
and doesn't need a lock protection right?


> >
> > 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);

I think there might be a simpler and more straight forward way for updating
t_max_wait.

I did look into the t_max_wait logic and where all we are updating it.

t_max_wait is the max wait time in starting (&attaching) a _new_ running
transaction by a handle. Is this understaning correct?
From code I don't see t_max_wait getting updated for the time taken in order
to start the handle by a existing running transaction.

Here is how -
update_t_max_wait() will only update t_max_wait if the
transaction->t_start is after ts
(ts is nothing but when start_this_handle() was called).

1. This means that for transaction->t_start to be greater than ts, it has to be
   the new transaction that gets started right (in start_this_handle() func)?

2. Second place where transaction->t_start is updated is just after the start of
   commit phase 7. But this only means that this transaction has become the
   commit transaction. That means someone has to alloc a new running transaction
   which again is case-1.

Now I think this spinlock was added since multiple processes can start a handle
in parallel and attach a running transaction.

Also this was then moved within CONFIG_JBD2_DEBUG since to avoid spinlock
contention on a SMP system in starting multiple handles by different processes.

Now looking at all of above, I think we can move update_t_max_wait()
inside jbd2_get_transaction() in start_this_handle(). Because that is where
a new transaction will be started and transaction->t_start will be greater then
ts. This also is protected within j_state_lock write_lock, so we don't need
spinlock.

This would also mean that we can move t_max_wait outside of CONFIG_JBD2_DEBUG
and jbd2_journal_enable_debug.

Jan, could you confirm if above understaning is correct and shall I go ahead
with above changes?

-ritesh

> >
> > 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 <[e-mail]>
> > SUSE Labs, CR

Jan Kara Jan. 17, 2022, 2:38 p.m. UTC | #4

On Mon 17-01-22 18:25:27, Ritesh Harjani wrote:
> On 22/01/13 06:08PM, Ritesh Harjani wrote:
> > 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 <[e-mail]>
> > >
> > > This patch looks good. Feel free to add:
> > >
> > > Reviewed-by: Jan Kara <[e-mail]>
> > >
> > > Actually looking at it, t_handle_lock seems to be very much unused. I agree
> 
> Thanks Jan for your help in this.
> I have dropped this patch from v2 in order to discuss few more things and I felt
> killing t_handle_lock completely can be sent in a seperate patch series.

Yes, probably a good choice.

> > 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.
> 
> I looked into jbd2_journal_extend and yes, we don't need t_handle_lock
> for updating transaction->t_outstanding_credits, since it already happens with
> atomic API calls.
> 
> Now I do see we update handle->h_**_credits in that function.
> But I think this is per process (based on task_struct, current->journal_info)
> and doesn't need a lock protection right?

Yes, handle is per process so no lock is needed there.

> > > 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);
> 
> I think there might be a simpler and more straight forward way for updating
> t_max_wait.
> 
> I did look into the t_max_wait logic and where all we are updating it.
> 
> t_max_wait is the max wait time in starting (&attaching) a _new_ running
> transaction by a handle. Is this understaning correct?

Correct. It is the maximum time we had to wait for a new transaction to be
created.

> From code I don't see t_max_wait getting updated for the time taken in order
> to start the handle by a existing running transaction.
> 
> Here is how -
> update_t_max_wait() will only update t_max_wait if the
> transaction->t_start is after ts
> (ts is nothing but when start_this_handle() was called).
> 
> 1. This means that for transaction->t_start to be greater than ts, it has to be
>    the new transaction that gets started right (in start_this_handle() func)?
>
> 2. Second place where transaction->t_start is updated is just after the start of
>    commit phase 7. But this only means that this transaction has become the
>    commit transaction. That means someone has to alloc a new running transaction
>    which again is case-1.
> 
> Now I think this spinlock was added since multiple processes can start a handle
> in parallel and attach a running transaction.
> 
> Also this was then moved within CONFIG_JBD2_DEBUG since to avoid spinlock
> contention on a SMP system in starting multiple handles by different processes.
> 
> Now looking at all of above, I think we can move update_t_max_wait()
> inside jbd2_get_transaction() in start_this_handle(). Because that is where
> a new transaction will be started and transaction->t_start will be greater then
> ts. This also is protected within j_state_lock write_lock, so we don't need
> spinlock.

All above is correct upto this point. The catch is there can be (and often
are) more processes in start_this_handle() waiting in
wait_transaction_switching() and then racing to create the new transaction.
The process calling jbd2_get_transaction() is not necessarily the one which
entered start_this_handle() first and thus t_max_wait would not be really
the maximum time someone had to wait.

								Honza

Patch

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);
 }
 
 /*