This repository has been archived by the owner on Feb 26, 2020. It is now read-only.
__taskq_destroy() hang #71
Assignees
Labels
Comments
|
Have you only seen this the one time we talked about? |
|
I seem to be able to reliably reproduce this issue running ZFS's zpios-sanity script on my ARCH VM. Here's another stack: And it looks to be hitting BUG_ON here: void kfree(const void *x)
{
struct page *page;
void *object = (void *)x;
trace_kfree(_RET_IP_, x);
if (unlikely(ZERO_OR_NULL_PTR(x)))
return;
page = virt_to_head_page(x);
if (unlikely(!PageSlab(page))) {
BUG_ON(!PageCompound(page));
kmemleak_free(x);
put_page(page);
return;
}
slab_free(page->slab, page, object, _RET_IP_);
}
EXPORT_SYMBOL(kfree);
|
|
Just a thought, but since your in a kmem_free() you might enable the basic spl debugging and the memory tracking. This will help you immediately catch any memory handling mistakes. In addition, adding an |
|
Hmm, this may be revealing.. The first time I ran zpios-sanity with debug enabled on the SPL side I hit this ASSERT: |
|
I wonder if I goofed something by introducing this change: @@ -481,10 +481,6 @@ taskq_thread(void *args)
if (pend_list) {
t = list_entry(pend_list->next, taskq_ent_t, tqent_list);
list_del_init(&t->tqent_list);
+ /* In order to support recursively dispatching a
+ * preallocated taskq_ent_t, tqent_id must be
+ * stored prior to executing tqent_func. */
+ id = t->tqent_id;
tqt->tqt_ent = t;
taskq_insert_in_order(tq, tqt);
tq->tq_nactive++;
@@ -497,6 +493,7 @@ taskq_thread(void *args)
tq->tq_nactive--;
list_del_init(&tqt->tqt_active_list);
tqt->tqt_ent = NULL;
- id = t->tqent_id;
task_done(tq, t);
/* When the current lowest outstanding taskqid is
|
|
That's a good start. It would be worthwhile to change that to an ASSERT3S so we can actually see the bogus values, perhaps the splat tests would hit this as well with debugging enabled. As for the change you mention I don't see how it could cause this unless the tqent_func is tinkering with the tqent_id which seems very unlikely. I'd be more suspicious of something like that taskq_insert_in_order() changes although those look good too. |
|
Well |
|
Well I ran the SPL's taskq splat tests with I also changed it to an ASSERT3S as you suggested and hit it again: |
|
So we're sure taskq_lowest_id() went backwards somehow. I think your idea about performing a cross check on taskq_lowest_id() and making sure we are really getting the lowest value is a good one. Perhaps we have an issue on the taskq_insert_in_order() side of things |
|
Can you provide a sanity check for me on this patch: diff --git a/module/spl/spl-taskq.c b/module/spl/spl-taskq.c
index b2b0e6c..1ac0e60 100644
--- a/module/spl/spl-taskq.c
+++ b/module/spl/spl-taskq.c
@@ -410,6 +410,9 @@ taskq_insert_in_order(taskq_t *tq, taskq_thread_t *tqt)
taskq_thread_t *w;
struct list_head *l;
+ taskq_thread_t *big = NULL;
+ taskq_thread_t *sml = NULL;
+
SENTRY;
ASSERT(tq);
ASSERT(tqt);
@@ -425,6 +428,14 @@ taskq_insert_in_order(taskq_t *tq, taskq_thread_t *tqt)
if (l == &tq->tq_active_list)
list_add(&tqt->tqt_active_list, &tq->tq_active_list);
+ list_for_each_prev(l, &tq->tq_active_list) {
+ sml = big;
+ big = list_entry(l, taskq_thread_t, tqt_active_list);
+ if (sml != NULL) {
+ ASSERT3S(big->tqt_ent->tqent_id, >, sml->tqt_ent->tqent_id);
+ }
+ }
+
SEXIT;
}
Am I traversing the list correctly (i.e. in the |
|
I must be confusing the order direction, I swapped the comparison operator and it passed. |
|
Right, your operator was just wrong. Using list_for_each_next instead of list_for_each_prev might make it easier to read. |
|
Ok, so it does appear the active list isn't staying sorted as it should.. I hit this: With this patch applied: diff --git a/module/spl/spl-taskq.c b/module/spl/spl-taskq.c
index b2b0e6c..79e0c9b 100644
--- a/module/spl/spl-taskq.c
+++ b/module/spl/spl-taskq.c
@@ -410,6 +410,9 @@ taskq_insert_in_order(taskq_t *tq, taskq_thread_t *tqt)
taskq_thread_t *w;
struct list_head *l;
+ taskq_thread_t *big = NULL;
+ taskq_thread_t *sml = NULL;
+
SENTRY;
ASSERT(tq);
ASSERT(tqt);
@@ -425,6 +428,14 @@ taskq_insert_in_order(taskq_t *tq, taskq_thread_t *tqt)
if (l == &tq->tq_active_list)
list_add(&tqt->tqt_active_list, &tq->tq_active_list);
+ list_for_each_prev(l, &tq->tq_active_list) {
+ big = sml;
+ sml = list_entry(l, taskq_thread_t, tqt_active_list);
+ if (big != NULL) {
+ ASSERT3S(big->tqt_ent->tqent_id, >=, sml->tqt_ent->tqent_id);
+ }
+ }
+
SEXIT;
}
|
|
With the taskq_thread id field changes we talked about, it passed the previous issues, but got caught up here: |
|
Hmm, yeah so far it seems that I may have picked up an old build artifact which caused the above assertion. I cleaned out my git repos ( |
|
Well, maybe I'm wrong about the |
|
Aha! So yes.. As we suspected, the flags going in are not necessarily the flags coming out. In this particular case, |
behlendorf
pushed a commit
to behlendorf/spl
that referenced
this issue
Dec 17, 2011
The taskq_t's active thread list is sorted based on its tqt_ent->tqent_id field. The list is kept sorted solely by inserting new taskq_thread_t's in their correct sorted location; no other means is used. This means that once inserted, if a taskq_thread_t's tqt_ent->tqent_id field changes, the list runs the risk of no longer being sorted. Prior to the introduction of the taskq_dispatch_prealloc() interface, this was not a problem as a taskq_ent_t actively being serviced under the old interface should always have a static tqent_id field. Thus, once the taskq_thread_t is added to the taskq_t's active thread list, the taskq_thread_t's tqt_ent->tqent_id field would remain constant. Now, this is no longer the case. Currently, if using the taskq_dispatch_prealloc() interface, any given taskq_ent_t actively being serviced _may_ have its tqent_id value incremented. This happens when the preallocated taskq_ent_t structure is recursively dispatched. Thus, a taskq_thread_t could potentially have its tqt_ent->tqent_id field silently modified from under its feet. If this were to happen to a taskq_thread_t on a taskq_t's active thread list, this would compromise the integrity of the order of the list (as the list _may_ no longer be sorted). To get around this, the taskq_thread_t's taskq_ent_t pointer was replaced with its own static copy of the tqent_id. So, as a taskq_ent_t is pulled off of the taskq_t's pending list, a static copy of its tqent_id is made and this copy is used to sort the active thread list. Using a static copy is key in ensuring the integrity of the order of the active thread list. Even if the underlying taskq_ent_t is recursively dispatched (as has its tqent_id modified), this static copy stored inside the taskq_thread_t will remain constant. Signed-off-by: Prakash Surya <surya1@llnl.gov> Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov> Issue openzfs#71
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
The following hang was observed (rarely) after running all of xfstests when it was unloading the zfs modules and destroying the pools. It looks like this was accidentally introduced by the recent taskq optimization is issue #65. There appears to be a case where we're waiting for a work it we think was queued but never gets executed.
The text was updated successfully, but these errors were encountered: