Re: [Devel] [PATCH hci-8.1 3/7] drivers/md/dm-qcow2: prepare process_one_qio() for fastpath

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] dm-qcow2: undocumented magic return codes 0/1/2 in handle_metadata()
--

commit c3720b83541dd3fa0f6258c37f4dfb59dfae2146
Author: Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>

drivers/md/dm-qcow2: prepare process_one_qio() for fastpath

This patch adds an avoid_metadata_changes flag to handle_metadata() and
process_one_qio(). When the flag is set and the qio would need a
metadata change or touches a compressed cluster, the functions return a
new code (2 from handle_metadata(), 1 from process_one_qio()) so that
the caller can fall back to the main kwork. process_deferred_qios()
passes false, so its behaviour is unchanged.

Link: https://virtuozzo.atlassian.net/browse/VSTOR-95494

> diff --git a/drivers/md/dm-qcow2-map.c b/drivers/md/dm-qcow2-map.c
> --- a/drivers/md/dm-qcow2-map.c
> +++ b/drivers/md/dm-qcow2-map.c

[ ... ]

> @@ -3543,15 +3543,20 @@ static int handle_metadata(struct qcow2 *qcow2, struct qio **qio,
>  	ret = parse_metadata(qcow2, qio, map);
>  	if (ret < 0 || !*qio) /* Error or postponed */
>  		goto check_err;
>  
> +	if (avoid_metadata_changes && map->compressed)
> +		return 2;
> +

[Severity: Low]
This isn't a functional bug, but handle_metadata() now returns the bare
literal 2 here and at several other sites. Its existing return value 1
means "continue processing", and the new 2 means "needs the slow path".

Could these be given named constants, or could the return contract be
documented in a comment above handle_metadata()?

[ ... ]

> -static void process_one_qio(struct qcow2 *qcow2, struct qio *qio)
> +static int process_one_qio(struct qcow2 *qcow2, struct qio *qio, bool avoid_metadata_changes)
>  {
>  	struct qcow2_map map = { .qcow2 = qcow2, };
>  	bool write;
> +	int ret;
>  
> -	if (!handle_metadata(qcow2, &qio, &map))
> -		return;
> +	ret = handle_metadata(qcow2, &qio, &map, avoid_metadata_changes);
> +	if (!ret)
> +		return 0;
> +	else if (ret == 2)
> +		return 1;

[Severity: Low]
Here the value 2 from handle_metadata() is turned into a return value of
1 from process_one_qio(), which means "not handled, requeue". The same
number 1 therefore means "continue" in handle_metadata() and "requeue" in
process_one_qio().

Would named constants or a comment describing the return values of
process_one_qio() make this clearer?

This isn't a bug, but the new process_one_qio() signature is also longer
than 80 columns:

static int process_one_qio(struct qcow2 *qcow2, struct qio *qio, bool avoid_metadata_changes)

Could it be wrapped?

[ ... ]

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261029140346.465768-1-andrey.zhadchenko@virtuozzo.com?part=3

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel