Re: [Devel] [PATCH hci-8.1 v5 11/11] dm-ploop: truncate unused tail of top delta on suspend

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] dm-ploop: no fsync between BAT writeback and top delta truncation in postsuspend
- [Low] dm-ploop: calc_used_end() WARNs on a clean-check that postsuspend does not fully wait for
--

commit 847969f12202a7013753f4e3c79c265f1c673b03
Author: Alexander Ivanov <alexander.ivanov@virtuozzo.com>

dm-ploop: truncate unused tail of top delta on suspend

This patch makes ploop_postsuspend() truncate the unused tail of the top
delta. The used end is computed by the new ploop_calc_used_end(), which
reads the on-disk BAT and the format extension offset. The truncation is
skipped for read-only, raw and standby devices, and when the file size
differs from top->file_size.

Link: https://virtuozzo.atlassian.net/browse/VSTOR-148171

> diff --git a/drivers/md/dm-ploop-bat.c b/drivers/md/dm-ploop-bat.c
> --- a/drivers/md/dm-ploop-bat.c
> +++ b/drivers/md/dm-ploop-bat.c

[ ... ]

> +loff_t ploop_calc_used_end(struct ploop *ploop, struct file *file)
> +{
> +	u32 i, end, nr_be, dst_clu, used_clus, *bat_entries;

[ ... ]

> +	/* BAT on disk may miss allocated clusters until writeback is done */
> +	if (WARN_ON_ONCE(!ploop_md_pages_are_clean(ploop)))
> +		return -EBUSY;

[Severity: Low]
The commit message says "Skip it if md pages are not clean", but this
code raises a kernel WARN and then the caller logs an error.

ploop_postsuspend() only waits for inflight_md:

    wait_event(ploop->inflight_md_wq, !atomic_read(&ploop->inflight_md));

ploop_md_pages_are_clean() also tests MD_DIRTY, MD_UPDATING,
wb_batch_llist, md->piwb and md->wait_llist.

In the error paths of ploop_process_one_deferred_bio(), the sequence is:

    ploop_break_bat_update(ploop, md, piwb);   /* drops inflight_md */
    ploop_pio_endio(pio);
    spin_lock_irqsave(&ploop->bat_lock, flags);
    if (add_to_wblist)
            clear_bit(MD_DIRTY, &md->status);
    clear_bit(MD_UPDATING, &md->status);

Can suspend complete and reach ploop_postsuspend() with inflight_md == 0
while MD_DIRTY or MD_UPDATING are still set? If so, this would hit the
WARN_ON_ONCE and the truncation would be silently skipped.

Would it make sense to treat unclean md pages as an expected skip, without
the WARN and without the error message from ploop_truncate_unused_tail()?

[ ... ]

> diff --git a/drivers/md/dm-ploop-target.c b/drivers/md/dm-ploop-target.c
> --- a/drivers/md/dm-ploop-target.c
> +++ b/drivers/md/dm-ploop-target.c

[ ... ]

> +	end = ploop_calc_used_end(ploop, top->file);
> +	if (end < 0) {
> +		PL_ERR("Can't find used end of top delta: %lld", end);
> +		return;
> +	}
> +	if (end >= top->file_size)
> +		return;
> +
> +	ret = do_ftruncate(top->file, end, 0);

[Severity: Medium]
Is a flush of the top delta needed before this truncate?

ploop_calc_used_end() reads the BAT back with vfs_iter_read(), which can be
served from the page cache. The BAT page writes are issued by
ploop_index_wb_submit() as plain writes, with no flush or FUA. The only
vfs_fsync() of the top delta in the suspend path is in ploop_presuspend(),
which runs before ploop_postsuspend() waits for inflight_md:

ploop_presuspend()
    vfs_fsync(ploop_top_delta(ploop)->file, 0);
ploop_postsuspend()
    wait_event(ploop->inflight_md_wq, !atomic_read(&ploop->inflight_md));
    ploop_truncate_unused_tail()
        do_ftruncate(top->file, end, 0);

A BAT writeback that completes in that wait, for example one zeroing a
discarded cluster, is not covered by the earlier fsync.

If the host crashes after the size change is committed but before those BAT
pages are durable, the on-disk BAT can still reference clusters beyond the
new EOF. The next ploop_ctr() or ploop_reload_metadata() would then fail in
ploop_convert_bat_entries() with -EPROTO, which is the failure the commit
message says it is avoiding.

Should a vfs_fsync() of top->file be done before do_ftruncate()?

[ ... ]

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

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel