diff mbox series

[02/25] btrfs: rework async transaction committing

Message ID 5a181fb41c864ca518a35defc2547abef30afb18.1636144971.git.josef@toxicpanda.com (mailing list archive)
State New, archived
Headers show
Series btrfs: extent tree v2 prep work for global roots | expand

Commit Message

Josef Bacik Nov. 5, 2021, 8:45 p.m. UTC
Currently we do this awful thing where we get another ref on a trans
handle, async off that handle and commit the transaction from that work.
Because we do this we have to mess with current->journal_info and the
freeze counting stuff.

We already have an async thing to kick for the transaction commit, the
transaction kthread.  Replace this work struct with a flag on the
fs_info to tell the kthread to go ahead and commit even if it's before
our timeout.  Then we can drastically simplify the async transaction
commit path.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/ctree.h       |  3 ++
 fs/btrfs/disk-io.c     |  3 +-
 fs/btrfs/ioctl.c       |  7 +----
 fs/btrfs/transaction.c | 64 ++++++++----------------------------------
 fs/btrfs/transaction.h |  2 +-
 5 files changed, 18 insertions(+), 61 deletions(-)

Comments

David Sterba Nov. 18, 2021, 12:12 p.m. UTC | #1
On Fri, Nov 05, 2021 at 04:45:28PM -0400, Josef Bacik wrote:
> Currently we do this awful thing where we get another ref on a trans
> handle, async off that handle and commit the transaction from that work.
> Because we do this we have to mess with current->journal_info and the
> freeze counting stuff.
> 
> We already have an async thing to kick for the transaction commit, the
> transaction kthread.  Replace this work struct with a flag on the
> fs_info to tell the kthread to go ahead and commit even if it's before
> our timeout.  Then we can drastically simplify the async transaction
> commit path.

I wanted to get rid of the async commit completely and reusing the
pending operations that's basically the same what you add as the new fs
bit.

https://lore.kernel.org/linux-btrfs/808c557f1ae3034fdfa2d2361e864aac90ba5a94.1622733245.git.dsterba@suse.com/

There's also a followup removing transaction_blocked_wait, I got some
hangs in btrfs/011. I haven't tested 3/4 separately so I don't know the
exact cause.

> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -595,6 +595,9 @@ enum {
>  	/* Indicate whether there are any tree modification log users */
>  	BTRFS_FS_TREE_MOD_LOG_USERS,
>  
> +	/* Indicate that we want the transaction kthread to commit right now. */
> +	BTRFS_FS_COMMIT_TRANS,

This is duplicating functionality behind BTRFS_PENDING_COMMIT, otherwise
simplifying the async commit is good of course.
David Sterba Nov. 23, 2021, 5:19 p.m. UTC | #2
On Thu, Nov 18, 2021 at 01:12:03PM +0100, David Sterba wrote:
> On Fri, Nov 05, 2021 at 04:45:28PM -0400, Josef Bacik wrote:
> > Currently we do this awful thing where we get another ref on a trans
> > handle, async off that handle and commit the transaction from that work.
> > Because we do this we have to mess with current->journal_info and the
> > freeze counting stuff.
> > 
> > We already have an async thing to kick for the transaction commit, the
> > transaction kthread.  Replace this work struct with a flag on the
> > fs_info to tell the kthread to go ahead and commit even if it's before
> > our timeout.  Then we can drastically simplify the async transaction
> > commit path.
> 
> I wanted to get rid of the async commit completely and reusing the
> pending operations that's basically the same what you add as the new fs
> bit.
> 
> https://lore.kernel.org/linux-btrfs/808c557f1ae3034fdfa2d2361e864aac90ba5a94.1622733245.git.dsterba@suse.com/
> 
> There's also a followup removing transaction_blocked_wait, I got some
> hangs in btrfs/011. I haven't tested 3/4 separately so I don't know the
> exact cause.
> 
> > --- a/fs/btrfs/ctree.h
> > +++ b/fs/btrfs/ctree.h
> > @@ -595,6 +595,9 @@ enum {
> >  	/* Indicate whether there are any tree modification log users */
> >  	BTRFS_FS_TREE_MOD_LOG_USERS,
> >  
> > +	/* Indicate that we want the transaction kthread to commit right now. */
> > +	BTRFS_FS_COMMIT_TRANS,
> 
> This is duplicating functionality behind BTRFS_PENDING_COMMIT, otherwise
> simplifying the async commit is good of course.

I'll add a note about that and leave it as a future cleanup so the
patchset can get merged.
diff mbox series

Patch

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index ce48ff69c77f..fb49162311b9 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -595,6 +595,9 @@  enum {
 	/* Indicate whether there are any tree modification log users */
 	BTRFS_FS_TREE_MOD_LOG_USERS,
 
+	/* Indicate that we want the transaction kthread to commit right now. */
+	BTRFS_FS_COMMIT_TRANS,
+
 #if BITS_PER_LONG == 32
 	/* Indicate if we have error/warn message printed on 32bit systems */
 	BTRFS_FS_32BIT_ERROR,
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 8d36950800f4..4c4357724ed0 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -1926,7 +1926,8 @@  static int transaction_kthread(void *arg)
 		}
 
 		delta = ktime_get_seconds() - cur->start_time;
-		if (cur->state < TRANS_STATE_COMMIT_START &&
+		if (!test_and_clear_bit(BTRFS_FS_COMMIT_TRANS, &fs_info->flags) &&
+		    cur->state < TRANS_STATE_COMMIT_START &&
 		    delta < fs_info->commit_interval) {
 			spin_unlock(&fs_info->trans_lock);
 			delay -= msecs_to_jiffies((delta - 1) * 1000);
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index c474f7e24163..44372f1e61ac 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -3622,7 +3622,6 @@  static noinline long btrfs_ioctl_start_sync(struct btrfs_root *root,
 {
 	struct btrfs_trans_handle *trans;
 	u64 transid;
-	int ret;
 
 	trans = btrfs_attach_transaction_barrier(root);
 	if (IS_ERR(trans)) {
@@ -3634,11 +3633,7 @@  static noinline long btrfs_ioctl_start_sync(struct btrfs_root *root,
 		goto out;
 	}
 	transid = trans->transid;
-	ret = btrfs_commit_transaction_async(trans);
-	if (ret) {
-		btrfs_end_transaction(trans);
-		return ret;
-	}
+	btrfs_commit_transaction_async(trans);
 out:
 	if (argp)
 		if (copy_to_user(argp, &transid, sizeof(transid)))
diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index b958e0fdfe10..befe04019271 100644
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -1861,50 +1861,14 @@  int btrfs_transaction_blocked(struct btrfs_fs_info *info)
 	return ret;
 }
 
-/*
- * commit transactions asynchronously. once btrfs_commit_transaction_async
- * returns, any subsequent transaction will not be allowed to join.
- */
-struct btrfs_async_commit {
-	struct btrfs_trans_handle *newtrans;
-	struct work_struct work;
-};
-
-static void do_async_commit(struct work_struct *work)
-{
-	struct btrfs_async_commit *ac =
-		container_of(work, struct btrfs_async_commit, work);
-
-	/*
-	 * We've got freeze protection passed with the transaction.
-	 * Tell lockdep about it.
-	 */
-	if (ac->newtrans->type & __TRANS_FREEZABLE)
-		__sb_writers_acquired(ac->newtrans->fs_info->sb, SB_FREEZE_FS);
-
-	current->journal_info = ac->newtrans;
-
-	btrfs_commit_transaction(ac->newtrans);
-	kfree(ac);
-}
-
-int btrfs_commit_transaction_async(struct btrfs_trans_handle *trans)
+void btrfs_commit_transaction_async(struct btrfs_trans_handle *trans)
 {
 	struct btrfs_fs_info *fs_info = trans->fs_info;
-	struct btrfs_async_commit *ac;
 	struct btrfs_transaction *cur_trans;
 
-	ac = kmalloc(sizeof(*ac), GFP_NOFS);
-	if (!ac)
-		return -ENOMEM;
-
-	INIT_WORK(&ac->work, do_async_commit);
-	ac->newtrans = btrfs_join_transaction(trans->root);
-	if (IS_ERR(ac->newtrans)) {
-		int err = PTR_ERR(ac->newtrans);
-		kfree(ac);
-		return err;
-	}
+	/* Kick the transaction kthread. */
+	set_bit(BTRFS_FS_COMMIT_TRANS, &fs_info->flags);
+	wake_up_process(fs_info->transaction_kthread);
 
 	/* take transaction reference */
 	cur_trans = trans->transaction;
@@ -1912,14 +1876,6 @@  int btrfs_commit_transaction_async(struct btrfs_trans_handle *trans)
 
 	btrfs_end_transaction(trans);
 
-	/*
-	 * Tell lockdep we've released the freeze rwsem, since the
-	 * async commit thread will be the one to unlock it.
-	 */
-	if (ac->newtrans->type & __TRANS_FREEZABLE)
-		__sb_writers_release(fs_info->sb, SB_FREEZE_FS);
-
-	schedule_work(&ac->work);
 	/*
 	 * Wait for the current transaction commit to start and block
 	 * subsequent transaction joins
@@ -1927,14 +1883,9 @@  int btrfs_commit_transaction_async(struct btrfs_trans_handle *trans)
 	wait_event(fs_info->transaction_blocked_wait,
 		   cur_trans->state >= TRANS_STATE_COMMIT_START ||
 		   TRANS_ABORTED(cur_trans));
-	if (current->journal_info == trans)
-		current->journal_info = NULL;
-
 	btrfs_put_transaction(cur_trans);
-	return 0;
 }
 
-
 static void cleanup_transaction(struct btrfs_trans_handle *trans, int err)
 {
 	struct btrfs_fs_info *fs_info = trans->fs_info;
@@ -2200,6 +2151,13 @@  int btrfs_commit_transaction(struct btrfs_trans_handle *trans)
 	wait_event(cur_trans->writer_wait,
 		   atomic_read(&cur_trans->num_writers) == 1);
 
+	/*
+	 * We've started the commit, clear the flag in case we were triggered to
+	 * do an async commit but somebody else started before the transaction
+	 * kthread could do the work.
+	 */
+	clear_bit(BTRFS_FS_COMMIT_TRANS, &fs_info->flags);
+
 	if (TRANS_ABORTED(cur_trans)) {
 		ret = cur_trans->aborted;
 		goto scrub_continue;
diff --git a/fs/btrfs/transaction.h b/fs/btrfs/transaction.h
index ba45065f9451..e4b9b251a29e 100644
--- a/fs/btrfs/transaction.h
+++ b/fs/btrfs/transaction.h
@@ -217,7 +217,7 @@  void btrfs_add_dead_root(struct btrfs_root *root);
 int btrfs_defrag_root(struct btrfs_root *root);
 int btrfs_clean_one_deleted_snapshot(struct btrfs_root *root);
 int btrfs_commit_transaction(struct btrfs_trans_handle *trans);
-int btrfs_commit_transaction_async(struct btrfs_trans_handle *trans);
+void btrfs_commit_transaction_async(struct btrfs_trans_handle *trans);
 int btrfs_end_transaction_throttle(struct btrfs_trans_handle *trans);
 bool btrfs_should_end_transaction(struct btrfs_trans_handle *trans);
 void btrfs_throttle(struct btrfs_fs_info *fs_info);