On 11.06.2018 10:40, Qu Wenruo wrote:
>
>
> 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.
you mean making it "struct btrfs_trans_handle * restrict trans". But
restrict deals with pointer aliasing it doesn't have any bearing on the
NULL-ability of trans. I'm afraid I don't follow your logic here, care
to explain further?
>
> 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 <nborisov@xxxxxxxx>
>>>> ---
>>>> 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@xxxxxxxxxxxxxxx
>>> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
