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 <mfas...@suse.de>
>>>>> Signed-off-by: Qu Wenruo <quwen...@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?
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?
>
>>
>>>
>>> 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.
<snipped>
--
Goldwyn
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
