From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mout.gmx.net ([212.227.17.22]:54933 "EHLO mout.gmx.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932407AbeFKHlE (ORCPT ); Mon, 11 Jun 2018 03:41:04 -0400 Subject: Re: [PATCH 02/15] btrfs-progs: Remove root argument from btrfs_del_csums To: Nikolay Borisov , linux-btrfs@vger.kernel.org References: <1528462078-24490-1-git-send-email-nborisov@suse.com> <1528462078-24490-3-git-send-email-nborisov@suse.com> <15794890-c9d4-a2da-d507-8836407c4bae@gmx.com> <5cf49c2c-15c6-762a-f0a0-156b309c7b03@suse.com> From: Qu Wenruo Message-ID: <6a63bd0b-83b0-b9e1-059d-255ec331cf59@gmx.com> Date: Mon, 11 Jun 2018 15:40:50 +0800 MIME-Version: 1.0 In-Reply-To: <5cf49c2c-15c6-762a-f0a0-156b309c7b03@suse.com> Content-Type: text/plain; charset=utf-8 Sender: linux-btrfs-owner@vger.kernel.org List-ID: On 2018年06月11日 15:02, Nikolay Borisov wrote: > > > On 11.06.2018 07:46, Qu Wenruo wrote: >> >> >> On 2018年06月08日 20:47, Nikolay Borisov wrote: >>> It's not needed, since we can obtain a reference to fs_info from the >>> passed transaction handle. This is needed by delayed refs code. >> >> This looks a little too aggressive to me. >> >> Normally we would expect parameters like @trans then @fs_info. >> Although @trans only let us get @fs_info, under certain case @trans >> could be NULL (like btrfs_search_slot). >> >> Although for btrfs_del_csums() @tran can be NULL, I still prefer the >> @trans, @fs_info parameters. > > The reason I'm making those prep patches i because I would like to get > rid of the root argument from _free_extent, For this part, I completely agree with you. The root doesn't make sense, getting rid of it is completely fine. The abuse of btrfs_root as parameter should be stopped, and we have tons of such cleanup both in kernel and btrfs-progs. The only concern is about possible NULL @trans parameter. However it could be easily addressed by adding 'restrict' prefix. So just adding 'restrict', and then the whole cleanup part is fine to me. Thanks, Qu > since if you look at the > later delayed refs patches - patch 12/15 you'd see that the adapter > function __free_extent2 doesn't really have a root pointer as an > argument. If you also take a look at 10/15 (add delayed refs > infrastructure), the functions which create the delayed refs also don't > take a struct btrfs_root as an argument. > > Alternatively I can perhaps wire up a call to btrfs_read_fs_root in > __free_extent which will be using the "ref->root" id to obtain struct > btrfs_root. However, due to the way FST fixup is implemented (I still > haven't sent that code) it's possible that we call __free_extent2 with > the freespace root set to 0 so if btrfs_read_fs_root is called it will > return an -ENOENT. > > In conclusion what you suggest *could* be done but it will require > re-engineering the currently-tested FST code + delayed refs code. Given > the purpose of the root argument in this callchain I'd rather eliminate > it altogether and not bother with it. > >> >> Thanks, >> Qu >> >> >>> >>> Signed-off-by: Nikolay Borisov >>> --- >>> btrfs-corrupt-block.c | 2 +- >>> ctree.h | 3 +-- >>> extent-tree.c | 2 +- >>> file-item.c | 20 ++++++++++---------- >>> 4 files changed, 13 insertions(+), 14 deletions(-) >>> >>> diff --git a/btrfs-corrupt-block.c b/btrfs-corrupt-block.c >>> index 4fbea26cda20..3add8e63b7bb 100644 >>> --- a/btrfs-corrupt-block.c >>> +++ b/btrfs-corrupt-block.c >>> @@ -926,7 +926,7 @@ static int delete_csum(struct btrfs_root *root, u64 bytenr, u64 bytes) >>> return PTR_ERR(trans); >>> } >>> >>> - ret = btrfs_del_csums(trans, root, bytenr, bytes); >>> + ret = btrfs_del_csums(trans, bytenr, bytes); >>> if (ret) >>> fprintf(stderr, "Error deleting csums %d\n", ret); >>> btrfs_commit_transaction(trans, root); >>> diff --git a/ctree.h b/ctree.h >>> index de4b1b7e6416..082726238b91 100644 >>> --- a/ctree.h >>> +++ b/ctree.h >>> @@ -2752,8 +2752,7 @@ int btrfs_del_inode_ref(struct btrfs_trans_handle *trans, >>> u64 ino, u64 parent_ino, u64 *index); >>> >>> /* file-item.c */ >>> -int btrfs_del_csums(struct btrfs_trans_handle *trans, >>> - struct btrfs_root *root, u64 bytenr, u64 len); >>> +int btrfs_del_csums(struct btrfs_trans_handle *trans, u64 bytenr, u64 len); >>> int btrfs_insert_file_extent(struct btrfs_trans_handle *trans, >>> struct btrfs_root *root, >>> u64 objectid, u64 pos, u64 offset, >>> diff --git a/extent-tree.c b/extent-tree.c >>> index cbc022f6cef6..c6f09b52800f 100644 >>> --- a/extent-tree.c >>> +++ b/extent-tree.c >>> @@ -2372,7 +2372,7 @@ static int __free_extent(struct btrfs_trans_handle *trans, >>> btrfs_release_path(path); >>> >>> if (is_data) { >>> - ret = btrfs_del_csums(trans, root, bytenr, num_bytes); >>> + ret = btrfs_del_csums(trans, bytenr, num_bytes); >>> BUG_ON(ret); >>> } >>> >>> diff --git a/file-item.c b/file-item.c >>> index 7b0ff3585509..71d4e89f78d1 100644 >>> --- a/file-item.c >>> +++ b/file-item.c >>> @@ -394,8 +394,7 @@ static noinline int truncate_one_csum(struct btrfs_root *root, >>> * deletes the csum items from the csum tree for a given >>> * range of bytes. >>> */ >>> -int btrfs_del_csums(struct btrfs_trans_handle *trans, >>> - struct btrfs_root *root, u64 bytenr, u64 len) >>> +int btrfs_del_csums(struct btrfs_trans_handle *trans, u64 bytenr, u64 len) >>> { >>> struct btrfs_path *path; >>> struct btrfs_key key; >>> @@ -403,11 +402,10 @@ int btrfs_del_csums(struct btrfs_trans_handle *trans, >>> u64 csum_end; >>> struct extent_buffer *leaf; >>> int ret; >>> - u16 csum_size = >>> - btrfs_super_csum_size(root->fs_info->super_copy); >>> - int blocksize = root->fs_info->sectorsize; >>> + u16 csum_size = btrfs_super_csum_size(trans->fs_info->super_copy); >>> + int blocksize = trans->fs_info->sectorsize; >>> + struct btrfs_root *csum_root = trans->fs_info->csum_root; >>> >>> - root = root->fs_info->csum_root; >>> >>> path = btrfs_alloc_path(); >>> if (!path) >>> @@ -418,7 +416,7 @@ int btrfs_del_csums(struct btrfs_trans_handle *trans, >>> key.offset = end_byte - 1; >>> key.type = BTRFS_EXTENT_CSUM_KEY; >>> >>> - ret = btrfs_search_slot(trans, root, &key, path, -1, 1); >>> + ret = btrfs_search_slot(trans, csum_root, &key, path, -1, 1); >>> if (ret > 0) { >>> if (path->slots[0] == 0) >>> goto out; >>> @@ -445,7 +443,7 @@ int btrfs_del_csums(struct btrfs_trans_handle *trans, >>> >>> /* delete the entire item, it is inside our range */ >>> if (key.offset >= bytenr && csum_end <= end_byte) { >>> - ret = btrfs_del_item(trans, root, path); >>> + ret = btrfs_del_item(trans, csum_root, path); >>> BUG_ON(ret); >>> } else if (key.offset < bytenr && csum_end > end_byte) { >>> unsigned long offset; >>> @@ -485,12 +483,14 @@ int btrfs_del_csums(struct btrfs_trans_handle *trans, >>> * btrfs_split_item returns -EAGAIN when the >>> * item changed size or key >>> */ >>> - ret = btrfs_split_item(trans, root, path, &key, offset); >>> + ret = btrfs_split_item(trans, csum_root, path, &key, >>> + offset); >>> BUG_ON(ret && ret != -EAGAIN); >>> >>> key.offset = end_byte - 1; >>> } else { >>> - ret = truncate_one_csum(root, path, &key, bytenr, len); >>> + ret = truncate_one_csum(csum_root, path, &key, bytenr, >>> + len); >>> BUG_ON(ret); >>> } >>> btrfs_release_path(path); >>> >> -- >> 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 >>