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

[Alexander Ivanov <alexander.ivanov@virtuozzo.com>]

> [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?
Wrapped ploop_reload_metadata() in ploop_preresume() with 
memalloc_noio_save()/restore() rather than switching to GFP_NOIO, so 
nested allocations (holes_bitmap, md pages, backing fs reads) are 
covered as well. The same applies to ploop_truncate_unused_tail() in 
ploop_postsuspend() in the next patch, fixed there too.

> [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?
Fixed.

>> +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?
Fixed in a separate patch "dm-ploop: fix lost wakeup in ploop_allocator()".

> [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?
The lost wakeup in ploop_allocator() is fixed earlier in this series by
"dm-ploop: fix lost wakeup in ploop_allocator()". Also this patch
switches the allocator to wake_up() for dispatcher_wq_prealloc, since
ploop_wait_prealloc() uses uninterruptible wait_event().

ploop_preallocate_cluster() always clears prealloc_size and
prealloc_in_progress, including the error path, and the allocator never
takes ctl_rwsem, so the wait is bounded by the fallocate itself.

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