[Devel] [PATCH hci-8.1 v10 09/10] dm-qcow2: retry R1/R2 writeback that failed with ENOSPC

[Alexander Ivanov]

>> @@ -1357,6 +1357,8 @@ static void clear_writeback_status(struct qcow2 *qcow2, struct md_page *md,
>>   		 * so here we care about R1R2 only.
>>   		 */
>>   		md->status |= MD_WRITEBACK_ERROR;
>> +		if (ret == -ENOSPC && !READ_ONCE(qcow2->tgt->wants_suspend))
>> +			qcow2_md_make_dirty(qcow2, md, true);
> [Severity: High]
> Can this redirty re-arm slow_wb_timer on a path that frees the qcow2
> right afterwards?
>
> The redirty is gated only on wants_suspend. Only qcow2_presuspend() sets
> wants_suspend, but qcow2_flush_deferred_activity() is also called from
> paths where it is false.
>
> One example is qcow2_merge_backward_complete():
>
>      qcow2_flush_deferred_activity(tgt, qcow2); /* Delayed md pages */
>      qcow2->lower = NULL;
>      ...
>      qcow2_destroy(qcow2);
>
> qcow2_flush_deferred_activity() runs only two iterations. If the
> underlying fs is still full, the sequence is:
>
>      iteration 0: R1/R2 write fails with -ENOSPC, page is redirtied
>      iteration 1: timer handler fires, the retry fails with -ENOSPC again
>      clear_writeback_status()
>        qcow2_md_make_dirty(qcow2, md, true)
>          list_add_tail(&md->wb_link, &qcow2->slow_wb_batch_list)
>          mod_timer(&qcow2->slow_wb_timer, jiffies + WB_TIMEOUT_JI)
>
> After the last iteration nothing drains the page or the timer.
>
> qcow2_destroy() then only does:
>
>      WARN_ON(!list_empty(&qcow2->paused_qios) ||
>              !list_empty(&qcow2->wb_batch_list) ||
>              !list_empty(&qcow2->slow_wb_batch_list) ||
>              timer_pending(&qcow2->slow_wb_timer));
>      qcow2_free_md_pages_tree(&qcow2->md_pages);
>      ...
>      kfree(qcow2);
>
> It does not call timer_shutdown_sync() or timer_delete_sync(). Could
> qcow2_slow_wb_timer_fn() then run on the freed struct qcow2 when the timer
> fires, up to WB_TIMEOUT_JI later? It does timer_container_of(),
> list_splice_init() and queue_work() on that memory. The WARN_ON would also
> trigger.
>
> The merge-error path in qcow2_merge_backward_work() calls
> qcow2_flush_deferred_activity(tgt, lower) with wants_suspend false in the
> same way. There the ENOSPC page is left redirtied and the timer re-armed
> on an image that is still live but no longer being merged.
>
> qcow2_tgt_destroy() also flushes and then calls qcow2_destroy(). It looks
> protected only if presuspend ran first.
>
> Before this patch, the completion handler never re-armed the timer after a
> failed R1/R2 write, so this exposure is new.
>
> The commit message says "A page that still fails then is not redirtied, as
> wants_suspend is set, so nothing is left pending on a suspended device".
> Is that true outside the suspend flush? As described above, the claim
> holds only when wants_suspend is set.
>
> Should the redirty be limited to cases where the flush cannot leave the
> timer armed, or should the flush and destroy paths cancel the timer and
> drain slow_wb_batch_list before the qcow2 is freed?
Fixed in the previous fixes.  The redirty is no longer gated on 
wants_suspend.
qcow2_flush_deferred_activity() sets md_wb_no_retry for its last 
iteration on
every caller (suspend, merge complete/error, tgt destroy). The md wb qio
drops its inflight ref only after do_md_page_write_complete(), so the 
retry's
completion runs while the flag is still set. A page that fails again is 
not redirtied,
and the timer is not re-armed before qcow2_destroy().