Skip to contentSkip to conversation

Btrfs: Add ACCESS_ONCE() to transaction->abort accesses

PATCH Superseded Miao Xie 2 participants 5 messages Jan 10, 2013 – Jan 15, 2013 Btrfs file system

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Miao Xie

author
#0 lore raw

PATCH 11/11 Btrfs: Add ACCESS_ONCE() to transaction->abort accesses

We may access and update transaction->abort on the different CPUs without lock, so we need ACCESS_ONCE() wrapper to make sure we can get the new value.

Signed-off-by Miao Xie <[email protected]>
---
 fs/btrfs/super.c       | 2 +-
 fs/btrfs/transaction.c | 9 ++++-----
 2 files changed, 5 insertions(+), 6 deletions(-)
fs/btrfs/super.c +1 -1
index f714379..0d88513 100644
@@ -267,7 +267,7 @@ void __btrfs_abort_transaction(struct btrfs_trans_handle *trans,
 			     function, line, errstr);
 		return;
 	}
-	trans->transaction->aborted = errno;
+	ACCESS_ONCE(trans->transaction->aborted) = errno;
 	__btrfs_std_error(root->fs_info, function, line, errno, NULL);
 }
 /*
fs/btrfs/transaction.c +4 -5
index a950d48..ee6cf27 100644
@@ -1468,11 +1468,6 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans,
 		goto cleanup_transaction;
 	}
 
-	if (cur_trans->aborted) {
-		ret = cur_trans->aborted;
-		goto cleanup_transaction;
-	}
-
 	/* make a pass through all the delayed refs we have so far
 	 * any runnings procs may add more while we are here
 	 */

@@ -1574,6 +1569,10 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans,
 	wait_event(cur_trans->writer_wait,
 		   atomic_read(&cur_trans->num_writers) == 1);
 
+	if (ACCESS_ONCE(cur_trans->aborted)) {
+		ret = cur_trans->aborted;
+		goto cleanup_transaction;
+	}
 	/*
 	 * the reloc mutex makes sure that we stop
 	 * the balancing code from coming in and moving
Signature · 1 line
1.7.11.7
#1 lore raw
Quoted 2 lines · Miao Xie

We may access and update transaction->abort on the different CPUs without lock, so we need ACCESS_ONCE() wrapper to make sure we can get the new value.

ACCESS_ONCE is not the right synchronization primitive to be used here, it is a way to force compiler to generate a single access to the data through out the varaible scope, this does not have impact on inter-cpu synchronization. This does not give a guarantee of the latest value of abort.

fs/btrfs/super.c void __btrfs_abort_transaction(struct btrfs_trans_handle *trans, · line 271

Quoted 9 lines
fs/btrfs/super.c +1 -1
@@ -267,7 +267,7 @@ void __btrfs_abort_transaction(struct btrfs_trans_handle *trans,
 			     function, line, errstr);
 		return;
 	}
-	trans->transaction->aborted = errno;
+	ACCESS_ONCE(trans->transaction->aborted) = errno;
 	__btrfs_std_error(root->fs_info, function, line, errno, NULL);

I don't think it's possible to reach 2 transaction aborts at the same time so that the 'aborted' value gets silently overwritten. The error message uses the 'errno' value passed to the function and thus this will be visible in the syslog. I don't see a better way how to decide which of the multiple 'aborted' values should win. A non-zero will abort, all of them are in syslog. Enough information for further debugging.

Quoted 7 lines
fs/btrfs/super.c +0 -0
 }
 /*
fs/btrfs/transaction.c +0 -0
index a950d48..ee6cf27 100644

fs/btrfs/transaction.c int btrfs_commit_transaction(struct btrfs_trans_handle *trans, · line 1474

Quoted 7 lines
fs/btrfs/transaction.c +0 -4
@@ -1468,11 +1468,6 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans,
 		goto cleanup_transaction;
 	}
 
-	if (cur_trans->aborted) {
-		ret = cur_trans->aborted;
-		goto cleanup_transaction;
-	}

This is called early in the function and stops commit actions in case the transaction has been aborted. Continuing despite that does not seem right, the filesystem is RO already.

Quoted 5 lines
fs/btrfs/transaction.c +0 -1
-
 	/* make a pass through all the delayed refs we have so far
 	 * any runnings procs may add more while we are here
 	 */

fs/btrfs/transaction.c int btrfs_commit_transaction(struct btrfs_trans_handle *trans, · line 1578

Quoted 10 lines
fs/btrfs/transaction.c +4 -0
@@ -1574,6 +1569,10 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans,
 	wait_event(cur_trans->writer_wait,
 		   atomic_read(&cur_trans->num_writers) == 1);
 
+	if (ACCESS_ONCE(cur_trans->aborted)) {
+		ret = cur_trans->aborted;
+		goto cleanup_transaction;
+	}
 	/*
 	 * the reloc mutex makes sure that we stop
 	 * the balancing code from coming in and moving

david

author
#2 lore raw
Quoted 9 lines · David Sterba
Quoted 2 lines · Miao Xie

We may access and update transaction->abort on the different CPUs without lock, so we need ACCESS_ONCE() wrapper to make sure we can get the new value.

ACCESS_ONCE is not the right synchronization primitive to be used here, it is a way to force compiler to generate a single access to the data through out the varaible scope, this does not have impact on inter-cpu synchronization. This does not give a guarantee of the latest value of abort.

ACCESS_ONCE here is not used to synchronize the transaction->abort, it is used to prevent the compiler from the using optimizations which might create unsolicited accesses or optimize accesses out of existence.

(Maybe My changelog is too short and the description is not exact. Sorry)

Quoted 17 lines

fs/btrfs/super.c void __btrfs_abort_transaction(struct btrfs_trans_handle *trans, · line 271

Quoted 10 lines
fs/btrfs/super.c +1 -1
@@ -267,7 +267,7 @@ void __btrfs_abort_transaction(struct btrfs_trans_handle *trans,
 			     function, line, errstr);
 		return;
 	}
-	trans->transaction->aborted = errno;
+	ACCESS_ONCE(trans->transaction->aborted) = errno;
 	__btrfs_std_error(root->fs_info, function, line, errno, NULL);

I don't think it's possible to reach 2 transaction aborts at the same time so that the 'aborted' value gets silently overwritten. The error message uses the 'errno' value passed to the function and thus this will be visible in the syslog. I don't see a better way how to decide which of the multiple 'aborted' values should win. A non-zero will abort, all of them are in syslog. Enough information for further debugging.

We don't know how the compiler would optimize the code. transaction->abort is not protected by the lock, and if the compiler is within its rights to manufacture an additional store by transforming the above code into the following:

trans->transaction->aborted = errno;

if (!trans->blocks_used) {
	trans->transaction->aborted = 0;
	......
}

The read side would get a wrong value.

As Linus said: "if you access unlocked values, you use ACCESS_ONCE(). You don't say "but it can't matter". Because you simply don't know."

Quoted 10 lines
Quoted 10 lines
fs/btrfs/super.c +0 -0
 }
 /*
fs/btrfs/transaction.c +0 -0
index a950d48..ee6cf27 100644
@@ -1468,11 +1468,6 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans,
 		goto cleanup_transaction;
 	}
Quoted 8 lines

fs/btrfs/transaction.c int btrfs_commit_transaction(struct btrfs_trans_handle *trans, · line 1474

Quoted 4 lines
fs/btrfs/transaction.c +0 -4
-	if (cur_trans->aborted) {
-		ret = cur_trans->aborted;
-		goto cleanup_transaction;
-	}

This is called early in the function and stops commit actions in case the transaction has been aborted. Continuing despite that does not seem right, the filesystem is RO already.

Here, it is possible that there are some tasks which still don't end up the transaction, and might set transaction->abort later.

Quoted 9 lines
Quoted 9 lines
fs/btrfs/transaction.c +0 -1
-
 	/* make a pass through all the delayed refs we have so far
 	 * any runnings procs may add more while we are here
 	 */

@@ -1574,6 +1569,10 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans,
 	wait_event(cur_trans->writer_wait,
 		   atomic_read(&cur_trans->num_writers) == 1);
Quoted 4 lines

fs/btrfs/transaction.c int btrfs_commit_transaction(struct btrfs_trans_handle *trans, · line 1575

Quoted 4 lines
fs/btrfs/transaction.c +4 -0
+	if (ACCESS_ONCE(cur_trans->aborted)) {
+		ret = cur_trans->aborted;
+		goto cleanup_transaction;
+	}

I add the check here is also not right. The tasks that save the inode cache and space cache may also update transaction->abort. I will make a new patch later.

Thanks
Miao

Quoted 11 lines

fs/btrfs/transaction.c int btrfs_commit_transaction(struct btrfs_trans_handle *trans, · line 1578

Quoted 3 lines
fs/btrfs/transaction.c +0 -0
 	/*
 	 * the reloc mutex makes sure that we stop
 	 * the balancing code from coming in and moving

david
--
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

author
#3 lore raw

PATCH v2 11/11 Btrfs: Add ACCESS_ONCE() to transaction->abort accesses

In reply to David Sterba

We may access and update transaction->aborted on the different CPUs without lock, so we need ACCESS_ONCE() wrapper to prevent the compiler from creating unsolicited accesses and make sure we can get the right value.

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

---
Changelog v1 -> v2:
- modify the changelog
- split the old patch into two patches, this is the first one - just add ACCESS_ONCE() wrapper.

---
 fs/btrfs/super.c       | 2 +-
 fs/btrfs/transaction.c | 3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)
fs/btrfs/super.c +1 -1
index f714379..0d88513 100644
@@ -267,7 +267,7 @@ void __btrfs_abort_transaction(struct btrfs_trans_handle *trans,
 			     function, line, errstr);
 		return;
 	}
-	trans->transaction->aborted = errno;
+	ACCESS_ONCE(trans->transaction->aborted) = errno;
 	__btrfs_std_error(root->fs_info, function, line, errno, NULL);
 }
 /*
fs/btrfs/transaction.c +2 -1
index a950d48..7dbfe2c 100644
@@ -1468,7 +1468,8 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans,
 		goto cleanup_transaction;
 	}
 
-	if (cur_trans->aborted) {
+	/* Stop the commit early if ->aborted is set */
+	if (unlikely(ACCESS_ONCE(cur_trans->aborted))) {
 		ret = cur_trans->aborted;
 		goto cleanup_transaction;
 	}
Signature · 1 line
1.7.11.7
author
#4 lore raw

PATCH 1/1 Btrfs: fix missed transaction->aborted check

In reply to David Sterba

First, though the current transaction->aborted check can stop the commit early and avoid unnecessary operations, it is too early, and some transaction handles don't end, those handles may set transaction->aborted after the check.

Second, when we commit the transaction, we will wake up some worker threads to flush the space cache and inode cache. Those threads also allocate some transaction handles and may set transaction->aborted if some serious error happens.

So we need more check for ->aborted when committing the transaction. Fix it.

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

---
This patch is against the following patch:
[PATCH V2 11/11] Btrfs: Add ACCESS_ONCE() to transaction->abort accesses

---
 fs/btrfs/transaction.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)
fs/btrfs/transaction.c +16 -0
index 7dbfe2c..50437b4 100644
@@ -1575,6 +1575,11 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans,
 	wait_event(cur_trans->writer_wait,
 		   atomic_read(&cur_trans->num_writers) == 1);
 
+	/* ->aborted might be set after the previous check, so check it */
+	if (unlikely(ACCESS_ONCE(cur_trans->aborted))) {
+		ret = cur_trans->aborted;
+		goto cleanup_transaction;
+	}
 	/*
 	 * the reloc mutex makes sure that we stop
 	 * the balancing code from coming in and moving

@@ -1658,6 +1663,17 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans,
 		goto cleanup_transaction;
 	}
 
+	/*
+	 * The tasks which save the space cache and inode cache may also
+	 * update ->aborted, check it.
+	 */
+	if (unlikely(ACCESS_ONCE(cur_trans->aborted))) {
+		ret = cur_trans->aborted;
+		mutex_unlock(&root->fs_info->tree_log_mutex);
+		mutex_unlock(&root->fs_info->reloc_mutex);
+		goto cleanup_transaction;
+	}
+
 	btrfs_prepare_extent_commit(trans, root);
 
 	cur_trans = root->fs_info->running_transaction;
Signature · 1 line
1.7.11.7