Josef Bacik
2011-Apr-11 19:49 UTC
[PATCH] Btrfs: avoid taking the trans_mutex in btrfs_end_transaction
I''ve been working on making our O_DIRECT latency not suck and I noticed we were taking the trans_mutex in btrfs_end_transaction. So to do this we convert num_writers and use_count to atomic_t''s and just decrement them in btrfs_end_transaction. I got rid of the put_transaction() in btrfs_end_transaction() since we will never free the transaction from btrfs_end_transaction(). Tested this with xfstests and everything went smoothly. Thanks, Signed-off-by: Josef Bacik <josef@redhat.com> --- fs/btrfs/disk-io.c | 2 +- fs/btrfs/transaction.c | 42 +++++++++++++++++++++--------------------- fs/btrfs/transaction.h | 4 ++-- 3 files changed, 24 insertions(+), 24 deletions(-) diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c index 8f1d44b..68c84c8 100644 --- a/fs/btrfs/disk-io.c +++ b/fs/btrfs/disk-io.c @@ -3057,7 +3057,7 @@ static int btrfs_cleanup_transaction(struct btrfs_root *root) btrfs_destroy_pinned_extent(root, root->fs_info->pinned_extents); - t->use_count = 0; + atomic_set(&t->use_count, 0); list_del_init(&t->list); memset(t, 0, sizeof(*t)); kmem_cache_free(btrfs_transaction_cachep, t); diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c index 4583008..dbacd2c 100644 --- a/fs/btrfs/transaction.c +++ b/fs/btrfs/transaction.c @@ -32,9 +32,8 @@ static noinline void put_transaction(struct btrfs_transaction *transaction) { - WARN_ON(transaction->use_count == 0); - transaction->use_count--; - if (transaction->use_count == 0) { + WARN_ON(atomic_read(&transaction->use_count) == 0); + if (atomic_dec_and_test(&transaction->use_count)) { list_del_init(&transaction->list); memset(transaction, 0, sizeof(*transaction)); kmem_cache_free(btrfs_transaction_cachep, transaction); @@ -60,14 +59,14 @@ static noinline int join_transaction(struct btrfs_root *root) if (!cur_trans) return -ENOMEM; root->fs_info->generation++; - cur_trans->num_writers = 1; + atomic_set(&cur_trans->num_writers, 1); cur_trans->num_joined = 0; cur_trans->transid = root->fs_info->generation; init_waitqueue_head(&cur_trans->writer_wait); init_waitqueue_head(&cur_trans->commit_wait); cur_trans->in_commit = 0; cur_trans->blocked = 0; - cur_trans->use_count = 1; + atomic_set(&cur_trans->use_count, 1); cur_trans->commit_done = 0; cur_trans->start_time = get_seconds(); @@ -88,7 +87,7 @@ static noinline int join_transaction(struct btrfs_root *root) root->fs_info->running_transaction = cur_trans; spin_unlock(&root->fs_info->new_trans_lock); } else { - cur_trans->num_writers++; + atomic_inc(&cur_trans->num_writers); cur_trans->num_joined++; } @@ -145,7 +144,7 @@ static void wait_current_trans(struct btrfs_root *root) cur_trans = root->fs_info->running_transaction; if (cur_trans && cur_trans->blocked) { DEFINE_WAIT(wait); - cur_trans->use_count++; + atomic_inc(&cur_trans->use_count); while (1) { prepare_to_wait(&root->fs_info->transaction_wait, &wait, TASK_UNINTERRUPTIBLE); @@ -205,7 +204,7 @@ again: } cur_trans = root->fs_info->running_transaction; - cur_trans->use_count++; + atomic_inc(&cur_trans->use_count); if (type != TRANS_JOIN_NOLOCK) mutex_unlock(&root->fs_info->trans_mutex); @@ -336,7 +335,7 @@ int btrfs_wait_for_commit(struct btrfs_root *root, u64 transid) goto out_unlock; /* nothing committing|committed */ } - cur_trans->use_count++; + atomic_inc(&cur_trans->use_count); mutex_unlock(&root->fs_info->trans_mutex); wait_for_commit(root, cur_trans); @@ -466,19 +465,20 @@ static int __btrfs_end_transaction(struct btrfs_trans_handle *trans, wake_up_process(info->transaction_kthread); } - if (lock) - mutex_lock(&info->trans_mutex); WARN_ON(cur_trans != info->running_transaction); - WARN_ON(cur_trans->num_writers < 1); - cur_trans->num_writers--; + WARN_ON(atomic_read(&cur_trans->num_writers) < 1); + atomic_dec(&cur_trans->num_writers); smp_mb(); if (waitqueue_active(&cur_trans->writer_wait)) wake_up(&cur_trans->writer_wait); - put_transaction(cur_trans); - if (lock) - mutex_unlock(&info->trans_mutex); + /* + * A trans handle will never actually be putting the last reference of a + * transaction, so just dec the use count to avoid taking the trans + * mutex. + */ + atomic_dec(&cur_trans->use_count); if (current->journal_info == trans) current->journal_info = NULL; memset(trans, 0, sizeof(*trans)); @@ -1187,7 +1187,7 @@ int btrfs_commit_transaction_async(struct btrfs_trans_handle *trans, /* take transaction reference */ mutex_lock(&root->fs_info->trans_mutex); cur_trans = trans->transaction; - cur_trans->use_count++; + atomic_inc(&cur_trans->use_count); mutex_unlock(&root->fs_info->trans_mutex); btrfs_end_transaction(trans, root); @@ -1246,7 +1246,7 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans, mutex_lock(&root->fs_info->trans_mutex); if (cur_trans->in_commit) { - cur_trans->use_count++; + atomic_inc(&cur_trans->use_count); mutex_unlock(&root->fs_info->trans_mutex); btrfs_end_transaction(trans, root); @@ -1268,7 +1268,7 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans, prev_trans = list_entry(cur_trans->list.prev, struct btrfs_transaction, list); if (!prev_trans->commit_done) { - prev_trans->use_count++; + atomic_inc(&prev_trans->use_count); mutex_unlock(&root->fs_info->trans_mutex); wait_for_commit(root, prev_trans); @@ -1309,14 +1309,14 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans, TASK_UNINTERRUPTIBLE); smp_mb(); - if (cur_trans->num_writers > 1) + if (atomic_read(&cur_trans->num_writers) > 1) schedule_timeout(MAX_SCHEDULE_TIMEOUT); else if (should_grow) schedule_timeout(1); mutex_lock(&root->fs_info->trans_mutex); finish_wait(&cur_trans->writer_wait, &wait); - } while (cur_trans->num_writers > 1 || + } while (atomic_read(&cur_trans->num_writers) > 1 || (should_grow && cur_trans->num_joined != joined)); ret = create_pending_snapshots(trans, root->fs_info); diff --git a/fs/btrfs/transaction.h b/fs/btrfs/transaction.h index 229a594..e441acc 100644 --- a/fs/btrfs/transaction.h +++ b/fs/btrfs/transaction.h @@ -27,11 +27,11 @@ struct btrfs_transaction { * total writers in this transaction, it must be zero before the * transaction can end */ - unsigned long num_writers; + atomic_t num_writers; unsigned long num_joined; int in_commit; - int use_count; + atomic_t use_count; int commit_done; int blocked; struct list_head list; -- 1.7.2.3 -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Chris Mason
2011-Apr-11 20:41 UTC
Re: [PATCH] Btrfs: avoid taking the trans_mutex in btrfs_end_transaction
Excerpts from Josef Bacik''s message of 2011-04-11 15:49:17 -0400:> I''ve been working on making our O_DIRECT latency not suck and I noticed we were > taking the trans_mutex in btrfs_end_transaction. So to do this we convert > num_writers and use_count to atomic_t''s and just decrement them in > btrfs_end_transaction. I got rid of the put_transaction() in > btrfs_end_transaction() since we will never free the transaction from > btrfs_end_transaction(). Tested this with xfstests and everything went > smoothly. Thanks,Hmmm, this is smart, there''s no reason to take the lock here. But there''s one little problem:> @@ -466,19 +465,20 @@ static int __btrfs_end_transaction(struct btrfs_trans_handle *trans, > wake_up_process(info->transaction_kthread); > } > > - if (lock) > - mutex_lock(&info->trans_mutex); > WARN_ON(cur_trans != info->running_transaction); > - WARN_ON(cur_trans->num_writers < 1); > - cur_trans->num_writers--; > + WARN_ON(atomic_read(&cur_trans->num_writers) < 1); > + atomic_dec(&cur_trans->num_writers);We''ve just decremented num_writers, which could have been the only thing preventing a commit from finishing. The entire commit could finish at any time after this line.> > smp_mb(); > if (waitqueue_active(&cur_trans->writer_wait)) > wake_up(&cur_trans->writer_wait); > - put_transaction(cur_trans); > - if (lock) > - mutex_unlock(&info->trans_mutex); > > + /* > + * A trans handle will never actually be putting the last reference of a > + * transaction, so just dec the use count to avoid taking the trans > + * mutex. > + */ > + atomic_dec(&cur_trans->use_count);Which could make this the last and final reference on cur_trans->use_count. There''s no reason we need the lock in put_transaction, other than manipulating the list of running transactions. But I don''t see any reason we can''t delete ourselves from the list when the commit is done, which would let you keep a lock less put_transaction. -chris -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html