Re: [PATCH] btrfs: tree-checker: annotate all error branches as unlikely

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

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



On 2020/11/21 上午12:10, David Sterba wrote:
> The tree checker is called many times as it verifies metadata at
> read/write time. The checks follow a simple pattern:
> 
>   if (error_condition) {
> 	  report_error();
> 	  return -EUCLEAN;
>   }
> 
> All the error reporting functions are annotated as __cold that is
> supposed to hint the compiler to move the statement block out of the hot
> path. This does not seem to happen that often.
> 
> As the error condition is expected to be false almost always, we can
> annotate it with 'unlikely' as this satisfies one of the few use cases
> for the annotation. The expected outcome is a stronger hint to compiler
> to reorder the checks
> 
>   test
>   jump to exit
>   test
>   jump to exit
>   ...
> 
> which can be observed in asm of eg. check_dir_item,
> btrfs_check_chunk_valid, check_root_item or check_leaf.
> 
> There's a measurable run time improvement reported by Josef, the testing
> workload went from 655 MiB/s to 677 MiB/s, which is about +3%.
> 
> There should be no functional changes but some of the conditions have
> been rewritten to produce more readable result, some lines are longer
> than 80, for the sake of readability.
> 
> Signed-off-by: David Sterba <dsterba@suse.com>

The patch itself is pretty awesome, but still some questionable
likely/unlikely branches, comment inlined below.
> ---
> 
> Josef, would be good if you can add some details about the workload and
> hw, I'll update the changelog. Thanks.
> 
...
> @@ -181,10 +182,10 @@ static bool check_prev_ino(struct extent_buffer *leaf,
>  	 * Only subvolume trees along with their reloc trees need this check.
>  	 * Things like log tree doesn't follow this ino requirement.
>  	 */
> -	if (!is_fstree(btrfs_header_owner(leaf)))
> +	if (likely(!is_fstree(btrfs_header_owner(leaf))))
>  		return true;

This likely() is questionable.

Although the biggest metadata user of btrfs is mostly csum tree, one can
still argue that for  fs with mostly inlined extents, fs trees can be
more common.

>  
> -	if (key->objectid == prev_key->objectid)
> +	if (likely(key->objectid == prev_key->objectid))
>  		return true;

This is also questionable, this is completely dependent on fs content.

Thus we should only use likely/unlikely on the return value of
check_prev_ino(), but not inside it.

The rest looks fine.

Thanks for the effort to enhance tree-checker,
Qu



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