Re: [PATCH 1/8] btrfs: delayed-ref: Introduce better documented delayed ref structures

[Nikolay Borisov <nborisov@suse.com>]

On 10.12.18 г. 13:20 ч., Qu Wenruo wrote:
> 
> 
> On 2018/12/10 下午5:48, Nikolay Borisov wrote:
>>
>>
>> On 6.12.18 г. 8:58 ч., Qu Wenruo wrote:
>>> Current delayed ref interface has several problems:
>>> - Longer and longer parameter lists
>>>   bytenr
>>>   num_bytes
>>>   parent
>>>   ref_root
>>>   owner
>>>   offset
>>>   for_reloc << Only qgroup code cares.
>>>

<snip>

>>> +struct btrfs_data_ref {
>>> +	/*
>>> +	 * For EXTENT_DATA_REF
>>> +	 *
>>> +	 * @ref_root:	current owner of the extent.
>>> +	 * 		may differ from btrfs_ref::real_root.
>>
>> Here 'owner' is a bit ambiguous. Isn't this the root of the owner
> 
> Well, the @ref_root is a little confusing.
> Here it could mean "the current btrfs_header_owner(eb)".
> 
> I don't really know the correct term here.

My point was that the word owner is being overloaded. I guess the whole
sentence can be rephrased as:

"The root this reference originates from. For cases where ref_root
differs from btrfs_ref::real_root consult btrfs_ref comments".  Or
something along those lines.

> 
>>
>>> +	 * @ino: 	inode number of the owner. 
>>
>> And the owner is really the owning inode?
> 
> Yes, the owning inode.
> 
>>
>>> +	 * @offset:	*CALCULATED* offset. Not EXTENT_DATA key offset.
>>
>> What does calculated mean here?
> 
> Because btrfs data backref instead of using the offset of the owning
> EXTENT_DATA key, it uses key->offset - extent_offset.

There's no way anyone reading: *CALCULATED* offset could infer that.
Please reword. Even this sentence is a bit terse. When writing
documentation assume people who are going to read it will have *very*
little knowledge of the internal structure of the code.

> 
>>
>>> +	 *
>>> +	 */
>>
>> Ugh, this is ugly, why not have a single /* */ line above each member
>> and document them like that?
> 
> Because the complexity of @ref_root can't really go one line.

What I meant is to have the documentation for every member right above
the respective member, rather than having the documentation in one
place, followed by the variables. If it takes more than a single line to
document a member so be it.

<snip>

>>> +	/*
>>> +	 * Use full backref(SHARED_BLOCK_REF or SHARED_DATA_REF) for this
>>> +	 * extent and its children.
>>> +	 * Set for reloc trees.
>>> +	 */
>>> +	unsigned int use_fullback:1;
>>
>> 'fullback' is too terse, use_backreferences or something conveying more
>> information?
> 
> @use_backreferences looks even stranger to me.
> 
> Here the point is, if this bit set, all backref will use
> SHARED_BLOCK_REF or SHARED_DATA_REF subtype, and just leave a pointer to
> its parent.
> 
> Any good idea for explaining this?

What's the alternative if this bit is not set, how would the tree look like?


> 
>> Also please use explicit bool type:
>>
>> bool xxxx:1
> 
> Is this valid? Haven't seen such usage in kernel code IIRC.

git grep 'bool .*:1' | wc -l
417

<snip>
>