Re: [Devel] [PATCH VZ10 v2 04/10] drivers/md/dm-qcow2: never trigger COW or allocation on discard

[Vasileios Almpanis <vasileios.almpanis@virtuozzo.com>]

> In handle_metadata() the partial cluster discard fallback is checked
> only after the COW branches. As a result a discard, whose range
> touches a shared (snapshot or compressed) cluster or crosses an
> unmapped unit border over a backing file, is routed into the data COW
> machinery. COW then tries to submit the discard payload as a data
> write, but discard qios carry no bvec array: the iov_iter is built
> from garbage, which triggers the WARN_ON() in __submit_rw_mapped()
> and writes random kernel memory into the COW'ed cluster.
> 
> Check for discard before the COW branches: discard is advisory and
> carries no data, so it must never COW or allocate. End discard
> early while handling metadata if possible. Otherwise do discard
> operations before COWs.
> 
> Feature: dm-qcow2: block device over QCOW2 files driver
> https://virtuozzo.atlassian.net/browse/VSTOR-139406
> Signed-off-by: Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>
>
> diff --git a/drivers/md/dm-qcow2-map.c b/drivers/md/dm-qcow2-map.c
> index 8d2c4e78a4c5..dcc65735881d 100644
> --- a/drivers/md/dm-qcow2-map.c
> +++ b/drivers/md/dm-qcow2-map.c
> @@ -3431,9 +3431,18 @@ static int handle_metadata(struct qcow2 *qcow2, struct qio **qio,
>  		/* Nothing to COW or L1 is mapped exactly once */
>  		qio_endio(*qio);
>  		ret = 0;
> -	} else if (unlikely(op_is_discard((*qio)->bi_op)) &&
> -		   qio_discard_updates_metadata(qcow2, *qio, map)) {
> -		ret = prepare_cluster_discard(qcow2, qio, map);
> +	} else if (unlikely(op_is_discard((*qio)->bi_op))) {
> +		/*
> +		 * Discard carries no data and advisory. If it does not
NIT: no data and is advisory
> +		 * trigger metadata changes or to-be-discarded cluster
> +		 * is not present, end early.
> +		 */
> +		if (qio_discard_updates_metadata(qcow2, *qio, map)) {
> +			ret = prepare_cluster_discard(qcow2, qio, map);
> +		} else if (!map->data_clu_alloced || map->clu_is_cow) {
> +			qio_endio(*qio);
> +			ret = 0;
> +		}
>  	} else if (write &&
>  		   (!qio_is_fully_alloced(qcow2, *qio, map) || map->clu_is_cow)) {
>  		if (map->clu_is_cow) {
> @@ -3449,13 +3458,6 @@ static int handle_metadata(struct qcow2 *qcow2, struct qio **qio,
>  			 */
>  			map->backing_file_cow = true;
>  			ret = prepare_l1l2_replace(qcow2, *qio, map);
> -		} else if (unlikely(op_is_discard((*qio)->bi_op) &&
> -				    (map->level & L2_LEVEL))) {
> -			if (!map->data_clu_alloced) {
> -				qio_endio(*qio);
> -				ret = 0;
> -			}
> -			/* Otherwise issue_discard(). */
>  		} else {
>  			/* Wants L1 or L2 entry allocation */
>  			ret = prepare_l1l2_allocation(qcow2, *qio, map);
> @@ -3569,8 +3571,8 @@ static void process_one_qio(struct qcow2 *qcow2, struct qio *qio)
>  
>  	write = op_is_write(qio->bi_op);
>  
> -	/* Discard with prepared metadata update, see prepare_cluster_discard() */
> -	if (unlikely(qio->flags & QIO_IS_DISCARD_FL)) {
> +	/* Process discard early. Don't do COW just to discard */
> +	if (unlikely(op_is_discard(qio->bi_op))) {
>  		issue_discard(&map, qio);
>  		return;
>  	}
> @@ -3588,8 +3590,6 @@ static void process_one_qio(struct qcow2 *qcow2, struct qio *qio)
>  			submit_read_whole_cow_clu(&map, qio);
>  		else if (unlikely(map.clu_is_cow || map.backing_file_cow))
>  			submit_read_sliced_cow_clu(&map, qio);
> -		else if (unlikely(op_is_discard(qio->bi_op)))
> -			issue_discard(&map, qio);
>  		else
>  			perform_rw_mapped(&map, qio);
>  	}
side note: for single-bio discard requests bi_io_vec is actually
a garbage pointer set in prepare_one_embedded_qio in the else 
branch. We set bvec to rq->bio->bi_inline_vecs which is never null its a
flexible array. I think we guard in all places for discards based on op so
we don't meet any issue but just wanted to point it out.

-- 
Vasileios Almpanis <vasileios.almpanis@virtuozzo.com>