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

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

On 10/13/26 13:38, no-reply@sashiko.virtuozzo.com wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Low] dm-ploop: ploop_postsuspend() holds ctl_rwsem across unbounded waits and fs I/O
> --
>
> commit 1aa3a8ea27bee4e747c238df1ff3b02cb36cb3fd
> Author: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
>
> dm-ploop: truncate unused tail of top delta on suspend
>
> This patch adds ploop_calc_used_end() and ploop_truncate_unused_tail().
> ploop_postsuspend() now truncates the unused tail of the top delta. It
> does this under ctl_rwsem, so that ploop messages don't change the image
> in parallel.
>
> Link: https://virtuozzo.atlassian.net/browse/VSTOR-148171
>
>> 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;
>> +	unsigned int noio_flags;
>>   
>> +	down_write(&ploop->ctl_rwsem);
>>   	/*
>>   	 * Let the next resumed ploop (maybe of reloaded table)
>>   	 * see final size of the top delta. Don't wait in standby
> [ ... ]
>
>>   	wait_event(ploop->inflight_md_wq, !atomic_read(&ploop->inflight_md));
>> -	ploop_set_suspended(ti, true);
>> +	/* The device is suspended, see ploop_preresume() */
>> +	noio_flags = memalloc_noio_save();
>> +	ploop_truncate_unused_tail(ploop);
>> +	memalloc_noio_restore(noio_flags);
>> +	ploop->suspended = true;
>> +	up_write(&ploop->ctl_rwsem);
>>   }
> [Severity: Low]
> Is it necessary to take ctl_rwsem for write before the waits in
> ploop_postsuspend()?
>
> The write lock is now taken before the killable ploop_wait_prealloc() and
> the uninterruptible wait_event(ploop->inflight_md_wq, ...), and it stays
> held through ploop_truncate_unused_tail(), which calls vfs_iter_read()
> (via ploop_calc_used_end()), vfs_fsync() and do_ftruncate().
>
> Before this patch the lock was only taken briefly in ploop_set_suspended(),
> after both waits. Those waits could already block for a long time. In
> standby, or with a dead or stuck backing file, inflight_md may never reach
> zero. Now other ctl_rwsem users block behind them as well:
>
> ploop_status()
>      down_read(&ploop->ctl_rwsem);   /* STATUSTYPE_TABLE */
>
> ploop_message()
>      if (read)
>          down_read(&ploop->ctl_rwsem);
>      else
>          down_write(&ploop->ctl_rwsem);
>
> Every message handler except get_event would hang while postsuspend is
> stalled. The same stall can come from a slow vfs_fsync() or do_ftruncate()
> on a stuck filesystem.
>
> I don't see a deadlock. Nothing on the inflight_md completion path or in
> the allocator takes ctl_rwsem. dm_suspend() also already hangs in that
> state with md->suspend_lock held, so the practical impact looks small.
>
> The two waits don't appear to need the lock. Only
> ploop_truncate_unused_tail() and the store to ploop->suspended do. Would
> it make sense to take the lock after the waits? That would avoid
> extending the blocking to status and message handlers while the wait
> phase is stuck.
The extra blocking only happens when postsuspend is already stuck: 
dm_suspend() hangs with md->suspend_lock held and the device can't be 
recovered either way. Holding the lock across the waits keeps messages 
(e.g. resize) from slipping in between the waits and the truncation and 
leaving new preallocation behind, which would make us skip the 
truncation. So I'd keep it as is.
_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel