dmu_redact.c does not call bqueue_destroy #11684
Conversation
Signed-off-by: Jorgen Lundman <lundman@lundman.net>
|
@pcd1193182 would you mind taking a look at this. |
| for (int i = 0; i < num_threads; i++) { | ||
| struct redact_thread_arg *targ = &thread_args[i]; | ||
| bqueue_destroy(&targ->q); | ||
| } |
There was a problem hiding this comment.
It looks to me like we don't need this chunk. In dmu_redact_snap()->perform_redaction() we should wait until the redact_merge_thread completes at which point we'll exit perform_redaction() and destroy the bqueue with you other addition. Though I could be mistaken, @pcd1193182 can you confirm this?
There was a problem hiding this comment.
This should work, though I would move it up into the previous for loop where we call avl_remove for each worker thread.
This is destroying the worker threads' bqueues, which are different from the queue from the merge thread to the main thread, so we do need to have a separate destroy call.
There was a problem hiding this comment.
@pcd1193182 thanks for taking a look at this. @lundman if you can refresh the patch to incorporate Paul's feedback and rebase it on master we can get this merged.
| for (int i = 0; i < num_threads; i++) { | ||
| struct redact_thread_arg *targ = &thread_args[i]; | ||
| bqueue_destroy(&targ->q); | ||
| } |
There was a problem hiding this comment.
This should work, though I would move it up into the previous for loop where we call avl_remove for each worker thread.
This is destroying the worker threads' bqueues, which are different from the queue from the merge thread to the main thread, so we do need to have a separate destroy call.
mmaybee
added
Status: Revision Needed
|
Could you address Paul's feedback and we can get this merged. |
|
Oh yeah, we deforked. I'll have to open a new PR |
Ensure all calls to
bqueue_init()has a corresponding call tobqueue_destroy().Motivation and Context
Leaking resources, avoiding panics.
Description
Weirdest fluke of things here, the bqueue structure would not call
bqueue_destroy()which means the mutex members inside are nevermutex_destroy()ed either. Memory is freed and later on re-used, in my case, but entirely unrelated memory, however, by chance the new memory use happened to have a mutex in the same offset, and would then panic due tomutex already initialised.I had to guess a bit on the locations of the
bqueue_destroy()calls, so hopefully I got it right.How Has This Been Tested?
Types of changes
Checklist:
Signed-off-by.