Re: [Devel] [PATCH hci-8.1 v6 6/6] dm-qcow2: truncate unused tail of image on suspend

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] dm-qcow2: shrink-to-used-end trusts stale refcounts of crash-dirty or v2 images
- [Medium] dm-qcow2: sticky lx_writeback_error set from a discarded image blocks resume
- [Medium] dm-qcow2: revert_l_entries_update() skips spin_lock() on WARN_ON_ONCE continue path
- [Medium] dm-qcow2: relocate_refcount_table() frees dirty R2 md pages still linked on slow_wb_batch_list
--

commit 782650161bf80812bb686df6d342136798d62726
Author: Alexander Ivanov <alexander.ivanov@virtuozzo.com>

dm-qcow2: truncate unused tail of image on suspend

This patch adds qcow2_calc_used_end(), which scans the refcount blocks
backward from file_preallocated_area_start and finds the end of the last
cluster with a non-zero refcount. qcow2_truncate_preallocations() then
truncates the image file to that end. The scan is skipped after md
writeback errors, after a reverted L1/L2 writeback (new MD_LX_REVERTED
and lx_writeback_error), and for dirty images when kernel_sets_dirty_bit
is set.

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

> 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

[ ... ]

> @@ -1363,6 +1367,8 @@ static void revert_l_entries_update(struct qcow2 *qcow2, struct wb_desc *wbd)
>  		if (cleared)
>  			continue; /* no cluster was allocated */
>  
> +		/* L1/L2 on disk may point to the cluster */
> +		wbd->md->status |= MD_LX_REVERTED;
>  		spin_unlock(&qcow2->md_pages_lock);
>  		pos &= ~LX_REFCOUNT_EXACTLY_ONE;
>  
>  		/*
>  		 * R1/R2 should be cached, since we was able
>  		 * to submit cluster allocation.
>  		 */
>  		ret = qcow2_handle_r1r2_maps(qcow2, pos, NULL, &r1, &r2, false);
>  		if (WARN_ON_ONCE(ret <= 0))
>  			continue;
>  
>  		mark_cluster_unused(qcow2, r2.md, r2.index_in_page, pos);
>  		spin_lock(&qcow2->md_pages_lock);

[Severity: Medium]
Can the continue taken on WARN_ON_ONCE(ret <= 0) leave this loop running
without md_pages_lock?

The lock is dropped just above with spin_unlock(), and it is only retaken
by the spin_lock() at the bottom of the loop body, which the continue
skips. qcow2_handle_r1r2_maps() can return a value <= 0 in several cases:

  qcow2_handle_r1r2_maps()
    __handle_r1r2_maps()
      calc_refcounters_map() fails with -EIO
      calc_r2_page_id() returns -EIO or -ENOENT
      __handle_md_page() returns 0 when the page is not cached and
        qio is NULL
    "refblock=%llu, while no snapshots" returns -EIO

After the continue, the next iteration calls get_u64_from_be_page() and
set_u64_to_be_page() on wbd->md->page without the lock. The caller
complete_wbd() then does:

  spin_lock_irqsave(&qcow2->md_pages_lock, flags);
  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);

So clear_writeback_status() also runs unlocked, and the final unlock is
unbalanced. The comment says R1/R2 should be cached, but nothing enforces
that. This patch adds the MD_LX_REVERTED assignment in this same
unlock window and relies on this error path. Should the lock be retaken
before the continue?

> @@ -2387,21 +2463,21 @@ static int relocate_refcount_table(struct qcow2 *qcow2, struct qio **qio)

[ ... ]

> @@ -2412,7 +2488,7 @@ static int relocate_refcount_table(struct qcow2 *qcow2, struct qio **qio)
>  	for (i = pos; i < r2_end; i += clu_size) {
>  		ret = qcow2_handle_r1r2_maps(qcow2, i, NULL, &r1, &r2, false);
>  		if (WARN_ON_ONCE(ret <= 0))
> -			goto err_free_r2_pages;
> +			goto err_hdr_written;
>  		mark_cluster_used(qcow2, r2.md, r2.index_in_page);
>  	}

[ ... ]

> +err_hdr_written:
> +	/* Header on disk points to new R1, refcounts may be inconsistent */
> +	QC_ERR(qcow2->tgt->ti, "failed to relocate refcount table");
> +	qcow2->tgt->md_writeback_error = true;
>  err_free_r2_pages:
>  	for (i = end; i < r2_end; i += clu_size) {
>  		ret = handle_md_page(qcow2, i >> PAGE_SHIFT, NULL, &md);
>  		if (ret <= 0)
>  			break;
>  		spin_lock_irq(&qcow2->md_pages_lock);
>  		qcow2_md_page_erase(qcow2, md);
>  		spin_unlock_irq(&qcow2->md_pages_lock);
>  		qcow2_free_md_page(md);
>  	}

[Severity: Medium]
Can this cleanup free R2 md pages that are still linked on
slow_wb_batch_list?

Earlier in the function, mark_cluster_used() calls
qcow2_md_make_dirty(qcow2, r2_md, true). That sets MD_DIRTY, does
list_add_tail(&md->wb_link, &qcow2->slow_wb_batch_list), and may arm
slow_wb_timer. The R2 pages for the clusters in [end, r2_end) are the
ones the cleanup loop frees.

If a later WARN_ON_ONCE(ret <= 0) check fails, the code reaches
err_hdr_written and falls into err_free_r2_pages. There,
qcow2_md_page_erase() only does rb_erase(&md->node, &qcow2->md_pages).
qcow2_free_md_page() then frees the page. Neither one does
list_del(&md->wb_link) or clears MD_DIRTY.

The freed md_page stays on slow_wb_batch_list. The timer callback or the
worker can later splice that list and read md->status and md->wbd of the
freed md. Could the dirty pages be unlinked from the writeback list
before they are freed here? This patch routes more failure paths into
this shared cleanup.

> diff --git a/drivers/md/dm-qcow2-target.c b/drivers/md/dm-qcow2-target.c
> --- a/drivers/md/dm-qcow2-target.c
> +++ b/drivers/md/dm-qcow2-target.c
> @@ -170,6 +170,7 @@ static void qcow2_free_md_pages_tree(struct rb_root *root)
>  void qcow2_flush_deferred_activity(struct qcow2_target *tgt, struct qcow2 *qcow2)

[ ... ]

> @@ -191,13 +192,17 @@ void qcow2_flush_deferred_activity(struct qcow2_target *tgt, struct qcow2 *qcow2
>  	     node; node = rb_next(node)) {
>  		md = rb_entry(node, struct md_page, node);
>  		/* FIXME: call md_make_dirty() and try once again? */
> -		if (md->status & MD_WRITEBACK_ERROR) {
> -			QC_ERR(tgt->ti, "Failed to write dirty pages");
> -			tgt->md_writeback_error = true;
> -			break;
> -		}
> +		if (md->status & MD_WRITEBACK_ERROR)
> +			wb_error = true;
> +		if (md->status & MD_LX_REVERTED)
> +			tgt->lx_writeback_error = true;
>  	}

[Severity: Medium]
Can this set the sticky tgt->lx_writeback_error from an image that is
about to be destroyed?

qcow2_flush_deferred_activity() is also called from a non-suspend path:

  qcow2_merge_backward_complete()
    qcow2_flush_deferred_activity(tgt, qcow2); /* Delayed md pages */
    qcow2->lower = NULL;
    ...
    qcow2_destroy(qcow2);

If a page of the image being removed from the chain is still marked
MD_LX_REVERTED, the target-wide flag is set. The image and its md pages
are then freed, but the flag stays set. Nothing clears it.

qcow2_wants_check() now includes the flag, so qcow2_preresume() fails
with:

  if (qcow2_wants_check(tgt)) {
          QC_ERR(ti, "image check and target reload are required");
          return -EIO;
  }

The surviving image is then refused resume, and
qcow2_truncate_preallocations() skips the shrink, even though the stale
page no longer exists. The existing md_writeback_error has the same
imprecision on this path, but is that intended for the new flag as well?

> @@ -1048,11 +1053,34 @@ static void qcow2_truncate_preallocations(struct dm_target *ti)

[ ... ]

> +	if (tgt->md_writeback_error) {
> +		QC_INFO(ti, "Skip shrinking to used end after md writeback error");
> +	} else if (tgt->lx_writeback_error) {
> +		QC_INFO(ti, "Skip shrinking to used end after reverted L1/L2 writeback");
> +	} else if (kernel_sets_dirty_bit &&
> +		   (qcow2->hdr.incompatible_features &
> +		    INCOMPATIBLE_FEATURES_DIRTY_BIT)) {
> +		QC_INFO(ti, "Skip shrinking to used end of dirty image");
> +	} else {
> +		used_end = qcow2_calc_used_end(qcow2);
> +		if (used_end < 0)
> +			QC_ERR(ti, "Can't find used end: %pe", ERR_PTR(used_end));
> +		else
> +			end = used_end;
> +	}

[Severity: Medium]
Is this guard enough to protect against truncating below clusters that
L1/L2 still reference after a crash?

The only protection against stale refcounts is:

  kernel_sets_dirty_bit && (qcow2->hdr.incompatible_features &
                            INCOMPATIBLE_FEATURES_DIRTY_BIT)

kernel_sets_dirty_bit is a static bool that defaults to false, so this
branch is inactive by default. In qcow2_check_convert_hdr(), v2 images
return early:

  if (hdr->version == 2)
          return 0;

  hdr->incompatible_features = be64_to_cpu(raw_hdr->incompatible_features);

That return comes before incompatible_features is read, so the field
stays 0 and the guard can never fire for v2 images. The open-time
dirty-bit check in qcow2_check_convert_hdr() is commented out:

  //	if (!is_ro && kernel_sets_dirty_bit !=
  //	    !(hdr->incompatible_features & INCOMPATIBLE_FEATURES_DIRTY_BIT))
  //		return kernel_sets_dirty_bit ? -EUCLEAN : -ENOLCK;

So the kernel does not enforce that the image was checked before an RW
open.

If refcounts are below real usage after a crash, qcow2_calc_used_end()
returns an end below clusters that L2 still points to. It is computed
purely from on-disk R2 entries. qcow2_truncate_safe() then cuts those
clusters off, even on a plain suspend, remove or table swap with no guest
writes in the session.

The commit message says find_unused_cluster() already relies on the same
userspace contract. That path is only reached once a guest write needs a
new cluster, while this truncation runs on every RW suspend, remove and
reload.

Should the shrink be skipped or enforced in-kernel for v2 images and for
the default kernel_sets_dirty_bit setup?

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261022121706.1890593-1-alexander.ivanov@virtuozzo.com?part=6

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