| Message ID | 20180804131057.9967-3-lufq.fnst@cn.fujitsu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | some trivial cleanup about btrfs_delete_subvolume | expand |
On Sat, Aug 04, 2018 at 09:10:54PM +0800, Lu Fengqi wrote: > After btrfs_qgroup_reserve_meta_prealloc(), num_bytes will be assigned > again by btrfs_calc_trans_metadata_size(). Therefore, once block_rsv > fails, we cannot properly free the num_bytes of the previous > qgroup_reserve. This does not look like a trivial cleanup at all. There was an unused parameter, removed in c4c129db5da8f070147f175 ("btrfs: drop unused parameter qgroup_reserved"), that introduced the bug. This was in this rc1 so it's a regression and I'll consider pushing it to the 4.18 final. -- 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
On Tue, Aug 07, 2018 at 06:19:12PM +0200, David Sterba wrote: >On Sat, Aug 04, 2018 at 09:10:54PM +0800, Lu Fengqi wrote: >> After btrfs_qgroup_reserve_meta_prealloc(), num_bytes will be assigned >> again by btrfs_calc_trans_metadata_size(). Therefore, once block_rsv >> fails, we cannot properly free the num_bytes of the previous >> qgroup_reserve. > >This does not look like a trivial cleanup at all. There was an unused >parameter, removed in c4c129db5da8f070147f175 ("btrfs: drop unused >parameter qgroup_reserved"), that introduced the bug. This was in this >rc1 so it's a regression and I'll consider pushing it to the 4.18 final. > > I apologize for the inconvenience. I should add the Fixes tag, and really shouldn't mix it into the trivial cleanup patch set.
On Wed, Aug 08, 2018 at 11:04:37AM +0800, Lu Fengqi wrote: > On Tue, Aug 07, 2018 at 06:19:12PM +0200, David Sterba wrote: > >On Sat, Aug 04, 2018 at 09:10:54PM +0800, Lu Fengqi wrote: > >> After btrfs_qgroup_reserve_meta_prealloc(), num_bytes will be assigned > >> again by btrfs_calc_trans_metadata_size(). Therefore, once block_rsv > >> fails, we cannot properly free the num_bytes of the previous > >> qgroup_reserve. > > > >This does not look like a trivial cleanup at all. There was an unused > >parameter, removed in c4c129db5da8f070147f175 ("btrfs: drop unused > >parameter qgroup_reserved"), that introduced the bug. This was in this > >rc1 so it's a regression and I'll consider pushing it to the 4.18 final. > > I apologize for the inconvenience. I should add the Fixes tag, and really > shouldn't mix it into the trivial cleanup patch set. As the bug does not qualify as urgent regression, I'm not going to forward it to 4.18. Please update the subject and changelog so it reflects that's an actual fix. I'll add it to the 4.19 queue then. Thanks. -- 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 <dsterba@suse.cz> 于2018年8月8日周三 下午9:57写道： > > On Wed, Aug 08, 2018 at 11:04:37AM +0800, Lu Fengqi wrote: > > On Tue, Aug 07, 2018 at 06:19:12PM +0200, David Sterba wrote: > > >On Sat, Aug 04, 2018 at 09:10:54PM +0800, Lu Fengqi wrote: > > >> After btrfs_qgroup_reserve_meta_prealloc(), num_bytes will be assigned > > >> again by btrfs_calc_trans_metadata_size(). Therefore, once block_rsv > > >> fails, we cannot properly free the num_bytes of the previous > > >> qgroup_reserve. > > > > > >This does not look like a trivial cleanup at all. There was an unused > > >parameter, removed in c4c129db5da8f070147f175 ("btrfs: drop unused > > >parameter qgroup_reserved"), that introduced the bug. This was in this > > >rc1 so it's a regression and I'll consider pushing it to the 4.18 final. > > > > I apologize for the inconvenience. I should add the Fixes tag, and really > > shouldn't mix it into the trivial cleanup patch set. > > As the bug does not qualify as urgent regression, I'm not going to > forward it to 4.18. Please update the subject and changelog so it > reflects that's an actual fix. I'll add it to the 4.19 queue then. > Thanks. No problem. I will send it tomorrow. ------------------------------------------------------------- Thanks, Lu > -- > 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
diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c index de6f75f5547b..8b009ae964cc 100644 --- a/fs/btrfs/extent-tree.c +++ b/fs/btrfs/extent-tree.c @@ -5800,7 +5800,7 @@ void btrfs_trans_release_chunk_metadata(struct btrfs_trans_handle *trans) * root: the root of the parent directory * rsv: block reservation * items: the number of items that we need do reservation - * qgroup_reserved: used to return the reserved size in qgroup + * use_global_rsv: allow fallback to the global block reservation * * This function is used to reserve the space for snapshot/subvolume * creation and deletion. Those operations are different with the @@ -5810,10 +5810,10 @@ void btrfs_trans_release_chunk_metadata(struct btrfs_trans_handle *trans) * the space reservation mechanism in start_transaction(). */ int btrfs_subvolume_reserve_metadata(struct btrfs_root *root, - struct btrfs_block_rsv *rsv, - int items, + struct btrfs_block_rsv *rsv, int items, bool use_global_rsv) { + u64 qgroup_num_bytes = 0; u64 num_bytes; int ret; struct btrfs_fs_info *fs_info = root->fs_info; @@ -5821,12 +5821,10 @@ int btrfs_subvolume_reserve_metadata(struct btrfs_root *root, if (test_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags)) { /* One for parent inode, two for dir entries */ - num_bytes = 3 * fs_info->nodesize; - ret = btrfs_qgroup_reserve_meta_prealloc(root, num_bytes, true); + qgroup_num_bytes = 3 * fs_info->nodesize; + ret = btrfs_qgroup_reserve_meta_prealloc(root, qgroup_num_bytes, true); if (ret) return ret; - } else { - num_bytes = 0; } num_bytes = btrfs_calc_trans_metadata_size(fs_info, items); @@ -5838,8 +5836,8 @@ int btrfs_subvolume_reserve_metadata(struct btrfs_root *root, if (ret == -ENOSPC && use_global_rsv) ret = btrfs_block_rsv_migrate(global_rsv, rsv, num_bytes, 1); - if (ret && num_bytes) - btrfs_qgroup_free_meta_prealloc(root, num_bytes); + if (ret && qgroup_num_bytes) + btrfs_qgroup_free_meta_prealloc(root, qgroup_num_bytes); return ret; }
After btrfs_qgroup_reserve_meta_prealloc(), num_bytes will be assigned again by btrfs_calc_trans_metadata_size(). Therefore, once block_rsv fails, we cannot properly free the num_bytes of the previous qgroup_reserve. Delete the comment for the qgroup_reserved that does not exist and add a comment about use_global_rsv. Signed-off-by: Lu Fengqi <lufq.fnst@cn.fujitsu.com> --- fs/btrfs/extent-tree.c | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-)