On 2020/7/16 1:41, Martin Wilck wrote: > On Tue, 2020-07-14 at 17:22 -0500, Benjamin Marzinski wrote: >> On Mon, Jul 13, 2020 at 09:59:43PM +0200, Martin Wilck wrote: >>> Comments welcome. >> >> We've agreed that dm_lib_release() is unnecessary, since we always >> call >> dm_udev_wait() when update_devs() is needed. dm_lib_exit() should >> already be safe, since we only call it after cleaning up the other >> threads. dm_task_set_cookie() looks safe to me as well, assuming that >> we >> have early initialization. So we are really talking about >> dm_task_run() >> and dm_udev_wait(). Clearly dm_udev_wait() always needs to run under >> a >> lock. Two threads trying to remove items from a list at the same time >> isn't safe, and there's no way to know if there will be items on the >> _node_ops list. The one thing I'm not sure of, is whether every call >> to >> dm_task_run() always needs to be locked. clearly we could, and it >> would >> be safer. Also, clearly for calls that add elements to _node_ops it >> must. But for calls like DM_DEVICE_TABLE or DM_DEVICE_LIST, the only >> issue that I can see with concurrent calls is the possibility that >> _ioctl_buffer_double_factor gets increased too much. Perhaps that's >> enough of a problem that we should just lock all dm_task_run() calls >> as >> well. > > Yes. We shouldn't try to be too smart. It's easier to verify that every > call is wrapped under a lock than to figure out later whether we made > the case distinction right. > > So, action plan: > > - remove dm_lib_release() calls > - Combine all possibly racy calls related to libdm initialization in > some early init call, protected by pthread_once() > - make sure dm_lib_exit() is handled correctly > - add locking around dm_task_run() and dm_udev_wait() > > Regards > Martin > Thanks for your detailed analysis. Look forward to the new version with solving the issue. Regards, Zhiqiang Liu -- dm-devel mailing list dm-devel@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/dm-devel
