thr3ads.net - Btrfs devel - [PATCH 11/11] Btrfs: Add ACCESS_ONCE() to transaction->abort accesses [Jan 2013]

If this information is useful, please help other people find it:
Share via:

Miao Xie

2013-Jan-10 12:53 UTC

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

David Sterba

2013-Jan-10 17:38 UTC

head link

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

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

Miao Xie

2013-Jan-14 07:17 UTC

head link

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

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

Miao Xie

2013-Jan-15 06:27 UTC

head link

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

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

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

Miao Xie

2013-Jan-15 06:29 UTC

head link

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

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

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

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

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

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

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

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

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