Re: [Devel] [PATCH hci-8.1 v2 5/6] dm-ploop: reload metadata on resume

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] dm-ploop: lost wakeup in ploop_allocator() can hang ploop_postsuspend() in wait_event()
- [Medium] dm-ploop: ploop_reload_metadata() uses GFP_KERNEL allocs with no NOIO scope
- [Medium] dm-ploop: postsuspend wait_event on prealloc can hang on allocator lost wakeup
- [Low] dm-ploop: ploop_reload_metadata() restore path leaves m_Sig/m_Type/m_Sectors modified
--

commit 17ae791b0a37f669147d370ddae2ae7351e7b6de
Author: Alexander Ivanov <alexander.ivanov@virtuozzo.com>

dm-ploop: reload metadata on resume

This patch makes ploop_preresume() re-read the top delta header, the BAT
of every delta and the delta file sizes, and rebuild the BAT tree and
holes_bitmap. It moves preallocation from ploop_ctr() to ploop_preresume(),
makes ploop_postsuspend() wait for pending preallocation, and takes
ctl_rwsem in ploop_llseek_hole().

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

[ ... ]

> +int ploop_reload_metadata(struct ploop *ploop)
> +{
> +	struct rb_root old_bat_entries = ploop->bat_entries;
> +	void *old_holes_bitmap = ploop->holes_bitmap;
> +	u32 old_nr_bat_entries = ploop->nr_bat_entries;
> +	u32 old_hb_nr = ploop->hb_nr;

[ ... ]

> +	old_deltas = kmemdup(ploop->deltas,
> +			     sizeof(*old_deltas) * ploop->nr_deltas,
> +			     GFP_KERNEL);
> +	page = alloc_page(GFP_KERNEL);

[Severity: Medium]
Can these GFP_KERNEL allocations deadlock under memory pressure?

ploop_preresume() runs in the DM ioctl task while the device is still
suspended, and no memalloc_noio scope is set on that path.  Direct reclaim
with __GFP_IO and __GFP_FS could write back dirty pages of a filesystem
mounted on this ploop device.  Those bios are held until resume completes,
but the resume is the task waiting in the allocation.

The same applies to the other allocations reached from here:

ploop_reload_metadata()
  ploop_setup_metadata()
    ploop_setup_holes_bitmap()
      ploop->holes_bitmap = kvmalloc(size, GFP_KERNEL);

and to ploop_prealloc_md_pages().  The ploop kthreads avoid this by running
with PF_MEMALLOC_NOIO (PLOOP_PFLAGS in dm-ploop-map.c), but the ioctl task
has no such protection.

Would it make sense to wrap ploop_reload_metadata() in
memalloc_noio_save()/memalloc_noio_restore(), or to use GFP_NOIO?

[ ... ]

> +	ploop->bat_entries = RB_ROOT;
> +	ploop->holes_bitmap = NULL;
> +
> +	/* Image is already checked for being in use in ploop_ctr() */
> +	ret = ploop_setup_metadata(ploop, page, false);
> +	if (ret)
> +		goto restore;

[ ... ]

> +restore:
> +	ploop_free_md_pages_tree(&ploop->bat_entries);
> +	kvfree(ploop->holes_bitmap);
> +	ploop->bat_entries = old_bat_entries;
> +	ploop->holes_bitmap = old_holes_bitmap;
> +	ploop->nr_bat_entries = old_nr_bat_entries;
> +	ploop->hb_nr = old_hb_nr;

[Severity: Low]
The commit message says "On error the old metadata is kept and resume
fails."  Is that fully true here?

ploop_setup_metadata() overwrites these fields before its later checks
('custom FirstBlockOffset', 'Too short BAT', ploop_setup_holes_bitmap()
returning -ENOMEM):

	memcpy(ploop->m_Sig, m_hdr->m_Sig, sizeof(ploop->m_Sig));
	ploop->m_Type = le32_to_cpu(m_hdr->m_Type);
	ploop->m_Sectors = le32_to_cpu(m_hdr->m_Sectors);

The restore label puts back bat_entries, holes_bitmap, nr_bat_entries,
hb_nr and deltas[], but not m_Sig, m_Type or m_Sectors.  The same applies
when ploop_prealloc_md_pages() or ploop_read_delta() fails.

m_Sig and m_Sectors are validated, so in practice only m_Type could differ,
and only if userspace rewrote the header.  ploop_delta_check_header()
compares against these fields, so a later delta check could then use a value
that does not match the retained old BAT.

Should these three fields be saved and restored along with the others?

> diff --git a/drivers/md/dm-ploop-map.c b/drivers/md/dm-ploop-map.c
> --- a/drivers/md/dm-ploop-map.c
> +++ b/drivers/md/dm-ploop-map.c
> @@ -1245,6 +1245,12 @@ static int ploop_no_pending_prealloc(struct ploop *ploop)
>  	return ret;
>  }
>  
> +void ploop_wait_prealloc(struct ploop *ploop)
> +{
> +	wait_event(ploop->dispatcher_wq_prealloc,
> +		   ploop_no_pending_prealloc(ploop));
> +}

[Severity: Medium]
Can this wait hang forever on a lost wakeup in the allocator?

This is an unbounded, uninterruptible wait whose only waker is the
wake_up() in ploop_allocator() after a preallocation completes.  In
ploop_allocator(), prealloc_size is tested locklessly before the task state
is set:

	if (ploop->prealloc_size) {
		...
	}
	if (kthread_should_stop()) {...}
	set_current_state(TASK_INTERRUPTIBLE);
	schedule();

Consider this interleaving:

  allocator: reads prealloc_size == 0, not yet in set_current_state()
  requester: ploop_req_prealloc() sets prealloc_size under bat_lock
  requester: wake_up_process(ploop->kt_allocator->task)  /* TASK_RUNNING */
  allocator: set_current_state(TASK_INTERRUPTIBLE); schedule();

The wakeup is a no-op and the allocator sleeps with prealloc_size != 0.
ploop_req_prealloc() returns early when the requested size is already
covered, so later IO would not re-kick it, and suspend has quiesced IO
anyway.  ploop_no_pending_prealloc() then never becomes true.

The race in the allocator predates this patch.  Before it, a lost wakeup
only delayed preallocation until the next request.  Now it can hang
ploop_postsuspend().  The new ploop_should_prealloc() call in
ploop_preresume() also adds a new place to trigger it.

Could the allocator re-check prealloc_size after set_current_state(), or
could the wait be made bounded or killable?

[ ... ]

> 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

[ ... ]

>  static void ploop_postsuspend(struct dm_target *ti)
>  {
> +	struct ploop *ploop = ti->private;
> +
> +	/*
> +	 * Let the next resumed ploop (maybe of reloaded table)
> +	 * see final size of the top delta.
> +	 */
> +	ploop_wait_prealloc(ploop);
>  	ploop_set_suspended(ti, true);
>  }

[Severity: Medium]
Can ploop_postsuspend() block here indefinitely?

If the allocator loses the wakeup described above, ploop_wait_prealloc()
never returns and dmsetup suspend hangs in D state.  This is a new
dependency of the suspend path on the allocator making progress.

The follow-up patch "dm-ploop: truncate unused tail of top delta on
suspend" does not touch ploop_allocator() or ploop_wait_prealloc().  It
makes ploop_postsuspend() call ploop_wait_prealloc() under
down_write(&ctl_rwsem), so in the final series state the hang would also
block every other ctl_rwsem user (message, status, ploop_llseek_hole()).

Would it be possible to fix the allocator wakeup, or to avoid waiting while
holding ctl_rwsem for write?

[ ... ]

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

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