Re: [PATCH v2 1/3] btrfs: qgroup: Refactor btrfs_qgroup_insert_dirty_extent()

[Qu Wenruo <quwenruo@cn.fujitsu.com>]

At 08/09/2016 10:24 AM, Goldwyn Rodrigues wrote:
>
>
> On 08/08/2016 08:12 PM, Qu Wenruo wrote:
>>
>>
>> At 08/09/2016 09:01 AM, Goldwyn Rodrigues wrote:
>>>
>>>
>>> On 08/08/2016 07:26 PM, Qu Wenruo wrote:
>>>>
>>>>
>>>> At 08/08/2016 10:54 AM, Goldwyn Rodrigues wrote:
>>>>>
>>>>>
>>>>> On 08/07/2016 07:39 PM, Qu Wenruo wrote:
>>>>>>
>>>>>>
>>>>>> At 08/07/2016 01:19 AM, Goldwyn Rodrigues wrote:
>>>>>>> Thanks Qu,
>>>>>>>
>>>>>>> This patch set fixes all the reported problems. However, I have some
>>>>>>> minor issues with coding style.
>>>>>>>
>>>>>>
>>>>>> Thanks for the test.
>>>>>>
>>>>>>>
>>>>>>> On 08/04/2016 09:29 PM, Qu Wenruo wrote:
>>>>>>>> Refactor btrfs_qgroup_insert_dirty_extent() function, to two
>>>>>>>> functions:
>>>>>>>> 1. _btrfs_qgroup_insert_dirty_extent()
>>>>>>>>    Almost the same with original code.
>>>>>>>>    For delayed_ref usage, which has delayed refs locked.
>>>>>>>>
>>>>>>>>    Change the return value type to int, since caller never needs the
>>>>>>>>    pointer, but only needs to know if they need to free the
>>>>>>>> allocated
>>>>>>>>    memory.
>>>>>>>>
>>>>>>>> 2. btrfs_qgroup_record_dirty_extent()
>>>>>>>>    The more encapsulated version.
>>>>>>>>
>>>>>>>>    Will do the delayed_refs lock, memory allocation, quota enabled
>>>>>>>> check
>>>>>>>>    and other misc things.
>>>>>>>>
>>>>>>>> The original design is to keep exported functions to minimal, but
>>>>>>>> since
>>>>>>>> more btrfs hacks exposed, like replacing path in balance, needs
>>>>>>>> us to
>>>>>>>> record dirty extents manually, so we have to add such functions.
>>>>>>>>
>>>>>>>> Also, add comment for both functions, to info developers how to keep
>>>>>>>> qgroup correct when doing hacks.
>>>>>>>>
>>>>>>>> Cc: Mark Fasheh <mfasheh@suse.de>
>>>>>>>> Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
>>>>>>>> ---
>>>>>>>>  fs/btrfs/delayed-ref.c |  5 +----
>>>>>>>>  fs/btrfs/extent-tree.c | 36 +++++-------------------------------
>>>>>>>>  fs/btrfs/qgroup.c      | 39 ++++++++++++++++++++++++++++++++++-----
>>>>>>>>  fs/btrfs/qgroup.h      | 44
>>>>>>>> +++++++++++++++++++++++++++++++++++++++++---
>>>>>>>>  4 files changed, 81 insertions(+), 43 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/fs/btrfs/delayed-ref.c b/fs/btrfs/delayed-ref.c
>>>>>>>> index 430b368..5eed597 100644
>>>>>>>> --- a/fs/btrfs/delayed-ref.c
>>>>>>>> +++ b/fs/btrfs/delayed-ref.c
>>>>>>>> @@ -541,7 +541,6 @@ add_delayed_ref_head(struct btrfs_fs_info
>>>>>>>> *fs_info,
>>>>>>>>      struct btrfs_delayed_ref_head *existing;
>>>>>>>>      struct btrfs_delayed_ref_head *head_ref = NULL;
>>>>>>>>      struct btrfs_delayed_ref_root *delayed_refs;
>>>>>>>> -    struct btrfs_qgroup_extent_record *qexisting;
>>>>>>>>      int count_mod = 1;
>>>>>>>>      int must_insert_reserved = 0;
>>>>>>>>
>>>>>>>> @@ -606,9 +605,7 @@ add_delayed_ref_head(struct btrfs_fs_info
>>>>>>>> *fs_info,
>>>>>>>>          qrecord->num_bytes = num_bytes;
>>>>>>>>          qrecord->old_roots = NULL;
>>>>>>>>
>>>>>>>> -        qexisting = btrfs_qgroup_insert_dirty_extent(delayed_refs,
>>>>>>>> -                                 qrecord);
>>>>>>>> -        if (qexisting)
>>>>>>>> +        if(_btrfs_qgroup_insert_dirty_extent(delayed_refs,
>>>>>>>> qrecord))
>>>>>>>>              kfree(qrecord);
>>>>>>>>      }
>>>>>>>>
>>>>>>>> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
>>>>>>>> index 9fcb8c9..47c85ff 100644
>>>>>>>> --- a/fs/btrfs/extent-tree.c
>>>>>>>> +++ b/fs/btrfs/extent-tree.c
>>>>>>>> @@ -8519,34 +8519,6 @@ reada:
>>>>>>>>      wc->reada_slot = slot;
>>>>>>>>  }
>>>>>>>>
>>>>>>>> -/*
>>>>>>>> - * These may not be seen by the usual inc/dec ref code so we
>>>>>>>> have to
>>>>>>>> - * add them here.
>>>>>>>> - */
>>>>>>>> -static int record_one_subtree_extent(struct btrfs_trans_handle
>>>>>>>> *trans,
>>>>>>>> -                     struct btrfs_root *root, u64 bytenr,
>>>>>>>> -                     u64 num_bytes)
>>>>>>>> -{
>>>>>>>> -    struct btrfs_qgroup_extent_record *qrecord;
>>>>>>>> -    struct btrfs_delayed_ref_root *delayed_refs;
>>>>>>>> -
>>>>>>>> -    qrecord = kmalloc(sizeof(*qrecord), GFP_NOFS);
>>>>>>>> -    if (!qrecord)
>>>>>>>> -        return -ENOMEM;
>>>>>>>> -
>>>>>>>> -    qrecord->bytenr = bytenr;
>>>>>>>> -    qrecord->num_bytes = num_bytes;
>>>>>>>> -    qrecord->old_roots = NULL;
>>>>>>>> -
>>>>>>>> -    delayed_refs = &trans->transaction->delayed_refs;
>>>>>>>> -    spin_lock(&delayed_refs->lock);
>>>>>>>> -    if (btrfs_qgroup_insert_dirty_extent(delayed_refs, qrecord))
>>>>>>>> -        kfree(qrecord);
>>>>>>>> -    spin_unlock(&delayed_refs->lock);
>>>>>>>> -
>>>>>>>> -    return 0;
>>>>>>>> -}
>>>>>>>> -
>>>>>>>>  static int account_leaf_items(struct btrfs_trans_handle *trans,
>>>>>>>>                    struct btrfs_root *root,
>>>>>>>>                    struct extent_buffer *eb)
>>>>>>>> @@ -8580,7 +8552,8 @@ static int account_leaf_items(struct
>>>>>>>> btrfs_trans_handle *trans,
>>>>>>>>
>>>>>>>>          num_bytes = btrfs_file_extent_disk_num_bytes(eb, fi);
>>>>>>>>
>>>>>>>> -        ret = record_one_subtree_extent(trans, root, bytenr,
>>>>>>>> num_bytes);
>>>>>>>> +        ret = btrfs_qgroup_record_dirty_extent(trans,
>>>>>>>> root->fs_info,
>>>>>>>> +                bytenr, num_bytes, GFP_NOFS);
>>>>>>>>          if (ret)
>>>>>>>>              return ret;
>>>>>>>>      }
>>>>>>>> @@ -8729,8 +8702,9 @@ walk_down:
>>>>>>>>              btrfs_set_lock_blocking_rw(eb, BTRFS_READ_LOCK);
>>>>>>>>              path->locks[level] = BTRFS_READ_LOCK_BLOCKING;
>>>>>>>>
>>>>>>>> -            ret = record_one_subtree_extent(trans, root,
>>>>>>>> child_bytenr,
>>>>>>>> -                            root->nodesize);
>>>>>>>> +            ret = btrfs_qgroup_record_dirty_extent(trans,
>>>>>>>> +                    root->fs_info, child_bytenr,
>>>>>>>> +                    root->nodesize, GFP_NOFS);
>>>>>>>>              if (ret)
>>>>>>>>                  goto out;
>>>>>>>>          }
>>>>>>>> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
>>>>>>>> index 9d4c05b..76d4f67 100644
>>>>>>>> --- a/fs/btrfs/qgroup.c
>>>>>>>> +++ b/fs/btrfs/qgroup.c
>>>>>>>> @@ -1453,9 +1453,9 @@ int
>>>>>>>> btrfs_qgroup_prepare_account_extents(struct
>>>>>>>> btrfs_trans_handle *trans,
>>>>>>>>      return ret;
>>>>>>>>  }
>>>>>>>>
>>>>>>>> -struct btrfs_qgroup_extent_record
>>>>>>>> -*btrfs_qgroup_insert_dirty_extent(struct btrfs_delayed_ref_root
>>>>>>>> *delayed_refs,
>>>>>>>> -                  struct btrfs_qgroup_extent_record *record)
>>>>>>>> +int _btrfs_qgroup_insert_dirty_extent(
>>>>>>>> +        struct btrfs_delayed_ref_root *delayed_refs,
>>>>>>>> +        struct btrfs_qgroup_extent_record *record)
>>>>>>>
>>>>>>> Why do you rename the function preceding with an underscore? just
>>>>>>> leave
>>>>>>> it as it is.
>>>>>>
>>>>>> Because these two functions have different usage.
>>>>>
>>>>> Where is the "other" function used? AFAICS, you are replacing the
>>>>> function name.
>>>>
>>>> See 2nd and 3rd patch.
>>>> Or be more specific, dirty hack fixes, which is the case where we need
>>>> to do manual tracking for qgroup.
>>>
>>> I still don't see it. Can you be more specific with function names to
>>> help me understand?
>>
>> _btrfs_qgroup_insert_dirty_extent() is the real function, which inserts
>> btrfs_qgroup_extent_record structure into dirty_extent_root.
>>
>> btrfs_qgroup_record_dirty_extent() is just the calling above
>> _btrfs_qgroup_insert_dirty_extent() with proper lock content.
>>
>> _btrfs_qgroup_insert_dirty_extent() should only be called in delayed_ref
>> codes, more specifically, add_delayed_ref_head() function.
>>
>> In add_delayed_ref_head(), the function already has already hold the
>> need lock, so _btrfs_qgroup_insert_dirty_extent() don't need to acquire
>> the lock.
>> And normally, that's all needed hook for qgroup.
>>
>> For special dirty hack routine, like merge_reloc_roots() and log replay
>> code, which doesn't go through normal delayed_ref to handle reference
>> modification, we need btrfs_qgroup_record_dirty_extent() function to
>> manually info qgroup to track specific extents.
>>
>> In that case, we need more encapsulated function, so that's
>> btrfs_qgroup_record_dirty_extent().
>>
>>
>>>
>>> All you are doing is adding is btrfs_qgroup_record_dirty_extent() which
>>> calls _btrfs_qgroup_insert_dirty_extent() with the locks. Is that what
>>> you are referring to?
>>
>> Yes.
>>
>>>
>>>
>>>>
>>>>>
>>>>>>
>>>>>> the underscore one are used when caller has already acquired needed
>>>>>> lock, which is used by delayed-refs code.
>>>>>>
>>>>>> And the one without underscore is for other usage.
>>>>>
>>>>> What/Where is the "other usage"? Please don't say "previous".
>>>>>
>>>>
>>>> See above.
>>>>
>>>>>>
>>>>>> Despite the lock, these two functions are all the same.
>>>>>>
>>>>>> So I prefer to use underscore.
>>>>>
>>>>> BTW, you missed include/trace/events/btrfs.h, the reference to
>>>>> btrfs_qgroup_insert_dirty_extent in DEFINE_EVENT.
>>>>>
>>>>
>>>> Since btrfs_qgroup_insert_dirty_extent() is just calling the underscore
>>>> one with proper lock content, so the trace is not affected.
>>>
>>> There is NO btrfs_qgroup_insert_dirty_extent(). Your patches REMOVED it
>>> and replaced it with _btrfs_qgroup_insert_dirty_extent(). Only
>>> btrfs_qgroup_record_dirty_extent() which is not in trace.
>>
>> My fault, I should rename the new function to
>> btrfs_qgroup_insert_dirty_extent(), or rename the old function to
>> _btrfs_qgroup_record_dirty_extent().
>>
>> I think that would reduce the confusion.
>>
>> Or, do you prefer adding a new 'nolock' parameter to the old
>> btrfs_qgroup_insert_dirty_extent()?
>>
>
> Honestly, I don't like the idea of a function name which looks like a
> "wrappee" function (as opposed to a wrapper function.. IOW a function
> preceded with underscores) to be in the header files.
>
> So, if I would to write it, the functions would be called as
> btrfs_qgroup_insert_extent() and btrfs_qgroup_insert_extent_nolock(),
> with former calling the latter. How does that sound?
>
>
Sounds good for me.

I'll update them in next version.

Thanks,
Qu