Btrfs: fix missing flush when committing a transaction

Quoted 16 lines · Liu Bo

Quoted 14 lines · Miao Xie

Consider the following case:
Task1 Task2
start_transaction
commit_transaction
check pending snapshots list and the list is empty.
add pending snapshot into list
skip the delalloc flush
end_transaction
...

And then the problem that the snapshot is different with the source subvolume happen.

Quoted 3 lines

This is weird, create_snapshot() will first add pending snapshot into list and then commit the transaction itself, regardless of if the snapshot is different with others or not.

But the transaction may be committed by the other task, and the snapshot creation task just wait until it ends.

Quoted 2 lines

How do you find this?

Just by review the code. I think it can be triggered

Thanks
Miao

Quoted 132 lines

thanks,
liubo

Quoted 127 lines

This patch fixes the above problem by flush all pending stuffs when all the other tasks end the transaction.

Signed-off-by Miao Xie <[email protected]>

---
 fs/btrfs/transaction.c |   74 ++++++++++++++++++++++++++++++-----------------
 1 files changed, 47 insertions(+), 27 deletions(-)

fs/btrfs/transaction.c +47 -27

index 6d0d5a0..d9a9a70 100644
@@ -1401,6 +1401,48 @@ static void cleanup_transaction(struct btrfs_trans_handle *trans,
 	kmem_cache_free(btrfs_trans_handle_cachep, trans);
 }
 
+static int btrfs_flush_all_pending_stuffs(struct btrfs_trans_handle *trans,
+					  struct btrfs_root *root)
+{
+	int flush_on_commit = btrfs_test_opt(root, FLUSHONCOMMIT);
+	int snap_pending = 0;
+	int ret;
+
+	if (!flush_on_commit) {
+		spin_lock(&root->fs_info->trans_lock);
+		if (!list_empty(&trans->transaction->pending_snapshots))
+			snap_pending = 1;
+		spin_unlock(&root->fs_info->trans_lock);
+	}
+
+	if (flush_on_commit || snap_pending) {
+		btrfs_start_delalloc_inodes(root, 1);
+		btrfs_wait_ordered_extents(root, 1);
+	}
+
+	ret = btrfs_run_delayed_items(trans, root);
+	if (ret)
+		return ret;
+
+	/*
+	 * running the delayed items may have added new refs. account
+	 * them now so that they hinder processing of more delayed refs
+	 * as little as possible.
+	 */
+	btrfs_delayed_refs_qgroup_accounting(trans, root->fs_info);
+
+	/*
+	 * rename don't use btrfs_join_transaction, so, once we
+	 * set the transaction to blocked above, we aren't going
+	 * to get any new ordered operations.  We can safely run
+	 * it here and no for sure that nothing new will be added
+	 * to the list
+	 */
+	btrfs_run_ordered_operations(root, 1);
+
+	return 0;
+}
+
 /*
  * btrfs_transaction state sequence:
  *    in_commit = 0, blocked = 0  (initial)

@@ -1418,7 +1460,6 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans,
 	int ret = -EIO;
 	int should_grow = 0;
 	unsigned long now = get_seconds();
-	int flush_on_commit = btrfs_test_opt(root, FLUSHONCOMMIT);
 
 	btrfs_run_ordered_operations(root, 0);
 

@@ -1491,39 +1532,14 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans,
 		should_grow = 1;
 
 	do {
-		int snap_pending = 0;
-
 		joined = cur_trans->num_joined;
-		if (!list_empty(&trans->transaction->pending_snapshots))
-			snap_pending = 1;
 
 		WARN_ON(cur_trans != trans->transaction);
 
-		if (flush_on_commit || snap_pending) {
-			btrfs_start_delalloc_inodes(root, 1);
-			btrfs_wait_ordered_extents(root, 1);
-		}
-
-		ret = btrfs_run_delayed_items(trans, root);
+		ret = btrfs_flush_all_pending_stuffs(trans, root);
 		if (ret)
 			goto cleanup_transaction;
 
-		/*
-		 * running the delayed items may have added new refs. account
-		 * them now so that they hinder processing of more delayed refs
-		 * as little as possible.
-		 */
-		btrfs_delayed_refs_qgroup_accounting(trans, root->fs_info);
-
-		/*
-		 * rename don't use btrfs_join_transaction, so, once we
-		 * set the transaction to blocked above, we aren't going
-		 * to get any new ordered operations.  We can safely run
-		 * it here and no for sure that nothing new will be added
-		 * to the list
-		 */
-		btrfs_run_ordered_operations(root, 1);
-
 		prepare_to_wait(&cur_trans->writer_wait, &wait,
 				TASK_UNINTERRUPTIBLE);
 

@@ -1536,6 +1552,10 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans,
 	} while (atomic_read(&cur_trans->num_writers) > 1 ||
 		 (should_grow && cur_trans->num_joined != joined));
 
+	ret = btrfs_flush_all_pending_stuffs(trans, root);
+	if (ret)
+		goto cleanup_transaction;
+
 	/*
 	 * Ok now we need to make sure to block out any other joins while we
 	 * commit the transaction.  We could have started a join before setting

Signature · 5 lines

1.7.6.5
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

(sorry, forgot to cc linux-btrfs.)

Quoted 17 lines · Miao Xie

Quoted 16 lines · Liu Bo

Quoted 14 lines · Miao Xie

Consider the following case:
Task1 Task2
start_transaction
commit_transaction
check pending snapshots list and the list is empty.
add pending snapshot into list
skip the delalloc flush
end_transaction
...

And then the problem that the snapshot is different with the source subvolume happen.

Quoted 7 lines

Quoted 3 lines

This is weird, create_snapshot() will first add pending snapshot into list and then commit the transaction itself, regardless of if the snapshot is different with others or not.

But the transaction may be committed by the other task, and the snapshot creation task just wait until it ends.

It's possible that a commit tranaction becomes a end transaction when it finds itself is already in commit.

So if snapshot creation starts the transaction, it will increment the transaction's num_writers, why does not the other task wait for its end_transacion?

I doubt if this can really happen anyway...

Can you elaborate the situation more?

thanks,
liubo

Quoted 142 lines

Quoted 2 lines

How do you find this?

Just by review the code. I think it can be triggered

Thanks
Miao

Quoted 132 lines

thanks,
liubo

Quoted 127 lines

This patch fixes the above problem by flush all pending stuffs when all the other tasks end the transaction.

Signed-off-by Miao Xie <[email protected]>

---
 fs/btrfs/transaction.c |   74 ++++++++++++++++++++++++++++++-----------------
 1 files changed, 47 insertions(+), 27 deletions(-)

fs/btrfs/transaction.c +47 -27

index 6d0d5a0..d9a9a70 100644
@@ -1401,6 +1401,48 @@ static void cleanup_transaction(struct btrfs_trans_handle *trans,
 	kmem_cache_free(btrfs_trans_handle_cachep, trans);
 }
 
+static int btrfs_flush_all_pending_stuffs(struct btrfs_trans_handle *trans,
+					  struct btrfs_root *root)
+{
+	int flush_on_commit = btrfs_test_opt(root, FLUSHONCOMMIT);
+	int snap_pending = 0;
+	int ret;
+
+	if (!flush_on_commit) {
+		spin_lock(&root->fs_info->trans_lock);
+		if (!list_empty(&trans->transaction->pending_snapshots))
+			snap_pending = 1;
+		spin_unlock(&root->fs_info->trans_lock);
+	}
+
+	if (flush_on_commit || snap_pending) {
+		btrfs_start_delalloc_inodes(root, 1);
+		btrfs_wait_ordered_extents(root, 1);
+	}
+
+	ret = btrfs_run_delayed_items(trans, root);
+	if (ret)
+		return ret;
+
+	/*
+	 * running the delayed items may have added new refs. account
+	 * them now so that they hinder processing of more delayed refs
+	 * as little as possible.
+	 */
+	btrfs_delayed_refs_qgroup_accounting(trans, root->fs_info);
+
+	/*
+	 * rename don't use btrfs_join_transaction, so, once we
+	 * set the transaction to blocked above, we aren't going
+	 * to get any new ordered operations.  We can safely run
+	 * it here and no for sure that nothing new will be added
+	 * to the list
+	 */
+	btrfs_run_ordered_operations(root, 1);
+
+	return 0;
+}
+
 /*
  * btrfs_transaction state sequence:
  *    in_commit = 0, blocked = 0  (initial)

@@ -1418,7 +1460,6 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans,
 	int ret = -EIO;
 	int should_grow = 0;
 	unsigned long now = get_seconds();
-	int flush_on_commit = btrfs_test_opt(root, FLUSHONCOMMIT);
 
 	btrfs_run_ordered_operations(root, 0);
 

@@ -1491,39 +1532,14 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans,
 		should_grow = 1;
 
 	do {
-		int snap_pending = 0;
-
 		joined = cur_trans->num_joined;
-		if (!list_empty(&trans->transaction->pending_snapshots))
-			snap_pending = 1;
 
 		WARN_ON(cur_trans != trans->transaction);
 
-		if (flush_on_commit || snap_pending) {
-			btrfs_start_delalloc_inodes(root, 1);
-			btrfs_wait_ordered_extents(root, 1);
-		}
-
-		ret = btrfs_run_delayed_items(trans, root);
+		ret = btrfs_flush_all_pending_stuffs(trans, root);
 		if (ret)
 			goto cleanup_transaction;
 
-		/*
-		 * running the delayed items may have added new refs. account
-		 * them now so that they hinder processing of more delayed refs
-		 * as little as possible.
-		 */
-		btrfs_delayed_refs_qgroup_accounting(trans, root->fs_info);
-
-		/*
-		 * rename don't use btrfs_join_transaction, so, once we
-		 * set the transaction to blocked above, we aren't going
-		 * to get any new ordered operations.  We can safely run
-		 * it here and no for sure that nothing new will be added
-		 * to the list
-		 */
-		btrfs_run_ordered_operations(root, 1);
-
 		prepare_to_wait(&cur_trans->writer_wait, &wait,
 				TASK_UNINTERRUPTIBLE);
 

@@ -1536,6 +1552,10 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans,
 	} while (atomic_read(&cur_trans->num_writers) > 1 ||
 		 (should_grow && cur_trans->num_joined != joined));
 
+	ret = btrfs_flush_all_pending_stuffs(trans, root);
+	if (ret)
+		goto cleanup_transaction;
+
 	/*
 	 * Ok now we need to make sure to block out any other joins while we
 	 * commit the transaction.  We could have started a join before setting

Signature · 5 lines

1.7.6.5
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

Quoted 30 lines · Liu Bo

(sorry, forgot to cc linux-btrfs.)

Quoted 17 lines · Miao Xie

Quoted 16 lines · Liu Bo

Quoted 14 lines · Miao Xie

Consider the following case:
Task1 Task2
start_transaction
commit_transaction
check pending snapshots list and the list is empty.
add pending snapshot into list
skip the delalloc flush
end_transaction
...

And then the problem that the snapshot is different with the source subvolume happen.

Quoted 7 lines

Quoted 3 lines

This is weird, create_snapshot() will first add pending snapshot into list and then commit the transaction itself, regardless of if the snapshot is different with others or not.

But the transaction may be committed by the other task, and the snapshot creation task just wait until it ends.

It's possible that a commit tranaction becomes a end transaction when it finds itself is already in commit.

Quoted 7 lines

So if snapshot creation starts the transaction, it will increment the transaction's num_writers, why does not the other task wait for its end_transacion?

I doubt if this can really happen anyway...

Can you elaborate the situation more?

Task1 Task2
start_transaction
start_transaction
commit_transaction
set in_commit to 1
check pending snapshots list and the list is empty.
add pending snapshot into list
skip the delalloc flush
commit_transaction
find in_commit is 1
end_transaction (num_writer--)
wait_for_commit
num_writer is 1
continue committing the transaction
...

Quoted 39 lines · Miao Xie

Quoted 30 lines · Liu Bo

(sorry, forgot to cc linux-btrfs.)

Quoted 17 lines · Miao Xie

Quoted 16 lines · Liu Bo

Quoted 14 lines · Miao Xie

Consider the following case:
Task1 Task2
start_transaction
commit_transaction
check pending snapshots list and the list is empty.
add pending snapshot into list
skip the delalloc flush
end_transaction
...

And then the problem that the snapshot is different with the source subvolume happen.

Quoted 7 lines

Quoted 3 lines

This is weird, create_snapshot() will first add pending snapshot into list and then commit the transaction itself, regardless of if the snapshot is different with others or not.

But the transaction may be committed by the other task, and the snapshot creation task just wait until it ends.

It's possible that a commit tranaction becomes a end transaction when it finds itself is already in commit.

Quoted 7 lines

So if snapshot creation starts the transaction, it will increment the transaction's num_writers, why does not the other task wait for its end_transacion?

I doubt if this can really happen anyway...

Can you elaborate the situation more?

Quoted 16 lines

Task1 Task2
start_transaction
start_transaction
commit_transaction
set in_commit to 1
check pending snapshots list and the list is empty.
add pending snapshot into list
skip the delalloc flush
commit_transaction
find in_commit is 1
end_transaction (num_writer--)
wait_for_commit
num_writer is 1
continue committing the transaction
...

Make sense.

Then I think we'd better put the flush part right after setting 'trans_no_join = 1'

since snapshot creation may also join an existing transaction.