[Devel] [PATCH hci-8.1 v6 5/6] dm-qcow2: keep the dirty bit of an image opened dirty

[Alexander Ivanov]

>> @@ -676,7 +676,8 @@ int qcow2_set_image_file_features(struct qcow2 *qcow2, bool dirty)
>>   	if (kernel_sets_dirty_bit) {
>>   		if (dirty)
>>   			raw_hdr->incompatible_features |= dirty_mask;
>> -		else
>> +		else if (!(qcow2->hdr.incompatible_features &
>> +			   INCOMPATIBLE_FEATURES_DIRTY_BIT))
>>   			raw_hdr->incompatible_features &= ~dirty_mask;
>>   	}
> [Severity: Low]
> Can qcow2->hdr.incompatible_features tell the driver's own dirty bit apart
> from one left by a crash? The header is re-parsed from disk on every
> preresume:
>
> qcow2_preresume()
>      qcow2_free_md_pages_all(tgt);
>      ...
>      qcow2_parse_metadata(ti, tgt);
>      ...
>      qcow2_set_image_file_features(tgt->top, true);
>
> If the kernel's bit is already on disk at parse time, the parsed header
> reports it as dirty-at-open, and every later call with dirty=false skips
> the clear.
>
> One way this happens is a failed clear in postsuspend:
>
> qcow2_postsuspend()
>      ret = qcow2_set_image_file_features(qcow2, false);
>      if (ret)
>          QC_ERR(ti, "Can't set features");
>
> If the qcow2_rw_page_sync() write fails, the error is only logged and the
> bit stays on disk. The next qcow2_preresume() then reads it back as dirty
> at open, and the driver never clears it afterwards. A retried preresume
> without an intervening postsuspend could do the same.
>
> The new "Skip shrinking to used end of dirty image" branch in
> qcow2_truncate_preallocations() from patch 6 tests the same parsed-header
> bit. A falsely dirty image would also lose its tail truncation.
>
> The commit message does not mention this behaviour. Could it be described,
> or could the driver track separately whether the bit was set at open?
If the clear fails, the image stays dirty and gets checked before the next
open. That's safe, so I'll leave it.

> [Severity: Low]
> Does the new condition also leave a clean lower image marked dirty after a
> cancelled backward merge? In dm-qcow2-cmd.c, qcow2_merge_backward_work()
> calls:
>
> ret = qcow2_set_image_file_features(lower, true);
>
> This sets the bit on lower's raw header page and on disk only. lower->hdr
> is not updated.
>
> If a cancel arrives while the merge is in the WAIT_COMPLETION state,
> qcow2_merge_backward_cancel() does only this:
>
> } else if (tgt->backward_merge.state == BACKWARD_MERGE_WAIT_COMPLETION) {
>      set_backward_merge_in_process(tgt, *tgt->backward_merge.pqcow2, false);
>      tgt->backward_merge.state = BACKWARD_MERGE_STOPPED;
>
> It never clears lower's on-disk dirty bit. On the next resume,
> qcow2_preresume() re-parses every image in the chain, so
> lower->hdr.incompatible_features now contains the kernel's own bit.
>
> A later merge_backward start can then go one of two ways:
>
> - If it fails, the clear in qcow2_merge_backward_work() is skipped:
>    ret2 = qcow2_set_image_file_features(lower, false);
>
> - If it completes, lower becomes the top image. The clear in
>    qcow2_postsuspend() is skipped for the same reason:
>    ret = qcow2_set_image_file_features(qcow2, false);
>
> Before this patch the second case cleared the bit. With this patch the
> image stays marked dirty on disk even though the kernel set the bit. The
> stale on-disk bit after cancel already existed, but it used to be cleared
> at the next postsuspend and is now permanent.
>
> Userspace may then run an unnecessary check, or refuse to open the image.
> The exposure needs kernel_sets_dirty_bit=1, a cancel in WAIT_COMPLETION, and
> a later merge or suspend cycle.
>
> Could the cancel path clear the bit, or could the driver record the open-time
> dirty state separately from the re-parsed header?
Will fix.