@@ -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().
_______________________________________________
Devel mailing list
[email protected]
https://lists.openvz.org/mailman/listinfo/devel
