Re: [PATCH 1/6] btrfs: qgroup: Skeleton to support separate qgroup reservation type

[Jeff Mahoney <jeffm@suse.com>]

[-- Attachment #1.1: Type: text/plain, Size: 7346 bytes --]

On 10/24/17 7:51 AM, Qu Wenruo wrote:
> 
> 
> On 2017年10月24日 19:00, Nikolay Borisov wrote:
>>
>>
>> On 24.10.2017 11:39, Qu Wenruo wrote:
>>> Instead of single qgroup->reserved, use a new structure btrfs_qgroup_rsv
>>> to restore different types of reservation.
>>>
>>> This patch only updates the header and needed modification to pass
>>> compile.
>>>
>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>> ---
>>>  fs/btrfs/qgroup.c | 16 ++++++++++------
>>>  fs/btrfs/qgroup.h | 27 +++++++++++++++++++++++++--
>>>  2 files changed, 35 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
>>> index e172d4843eae..fe3adb996883 100644
>>> --- a/fs/btrfs/qgroup.c
>>> +++ b/fs/btrfs/qgroup.c
>>> @@ -2444,7 +2444,8 @@ static int qgroup_reserve(struct btrfs_root *root, u64 num_bytes, bool enforce)
>>>  }
>>>  
>>>  void btrfs_qgroup_free_refroot(struct btrfs_fs_info *fs_info,
>>> -			       u64 ref_root, u64 num_bytes)
>>> +			       u64 ref_root, u64 num_bytes,
>>> +			       enum btrfs_qgroup_rsv_type type)
>>>  {
>>>  	struct btrfs_root *quota_root;
>>>  	struct btrfs_qgroup *qgroup;
>>> @@ -2936,7 +2937,8 @@ static int qgroup_free_reserved_data(struct inode *inode,
>>>  			goto out;
>>>  		freed += changeset.bytes_changed;
>>>  	}
>>> -	btrfs_qgroup_free_refroot(root->fs_info, root->objectid, freed);
>>> +	btrfs_qgroup_free_refroot(root->fs_info, root->objectid, freed,
>>> +				  BTRFS_QGROUP_RSV_DATA);
>>>  	ret = freed;
>>>  out:
>>>  	extent_changeset_release(&changeset);
>>> @@ -2968,7 +2970,7 @@ static int __btrfs_qgroup_release_data(struct inode *inode,
>>>  	if (free)
>>>  		btrfs_qgroup_free_refroot(BTRFS_I(inode)->root->fs_info,
>>>  				BTRFS_I(inode)->root->objectid,
>>> -				changeset.bytes_changed);
>>> +				changeset.bytes_changed, BTRFS_QGROUP_RSV_DATA);
>>>  	ret = changeset.bytes_changed;
>>>  out:
>>>  	extent_changeset_release(&changeset);
>>> @@ -3045,7 +3047,8 @@ void btrfs_qgroup_free_meta_all(struct btrfs_root *root)
>>>  	if (reserved == 0)
>>>  		return;
>>>  	trace_qgroup_meta_reserve(root, -(s64)reserved);
>>> -	btrfs_qgroup_free_refroot(fs_info, root->objectid, reserved);
>>> +	btrfs_qgroup_free_refroot(fs_info, root->objectid, reserved,
>>> +				  BTRFS_QGROUP_RSV_META);
>>>  }
>>>  
>>>  void btrfs_qgroup_free_meta(struct btrfs_root *root, int num_bytes)
>>> @@ -3060,7 +3063,8 @@ void btrfs_qgroup_free_meta(struct btrfs_root *root, int num_bytes)
>>>  	WARN_ON(atomic64_read(&root->qgroup_meta_rsv) < num_bytes);
>>>  	atomic64_sub(num_bytes, &root->qgroup_meta_rsv);
>>>  	trace_qgroup_meta_reserve(root, -(s64)num_bytes);
>>> -	btrfs_qgroup_free_refroot(fs_info, root->objectid, num_bytes);
>>> +	btrfs_qgroup_free_refroot(fs_info, root->objectid, num_bytes,
>>> +				  BTRFS_QGROUP_RSV_META);
>>>  }
>>>  
>>>  /*
>>> @@ -3088,7 +3092,7 @@ void btrfs_qgroup_check_reserved_leak(struct inode *inode)
>>>  		}
>>>  		btrfs_qgroup_free_refroot(BTRFS_I(inode)->root->fs_info,
>>>  				BTRFS_I(inode)->root->objectid,
>>> -				changeset.bytes_changed);
>>> +				changeset.bytes_changed, BTRFS_QGROUP_RSV_DATA);
>>>  
>>>  	}
>>>  	extent_changeset_release(&changeset);
>>> diff --git a/fs/btrfs/qgroup.h b/fs/btrfs/qgroup.h
>>> index d9984e87cddf..0b04cbc5b5ce 100644
>>> --- a/fs/btrfs/qgroup.h
>>> +++ b/fs/btrfs/qgroup.h
>>> @@ -61,6 +61,26 @@ struct btrfs_qgroup_extent_record {
>>>  	struct ulist *old_roots;
>>>  };
>>>  
>>> +enum btrfs_qgroup_rsv_type {
>>> +	BTRFS_QGROUP_RSV_DATA = 0,
>>> +	BTRFS_QGROUP_RSV_META = 1,
>>> +	BTRFS_QGROUP_RSV_TYPES = 1,
>>
>> nit: Why not BTRFS_QGROUP_RSV_TYPES_MAX = 2;
> 
> My original plan is just as the same as yours.
> 
> However I still remember I did it before and David fixed it by using
> TYPES, so I follow his naming schema here.
> 
> Kernel is also using this naming schema else where:
> d91876496bcf ("btrfs: compress: put variables defined per compress type
> in struct to make cache friendly")

The COMPRESS_TYPES pattern isn't the right pattern to follow here.
That's a special case since there's a _NONE that doesn't have anything
associated with it, so we don't need to take a slot in the array.

We also don't care about any of the specific values, just that they
start at 0.  The BTRFS_COMPRESS_TYPES example also has a
BTRFS_COMPRESS_LAST item in the enum, which serves the same purpose as
MAX.  I don't have a strong opinion on the naming, just that we don't
play games with +1 when handling arrays since, as you say, that's just
waiting for subtle bugs later.

enum btrfs_qgroup_rsv_type {
	BTRFS_QGROUP_RSV_DATA = 0,
	BTRFS_QGROUP_RSV_META,
	BTRFS_QGROUP_RSV_LAST,
};

>>> +};
>>> +
>>> +/*
>>> + * Represents how many bytes we reserved for this qgroup.
>>> + *
>>> + * Each type should have different reservation behavior.
>>> + * E.g, data follows its io_tree flag modification, while
>>> + * *currently* meta is just reserve-and-clear during transcation.
>>> + *
>>> + * TODO: Add new type for delalloc, which can exist across several
>>> + * transaction.
>>> + */

Minor nit: It's not just delalloc.  Delayed items and inodes can as
well.  The general rule is that qgroup reservations aren't essentially
different from block reservations and follow the same usage patterns
when operating on leaf nodes.

>>> +struct btrfs_qgroup_rsv {
>>> +	u64 values[BTRFS_QGROUP_RSV_TYPES + 1];
>>
>> nit: And here just BTRFS_QGROUP_RSV_TYPES_MAX rather than the +1 here,
>> seems more idiomatic to me.
> 
> To follow same naming schema from David.
> (IIRC it was about tree-checker patchset, checking file extent type part)
> 
> In fact, I crashed kernel several times due to the tiny +1, without even
> a clue for hours just testing blindly, until latest gcc gives warning
> about it.

BTRFS_QGROUP_RSV_LAST would do the job here.

-Jeff

>>
>>> +};
>>> +
>>>  /*
>>>   * one struct for each qgroup, organized in fs_info->qgroup_tree.
>>>   */
>>> @@ -88,6 +108,7 @@ struct btrfs_qgroup {
>>>  	 * reservation tracking
>>>  	 */
>>>  	u64 reserved;
>>> +	struct btrfs_qgroup_rsv rsv;
>>>  
>>>  	/*
>>>  	 * lists
>>> @@ -228,12 +249,14 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans,
>>>  			 struct btrfs_fs_info *fs_info, u64 srcid, u64 objectid,
>>>  			 struct btrfs_qgroup_inherit *inherit);
>>>  void btrfs_qgroup_free_refroot(struct btrfs_fs_info *fs_info,
>>> -			       u64 ref_root, u64 num_bytes);
>>> +			       u64 ref_root, u64 num_bytes,
>>> +			       enum btrfs_qgroup_rsv_type type);
>>>  static inline void btrfs_qgroup_free_delayed_ref(struct btrfs_fs_info *fs_info,
>>>  						 u64 ref_root, u64 num_bytes)
>>>  {
>>>  	trace_btrfs_qgroup_free_delayed_ref(fs_info, ref_root, num_bytes);
>>> -	btrfs_qgroup_free_refroot(fs_info, ref_root, num_bytes);
>>> +	btrfs_qgroup_free_refroot(fs_info, ref_root, num_bytes,
>>> +				  BTRFS_QGROUP_RSV_DATA);
>>>  }
>>>  
>>>  #ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS
>>>
>> --
>> 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
>>
> 


-- 
Jeff Mahoney
SUSE Labs


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]