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

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

[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.

Signed-off-by: Miao Xie <miaox@cn.fujitsu.com>
---
 fs/btrfs/super.c       | 2 +-
 fs/btrfs/transaction.c | 9 ++++-----
 2 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index f714379..0d88513 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -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);
 }
 /*
diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index a950d48..ee6cf27 100644
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -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
-- 
1.7.11.7

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

[David Sterba]

On Thu, Jan 10, 2013 at 08:53:03PM +0800, Miao Xie wrote:
> 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.

> --- a/fs/btrfs/super.c
> +++ b/fs/btrfs/super.c
> @@ -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.

>  }
>  /*
> diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
> index a950d48..ee6cf27 100644
> --- a/fs/btrfs/transaction.c
> +++ b/fs/btrfs/transaction.c
> @@ -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.

> -
>  	/* 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


david

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

[Miao Xie]

On thu, 10 Jan 2013 18:38:00 +0100, David Sterba wrote:
> On Thu, Jan 10, 2013 at 08:53:03PM +0800, Miao Xie wrote:
>> 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)

> 
>> --- a/fs/btrfs/super.c
>> +++ b/fs/btrfs/super.c
>> @@ -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."

>>  }
>>  /*
>> diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
>> index a950d48..ee6cf27 100644
>> --- a/fs/btrfs/transaction.c
>> +++ b/fs/btrfs/transaction.c
>> @@ -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.

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

> 
>> -
>>  	/* 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;
>> +	}

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

>>  	/*
>>  	 * 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

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

[Miao Xie]

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 <miaox@cn.fujitsu.com>
---
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(-)

diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index f714379..0d88513 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -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);
 }
 /*
diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index a950d48..7dbfe2c 100644
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -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;
 	}
-- 
1.7.11.7

[PATCH] Btrfs: fix missed transaction->aborted check

[Miao Xie]

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 <miaox@cn.fujitsu.com>
---
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(+)

diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index 7dbfe2c..50437b4 100644
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -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;
-- 
1.7.11.7