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

[Alexander Ivanov]

>> @@ -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.
1, 2: pre-existing, reachable only via WARN paths; this patch neither adds
new jumps into err_free_r2_pages nor sets MD_LX_REVERTED outside the
lock. Will be fixed separately.
>> 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?
Intentional, matches md_writeback_error on the same path; it only fires
after a failed L1/L2 writeback during the merge.
>> @@ -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?
Covered in the commit message: userspace checks the image before
an RW open, including v2 images; find_unused_cluster() relies on the
same contract.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openvz.org/pipermail/devel/attachments/20261022/b81920ad/attachment-0001.html>