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