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 -- dm-devel mailing list dm-devel@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/dm-devel
