| 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
| ||
|
Message-ID: <4EF7BDE2.4000309@jp.fujitsu.com>
Date: Mon, 26 Dec 2011 09:20:50 +0900
From: Toshiyuki Okajima <toshi.okajima@...fujitsu.com>
To: Yongqiang Yang <xiaoqiangnk@...il.com>
CC: toshi.okajima@...fujitsu.com, tytso@....edu,
adilger.kernel@...ger.ca, linux-ext4@...r.kernel.org
Subject: Re: [PATCH 2/2] jbd2: remove all t_handle_lock statements
Hi Yongqiang,
(2011/12/22 23:07), Yongqiang Yang wrote:
> On Thu, Dec 22, 2011 at 8:00 PM, Toshiyuki Okajima
> <toshi.okajima@...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@...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@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists