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

[Alexander Ivanov]

>> @@ -2351,6 +2351,8 @@ 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 relocate refcount table");
>> +		qcow2->tgt->md_writeback_error = true;
>>   		goto err_free_r2_pages;
>>   	}
> [Severity: Low]
> This isn't a bug, but the message logged here after a failed
> qcow2_rw_page_sync() of the header is identical to the one logged at
> err_hdr_written.
>
> Can an administrator tell these two failures apart from the log?
>
> In the header write failure case the cached header is restored and a clean
> failure is possible, while at err_hdr_written the header has already been
> written and the in-memory state is partly converted. Would distinct
> messages make the two cases distinguishable?
>
> Setting md_writeback_error here is conservative, since a short write may
> still have reached disk, so that choice looks acceptable.
The two cases are not really different: a failed header write may still have
reached disk, so both need the same check, and every late failure is already
preceded by a WARN splat that tells them apart in the log.
>> @@ -2405,12 +2407,16 @@ 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))
>> -			return -EIO;
>> +			goto err_hdr_written;
>>   		mark_cluster_unused(qcow2, r2.md, r2.index_in_page, i);
>>   	}
>>   
>>   	return 1;
>>   
>> +err_hdr_written:
>> +	QC_ERR(qcow2->tgt->ti, "failed to relocate refcount table");
>> +	qcow2->tgt->md_writeback_error = true;
>> +	return -EIO;
>>   err_free_r2_pages:
> [Severity: Medium]
> Does the target stay live on half-converted metadata after this path
> returns -EIO?
>
> By the time err_hdr_written is reached, hdr.refcount_table_offset and
> hdr.refcount_table_clusters already point at the new reftable. Only some of
> the R1 pages may have been renumbered, the new R2 may be only partly
> connected, and the used/unused marking may be incomplete.
>
> The only containment here is the md_writeback_error flag, and it is
> consulted only in a few places:
>
> dm-qcow2-target.c:qcow2_preresume()
>      if (qcow2_wants_check(tgt)) {
>          QC_ERR(ti, "image check and target reload are required");
>          return -EIO;
>
> Besides preresume, the flag is read only in the status dump, the
> dirty-bit decision and the suspend-time truncate logic. dm-qcow2-map.c
> only sets it and never reads it.
>
> The allocation path does not check it either:
>
> allocate_cluster()
>      if (unlikely(pos == -ENOENT)) {
>          ret = relocate_refcount_table(qcow2, &qio);
>          if (ret <= 0)
>              return ret;
>          goto again;
>      }
>
> Can a later allocation that finds no free cluster re-enter
> relocate_refcount_table() against the already modified hdr and cached
> parameters? Can the target keep servicing writes and cause further
> refcount inconsistency until suspend?
>
> Would it make sense to make the target read-only, or to have
> allocate_cluster() and the relocation entry check md_writeback_error? The
> /* TODO: switch to RO */ comment is still present in the err_free_r2_pages
> path of this function.
This patch only adds reporting to error paths that already returned -EIO
and left the target running, so it is not a regression; containing the 
target
after such a failure, by making it read-only or blocking further relocation,
is a separate change.