* [PATCH] btrfs: Remove unnecessary check from join_running_log_trans
@ 2019-05-23 11:51 Nikolay Borisov
2019-07-12 11:24 ` Filipe Manana
2019-08-01 13:03 ` David Sterba
0 siblings, 2 replies; 3+ messages in thread
From: Nikolay Borisov @ 2019-05-23 11:51 UTC (permalink / raw)
To: linux-btrfs; +Cc: Nikolay Borisov
join_running_log_trans checks btrfs_root::log_root outside of
btrfs_root::log_mutex to avoid contention on the mutex. Turns out this
check is not necessary because the two callers of join_running_log_trans
(both of which deal with removing entries from the tree-log during
unlink) explicitly check whether the respective inode has been logged in
the current transaction. If it hasn't then it won't have any items in
the tree-log and call path will return before calling
join_running_log_trans. If the check passes, however, then it's
guaranteed that btrfs_root::log_root is set because the inode is logged.
Those guarantees allows us to remove the speculative as well as the
implicity and tricky memory barrier. No functionl changes.
Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
I have evaluated if there is any performance impact (there isn't one). Here's
the script used:
for i in {1..10}; do
echo "Testun run : $i"
time ./ltp/fsstress -d /media/scratch/ -p5 -n 100000 -z -fcreat=100 -f write=100 -f fsync=70 -f unlink=80
rm -rf /media/scratch/*
echo "Executions of join_running_trans : $(trace-cmd show | wc -l)"
trace-cmd clear
done
And the result :
Unpatched (Sys) Unpatched (Real) Unpatched (JRT exec) Patched (Sys) Patched(Real) Patched (JRT exec)
161 387 153215 183 393 149153
165 392 158490 159 404 158118
140 381 147707 145 373 145676
143 394 147129 148 383 131029
206 410 157987 152 383 136134
152 376 157771 143 387 131048
140 371 153929 146 376 149885
149 376 152723 207 407 147477
164 396 157385 160 393 155272
146 373 147937 148 384 152828
stddev 19.75 12.44 4525 20.5 11.06 9722
mean 156.6 385.6 153427.3 159.1 388.3 145662
median 150.5 384 153572 150 385.5 148315
JRT exec means executions of join_running_transaction during that iteration of
the test case.
fs/btrfs/tree-log.c | 4 ----
1 file changed, 4 deletions(-)
diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
index 6c47f6ed3e94..6c8aff105b0c 100644
--- a/fs/btrfs/tree-log.c
+++ b/fs/btrfs/tree-log.c
@@ -188,10 +188,6 @@ static int join_running_log_trans(struct btrfs_root *root)
{
int ret = -ENOENT;
- smp_mb();
- if (!root->log_root)
- return -ENOENT;
-
mutex_lock(&root->log_mutex);
if (root->log_root) {
ret = 0;
--
2.17.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] btrfs: Remove unnecessary check from join_running_log_trans
2019-05-23 11:51 [PATCH] btrfs: Remove unnecessary check from join_running_log_trans Nikolay Borisov
@ 2019-07-12 11:24 ` Filipe Manana
2019-08-01 13:03 ` David Sterba
1 sibling, 0 replies; 3+ messages in thread
From: Filipe Manana @ 2019-07-12 11:24 UTC (permalink / raw)
To: Nikolay Borisov; +Cc: linux-btrfs
On Thu, May 23, 2019 at 12:53 PM Nikolay Borisov <nborisov@suse.com> wrote:
>
> join_running_log_trans checks btrfs_root::log_root outside of
> btrfs_root::log_mutex to avoid contention on the mutex. Turns out this
> check is not necessary because the two callers of join_running_log_trans
> (both of which deal with removing entries from the tree-log during
> unlink) explicitly check whether the respective inode has been logged in
> the current transaction. If it hasn't then it won't have any items in
> the tree-log and call path will return before calling
> join_running_log_trans. If the check passes, however, then it's
> guaranteed that btrfs_root::log_root is set because the inode is logged.
>
> Those guarantees allows us to remove the speculative as well as the
> implicity and tricky memory barrier. No functionl changes.
>
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
Reviewed-by: Filipe Manana <fdmanana@suse.com>
Looks good, thanks.
> ---
>
> I have evaluated if there is any performance impact (there isn't one). Here's
> the script used:
>
> for i in {1..10}; do
> echo "Testun run : $i"
> time ./ltp/fsstress -d /media/scratch/ -p5 -n 100000 -z -fcreat=100 -f write=100 -f fsync=70 -f unlink=80
> rm -rf /media/scratch/*
> echo "Executions of join_running_trans : $(trace-cmd show | wc -l)"
> trace-cmd clear
> done
>
> And the result :
>
> Unpatched (Sys) Unpatched (Real) Unpatched (JRT exec) Patched (Sys) Patched(Real) Patched (JRT exec)
> 161 387 153215 183 393 149153
> 165 392 158490 159 404 158118
> 140 381 147707 145 373 145676
> 143 394 147129 148 383 131029
> 206 410 157987 152 383 136134
> 152 376 157771 143 387 131048
> 140 371 153929 146 376 149885
> 149 376 152723 207 407 147477
> 164 396 157385 160 393 155272
> 146 373 147937 148 384 152828
>
> stddev 19.75 12.44 4525 20.5 11.06 9722
> mean 156.6 385.6 153427.3 159.1 388.3 145662
> median 150.5 384 153572 150 385.5 148315
>
>
> JRT exec means executions of join_running_transaction during that iteration of
> the test case.
>
> fs/btrfs/tree-log.c | 4 ----
> 1 file changed, 4 deletions(-)
>
> diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
> index 6c47f6ed3e94..6c8aff105b0c 100644
> --- a/fs/btrfs/tree-log.c
> +++ b/fs/btrfs/tree-log.c
> @@ -188,10 +188,6 @@ static int join_running_log_trans(struct btrfs_root *root)
> {
> int ret = -ENOENT;
>
> - smp_mb();
> - if (!root->log_root)
> - return -ENOENT;
> -
> mutex_lock(&root->log_mutex);
> if (root->log_root) {
> ret = 0;
> --
> 2.17.1
>
--
Filipe David Manana,
“Whether you think you can, or you think you can't — you're right.”
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] btrfs: Remove unnecessary check from join_running_log_trans
2019-05-23 11:51 [PATCH] btrfs: Remove unnecessary check from join_running_log_trans Nikolay Borisov
2019-07-12 11:24 ` Filipe Manana
@ 2019-08-01 13:03 ` David Sterba
1 sibling, 0 replies; 3+ messages in thread
From: David Sterba @ 2019-08-01 13:03 UTC (permalink / raw)
To: Nikolay Borisov; +Cc: linux-btrfs
On Thu, May 23, 2019 at 02:51:26PM +0300, Nikolay Borisov wrote:
> join_running_log_trans checks btrfs_root::log_root outside of
> btrfs_root::log_mutex to avoid contention on the mutex. Turns out this
> check is not necessary because the two callers of join_running_log_trans
> (both of which deal with removing entries from the tree-log during
> unlink) explicitly check whether the respective inode has been logged in
> the current transaction. If it hasn't then it won't have any items in
> the tree-log and call path will return before calling
> join_running_log_trans. If the check passes, however, then it's
> guaranteed that btrfs_root::log_root is set because the inode is logged.
>
> Those guarantees allows us to remove the speculative as well as the
> implicity and tricky memory barrier. No functionl changes.
>
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
Added to misc-next, thanks.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-08-01 13:03 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-23 11:51 [PATCH] btrfs: Remove unnecessary check from join_running_log_trans Nikolay Borisov
2019-07-12 11:24 ` Filipe Manana
2019-08-01 13:03 ` David Sterba
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).