From mboxrd@z Thu Jan 1 00:00:00 1970 From: Pavel Tikhomirov Date: Fri, 21 Aug 2026 12:43:31 +0200 Subject: Re: [Devel] [PATCH VZ10 v2 09/10] drivers/md/dm-qcow2: allow shared L1 entries during merge from RO image In-Reply-To: <20260812202005.288994-10-andrey.zhadchenko@virtuozzo.com> References: <20260812202005.288994-1-andrey.zhadchenko@virtuozzo.com> <20260812202005.288994-10-andrey.zhadchenko@virtuozzo.com> Message-ID: List-Id: On 8/12/26 22:20, Andrey Zhadchenko wrote: > For RO images with internal snapshots L1 entries are shared. > Write-mode metadata parsing stops at such entries to avoid > modifying a shared L2 table, which made prepare_backward_merge() > see the cluster as unallocated and skip it with > "nothing to merge": the merged result would silently lose all > data under shared L1 entries. > Allow such scenario in parse_l1() and ease WARN in > prepare_backward_merge(). > > https://virtuozzo.atlassian.net/browse/VSTOR-138288 > Feature: dm-qcow2: block device over QCOW2 files driver > Signed-off-by: Andrey Zhadchenko > --- > drivers/md/dm-qcow2-cmd.c | 2 -- > drivers/md/dm-qcow2-map.c | 18 ++++++++++++++++-- > 2 files changed, 16 insertions(+), 4 deletions(-) > > diff --git a/drivers/md/dm-qcow2-cmd.c b/drivers/md/dm-qcow2-cmd.c > index f2c8f51e17af6..fa3762c58372b 100644 > --- a/drivers/md/dm-qcow2-cmd.c > +++ b/drivers/md/dm-qcow2-cmd.c > @@ -266,8 +266,6 @@ static int qcow2_merge_backward_start(struct qcow2_target *tgt, int efd, u32 dep > return -ENOENT; > if (!qcow2_file_is_writable(lower)) > return -EACCES; > - if (!qcow2_file_is_writable(qcow2) && qcow2->hdr.nb_snapshots) > - return -EACCES; Having all these add in one patch remove in next one blocks does not feel right. Let's try to merge all fixups in final version. > if (qcow2->clu_size != lower->clu_size) > return -EOPNOTSUPP; > if (lower->hdr.size < qcow2->hdr.size) > diff --git a/drivers/md/dm-qcow2-map.c b/drivers/md/dm-qcow2-map.c > index ebe68e8d0d3d8..baa6778f1686c 100644 > --- a/drivers/md/dm-qcow2-map.c > +++ b/drivers/md/dm-qcow2-map.c > @@ -1726,6 +1726,14 @@ static bool qio_is_fully_alloced(struct qcow2 *qcow2, struct qio *qio, > return !(subclus_mask & ~alloced_mask); > } > > +static bool qio_may_modify_image(struct qcow2 *qcow2, struct qio *qio) > +{ > + if (qcow2_file_is_writable(qcow2)) > + return true; > + > + return !fake_merge_qio(qio); > +} > + > static loff_t parse_l1(struct qcow2 *qcow2, struct qcow2_map *map, > struct qio **qio, bool write) > > @@ -1762,7 +1770,8 @@ static loff_t parse_l1(struct qcow2 *qcow2, struct qcow2_map *map, > goto out; > if (delay_if_dirty(qcow2, l1->md, l1->index_in_page, qio)) > goto out; > - if (write && map->clu_is_cow) > + /* Don't refuse L1 parse for merge qios with readonly disks */ > + if (write && map->clu_is_cow && qio_may_modify_image(qcow2, *qio)) > goto out; /* Avoid to return pos */ > > ret = pos; > @@ -2061,6 +2070,10 @@ static int parse_metadata(struct qcow2 *qcow2, struct qio **qio, > qio_discard_unmaps_cluster(qcow2, *qio, map))) > return 0; > > + /* Don't need refcount table if we don't modify the image */ > + if (!qio_may_modify_image(qcow2, *qio)) > + return 0; > + > /* Now refcounters table/block */ > ret = qcow2_handle_r1r2_maps(qcow2, pos, qio, &map->r1, > &map->r2, map->compressed); > @@ -2749,7 +2762,8 @@ static int prepare_backward_merge(struct qcow2 *qcow2, struct qio **qio, > } > > if (!map->data_clu_alloced) { > - WARN_ON_ONCE(map->clu_is_cow); /* Strange COW at L1 */ > + /* Strange COW at L1, except the merge from RO image */ > + WARN_ON_ONCE(map->clu_is_cow && qio_may_modify_image(qcow2, *qio)); > if (fake_merge_qio(*qio)) { > /* Nothing is to merge */ > goto endio; -- Best regards, Pavel Tikhomirov Senior Software Developer, Virtuozzo.