Btrfs: do not set log for full commit when creating non-data block groups
PATCH 1/1 Btrfs: do not set log for full commit when creating non-data block groups
From: Filipe Manana <[email protected]>
When creating a block group we don't need to set the log for full commit if the new block group is not used for data. Logged items can only point to logical addresses of data block groups (through file extent items) so there is no need to for the next fsync to fallback to a transaction commit if the new block group is for metadata.
--- fs/btrfs/extent-tree.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
fs/btrfs/extent-tree.c +2 -1
@@ -10112,7 +10112,8 @@ int btrfs_make_block_group(struct btrfs_trans_handle *trans, u64 bytes_used,struct btrfs_block_group_cache *cache;int ret; -btrfs_set_log_full_commit (fs_info, trans); +if (type &BTRFS_BLOCK_GROUP_DATA ) +btrfs_set_log_full_commit (fs_info, trans); cache =btrfs_create_block_group_cache (fs_info, chunk_offset, size);if (!cache)
Signature · 1 line
2.11.0
Quoted 7 lines · On 2018/11/8 下午9:17, [email protected] wrote:
From: Filipe Manana <[email protected]>
When creating a block group we don't need to set the log for full commit if the new block group is not used for data. Logged items can only point to logical addresses of data block groups (through file extent items) so there is no need to for the next fsync to fallback to a transaction commit if the new block group is for metadata.
Is it possible for the log tree blocks to be allocated in that new block group?
Thanks,
Qu
Quoted 21 lines
Signed-off-by Filipe Manana <[email protected]>--- fs/btrfs/extent-tree.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)fs/btrfs/extent-tree.c +2
-1@@ -10112,7 +10112,8 @@ int btrfs_make_block_group(struct btrfs_trans_handle *trans, u64 bytes_used,struct btrfs_block_group_cache *cache;int ret; -btrfs_set_log_full_commit (fs_info, trans); +if (type &BTRFS_BLOCK_GROUP_DATA ) +btrfs_set_log_full_commit (fs_info, trans); cache =btrfs_create_block_group_cache (fs_info, chunk_offset, size);if (!cache)
Quoted 6 lines · Qu Wenruo
Quoted 2 lines · On 2018/11/8 下午9:17, [email protected] wrote:
From: Filipe Manana <[email protected]>
Quoted 8 lines
Quoted 5 lines
When creating a block group we don't need to set the log for full commit if the new block group is not used for data. Logged items can only point to logical addresses of data block groups (through file extent items) so there is no need to for the next fsync to fallback to a transaction commit if the new block group is for metadata.
Is it possible for the log tree blocks to be allocated in that new block group?
Yes.
Quoted 26 lines
Thanks,
QuQuoted 21 lines
Signed-off-by Filipe Manana <[email protected]>--- fs/btrfs/extent-tree.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)fs/btrfs/extent-tree.c +2
-1@@ -10112,7 +10112,8 @@ int btrfs_make_block_group(struct btrfs_trans_handle *trans, u64 bytes_used,struct btrfs_block_group_cache *cache;int ret; -btrfs_set_log_full_commit (fs_info, trans); +if (type &BTRFS_BLOCK_GROUP_DATA ) +btrfs_set_log_full_commit (fs_info, trans); cache =btrfs_create_block_group_cache (fs_info, chunk_offset, size);if (!cache)
Quoted 14 lines · Filipe Manana
Quoted 12 lines · Qu Wenruo
Quoted 7 lines · On 2018/11/8 下午9:17, [email protected] wrote:
From: Filipe Manana <[email protected]>
When creating a block group we don't need to set the log for full commit if the new block group is not used for data. Logged items can only point to logical addresses of data block groups (through file extent items) so there is no need to for the next fsync to fallback to a transaction commit if the new block group is for metadata.
Quoted 4 lines
Quoted 2 lines
Is it possible for the log tree blocks to be allocated in that new block group?
Yes.
Now I realize what might be your concern, and this would cause trouble. Surprised this didn't trigger any problem and I had this (together with other changes) running tests for some weeks already.
Quoted 27 lines
Quoted 27 lines
Thanks,
QuQuoted 21 lines
Signed-off-by Filipe Manana <[email protected]>--- fs/btrfs/extent-tree.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)fs/btrfs/extent-tree.c +2
-1@@ -10112,7 +10112,8 @@ int btrfs_make_block_group(struct btrfs_trans_handle *trans, u64 bytes_used,struct btrfs_block_group_cache *cache;int ret; -btrfs_set_log_full_commit (fs_info, trans); +if (type &BTRFS_BLOCK_GROUP_DATA ) +btrfs_set_log_full_commit (fs_info, trans); cache =btrfs_create_block_group_cache (fs_info, chunk_offset, size);if (!cache)
Quoted 15 lines · Filipe Manana
Quoted 14 lines · Filipe Manana
Quoted 12 lines · Qu Wenruo
Quoted 7 lines · On 2018/11/8 下午9:17, [email protected] wrote:
From: Filipe Manana <[email protected]>
When creating a block group we don't need to set the log for full commit if the new block group is not used for data. Logged items can only point to logical addresses of data block groups (through file extent items) so there is no need to for the next fsync to fallback to a transaction commit if the new block group is for metadata.
Quoted 8 lines
Quoted 4 lines
Quoted 2 lines
Is it possible for the log tree blocks to be allocated in that new block group?
Yes.
Now I realize what might be your concern, and this would cause trouble. Surprised this didn't trigger any problem and I had this (together with other changes) running tests for some weeks already.
Maybe it's related metadata chunk pre-allocation so it will be super hard to hit in normal case, or some extent allocation policy preventing us from allocating tree block of newly created bg.
Thanks,
Qu
Quoted 28 lines
Quoted 28 lines
Quoted 28 lines
Thanks,
QuQuoted 21 lines
Signed-off-by Filipe Manana <[email protected]>--- fs/btrfs/extent-tree.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)fs/btrfs/extent-tree.c +2
-1@@ -10112,7 +10112,8 @@ int btrfs_make_block_group(struct btrfs_trans_handle *trans, u64 bytes_used,struct btrfs_block_group_cache *cache;int ret; -btrfs_set_log_full_commit (fs_info, trans); +if (type &BTRFS_BLOCK_GROUP_DATA ) +btrfs_set_log_full_commit (fs_info, trans); cache =btrfs_create_block_group_cache (fs_info, chunk_offset, size);if (!cache)
Quoted 24 lines · Qu Wenruo
Quoted 20 lines · Filipe Manana
Quoted 18 lines · Filipe Manana
Quoted 6 lines · Qu Wenruo
Quoted 2 lines · On 2018/11/8 下午9:17, [email protected] wrote:
From: Filipe Manana <[email protected]>
Quoted 8 lines
Quoted 5 lines
When creating a block group we don't need to set the log for full commit if the new block group is not used for data. Logged items can only point to logical addresses of data block groups (through file extent items) so there is no need to for the next fsync to fallback to a transaction commit if the new block group is for metadata.
Is it possible for the log tree blocks to be allocated in that new block group?
Yes.
Quoted 7 lines
Quoted 3 lines
Now I realize what might be your concern, and this would cause trouble. Surprised this didn't trigger any problem and I had this (together with other changes) running tests for some weeks already.
Maybe it's related metadata chunk pre-allocation so it will be super hard to hit in normal case, or some extent allocation policy preventing us from allocating tree block of newly created bg.
No, I don't think we have such kind of policy, would have noticed it over the years if we did have one.
Metadata chunk allocation just happens much less frequently for workloads the tests exercised.
Quoted 33 lines
Thanks,
QuQuoted 28 lines
Quoted 28 lines
Quoted 28 lines
Thanks,
QuQuoted 21 lines
Signed-off-by Filipe Manana <[email protected]>--- fs/btrfs/extent-tree.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)fs/btrfs/extent-tree.c +2
-1@@ -10112,7 +10112,8 @@ int btrfs_make_block_group(struct btrfs_trans_handle *trans, u64 bytes_used,struct btrfs_block_group_cache *cache;int ret; -btrfs_set_log_full_commit (fs_info, trans); +if (type &BTRFS_BLOCK_GROUP_DATA ) +btrfs_set_log_full_commit (fs_info, trans); cache =btrfs_create_block_group_cache (fs_info, chunk_offset, size);if (!cache)
Quoted 29 lines · Filipe Manana
Quoted 28 lines · Qu Wenruo
Quoted 23 lines · Filipe Manana
Quoted 14 lines · Filipe Manana
Quoted 12 lines · Qu Wenruo
Quoted 7 lines · On 2018/11/8 下午9:17, [email protected] wrote:
From: Filipe Manana <[email protected]>
When creating a block group we don't need to set the log for full commit if the new block group is not used for data. Logged items can only point to logical addresses of data block groups (through file extent items) so there is no need to for the next fsync to fallback to a transaction commit if the new block group is for metadata.
Quoted 4 lines
Quoted 2 lines
Is it possible for the log tree blocks to be allocated in that new block group?
Yes.
Now I realize what might be your concern, and this would cause trouble. Surprised this didn't trigger any problem and I had this (together with other changes) running tests for some weeks already.
Quoted 8 lines
Quoted 3 lines
Maybe it's related metadata chunk pre-allocation so it will be super hard to hit in normal case, or some extent allocation policy preventing us from allocating tree block of newly created bg.
No, I don't think we have such kind of policy, would have noticed it over the years if we did have one.
Metadata chunk allocation just happens much less frequently for workloads the tests exercised.
Then smaller fs along with 4K nodesize would help, but I'm still not sure if we could hit such case.
Thanks,
Qu
Quoted 34 lines
Quoted 34 lines
Thanks,
QuQuoted 28 lines
Quoted 28 lines
Quoted 28 lines
Thanks,
QuQuoted 21 lines
Signed-off-by Filipe Manana <[email protected]>--- fs/btrfs/extent-tree.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)fs/btrfs/extent-tree.c +2
-1@@ -10112,7 +10112,8 @@ int btrfs_make_block_group(struct btrfs_trans_handle *trans, u64 bytes_used,struct btrfs_block_group_cache *cache;int ret; -btrfs_set_log_full_commit (fs_info, trans); +if (type &BTRFS_BLOCK_GROUP_DATA ) +btrfs_set_log_full_commit (fs_info, trans); cache =btrfs_create_block_group_cache (fs_info, chunk_offset, size);if (!cache)
Quoted 15 lines · Filipe Manana
Quoted 14 lines · Filipe Manana
Quoted 12 lines · Qu Wenruo
Quoted 7 lines · On 2018/11/8 下午9:17, [email protected] wrote:
From: Filipe Manana <[email protected]>
When creating a block group we don't need to set the log for full commit if the new block group is not used for data. Logged items can only point to logical addresses of data block groups (through file extent items) so there is no need to for the next fsync to fallback to a transaction commit if the new block group is for metadata.
Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
Is it possible for the log tree blocks to be allocated in that new block group?
Yes.
Now I realize what might be your concern, and this would cause trouble.
Is this patch ok for for-next or does it need more work? Thanks.
Quoted 17 lines · David Sterba
Quoted 15 lines · Filipe Manana
Quoted 14 lines · Filipe Manana
Quoted 12 lines · Qu Wenruo
Quoted 7 lines · On 2018/11/8 下午9:17, [email protected] wrote:
From: Filipe Manana <[email protected]>
When creating a block group we don't need to set the log for full commit if the new block group is not used for data. Logged items can only point to logical addresses of data block groups (through file extent items) so there is no need to for the next fsync to fallback to a transaction commit if the new block group is for metadata.
Quoted 8 lines
Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
Is it possible for the log tree blocks to be allocated in that new block group?
Yes.
Now I realize what might be your concern, and this would cause trouble.
Is this patch ok for for-next or does it need more work? Thanks.
Nop, it's no good (despite not triggering problems initially), due to Qu's first question.
So just drop it and forget it.
Thanks.