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

[no-reply at sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] dm-qcow2: ENOSPC redirty re-arms slow_wb_timer before qcow2_destroy()
--

Subject: Re: [PATCH] dm-qcow2: retry R1/R2 writeback that failed with ENOSPC

commit d478597d2191e210e3a91e1dd7282242553b0e83
Author: Alexander Ivanov <alexander.ivanov at virtuozzo.com>

dm-qcow2: retry R1/R2 writeback that failed with ENOSPC

This patch redirties an R1/R2 metadata page when its writeback fails with
-ENOSPC, so the slow writeback timer retries it. It also passes the errno
of a failed metadata write through complete_metadata_writeback() instead of
converting it to -EIO.

> diff --git a/drivers/md/dm-qcow2-map.c b/drivers/md/dm-qcow2-map.c
> --- a/drivers/md/dm-qcow2-map.c
> +++ b/drivers/md/dm-qcow2-map.c

[ ... ]

> @@ -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?

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261109115856.1362416-1-alexander.ivanov@virtuozzo.com?part=9