Re: [Devel] [PATCH hci-8.1 v10 07/10] dm-qcow2: flag the image for check if reftable relocation fails late

[Alexander Ivanov <alexander.ivanov@virtuozzo.com>]

>> @@ -2351,6 +2351,10 @@ static int relocate_refcount_table(struct qcow2 *qcow2, struct qio **qio)
>>   	if (ret) {
>>   		/* Restore old R1 */
>>   		set_reftable_in_raw_hdr(md0->page, old_pos, old_clus);
>> +		QC_ERR(qcow2->tgt->ti,
>> +		       "failed to write header with relocated refcount table: %d",
>> +		       ret);
>> +		qcow2->tgt->md_writeback_error = true;
>>   		goto err_free_r2_pages;
>>   	}
> [Severity: Low]
> This isn't a bug, but the block comment above relocate_refcount_table()
> is now out of date:
>
>   * Then, we try to write new hdr on disk. In case of failure, we
>   * restore old hdr in memory and do safe rollback. In case of success,
>
> A failure of the header write now also sets md_writeback_error, so the
> image is reported by wants_check and resume is refused. Could the comment
> be updated to say so?
>
> The comment at the top of the function also says "We act in the way to
> provide safe rollback throughout whole function", which is no longer
> accurate.
Fixed.
>> @@ -2372,24 +2376,25 @@ static int relocate_refcount_table(struct qcow2 *qcow2, struct qio **qio)
> [ ... ]
>
>>   	/* Connect new R2 to new R1: one for each group new R1 and R2 span */
>>   	group_size = (u64)qcow2->refblock_entries * clu_size;
>>   	for (i = pos, r2_pos = end; i < r2_end; i += group_size, r2_pos += clu_size) {
>>   		if (WARN_ON_ONCE(r2_pos >= r2_end))
>> -			goto err_eio;
>> +			goto err_hdr_written;
> [Severity: Low]
> Is ret assigned on this path before the jump? In
> relocate_refcount_table(), the WARN_ON_ONCE(r2_pos >= r2_end) branch goes
> to err_hdr_written without setting ret.
>
> At that point ret holds either 0 (from the header write or from
> calc_refcounters_map()) or the positive value left by handle_md_page() in
> an earlier iteration.
>
> The label then logs:
>
> 	QC_ERR(qcow2->tgt->ti, "failed to fill relocated refcount table: %d",
> 	       ret ?: -ENOENT);
>
> The ?: operator only replaces 0, so a stale positive ret is printed as if
> it were an error code, for example "failed to fill relocated refcount
> table: 1". Could this branch set ret to a real errno, as the other
> converted err_eio sites do?
Fixed.
>> @@ -2408,12 +2413,18 @@ static int relocate_refcount_table(struct qcow2 *qcow2, struct qio **qio)
>>   	for (i = old_pos; i < old_end; i += clu_size) {
>>   		ret = qcow2_handle_r1r2_maps(qcow2, i, NULL, &r1, &r2, false);
>>   		if (WARN_ON_ONCE(ret <= 0))
>> -			goto err_eio;
>> +			goto err_hdr_written;
>>   		mark_cluster_unused(qcow2, r2.md, r2.index_in_page, i);
>>   	}
>>   
>>   	return 1;
>>   
>> +err_hdr_written:
>> +	/* 0: md page is not cached */
>> +	QC_ERR(qcow2->tgt->ti, "failed to fill relocated refcount table: %d",
>> +	       ret ?: -ENOENT);
>> +	qcow2->tgt->md_writeback_error = true;
>> +	return -EIO;
> [Severity: Medium]
> Does this path leave a half-updated reftable live while the target keeps
> running?
>
> When control reaches err_hdr_written, qcow2->hdr.refcount_table_offset and
> refcount_table_clusters already point at the new reftable. Only some of the
> R1 pages may have been renumbered. The new R2 clusters may not be linked
> into R1. The new R1 and R2 clusters may not be marked used, and the old R1
> clusters may or may not be marked unused. Nothing rolls this back.
>
> The caller is allocate_cluster():
>
> allocate_cluster()
>    find_unused_cluster() returns -ENOENT
>      relocate_refcount_table()
>        -> err_hdr_written, returns -EIO
>    if (ret <= 0) return ret;
>
> The -EIO only fails the current qio. As far as I can see, md_writeback_error
> is read only by the status output, qcow2_truncate_unused_tail() and
> wants_check(). No allocator or map path reads it. Later qios can therefore
> keep calling find_unused_cluster() and mark_cluster_used() against the
> inconsistent refcounts.
>
> Could those qios hand out clusters that belong to the new reftable or R2
> area, or call relocate_refcount_table() again on the corrupt state and
> overwrite metadata or data?
>
> The commit message only claims that the image is flagged and that resume is
> refused, and that claim holds. But the "/* TODO: switch to RO */" comment
> now remains only on the err_free_r2_pages path. Should the late-failure
> path also stop further allocations, for example by switching the target to
> read-only?
The late failure path is not a regression. Before this patch the same 
branches
went to err_eio, returned -EIO with no rollback, and left no trace at 
all. The patch
only flags the image and logs the failure, which is what the commit message
claims. These branches are also reachable only through a driver bug: 
every md
page they use is cached before the header write, and each failure is under
WARN_ON_ONCE. Switching the target to read-only would need a separate
"in-memory metadata is inconsistent" state and refusing writes on it, since
md_writeback_error is also set when in-memory state is consistent. That is
out of scope for this fix, so no changes here.
_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel