Re: [PATCH v2] shared/298: Wire btrfs support in get_free_sectors

From: Nikolay Borisov <nborisov@suse.com>
To: Eryu Guan <guaneryu@gmail.com>
Cc: dsterba@suse.cz, fstests@vger.kernel.org, linux-btrfs@vger.kernel.org
Subject: Re: [PATCH v2] shared/298: Wire btrfs support in get_free_sectors
Date: Fri, 22 Feb 2019 08:23:46 +0200	[thread overview]
Message-ID: <d72dcb14-eb87-6173-cf47-0276196b6a72@suse.com> (raw)
In-Reply-To: <20190221171051.GC2713@desktop>

On 21.02.19 г. 19:10 ч., Eryu Guan wrote:
> On Fri, Feb 08, 2019 at 01:44:04PM +0200, Nikolay Borisov wrote:
>> Add support for btrfs in shared/298. Achieve this by introducing 2
>> new awk scripts that parse relevant btrfs structures and print holes.
>> Additionally modify the test to create larger - 3gb filesystem in the
>> case of btrfs. This is needed so that distinct block groups are used
>> for data and metadata.
>>
>> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> 
> Sorry for the late review.. I find that parsing btrfs extent and dev
> tree is very btrfs-specific, it'd be great if btrfs folks could help
> review the two awk scripts as well!
> 
>> ---
>>
>> V2: 
>>  * Changed the way args are passed to mkfs.btrfs to preserve existing 
>>  options, yet override data/metadata profile settings
>>
>>  parse-dev-tree.awk    |  47 +++++++++++++++++++
>>  parse-extent-tree.awk | 125 ++++++++++++++++++++++++++++++++++++++++++++++++++
> 
> I'd prefer placing these files in src dir, instead of dumping them in
> top dir directly.

Ok

> 
>>  tests/shared/298      |  36 +++++++++++++--
>>  3 files changed, 205 insertions(+), 3 deletions(-)
>>  create mode 100755 parse-dev-tree.awk
>>  create mode 100755 parse-extent-tree.awk
>>
>> diff --git a/parse-dev-tree.awk b/parse-dev-tree.awk
>> new file mode 100755
>> index 000000000000..52f9c0aadc25
>> --- /dev/null
>> +++ b/parse-dev-tree.awk
>> @@ -0,0 +1,47 @@
>> +# SPDX-License-Identifier: GPL-2.0
>> +# Copyright (c) 2019 Nikolay Borisov, SUSE LLC.  All Rights Reserved.
>> +#
>> +# Parses btrfs' device tree for holes, required parameters passed on command
> 
> I find this description not very useful, would you please describe the
> expected output and format as well?

Ok will make it a bit more verbose

> 
>> +# line:
>> +#     * spb - how many bytes per sector, this is used so that the numbers 
>            ^^^ This is misleading, in shared/298 spb represents "sector
> 	   per block", but here it's really sector size.

Yeah probably should rename it.

> 
>> +# 		returned by the script are in sectors.
>> +#	  * devsize - size of the device in bytes, used to output the final 
> 
> This line is not aligned with above line, it contains leading tab.
> 
>> +# 		hole (if any)
>> +
>> +function get_extent_size(line,  tmp) {
> 
> Would you please document the expected intput and output in comment as
> well? So it's easier to review.
> 
> Also, is the 'tmp' argument really needed?

tmp in this case is really a function-local variable as per:
https://www.gnu.org/software/gawk/manual/html_node/Variable-Scope.html :

"Unlike in many languages, there is no way to make a variable local to a
{ … } block in awk, but you can make a variable local to a function. It
is good practice to do so whenever a variable is needed only in that
function."

> 
>> +	split(line, tmp)
>> +	return tmp[6]
>> +}
>> +
>> +function get_extent_offset(line,  tmp) {
> 
> Same here.
> 
>> +	split(line, tmp)
>> +	gsub(/\)/,"", tmp[6])
>> +	return tmp[6]
>> +}
>> +
>> +BEGIN {
>> +	dev_extent_match="^.item [0-9]* key \\([0-9]* DEV_EXTENT [0-9]*\\).*"
>> +	dev_extent_len_match="^\\s*chunk_objectid [0-9]* chunk_offset [0-9]* length [0-9]*$"
>> +}
>> +
>> +{
>> +	if (match($0,dev_extent_match)) {
>> +		extent_offset = get_extent_offset($0)		
>> +		if (prev_extent_end) {
>> +			hole_size = extent_offset - prev_extent_end
>> +			if (hole_size > 0) {
>> +				print prev_extent_end / spb, int((extent_offset - 1) / spb)
>> +			}
>> +		} 
>> +	} else if (match($0, dev_extent_len_match)) {
>> +		extent_size = get_extent_size($0)
>> +		prev_extent_end = extent_offset + extent_size
>> +	}
>> +}
>> +
>> +END {
>> +	if (prev_extent_end) {
>> +		print prev_extent_end / spb, int((devsize - 1) / spb)
>> +	}
>> +}
>> +
>> diff --git a/parse-extent-tree.awk b/parse-extent-tree.awk
>> new file mode 100755
>> index 000000000000..01c61254cfef
>> --- /dev/null
>> +++ b/parse-extent-tree.awk
>> @@ -0,0 +1,125 @@
>> +# SPDX-License-Identifier: GPL-2.0
>> +# Copyright (c) 2019 Nikolay Borisov, SUSE LLC.  All Rights Reserved.
>> +#
>> +# Parses btrfs' extent tree for holes, required parameters passed on command
> 
> Same here, please provide more details.
> 
>> +# line:
>> +#     * spb - how many bytes per sector, this is used so that the numbers 
> 
> And replace 'spb' with a more proper variable name.
> 
>> +# 		returned by the script are in sectors.
>> +#	  * nodesize - size of metadata extents, used for internal calculations
> 
> Indention issue too.
> 
>> +
>> +function get_extent_size(line,  tmp) {
>> +	if (line ~ data_match || line ~ bg_match) {
>> +		split(line, tmp)
>> +		gsub(/\)/,"", tmp[6])
>> +		return tmp[6]
>> +	} else if (line ~ metadata_match) {
>> +		return nodesize
>> +	}
>> +}
>> +
>> +function get_extent_offset(line,  tmp) {
>> +	split(line, tmp)
>> +	gsub(/\(/,"",tmp[4])
>> +	return tmp[4]
>> +}
>> +
>> +function print_array(         base_offset, bg_line)
> 
> Document the expected input and output of these functions too.
> 
> And why there're so many spaces before 'base_offset' argument?

Again, those 2 are used as local variables as per early cited documentation.

> 
>> +{
>> +	if (match(lines[0], bg_match)) {
>> +		#we don't have an extent at the beginning of of blockgroup, so we
> 
> Add a space after '#' for comments.

Ok

<snip>

>>
> 