From mboxrd@z Thu Jan 1 00:00:00 1970 From: Pavel Tikhomirov Date: Wed, 19 Aug 2026 12:27:37 +0200 Subject: Re: [Devel] [PATCH VZ10 v2 01/10] drivers/md/dm-qcow2: fix revert_cluster_alloc() for ext_l2 case In-Reply-To: <20260812202005.288994-2-andrey.zhadchenko@virtuozzo.com> References: <20260812202005.288994-1-andrey.zhadchenko@virtuozzo.com> <20260812202005.288994-2-andrey.zhadchenko@virtuozzo.com> Message-ID: <83950005-16ac-44dc-b642-d38188fdace2@virtuozzo.com> List-Id: On 8/12/26 22:19, Andrey Zhadchenko wrote: > This function walks over all changed u64 values in md. With ext_l2 > half of them holds subcluster description. Firstly it reverts these > values to saved pe_page, which is fine, but then it tries to revert > r1r2 changes. It makes no sense when the value is a subcluster > description. > Teach the function to skip subcluster descriptions: do it based on > a new lx_level in struct wb_desc. Add new argument to > prepare_l_entry_update() and set it there. > The warning also could have tripped for ext_l2 entries, so drop > it entirely. > The function effectively reverts not only cluster alloc, so rename > it to revert_l_entries_update() to mimic prepare_l_entries_update. prepare_l_entrie(s)_update is probably a misspelled prepare_l_entry_update > > Feature: dm-qcow2: block device over QCOW2 files driver > https://virtuozzo.atlassian.net/browse/VSTOR-139406 > Signed-off-by: Andrey Zhadchenko > --- > drivers/md/dm-qcow2-map.c | 28 ++++++++++++++++++---------- > drivers/md/dm-qcow2-target.c | 2 +- > drivers/md/dm-qcow2.h | 1 + > 3 files changed, 20 insertions(+), 11 deletions(-) > > diff --git a/drivers/md/dm-qcow2-map.c b/drivers/md/dm-qcow2-map.c > index db21efb45e17a..2cd8e174049f8 100644 > --- a/drivers/md/dm-qcow2-map.c > +++ b/drivers/md/dm-qcow2-map.c > @@ -1297,18 +1297,19 @@ static void do_md_page_read_complete(int ret, struct qcow2 *qcow2, > } > > /* Be careful with dirty_or_writeback()/etc! Check races. */ > -static void revert_clusters_alloc(struct qcow2 *qcow2, struct wb_desc *wbd) > +static void revert_l_entries_update(struct qcow2 *qcow2, struct wb_desc *wbd) > { > struct qcow2_map_item r1, r2; > struct page *pe_page; > + bool skip_odd; > u64 pos, old; > int i, ret; > > + skip_odd = qcow2->ext_l2 && wbd->lx_level == L2_LEVEL; > + > lockdep_assert_held(&qcow2->md_pages_lock); > for_each_set_bit(i, wbd->changed_indexes, LX_INDEXES_PER_PAGE) { > pos = get_u64_from_be_page(wbd->md->page, i); > - WARN_ON_ONCE(!(pos & ~LX_REFCOUNT_EXACTLY_ONE) || > - !(pos & LX_REFCOUNT_EXACTLY_ONE)); > > /* Here we restore prealloced and compressed clu mappings */ > pe_page = wbd->pe_page; > @@ -1321,6 +1322,9 @@ static void revert_clusters_alloc(struct qcow2 *qcow2, struct wb_desc *wbd) > } > > set_u64_to_be_page(wbd->md->page, i, 0); > + if (skip_odd && (i & 1)) > + continue; /* pos contains ext_l2 part of L2 entry */ > + > spin_unlock(&qcow2->md_pages_lock); > pos &= ~LX_REFCOUNT_EXACTLY_ONE; > > @@ -1369,7 +1373,7 @@ static void complete_wbd(struct qcow2 *qcow2, struct wb_desc *wbd) > unsigned long flags; > > spin_lock_irqsave(&qcow2->md_pages_lock, flags); > - revert_clusters_alloc(qcow2, wbd); > + revert_l_entries_update(qcow2, wbd); > clear_writeback_status(qcow2, wbd->md, wbd->ret, > &wait_list, &end_list); > spin_unlock_irqrestore(&qcow2->md_pages_lock, flags); > @@ -2433,7 +2437,7 @@ static loff_t allocate_cluster(struct qcow2 *qcow2, struct qio *qio, > #define LU_IGN_CHANGED_IND (1 << 3) > static int prepare_l_entry_update(struct qcow2 *qcow2, struct qio *qio, > struct md_page *md, u32 index_in_page, > - u64 *pval, u32 arg_mask) > + u64 *pval, u32 arg_mask, u8 lx_level) > { > bool wants_pe_page = (arg_mask & LU_WANTS_PE_PAGE); > struct wb_desc *new_wbd = NULL; > @@ -2453,6 +2457,7 @@ static int prepare_l_entry_update(struct qcow2 *qcow2, struct qio *qio, > if (!new_wbd) > return -ENOMEM; > new_wbd->md = md; > + new_wbd->lx_level = lx_level; > } else if (wants_pe_page && !md->wbd->pe_page) { > pe_page = alloc_page(GFP_NOIO|__GFP_ZERO); > if (!pe_page) > @@ -2515,7 +2520,8 @@ static int prepare_l1l2_allocation(struct qcow2 *qcow2, struct qio *qio, > /* Allocate cluster for L2 entries, and prepare L1 update */ > ret = prepare_l_entry_update(qcow2, qio, map->l1.md, > map->l1.index_in_page, &val, > - LU_SET_ONE_MASK|LU_WANTS_ALLOC); > + LU_SET_ONE_MASK|LU_WANTS_ALLOC, > + L1_LEVEL); > if (ret <= 0) > return ret; > > @@ -2541,7 +2547,8 @@ static int prepare_l1l2_allocation(struct qcow2 *qcow2, struct qio *qio, > > ret = prepare_l_entry_update(qcow2, qio, map->l2.md, > map->l2.index_in_page, > - &map->data_clu_pos, arg_mask); > + &map->data_clu_pos, arg_mask, > + L2_LEVEL); > if (ret <= 0) > return ret; > > @@ -2562,7 +2569,7 @@ static int prepare_l1l2_allocation(struct qcow2 *qcow2, struct qio *qio, > > return prepare_l_entry_update(qcow2, qio, map->l2.md, > map->l2.index_in_page + 1, > - &val, arg_mask); > + &val, arg_mask, L2_LEVEL); > } > > /* > @@ -3975,7 +3982,7 @@ static void process_cow_indexes_write(struct qcow2 *qcow2, > ret = prepare_l_entry_update(qcow2, qio, lx_md, > ext->lx_index_in_page, > &ext->allocated_clu_pos, > - arg_mask); > + arg_mask, ext->cow_level); > if (ret < 0) { > qio->bi_status = errno_to_blk_status(ret); > qio_endio(qio); > @@ -3986,7 +3993,8 @@ static void process_cow_indexes_write(struct qcow2 *qcow2, > arg_mask &= ~LU_SET_ONE_MASK; > ret = prepare_l_entry_update(qcow2, qio, lx_md, > ext->lx_index_in_page + 1, > - &ext->new_ext_l2, arg_mask); > + &ext->new_ext_l2, arg_mask, > + L2_LEVEL); > WARN_ON_ONCE(ret < 0); > } > > diff --git a/drivers/md/dm-qcow2-target.c b/drivers/md/dm-qcow2-target.c > index 3f65897ce9da2..0f1d3e3c5258e 100644 > --- a/drivers/md/dm-qcow2-target.c > +++ b/drivers/md/dm-qcow2-target.c > @@ -174,7 +174,7 @@ void qcow2_flush_deferred_activity(struct qcow2_target *tgt, struct qcow2 *qcow2 > int i; > > /* > - * We need second iteration, since revert_clusters_alloc() > + * We need second iteration, since revert_l_entries_update() > * may start timer again after failed wb. > */ > for (i = 0; i < 2; i++) { > diff --git a/drivers/md/dm-qcow2.h b/drivers/md/dm-qcow2.h > index 86f0688e7345a..aa3487007523f 100644 > --- a/drivers/md/dm-qcow2.h > +++ b/drivers/md/dm-qcow2.h > @@ -118,6 +118,7 @@ struct wb_desc { > struct list_head dependent_list; > int nr_submitted; > bool completed; > + u8 lx_level; > int ret; > }; > -- Best regards, Pavel Tikhomirov Senior Software Developer, Virtuozzo.