From mboxrd@z Thu Jan 1 00:00:00 1970 From: Vasileios Almpanis Date: Wed, 19 Aug 2026 11:05:02 +0000 Subject: Re: [Devel] [PATCH VZ10 v2 04/10] drivers/md/dm-qcow2: never trigger COW or allocation on discard In-Reply-To: <20260812202005.288994-5-andrey.zhadchenko@virtuozzo.com> References: <20260812202005.288994-1-andrey.zhadchenko@virtuozzo.com> <20260812202005.288994-5-andrey.zhadchenko@virtuozzo.com> Message-ID: <178713750239.404937.3004358185512206751.b4-review@b4> List-Id: > 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 > > 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