| Message ID | 2a76b8005eb7208eda97e62a944ae456cbe8386f.1662705863.git.wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: don't update the block group item if used bytes are the same | expand |
On Fri, Sep 09, 2022 at 02:45:22PM +0800, Qu Wenruo wrote: > [BACKGROUND] > > When committing a transaction, we will update block group items for all > dirty block groups. > > But in fact, dirty block groups don't always need to update their block > group items. > It's pretty common to have a metadata block group which experienced > several CoW operations, but still have the same amount of used bytes. > > In that case, we may unnecessarily CoW a tree block doing nothing. > > [ENHANCEMENT] > > This patch will introduce btrfs_block_group::commit_used member to > remember the last used bytes, and use that new member to skip > unnecessary block group item update. > > This would be more common for large fs, which metadata block group can > be as large as 1GiB, containing at most 64K metadata items. > > In that case, if CoW added and the deleted one metadata item near the end > of the block group, then it's completely possible we don't need to touch > the block group item at all. > > [BENCHMARK] > > The patchset itself can have quite a high chance (20~80%) to skip block > group item updates in a lot of workload. > > As a result, it would result shorter time spent on > btrfs_write_dirty_block_groups(), and overall reduce the execution time > of the critical section of btrfs_commit_transaction(). > > Here comes a fio command, which will do random writes in 4K block size, > causing a very heavy metadata updates. > > fio --filename=$mnt/file --size=512M --rw=randwrite --direct=1 --bs=4k \ > --ioengine=libaio --iodepth=64 --runtime=300 --numjobs=4 \ > --name=random_write --fallocate=none --time_based --fsync_on_close=1 > > The file size (512M) and number of threads (4) means 2GiB file size in > total, but during the full 300s run time, my dedicated SATA SSD is able > to write around 20~25GiB, which is over 10 times the file size. > > Thus after we fill the initial 2G, we should not cause much block group items > updates. > > Please note, the fio numbers by itself doesn't have much change, but if > we look deeper, there is some reduced execution time, especially > for the critical section of btrfs_commit_transaction(). > > I added extra trace_printk() to measure the following per-transaction > execution time > > - Critical section of btrfs_commit_transaction() > By re-using the existing update_commit_stats() function, which > has already calculated the interval correctly. > > - The while() loop for btrfs_write_dirty_block_groups() > Although this includes the execution time of btrfs_run_delayed_refs(), > it should still be representative overall. > > Both result involves transid 7~30, the same amount of transaction > committed. > > The result looks like this: > > | Before | After | Diff > ----------------------+-------------------+----------------+-------- > Transaction interval | 229247198.5 | 215016933.6 | -6.2% > Block group interval | 23133.33333 | 18970.83333 | -18.0% > > The change in block group item updates is more obvious, as skipped bg > item updates also means less delayed refs, thus the change is more > obvious. > > And the overall execution time for that bg update loop is pretty small, > thus we can assume the extent tree is already mostly cached. > If we can skip a uncached tree block, it would cause more obvious > change. > > Unfortunately the overall reduce in commit transaction critical section > is much smaller, as the block group item updates loop is not really the > major part, at least for the above fio script. > > But still we have a observable reduction in the critical section. > > Reviewed-by: Josef Bacik <josef@toxicpanda.com> > Signed-off-by: Qu Wenruo <wqu@suse.com> > [Josef pinned down the race and provided a fix] > Signed-off-by: Josef Bacik <josef@toxicpanda.com> Thanks for the numbers, it seems worth and now that we have the fixed version I'll add it to 6.1 queue as we have the other perf improvements there.
Just for the record, I cherry-picked this patch for my storage (>120TB) and no problem at all since it was released. Thanks a lot Qu! Ciao, Gelma Il giorno mar 13 set 2022 alle ore 17:44 David Sterba <dsterba@suse.cz> ha scritto: > > On Fri, Sep 09, 2022 at 02:45:22PM +0800, Qu Wenruo wrote: > > [BACKGROUND] > > > > When committing a transaction, we will update block group items for all > > dirty block groups. > > > > But in fact, dirty block groups don't always need to update their block > > group items. > > It's pretty common to have a metadata block group which experienced > > several CoW operations, but still have the same amount of used bytes. > > > > In that case, we may unnecessarily CoW a tree block doing nothing. > > > > [ENHANCEMENT] > > > > This patch will introduce btrfs_block_group::commit_used member to > > remember the last used bytes, and use that new member to skip > > unnecessary block group item update. > > > > This would be more common for large fs, which metadata block group can > > be as large as 1GiB, containing at most 64K metadata items. > > > > In that case, if CoW added and the deleted one metadata item near the end > > of the block group, then it's completely possible we don't need to touch > > the block group item at all. > > > > [BENCHMARK] > > > > The patchset itself can have quite a high chance (20~80%) to skip block > > group item updates in a lot of workload. > > > > As a result, it would result shorter time spent on > > btrfs_write_dirty_block_groups(), and overall reduce the execution time > > of the critical section of btrfs_commit_transaction(). > > > > Here comes a fio command, which will do random writes in 4K block size, > > causing a very heavy metadata updates. > > > > fio --filename=$mnt/file --size=512M --rw=randwrite --direct=1 --bs=4k \ > > --ioengine=libaio --iodepth=64 --runtime=300 --numjobs=4 \ > > --name=random_write --fallocate=none --time_based --fsync_on_close=1 > > > > The file size (512M) and number of threads (4) means 2GiB file size in > > total, but during the full 300s run time, my dedicated SATA SSD is able > > to write around 20~25GiB, which is over 10 times the file size. > > > > Thus after we fill the initial 2G, we should not cause much block group items > > updates. > > > > Please note, the fio numbers by itself doesn't have much change, but if > > we look deeper, there is some reduced execution time, especially > > for the critical section of btrfs_commit_transaction(). > > > > I added extra trace_printk() to measure the following per-transaction > > execution time > > > > - Critical section of btrfs_commit_transaction() > > By re-using the existing update_commit_stats() function, which > > has already calculated the interval correctly. > > > > - The while() loop for btrfs_write_dirty_block_groups() > > Although this includes the execution time of btrfs_run_delayed_refs(), > > it should still be representative overall. > > > > Both result involves transid 7~30, the same amount of transaction > > committed. > > > > The result looks like this: > > > > | Before | After | Diff > > ----------------------+-------------------+----------------+-------- > > Transaction interval | 229247198.5 | 215016933.6 | -6.2% > > Block group interval | 23133.33333 | 18970.83333 | -18.0% > > > > The change in block group item updates is more obvious, as skipped bg > > item updates also means less delayed refs, thus the change is more > > obvious. > > > > And the overall execution time for that bg update loop is pretty small, > > thus we can assume the extent tree is already mostly cached. > > If we can skip a uncached tree block, it would cause more obvious > > change. > > > > Unfortunately the overall reduce in commit transaction critical section > > is much smaller, as the block group item updates loop is not really the > > major part, at least for the above fio script. > > > > But still we have a observable reduction in the critical section. > > > > Reviewed-by: Josef Bacik <josef@toxicpanda.com> > > Signed-off-by: Qu Wenruo <wqu@suse.com> > > [Josef pinned down the race and provided a fix] > > Signed-off-by: Josef Bacik <josef@toxicpanda.com> > > Thanks for the numbers, it seems worth and now that we have the fixed > version I'll add it to 6.1 queue as we have the other perf improvements > there.
diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c index e7b5a54c8258..0df4d98df278 100644 --- a/fs/btrfs/block-group.c +++ b/fs/btrfs/block-group.c @@ -2002,6 +2002,7 @@ static int read_one_block_group(struct btrfs_fs_info *info, cache->length = key->offset; cache->used = btrfs_stack_block_group_used(bgi); + cache->commit_used = cache->used; cache->flags = btrfs_stack_block_group_flags(bgi); cache->global_root_id = btrfs_stack_block_group_chunk_objectid(bgi); @@ -2693,6 +2694,22 @@ static int update_block_group_item(struct btrfs_trans_handle *trans, struct extent_buffer *leaf; struct btrfs_block_group_item bgi; struct btrfs_key key; + u64 used; + + /* + * Block group items update can be triggered out of commit transaction + * critical section, thus we need a consistent view of used bytes. + * We can not direct use cache->used out of the spin lock, as it + * may be changed. + */ + spin_lock(&cache->lock); + used = cache->used; + /* No change in used bytes, can safely skip it. */ + if (cache->commit_used == used) { + spin_unlock(&cache->lock); + return 0; + } + spin_unlock(&cache->lock); key.objectid = cache->start; key.type = BTRFS_BLOCK_GROUP_ITEM_KEY; @@ -2707,12 +2724,13 @@ static int update_block_group_item(struct btrfs_trans_handle *trans, leaf = path->nodes[0]; bi = btrfs_item_ptr_offset(leaf, path->slots[0]); - btrfs_set_stack_block_group_used(&bgi, cache->used); + btrfs_set_stack_block_group_used(&bgi, used); btrfs_set_stack_block_group_chunk_objectid(&bgi, cache->global_root_id); btrfs_set_stack_block_group_flags(&bgi, cache->flags); write_extent_buffer(leaf, &bgi, bi, sizeof(bgi)); btrfs_mark_buffer_dirty(leaf); + cache->commit_used = used; fail: btrfs_release_path(path); return ret; diff --git a/fs/btrfs/block-group.h b/fs/btrfs/block-group.h index f48db81d1d56..b57718020104 100644 --- a/fs/btrfs/block-group.h +++ b/fs/btrfs/block-group.h @@ -84,6 +84,12 @@ struct btrfs_block_group { u64 cache_generation; u64 global_root_id; + /* + * The last committed used bytes of this block group, if above @used + * is still the same as @commit_used, we don't need to update block + * group item of this block group. + */ + u64 commit_used; /* * If the free space extent count exceeds this number, convert the block * group to bitmaps.
[BACKGROUND] When committing a transaction, we will update block group items for all dirty block groups. But in fact, dirty block groups don't always need to update their block group items. It's pretty common to have a metadata block group which experienced several CoW operations, but still have the same amount of used bytes. In that case, we may unnecessarily CoW a tree block doing nothing. [ENHANCEMENT] This patch will introduce btrfs_block_group::commit_used member to remember the last used bytes, and use that new member to skip unnecessary block group item update. This would be more common for large fs, which metadata block group can be as large as 1GiB, containing at most 64K metadata items. In that case, if CoW added and the deleted one metadata item near the end of the block group, then it's completely possible we don't need to touch the block group item at all. [BENCHMARK] The patchset itself can have quite a high chance (20~80%) to skip block group item updates in a lot of workload. As a result, it would result shorter time spent on btrfs_write_dirty_block_groups(), and overall reduce the execution time of the critical section of btrfs_commit_transaction(). Here comes a fio command, which will do random writes in 4K block size, causing a very heavy metadata updates. fio --filename=$mnt/file --size=512M --rw=randwrite --direct=1 --bs=4k \ --ioengine=libaio --iodepth=64 --runtime=300 --numjobs=4 \ --name=random_write --fallocate=none --time_based --fsync_on_close=1 The file size (512M) and number of threads (4) means 2GiB file size in total, but during the full 300s run time, my dedicated SATA SSD is able to write around 20~25GiB, which is over 10 times the file size. Thus after we fill the initial 2G, we should not cause much block group items updates. Please note, the fio numbers by itself doesn't have much change, but if we look deeper, there is some reduced execution time, especially for the critical section of btrfs_commit_transaction(). I added extra trace_printk() to measure the following per-transaction execution time - Critical section of btrfs_commit_transaction() By re-using the existing update_commit_stats() function, which has already calculated the interval correctly. - The while() loop for btrfs_write_dirty_block_groups() Although this includes the execution time of btrfs_run_delayed_refs(), it should still be representative overall. Both result involves transid 7~30, the same amount of transaction committed. The result looks like this: | Before | After | Diff ----------------------+-------------------+----------------+-------- Transaction interval | 229247198.5 | 215016933.6 | -6.2% Block group interval | 23133.33333 | 18970.83333 | -18.0% The change in block group item updates is more obvious, as skipped bg item updates also means less delayed refs, thus the change is more obvious. And the overall execution time for that bg update loop is pretty small, thus we can assume the extent tree is already mostly cached. If we can skip a uncached tree block, it would cause more obvious change. Unfortunately the overall reduce in commit transaction critical section is much smaller, as the block group item updates loop is not really the major part, at least for the above fio script. But still we have a observable reduction in the critical section. Reviewed-by: Josef Bacik <josef@toxicpanda.com> Signed-off-by: Qu Wenruo <wqu@suse.com> [Josef pinned down the race and provided a fix] Signed-off-by: Josef Bacik <josef@toxicpanda.com> --- Changelog: v3: - Further improve the benchmark No code change. * Make it time based, so we can overwrite the file several times * Make it much longer * Reduce the total file size to avoid ENOSPC pressure * Measure both commit transaction interval and block group item updates interval to get a more representative result. v2: - Add a new benchmark I added btrfs_transaction::total_bg_updates and skipped_bg_updates atomics, the total one will get increased every time update_block_group_item() get called, and the skipped one will get increased when we skip one update. Then I go trace_printk() at btrfs_commit_transaction() to print the two atomics. The full benchmark is way better than I initially though, after the initial increase in metadata usage, later transactions all got a high percentage of skipped bg updates, between 40~80%. - Thanks Josef for pinning down and fixing a race Previously, update_block_group_item() only access cache->used once, thus it doesn't really need extra protection. But now we need to access it at least 3 times (one to check if we can skip, one to update the block group item, one to update commit_used). This requires a lock to prevent the cache->used changed halfway, and lead to incorrect used bytes in block group item. --- fs/btrfs/block-group.c | 20 +++++++++++++++++++- fs/btrfs/block-group.h | 6 ++++++ 2 files changed, 25 insertions(+), 1 deletion(-)