provider: don't idle on single thread each device for multithread probe all devices#1459
provider: don't idle on single thread each device for multithread probe all devices#1459GermanAizek wants to merge 1 commit into
Conversation
…ead probe all devices
tbzatek
left a comment
tbzatek
left a comment
There was a problem hiding this comment.
I started contribute to your project first time
This is actually your fifth pull request, most of which I refused to merge due to excessive microoptimization. Anyway, this PR looks to be a good addition! For CDROM, ATA or NVMe devices where additional I/O is done as part of UDisksLinuxDevice probing this may significantly speedup multiple device uevents handling. For other uevents this brings additional overhead of creating new threads that perform very little to no tasks, but I think that should be okay.
I was also thinking about a compromise of a selective GTask submission for ATA and NVMe devices, however that would involve additional udev attr checks, bringing additional overhead for every uevent itself. So I think your approach is a good compromise for now.
That said, we have never done any real-world performance profiling other than scenarios like 5000 scsi_debug devices appearing at the same time or 10k LVs activated at once, but that doesn't really reflect reality.
Either way, the most important rule here is to always spawn the on_idle_with_probed_uevent() callback in the main thread since that's when UDisksObject creation starts and the code that follows is not thread safe. And that is unchanged and working fine with this proposal. So full paralelization like that is mostly impossible at the current state.
| /* probe the device - this may take a while */ | ||
| request->udisks_device = udisks_linux_device_new_sync (request->udev_device, provider->gudev_client); | ||
| { | ||
| probe_request_free (request); |
There was a problem hiding this comment.
Good catch!
I make separate PR with this fix leak.
|
|
||
| /* now that we've probed the device, post the request back to the main thread */ | ||
| g_idle_add (on_idle_with_probed_uevent, request); | ||
| GTask *task = g_task_new (provider, NULL, on_probe_job_completed, NULL); |
There was a problem hiding this comment.
Please move the variable declaration to the top of the function along to others.
| probe_device_thread_func (GTask *task, gpointer source_object, gpointer task_data, GCancellable *cancellable) | ||
| { | ||
| ProbeRequest *request = task_data; | ||
| request->udisks_device = udisks_linux_device_new_sync (request->udev_device, udisks_linux_provider_get_udev_client(request->provider)); |
There was a problem hiding this comment.
Missing space before the opening parenthesis.
| /* now that we've probed the device, post the request back to the main thread */ | ||
| g_idle_add (on_idle_with_probed_uevent, request); | ||
| GTask *task = g_task_new (provider, NULL, on_probe_job_completed, NULL); | ||
| g_task_set_task_data (task, request, (GDestroyNotify) probe_request_free); |
There was a problem hiding this comment.
probe_request_free() is called within on_idle_with_probed_uevent(), this will actually result in double-free. I believe the destroy func is only called when replacing existing data, i.e. when this function is called twice and more, which is not the case here.
There was a problem hiding this comment.
probe_request_free()is called withinon_idle_with_probed_uevent(), this will actually result in double-free. I believe the destroy func is only called when replacing existingdata, i.e. when this function is called twice and more, which is not the case here.
Do you have any assumptions on how to get around double free, maybe do the check inside function?
There was a problem hiding this comment.
Just pass NULL here as the thread func will always succeed.
|
Jenkins, ok to test. |
2 participants
@tbzatek, I started contribute to your project first time, but probe is not very dependent and can be parallelized to go through process probing all block devices faster.