| Message ID | 20241025065448.3231672-3-haisuwang@tencent.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: fix the length of reserved qgroup to free | expand |
在 2024/10/25 17:24, iamhswang@gmail.com 写道: > From: Haisu Wang <haisuwang@tencent.com> > > Simplify the regions mark by using cur_alloc_size only to present > the reserved but may failed to alloced extent. Remove the ram_size > as well since it is always consistent to the cur_alloc_size in the > context. Advanced the start mark in normal path until extent succeed > alloced and keep the start unchanged in error handling path. > > PASSed the fstest generic/475 test for a hundred times with quota > enabled. And a modified generic/475 test by removing the sleep time > for a hundred times. About one tenth of the tests do enter the error > handling path due to fail to reserve extent. > Although this patch is already merged into for-next, it looks like the next patch will again change the error handling, mostly render the this one useless: https://lore.kernel.org/linux-btrfs/2a0925f0264daf90741ed0a7ba7ed4b4888cf778.1728725060.git.wqu@suse.com/ The newer patch will change the error handling to a simpler one, so instead of 3 regions, there will be only 2. There will be no change needed from your side, I will update my patches to solve the conflicts, just in case if you find the error handling is different in the future. Thanks, Qu > Suggested-by: Qu Wenruo <wqu@suse.com> > Signed-off-by: Haisu Wang <haisuwang@tencent.com> > --- > fs/btrfs/inode.c | 32 ++++++++++++++------------------ > 1 file changed, 14 insertions(+), 18 deletions(-) > > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c > index 3646734a7e59..7e67a6d50be2 100644 > --- a/fs/btrfs/inode.c > +++ b/fs/btrfs/inode.c > @@ -1359,7 +1359,6 @@ static noinline int cow_file_range(struct btrfs_inode *inode, > u64 alloc_hint = 0; > u64 orig_start = start; > u64 num_bytes; > - unsigned long ram_size; > u64 cur_alloc_size = 0; > u64 min_alloc_size; > u64 blocksize = fs_info->sectorsize; > @@ -1367,7 +1366,6 @@ static noinline int cow_file_range(struct btrfs_inode *inode, > struct extent_map *em; > unsigned clear_bits; > unsigned long page_ops; > - bool extent_reserved = false; > int ret = 0; > > if (btrfs_is_free_space_inode(inode)) { > @@ -1421,8 +1419,7 @@ static noinline int cow_file_range(struct btrfs_inode *inode, > struct btrfs_ordered_extent *ordered; > struct btrfs_file_extent file_extent; > > - cur_alloc_size = num_bytes; > - ret = btrfs_reserve_extent(root, cur_alloc_size, cur_alloc_size, > + ret = btrfs_reserve_extent(root, num_bytes, num_bytes, > min_alloc_size, 0, alloc_hint, > &ins, 1, 1); > if (ret == -EAGAIN) { > @@ -1453,9 +1450,7 @@ static noinline int cow_file_range(struct btrfs_inode *inode, > if (ret < 0) > goto out_unlock; > cur_alloc_size = ins.offset; > - extent_reserved = true; > > - ram_size = ins.offset; > file_extent.disk_bytenr = ins.objectid; > file_extent.disk_num_bytes = ins.offset; > file_extent.num_bytes = ins.offset; > @@ -1463,14 +1458,14 @@ static noinline int cow_file_range(struct btrfs_inode *inode, > file_extent.offset = 0; > file_extent.compression = BTRFS_COMPRESS_NONE; > > - lock_extent(&inode->io_tree, start, start + ram_size - 1, > + lock_extent(&inode->io_tree, start, start + cur_alloc_size - 1, > &cached); > > em = btrfs_create_io_em(inode, start, &file_extent, > BTRFS_ORDERED_REGULAR); > if (IS_ERR(em)) { > unlock_extent(&inode->io_tree, start, > - start + ram_size - 1, &cached); > + start + cur_alloc_size - 1, &cached); > ret = PTR_ERR(em); > goto out_reserve; > } > @@ -1480,7 +1475,7 @@ static noinline int cow_file_range(struct btrfs_inode *inode, > 1 << BTRFS_ORDERED_REGULAR); > if (IS_ERR(ordered)) { > unlock_extent(&inode->io_tree, start, > - start + ram_size - 1, &cached); > + start + cur_alloc_size - 1, &cached); > ret = PTR_ERR(ordered); > goto out_drop_extent_cache; > } > @@ -1501,7 +1496,7 @@ static noinline int cow_file_range(struct btrfs_inode *inode, > */ > if (ret) > btrfs_drop_extent_map_range(inode, start, > - start + ram_size - 1, > + start + cur_alloc_size - 1, > false); > } > btrfs_put_ordered_extent(ordered); > @@ -1519,7 +1514,7 @@ static noinline int cow_file_range(struct btrfs_inode *inode, > page_ops = (keep_locked ? 0 : PAGE_UNLOCK); > page_ops |= PAGE_SET_ORDERED; > > - extent_clear_unlock_delalloc(inode, start, start + ram_size - 1, > + extent_clear_unlock_delalloc(inode, start, start + cur_alloc_size - 1, > locked_folio, &cached, > EXTENT_LOCKED | EXTENT_DELALLOC, > page_ops); > @@ -1529,7 +1524,7 @@ static noinline int cow_file_range(struct btrfs_inode *inode, > num_bytes -= cur_alloc_size; > alloc_hint = ins.objectid + ins.offset; > start += cur_alloc_size; > - extent_reserved = false; > + cur_alloc_size = 0; > > /* > * btrfs_reloc_clone_csums() error, since start is increased > @@ -1545,7 +1540,7 @@ static noinline int cow_file_range(struct btrfs_inode *inode, > return ret; > > out_drop_extent_cache: > - btrfs_drop_extent_map_range(inode, start, start + ram_size - 1, false); > + btrfs_drop_extent_map_range(inode, start, start + cur_alloc_size - 1, false); > out_reserve: > btrfs_dec_block_group_reservations(fs_info, ins.objectid); > btrfs_free_reserved_extent(fs_info, ins.objectid, ins.offset, 1); > @@ -1599,13 +1594,12 @@ static noinline int cow_file_range(struct btrfs_inode *inode, > * to decrement again the data space_info's bytes_may_use counter, > * therefore we do not pass it the flag EXTENT_CLEAR_DATA_RESV. > */ > - if (extent_reserved) { > + if (cur_alloc_size) { > extent_clear_unlock_delalloc(inode, start, > start + cur_alloc_size - 1, > locked_folio, &cached, clear_bits, > page_ops); > btrfs_qgroup_free_data(inode, NULL, start, cur_alloc_size, NULL); > - start += cur_alloc_size; > } > > /* > @@ -1614,11 +1608,13 @@ static noinline int cow_file_range(struct btrfs_inode *inode, > * space_info's bytes_may_use counter, reserved in > * btrfs_check_data_free_space(). > */ > - if (start < end) { > + if (start + cur_alloc_size < end) { > clear_bits |= EXTENT_CLEAR_DATA_RESV; > - extent_clear_unlock_delalloc(inode, start, end, locked_folio, > + extent_clear_unlock_delalloc(inode, start + cur_alloc_size, > + end, locked_folio, > &cached, clear_bits, page_ops); > - btrfs_qgroup_free_data(inode, NULL, start, end - start + 1, NULL); > + btrfs_qgroup_free_data(inode, NULL, start + cur_alloc_size, > + end - start - cur_alloc_size + 1, NULL); > } > return ret; > }
Qu Wenruo <quwenruo.btrfs@gmx.com> 于2024年10月30日周三 11:01写道： > > > > 在 2024/10/25 17:24, iamhswang@gmail.com 写道: > > From: Haisu Wang <haisuwang@tencent.com> > > > > Simplify the regions mark by using cur_alloc_size only to present > > the reserved but may failed to alloced extent. Remove the ram_size > > as well since it is always consistent to the cur_alloc_size in the > > context. Advanced the start mark in normal path until extent succeed > > alloced and keep the start unchanged in error handling path. > > > > PASSed the fstest generic/475 test for a hundred times with quota > > enabled. And a modified generic/475 test by removing the sleep time > > for a hundred times. About one tenth of the tests do enter the error > > handling path due to fail to reserve extent. > > > > Although this patch is already merged into for-next, it looks like the > next patch will again change the error handling, mostly render the this > one useless: > > https://lore.kernel.org/linux-btrfs/2a0925f0264daf90741ed0a7ba7ed4b4888cf778.1728725060.git.wqu@suse.com/ > > The newer patch will change the error handling to a simpler one, so > instead of 3 regions, there will be only 2. > Sounds better and a completely rewrite, i will catch up the details. > There will be no change needed from your side, I will update my patches > to solve the conflicts, just in case if you find the error handling is > different in the future. > Thanks. Understanding the changes, may compile and play after the polishment. Best regards, Haisu Wang > Thanks, > Qu > > > Suggested-by: Qu Wenruo <wqu@suse.com> > > Signed-off-by: Haisu Wang <haisuwang@tencent.com> > > --- > > fs/btrfs/inode.c | 32 ++++++++++++++------------------ > > 1 file changed, 14 insertions(+), 18 deletions(-) > > > > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c > > index 3646734a7e59..7e67a6d50be2 100644 > > --- a/fs/btrfs/inode.c > > +++ b/fs/btrfs/inode.c > > @@ -1359,7 +1359,6 @@ static noinline int cow_file_range(struct btrfs_inode *inode, > > u64 alloc_hint = 0; > > u64 orig_start = start; > > u64 num_bytes; > > - unsigned long ram_size; > > u64 cur_alloc_size = 0; > > u64 min_alloc_size; > > u64 blocksize = fs_info->sectorsize; > > @@ -1367,7 +1366,6 @@ static noinline int cow_file_range(struct btrfs_inode *inode, > > struct extent_map *em; > > unsigned clear_bits; > > unsigned long page_ops; > > - bool extent_reserved = false; > > int ret = 0; > > > > if (btrfs_is_free_space_inode(inode)) { > > @@ -1421,8 +1419,7 @@ static noinline int cow_file_range(struct btrfs_inode *inode, > > struct btrfs_ordered_extent *ordered; > > struct btrfs_file_extent file_extent; > > > > - cur_alloc_size = num_bytes; > > - ret = btrfs_reserve_extent(root, cur_alloc_size, cur_alloc_size, > > + ret = btrfs_reserve_extent(root, num_bytes, num_bytes, > > min_alloc_size, 0, alloc_hint, > > &ins, 1, 1); > > if (ret == -EAGAIN) { > > @@ -1453,9 +1450,7 @@ static noinline int cow_file_range(struct btrfs_inode *inode, > > if (ret < 0) > > goto out_unlock; > > cur_alloc_size = ins.offset; > > - extent_reserved = true; > > > > - ram_size = ins.offset; > > file_extent.disk_bytenr = ins.objectid; > > file_extent.disk_num_bytes = ins.offset; > > file_extent.num_bytes = ins.offset; > > @@ -1463,14 +1458,14 @@ static noinline int cow_file_range(struct btrfs_inode *inode, > > file_extent.offset = 0; > > file_extent.compression = BTRFS_COMPRESS_NONE; > > > > - lock_extent(&inode->io_tree, start, start + ram_size - 1, > > + lock_extent(&inode->io_tree, start, start + cur_alloc_size - 1, > > &cached); > > > > em = btrfs_create_io_em(inode, start, &file_extent, > > BTRFS_ORDERED_REGULAR); > > if (IS_ERR(em)) { > > unlock_extent(&inode->io_tree, start, > > - start + ram_size - 1, &cached); > > + start + cur_alloc_size - 1, &cached); > > ret = PTR_ERR(em); > > goto out_reserve; > > } > > @@ -1480,7 +1475,7 @@ static noinline int cow_file_range(struct btrfs_inode *inode, > > 1 << BTRFS_ORDERED_REGULAR); > > if (IS_ERR(ordered)) { > > unlock_extent(&inode->io_tree, start, > > - start + ram_size - 1, &cached); > > + start + cur_alloc_size - 1, &cached); > > ret = PTR_ERR(ordered); > > goto out_drop_extent_cache; > > } > > @@ -1501,7 +1496,7 @@ static noinline int cow_file_range(struct btrfs_inode *inode, > > */ > > if (ret) > > btrfs_drop_extent_map_range(inode, start, > > - start + ram_size - 1, > > + start + cur_alloc_size - 1, > > false); > > } > > btrfs_put_ordered_extent(ordered); > > @@ -1519,7 +1514,7 @@ static noinline int cow_file_range(struct btrfs_inode *inode, > > page_ops = (keep_locked ? 0 : PAGE_UNLOCK); > > page_ops |= PAGE_SET_ORDERED; > > > > - extent_clear_unlock_delalloc(inode, start, start + ram_size - 1, > > + extent_clear_unlock_delalloc(inode, start, start + cur_alloc_size - 1, > > locked_folio, &cached, > > EXTENT_LOCKED | EXTENT_DELALLOC, > > page_ops); > > @@ -1529,7 +1524,7 @@ static noinline int cow_file_range(struct btrfs_inode *inode, > > num_bytes -= cur_alloc_size; > > alloc_hint = ins.objectid + ins.offset; > > start += cur_alloc_size; > > - extent_reserved = false; > > + cur_alloc_size = 0; > > > > /* > > * btrfs_reloc_clone_csums() error, since start is increased > > @@ -1545,7 +1540,7 @@ static noinline int cow_file_range(struct btrfs_inode *inode, > > return ret; > > > > out_drop_extent_cache: > > - btrfs_drop_extent_map_range(inode, start, start + ram_size - 1, false); > > + btrfs_drop_extent_map_range(inode, start, start + cur_alloc_size - 1, false); > > out_reserve: > > btrfs_dec_block_group_reservations(fs_info, ins.objectid); > > btrfs_free_reserved_extent(fs_info, ins.objectid, ins.offset, 1); > > @@ -1599,13 +1594,12 @@ static noinline int cow_file_range(struct btrfs_inode *inode, > > * to decrement again the data space_info's bytes_may_use counter, > > * therefore we do not pass it the flag EXTENT_CLEAR_DATA_RESV. > > */ > > - if (extent_reserved) { > > + if (cur_alloc_size) { > > extent_clear_unlock_delalloc(inode, start, > > start + cur_alloc_size - 1, > > locked_folio, &cached, clear_bits, > > page_ops); > > btrfs_qgroup_free_data(inode, NULL, start, cur_alloc_size, NULL); > > - start += cur_alloc_size; > > } > > > > /* > > @@ -1614,11 +1608,13 @@ static noinline int cow_file_range(struct btrfs_inode *inode, > > * space_info's bytes_may_use counter, reserved in > > * btrfs_check_data_free_space(). > > */ > > - if (start < end) { > > + if (start + cur_alloc_size < end) { > > clear_bits |= EXTENT_CLEAR_DATA_RESV; > > - extent_clear_unlock_delalloc(inode, start, end, locked_folio, > > + extent_clear_unlock_delalloc(inode, start + cur_alloc_size, > > + end, locked_folio, > > &cached, clear_bits, page_ops); > > - btrfs_qgroup_free_data(inode, NULL, start, end - start + 1, NULL); > > + btrfs_qgroup_free_data(inode, NULL, start + cur_alloc_size, > > + end - start - cur_alloc_size + 1, NULL); > > } > > return ret; > > } >
On Wed, Oct 30, 2024 at 01:31:15PM +1030, Qu Wenruo wrote: > > > 在 2024/10/25 17:24, iamhswang@gmail.com 写道: > > From: Haisu Wang <haisuwang@tencent.com> > > > > Simplify the regions mark by using cur_alloc_size only to present > > the reserved but may failed to alloced extent. Remove the ram_size > > as well since it is always consistent to the cur_alloc_size in the > > context. Advanced the start mark in normal path until extent succeed > > alloced and keep the start unchanged in error handling path. > > > > PASSed the fstest generic/475 test for a hundred times with quota > > enabled. And a modified generic/475 test by removing the sleep time > > for a hundred times. About one tenth of the tests do enter the error > > handling path due to fail to reserve extent. > > > > Although this patch is already merged into for-next, it looks like the > next patch will again change the error handling, mostly render the this > one useless: > > https://lore.kernel.org/linux-btrfs/2a0925f0264daf90741ed0a7ba7ed4b4888cf778.1728725060.git.wqu@suse.com/ > > The newer patch will change the error handling to a simpler one, so > instead of 3 regions, there will be only 2. > > There will be no change needed from your side, I will update my patches > to solve the conflicts, just in case if you find the error handling is > different in the future. Please take care of that, the only request I have is that it's done by the end of this week so we have the code in linux-next and that a fix should come before a refactoring (due to backports). Update for-next as you need.
在 2024/10/31 01:58, David Sterba 写道: > On Wed, Oct 30, 2024 at 01:31:15PM +1030, Qu Wenruo wrote: >> >> >> 在 2024/10/25 17:24, iamhswang@gmail.com 写道: >>> From: Haisu Wang <haisuwang@tencent.com> >>> >>> Simplify the regions mark by using cur_alloc_size only to present >>> the reserved but may failed to alloced extent. Remove the ram_size >>> as well since it is always consistent to the cur_alloc_size in the >>> context. Advanced the start mark in normal path until extent succeed >>> alloced and keep the start unchanged in error handling path. >>> >>> PASSed the fstest generic/475 test for a hundred times with quota >>> enabled. And a modified generic/475 test by removing the sleep time >>> for a hundred times. About one tenth of the tests do enter the error >>> handling path due to fail to reserve extent. >>> >> >> Although this patch is already merged into for-next, it looks like the >> next patch will again change the error handling, mostly render the this >> one useless: >> >> https://lore.kernel.org/linux-btrfs/2a0925f0264daf90741ed0a7ba7ed4b4888cf778.1728725060.git.wqu@suse.com/ >> >> The newer patch will change the error handling to a simpler one, so >> instead of 3 regions, there will be only 2. >> >> There will be no change needed from your side, I will update my patches >> to solve the conflicts, just in case if you find the error handling is >> different in the future. > > Please take care of that, the only request I have is that it's done by > the end of this week so we have the code in linux-next and that a fix > should come before a refactoring (due to backports). Update for-next as > you need. Then everything is done. The patch itself is not touched and already in for-next branch. The new one is part of the subpage enhancement series now, which is not that urgent. The subpage compression write support is already large enough for the next release cycle. Thanks, Qu
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c index 3646734a7e59..7e67a6d50be2 100644 --- a/fs/btrfs/inode.c +++ b/fs/btrfs/inode.c @@ -1359,7 +1359,6 @@ static noinline int cow_file_range(struct btrfs_inode *inode, u64 alloc_hint = 0; u64 orig_start = start; u64 num_bytes; - unsigned long ram_size; u64 cur_alloc_size = 0; u64 min_alloc_size; u64 blocksize = fs_info->sectorsize; @@ -1367,7 +1366,6 @@ static noinline int cow_file_range(struct btrfs_inode *inode, struct extent_map *em; unsigned clear_bits; unsigned long page_ops; - bool extent_reserved = false; int ret = 0; if (btrfs_is_free_space_inode(inode)) { @@ -1421,8 +1419,7 @@ static noinline int cow_file_range(struct btrfs_inode *inode, struct btrfs_ordered_extent *ordered; struct btrfs_file_extent file_extent; - cur_alloc_size = num_bytes; - ret = btrfs_reserve_extent(root, cur_alloc_size, cur_alloc_size, + ret = btrfs_reserve_extent(root, num_bytes, num_bytes, min_alloc_size, 0, alloc_hint, &ins, 1, 1); if (ret == -EAGAIN) { @@ -1453,9 +1450,7 @@ static noinline int cow_file_range(struct btrfs_inode *inode, if (ret < 0) goto out_unlock; cur_alloc_size = ins.offset; - extent_reserved = true; - ram_size = ins.offset; file_extent.disk_bytenr = ins.objectid; file_extent.disk_num_bytes = ins.offset; file_extent.num_bytes = ins.offset; @@ -1463,14 +1458,14 @@ static noinline int cow_file_range(struct btrfs_inode *inode, file_extent.offset = 0; file_extent.compression = BTRFS_COMPRESS_NONE; - lock_extent(&inode->io_tree, start, start + ram_size - 1, + lock_extent(&inode->io_tree, start, start + cur_alloc_size - 1, &cached); em = btrfs_create_io_em(inode, start, &file_extent, BTRFS_ORDERED_REGULAR); if (IS_ERR(em)) { unlock_extent(&inode->io_tree, start, - start + ram_size - 1, &cached); + start + cur_alloc_size - 1, &cached); ret = PTR_ERR(em); goto out_reserve; } @@ -1480,7 +1475,7 @@ static noinline int cow_file_range(struct btrfs_inode *inode, 1 << BTRFS_ORDERED_REGULAR); if (IS_ERR(ordered)) { unlock_extent(&inode->io_tree, start, - start + ram_size - 1, &cached); + start + cur_alloc_size - 1, &cached); ret = PTR_ERR(ordered); goto out_drop_extent_cache; } @@ -1501,7 +1496,7 @@ static noinline int cow_file_range(struct btrfs_inode *inode, */ if (ret) btrfs_drop_extent_map_range(inode, start, - start + ram_size - 1, + start + cur_alloc_size - 1, false); } btrfs_put_ordered_extent(ordered); @@ -1519,7 +1514,7 @@ static noinline int cow_file_range(struct btrfs_inode *inode, page_ops = (keep_locked ? 0 : PAGE_UNLOCK); page_ops |= PAGE_SET_ORDERED; - extent_clear_unlock_delalloc(inode, start, start + ram_size - 1, + extent_clear_unlock_delalloc(inode, start, start + cur_alloc_size - 1, locked_folio, &cached, EXTENT_LOCKED | EXTENT_DELALLOC, page_ops); @@ -1529,7 +1524,7 @@ static noinline int cow_file_range(struct btrfs_inode *inode, num_bytes -= cur_alloc_size; alloc_hint = ins.objectid + ins.offset; start += cur_alloc_size; - extent_reserved = false; + cur_alloc_size = 0; /* * btrfs_reloc_clone_csums() error, since start is increased @@ -1545,7 +1540,7 @@ static noinline int cow_file_range(struct btrfs_inode *inode, return ret; out_drop_extent_cache: - btrfs_drop_extent_map_range(inode, start, start + ram_size - 1, false); + btrfs_drop_extent_map_range(inode, start, start + cur_alloc_size - 1, false); out_reserve: btrfs_dec_block_group_reservations(fs_info, ins.objectid); btrfs_free_reserved_extent(fs_info, ins.objectid, ins.offset, 1); @@ -1599,13 +1594,12 @@ static noinline int cow_file_range(struct btrfs_inode *inode, * to decrement again the data space_info's bytes_may_use counter, * therefore we do not pass it the flag EXTENT_CLEAR_DATA_RESV. */ - if (extent_reserved) { + if (cur_alloc_size) { extent_clear_unlock_delalloc(inode, start, start + cur_alloc_size - 1, locked_folio, &cached, clear_bits, page_ops); btrfs_qgroup_free_data(inode, NULL, start, cur_alloc_size, NULL); - start += cur_alloc_size; } /* @@ -1614,11 +1608,13 @@ static noinline int cow_file_range(struct btrfs_inode *inode, * space_info's bytes_may_use counter, reserved in * btrfs_check_data_free_space(). */ - if (start < end) { + if (start + cur_alloc_size < end) { clear_bits |= EXTENT_CLEAR_DATA_RESV; - extent_clear_unlock_delalloc(inode, start, end, locked_folio, + extent_clear_unlock_delalloc(inode, start + cur_alloc_size, + end, locked_folio, &cached, clear_bits, page_ops); - btrfs_qgroup_free_data(inode, NULL, start, end - start + 1, NULL); + btrfs_qgroup_free_data(inode, NULL, start + cur_alloc_size, + end - start - cur_alloc_size + 1, NULL); } return ret; }