Patchwork [2/2] jbd2: remove all t_handle_lock statements

login
register
mail settings
Submitter Toshiyuki Okajima
Date Dec. 22, 2011, noon
Message ID <20111222210007.ca8fb54f.toshi.okajima@jp.fujitsu.com>
Download mbox | patch
Permalink /patch/132833/
State New
Headers show

Comments

Toshiyuki Okajima - Dec. 22, 2011, noon
Remove all t_handle_lock statements because they are not necessary anymore.

Because there is read_lock(&journal->j_state_lock) or 
write_lock(&journal->j_state_lock) on all the forward codes beyond the place 
which needs a spin_lock(&transaction->t_handle_lock).

Signed-off-by: Toshiyuki Okajima <toshi.okajima@jp.fujitsu.com>
Cc: 
---
 fs/jbd2/commit.c      |    4 ----
 fs/jbd2/transaction.c |   18 +++---------------
 include/linux/jbd2.h  |    8 --------
 3 files changed, 3 insertions(+), 27 deletions(-)
Yongqiang Yang - Dec. 22, 2011, 2:07 p.m.
On Thu, Dec 22, 2011 at 8:00 PM, Toshiyuki Okajima
<toshi.okajima@jp.fujitsu.com> wrote:
> Remove all t_handle_lock statements because they are not necessary anymore.
>
> Because there is read_lock(&journal->j_state_lock) or
> write_lock(&journal->j_state_lock) on all the forward codes beyond the place
> which needs a spin_lock(&transaction->t_handle_lock).
>
> Signed-off-by: Toshiyuki Okajima <toshi.okajima@jp.fujitsu.com>
> Cc:
> ---
>  fs/jbd2/commit.c      |    4 ----
>  fs/jbd2/transaction.c |   18 +++---------------
>  include/linux/jbd2.h  |    8 --------
>  3 files changed, 3 insertions(+), 27 deletions(-)
>
> diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c
> index 68d704d..1030d47 100644
> --- a/fs/jbd2/commit.c
> +++ b/fs/jbd2/commit.c
> @@ -364,22 +364,18 @@ void jbd2_journal_commit_transaction(journal_t *journal)
>        stats.run.rs_running = jbd2_time_diff(commit_transaction->t_start,
>                                              stats.run.rs_locked);
>
> -       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);
>
>        J_ASSERT (atomic_read(&commit_transaction->t_outstanding_credits) <=
>                        journal->j_max_transaction_buffers);
> diff --git a/fs/jbd2/transaction.c b/fs/jbd2/transaction.c
> index 76f2eca..7f84e3f 100644
> --- a/fs/jbd2/transaction.c
> +++ b/fs/jbd2/transaction.c
> @@ -56,7 +56,6 @@ jbd2_get_transaction(journal_t *journal, transaction_t *transaction)
>        transaction->t_start_time = ktime_get();
>        transaction->t_tid = journal->j_transaction_sequence++;
>        transaction->t_expires = jiffies + journal->j_commit_interval;
> -       spin_lock_init(&transaction->t_handle_lock);
>        atomic_set(&transaction->t_updates, 0);
>        atomic_set(&transaction->t_outstanding_credits, 0);
>        atomic_set(&transaction->t_handle_count, 0);
> @@ -100,10 +99,8 @@ static inline void update_t_max_wait(transaction_t *transaction,
>        if (jbd2_journal_enable_debug &&
>            time_after(transaction->t_start, ts)) {
>                ts = jbd2_time_diff(ts, transaction->t_start);
> -               spin_lock(&transaction->t_handle_lock);
>                if (ts > transaction->t_max_wait)
>                        transaction->t_max_wait = ts;
> -               spin_unlock(&transaction->t_handle_lock);
Hi Toshiyuki,

     I think you removed too much.  t_handle_lock is needed here to
protect t_max_wait.   I meant just removing t_handle_lock from where I
commented in your patch post out last time.

Yongqiang.
>        }
>  #endif
>  }
> @@ -401,19 +398,18 @@ int jbd2_journal_extend(handle_t *handle, int nblocks)
>                goto error_out;
>        }
>
> -       spin_lock(&transaction->t_handle_lock);
>        wanted = atomic_read(&transaction->t_outstanding_credits) + nblocks;
>
>        if (wanted > journal->j_max_transaction_buffers) {
>                jbd_debug(3, "denied handle %p %d blocks: "
>                          "transaction too large\n", handle, nblocks);
> -               goto unlock;
> +               goto error_out;
>        }
>
>        if (wanted > __jbd2_log_space_left(journal)) {
>                jbd_debug(3, "denied handle %p %d blocks: "
>                          "insufficient log space\n", handle, nblocks);
> -               goto unlock;
> +               goto error_out;
>        }
>
>        handle->h_buffer_credits += nblocks;
> @@ -421,8 +417,6 @@ int jbd2_journal_extend(handle_t *handle, int nblocks)
>        result = 0;
>
>        jbd_debug(3, "extended handle %p by %d\n", handle, nblocks);
> -unlock:
> -       spin_unlock(&transaction->t_handle_lock);
>  error_out:
>        read_unlock(&journal->j_state_lock);
>  out:
> @@ -464,12 +458,10 @@ int jbd2__journal_restart(handle_t *handle, int nblocks, gfp_t gfp_mask)
>        J_ASSERT(journal_current_handle() == handle);
>
>        read_lock(&journal->j_state_lock);
> -       spin_lock(&transaction->t_handle_lock);
>        atomic_sub(handle->h_buffer_credits,
>                   &transaction->t_outstanding_credits);
>        if (atomic_dec_and_test(&transaction->t_updates))
>                wake_up(&journal->j_wait_updates);
> -       spin_unlock(&transaction->t_handle_lock);
>
>        jbd_debug(2, "restarting handle %p\n", handle);
>        tid = transaction->t_tid;
> @@ -516,14 +508,10 @@ void jbd2_journal_lock_updates(journal_t *journal)
>                if (!transaction)
>                        break;
>
> -               spin_lock(&transaction->t_handle_lock);
> -               if (!atomic_read(&transaction->t_updates)) {
> -                       spin_unlock(&transaction->t_handle_lock);
> +               if (!atomic_read(&transaction->t_updates))
>                        break;
> -               }
>                prepare_to_wait(&journal->j_wait_updates, &wait,
>                                TASK_UNINTERRUPTIBLE);
> -               spin_unlock(&transaction->t_handle_lock);
>                write_unlock(&journal->j_state_lock);
>                schedule();
>                finish_wait(&journal->j_wait_updates, &wait);
> diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
> index 2092ea2..55f7a8c 100644
> --- a/include/linux/jbd2.h
> +++ b/include/linux/jbd2.h
> @@ -440,9 +440,6 @@ struct transaction_chp_stats_s {
>  *    ->j_list_lock
>  *
>  *    j_state_lock
> - *    ->t_handle_lock
> - *
> - *    j_state_lock
>  *    ->j_list_lock                    (journal_unmap_buffer)
>  *
>  */
> @@ -538,11 +535,6 @@ struct transaction_s
>        struct list_head        t_inode_list;
>
>        /*
> -        * Protects info related to handles
> -        */
> -       spinlock_t              t_handle_lock;
> -
> -       /*
>         * Longest time some handle had to wait for running transaction
>         */
>        unsigned long           t_max_wait;
> --
> 1.5.5.6
Toshiyuki Okajima - Dec. 26, 2011, 12:20 a.m.
Hi Yongqiang,

(2011/12/22 23:07), Yongqiang Yang wrote:
> On Thu, Dec 22, 2011 at 8:00 PM, Toshiyuki Okajima
> <toshi.okajima@jp.fujitsu.com> wrote:
>> Remove all t_handle_lock statements because they are not necessary anymore.
>>
>> Because there is read_lock(&journal->j_state_lock) or
>> write_lock(&journal->j_state_lock) on all the forward codes beyond the place
>> which needs a spin_lock(&transaction->t_handle_lock).
>>
>> Signed-off-by: Toshiyuki Okajima <toshi.okajima@jp.fujitsu.com>
>> Cc:
>> ---
>>  fs/jbd2/commit.c      |    4 ----
>>  fs/jbd2/transaction.c |   18 +++---------------
>>  include/linux/jbd2.h  |    8 --------
>>  3 files changed, 3 insertions(+), 27 deletions(-)
>>
>> diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c
>> index 68d704d..1030d47 100644
>> --- a/fs/jbd2/commit.c
>> +++ b/fs/jbd2/commit.c
>> @@ -364,22 +364,18 @@ void jbd2_journal_commit_transaction(journal_t *journal)
>>        stats.run.rs_running = jbd2_time_diff(commit_transaction->t_start,
>>                                              stats.run.rs_locked);
>>
>> -       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);
>>
>>        J_ASSERT (atomic_read(&commit_transaction->t_outstanding_credits) <=
>>                        journal->j_max_transaction_buffers);
>> diff --git a/fs/jbd2/transaction.c b/fs/jbd2/transaction.c
>> index 76f2eca..7f84e3f 100644
>> --- a/fs/jbd2/transaction.c
>> +++ b/fs/jbd2/transaction.c
>> @@ -56,7 +56,6 @@ jbd2_get_transaction(journal_t *journal, transaction_t *transaction)
>>        transaction->t_start_time = ktime_get();
>>        transaction->t_tid = journal->j_transaction_sequence++;
>>        transaction->t_expires = jiffies + journal->j_commit_interval;
>> -       spin_lock_init(&transaction->t_handle_lock);
>>        atomic_set(&transaction->t_updates, 0);
>>        atomic_set(&transaction->t_outstanding_credits, 0);
>>        atomic_set(&transaction->t_handle_count, 0);

>> @@ -100,10 +99,8 @@ static inline void update_t_max_wait(transaction_t *transaction,
>>        if (jbd2_journal_enable_debug &&
>>            time_after(transaction->t_start, ts)) {
>>                ts = jbd2_time_diff(ts, transaction->t_start);
>> -               spin_lock(&transaction->t_handle_lock);
>>                if (ts > transaction->t_max_wait)
>>                        transaction->t_max_wait = ts;
>> -               spin_unlock(&transaction->t_handle_lock);
> Hi Toshiyuki,
> 
>      I think you removed too much.  t_handle_lock is needed here to
> protect t_max_wait.   I meant just removing t_handle_lock from where I
> commented in your patch post out last time.
Thanks for your comment. OK, I understand.

This function tries to protect itself by 
 read_lock(&journal->j_state_lock), but by only 
read_lock(&journal->j_state_lock) cannot protect this critical code 
section.

  96 static inline void update_t_max_wait(transaction_t *transaction,
  97                                      unsigned long ts)
  98 {
  99 #ifdef CONFIG_JBD2_DEBUG
 100         if (jbd2_journal_enable_debug &&
 101             time_after(transaction->t_start, ts)) {
 102                 ts = jbd2_time_diff(ts, transaction->t_start);
 103                 spin_lock(&transaction->t_handle_lock);
 104                 if (ts > transaction->t_max_wait)
 105                         transaction->t_max_wait = ts;
 106                 spin_unlock(&transaction->t_handle_lock);
 107         }
 108 #endif
 109 }

I will delete the part of my patch for update_t_max_wait().

Due to the same reason, 
 - jbd2_journal_extend
 - jbd2_journal_restart
cannot be protected by my patch. So, I will delete these parts, too.

Best Regards,
Toshiyuki Okajima

> 
> Yongqiang.
>>        }
>>  #endif
>>  }
>> @@ -401,19 +398,18 @@ int jbd2_journal_extend(handle_t *handle, int nblocks)
>>                goto error_out;
>>        }
>>
>> -       spin_lock(&transaction->t_handle_lock);
>>        wanted = atomic_read(&transaction->t_outstanding_credits) + nblocks;
>>
>>        if (wanted > journal->j_max_transaction_buffers) {
>>                jbd_debug(3, "denied handle %p %d blocks: "
>>                          "transaction too large\n", handle, nblocks);
>> -               goto unlock;
>> +               goto error_out;
>>        }
>>
>>        if (wanted > __jbd2_log_space_left(journal)) {
>>                jbd_debug(3, "denied handle %p %d blocks: "
>>                          "insufficient log space\n", handle, nblocks);
>> -               goto unlock;
>> +               goto error_out;
>>        }
>>
>>        handle->h_buffer_credits += nblocks;
>> @@ -421,8 +417,6 @@ int jbd2_journal_extend(handle_t *handle, int nblocks)
>>        result = 0;
>>
>>        jbd_debug(3, "extended handle %p by %d\n", handle, nblocks);
>> -unlock:
>> -       spin_unlock(&transaction->t_handle_lock);
>>  error_out:
>>        read_unlock(&journal->j_state_lock);
>>  out:
>> @@ -464,12 +458,10 @@ int jbd2__journal_restart(handle_t *handle, int nblocks, gfp_t gfp_mask)
>>        J_ASSERT(journal_current_handle() == handle);
>>
>>        read_lock(&journal->j_state_lock);
>> -       spin_lock(&transaction->t_handle_lock);
>>        atomic_sub(handle->h_buffer_credits,
>>                   &transaction->t_outstanding_credits);
>>        if (atomic_dec_and_test(&transaction->t_updates))
>>                wake_up(&journal->j_wait_updates);
>> -       spin_unlock(&transaction->t_handle_lock);
>>
>>        jbd_debug(2, "restarting handle %p\n", handle);
>>        tid = transaction->t_tid;
>> @@ -516,14 +508,10 @@ void jbd2_journal_lock_updates(journal_t *journal)
>>                if (!transaction)
>>                        break;
>>
>> -               spin_lock(&transaction->t_handle_lock);
>> -               if (!atomic_read(&transaction->t_updates)) {
>> -                       spin_unlock(&transaction->t_handle_lock);
>> +               if (!atomic_read(&transaction->t_updates))
>>                        break;
>> -               }
>>                prepare_to_wait(&journal->j_wait_updates, &wait,
>>                                TASK_UNINTERRUPTIBLE);
>> -               spin_unlock(&transaction->t_handle_lock);
>>                write_unlock(&journal->j_state_lock);
>>                schedule();
>>                finish_wait(&journal->j_wait_updates, &wait);
>> diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
>> index 2092ea2..55f7a8c 100644
>> --- a/include/linux/jbd2.h
>> +++ b/include/linux/jbd2.h
>> @@ -440,9 +440,6 @@ struct transaction_chp_stats_s {
>>  *    ->j_list_lock
>>  *
>>  *    j_state_lock
>> - *    ->t_handle_lock
>> - *
>> - *    j_state_lock
>>  *    ->j_list_lock                    (journal_unmap_buffer)
>>  *
>>  */
>> @@ -538,11 +535,6 @@ struct transaction_s
>>        struct list_head        t_inode_list;
>>
>>        /*
>> -        * Protects info related to handles
>> -        */
>> -       spinlock_t              t_handle_lock;
>> -
>> -       /*
>>         * Longest time some handle had to wait for running transaction
>>         */
>>        unsigned long           t_max_wait;
>> --
>> 1.5.5.6
> 
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c
index 68d704d..1030d47 100644
--- a/fs/jbd2/commit.c
+++ b/fs/jbd2/commit.c
@@ -364,22 +364,18 @@  void jbd2_journal_commit_transaction(journal_t *journal)
 	stats.run.rs_running = jbd2_time_diff(commit_transaction->t_start,
 					      stats.run.rs_locked);
 
-	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);
 
 	J_ASSERT (atomic_read(&commit_transaction->t_outstanding_credits) <=
 			journal->j_max_transaction_buffers);
diff --git a/fs/jbd2/transaction.c b/fs/jbd2/transaction.c
index 76f2eca..7f84e3f 100644
--- a/fs/jbd2/transaction.c
+++ b/fs/jbd2/transaction.c
@@ -56,7 +56,6 @@  jbd2_get_transaction(journal_t *journal, transaction_t *transaction)
 	transaction->t_start_time = ktime_get();
 	transaction->t_tid = journal->j_transaction_sequence++;
 	transaction->t_expires = jiffies + journal->j_commit_interval;
-	spin_lock_init(&transaction->t_handle_lock);
 	atomic_set(&transaction->t_updates, 0);
 	atomic_set(&transaction->t_outstanding_credits, 0);
 	atomic_set(&transaction->t_handle_count, 0);
@@ -100,10 +99,8 @@  static inline void update_t_max_wait(transaction_t *transaction,
 	if (jbd2_journal_enable_debug &&
 	    time_after(transaction->t_start, ts)) {
 		ts = jbd2_time_diff(ts, transaction->t_start);
-		spin_lock(&transaction->t_handle_lock);
 		if (ts > transaction->t_max_wait)
 			transaction->t_max_wait = ts;
-		spin_unlock(&transaction->t_handle_lock);
 	}
 #endif
 }
@@ -401,19 +398,18 @@  int jbd2_journal_extend(handle_t *handle, int nblocks)
 		goto error_out;
 	}
 
-	spin_lock(&transaction->t_handle_lock);
 	wanted = atomic_read(&transaction->t_outstanding_credits) + nblocks;
 
 	if (wanted > journal->j_max_transaction_buffers) {
 		jbd_debug(3, "denied handle %p %d blocks: "
 			  "transaction too large\n", handle, nblocks);
-		goto unlock;
+		goto error_out;
 	}
 
 	if (wanted > __jbd2_log_space_left(journal)) {
 		jbd_debug(3, "denied handle %p %d blocks: "
 			  "insufficient log space\n", handle, nblocks);
-		goto unlock;
+		goto error_out;
 	}
 
 	handle->h_buffer_credits += nblocks;
@@ -421,8 +417,6 @@  int jbd2_journal_extend(handle_t *handle, int nblocks)
 	result = 0;
 
 	jbd_debug(3, "extended handle %p by %d\n", handle, nblocks);
-unlock:
-	spin_unlock(&transaction->t_handle_lock);
 error_out:
 	read_unlock(&journal->j_state_lock);
 out:
@@ -464,12 +458,10 @@  int jbd2__journal_restart(handle_t *handle, int nblocks, gfp_t gfp_mask)
 	J_ASSERT(journal_current_handle() == handle);
 
 	read_lock(&journal->j_state_lock);
-	spin_lock(&transaction->t_handle_lock);
 	atomic_sub(handle->h_buffer_credits,
 		   &transaction->t_outstanding_credits);
 	if (atomic_dec_and_test(&transaction->t_updates))
 		wake_up(&journal->j_wait_updates);
-	spin_unlock(&transaction->t_handle_lock);
 
 	jbd_debug(2, "restarting handle %p\n", handle);
 	tid = transaction->t_tid;
@@ -516,14 +508,10 @@  void jbd2_journal_lock_updates(journal_t *journal)
 		if (!transaction)
 			break;
 
-		spin_lock(&transaction->t_handle_lock);
-		if (!atomic_read(&transaction->t_updates)) {
-			spin_unlock(&transaction->t_handle_lock);
+		if (!atomic_read(&transaction->t_updates))
 			break;
-		}
 		prepare_to_wait(&journal->j_wait_updates, &wait,
 				TASK_UNINTERRUPTIBLE);
-		spin_unlock(&transaction->t_handle_lock);
 		write_unlock(&journal->j_state_lock);
 		schedule();
 		finish_wait(&journal->j_wait_updates, &wait);
diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
index 2092ea2..55f7a8c 100644
--- a/include/linux/jbd2.h
+++ b/include/linux/jbd2.h
@@ -440,9 +440,6 @@  struct transaction_chp_stats_s {
  *    ->j_list_lock
  *
  *    j_state_lock
- *    ->t_handle_lock
- *
- *    j_state_lock
  *    ->j_list_lock			(journal_unmap_buffer)
  *
  */
@@ -538,11 +535,6 @@  struct transaction_s
 	struct list_head	t_inode_list;
 
 	/*
-	 * Protects info related to handles
-	 */
-	spinlock_t		t_handle_lock;
-
-	/*
 	 * Longest time some handle had to wait for running transaction
 	 */
 	unsigned long		t_max_wait;