On Thu, Apr 30, 2015 at 10:51 AM, Arne Jansen <[email protected]> wrote:
> On 30/04/15 17:08, Dan McDonald wrote: > > >> * You do a taskq_wait() in dmu_objset_find_dp() prior to destroying the >> taskq. This is fine for waiting on the dispatch that function issues. >> OTOH, dmu_objset_find_dp_impl(), which is called by >> dmu_objset_find_dp_cb(), may dispatch many more tasks on that taskq, even >> recursively dispatching more. At least for the ddi versions of these >> functions, it says: >> >> The ddi_taskq_destroy() function waits for any scheduled tasks to >> complete, then destroys the taskq. The caller should guarantee >> that no >> new tasks are scheduled for the closing taskq. >> >> The ddi_taskq_wait() function waits for all previously scheduled >> tasks >> to complete. Note that this function does not stop any new task >> dispatches. >> >> Isn't it possible that if you've a lot of children, the >> ddi_taskq_destroy() at the end of dmu_objset_find_dp() may actually be >> destroying a taskq that may have tasks WHICH SCHEDULE MORE TASKS? >> >> > Judging from that description I'd agree that this could be a problem. > But reading the code I'd state the function of taskq_wait more > like: > taskq_wait() waits until no more tasks are scheduled and all running > tasks finished. > It would only be a problem if anyone would schedule a task after all > running tasks have finished, but that doesn't happen as we always > schedule new tasks from within running tasks. > > The code in taskq_wait looks like this: > > mutex_enter(&tq->tq_lock); > while (tq->tq_task.tqent_next != &tq->tq_task || tq->tq_active != 0) > cv_wait(&tq->tq_wait_cv, &tq->tq_lock); > mutex_exit(&tq->tq_lock); > > From this I'd judge we're safe, although I have to admit I haven't read > the full taskq.c. > > I think this analysis is correct. (Note, however, that I think you need the explicit call to taskq_wait(), and can't rely on taskq_destroy() in this case, contrary to George's suggestion.) However, it seems that the taskq implementation on other platforms does not work this way, per Justin Gibbs' comments on a different bug: "Change eviction processing to handle recursive evictions from within a single taskq thread so a single call to taskq_wait() works even on platforms like FreeBSD that ignore taskq entries that are queued to a taskq after taskq_wait() is invoked (which guarantees taskq_wait() cannot suffer from live lock)." This is from https://reviews.csiden.org/r/131/ So I think this code is OK for illumos, but it does create a bit of a time bomb for other platforms that use ZFS. I'm not sure the best way to address this... If we're going to put the burden on other platforms to change their taskq_wait, we should at least document ours as working in this situation. Alternatively, we could essentially implement our own taskq_wait() that first waits for the number of active tasks to go to zero (tracking the number of active tasks from within the callback, so that it works for any taskq implementation). This seems kind of gross. Maybe instead FreeBSD should have a complete wrapper around its taskq interfaces that implements the count of active tasks? --matt -Arne > > > > > ------------------------------------------- > illumos-zfs > Archives: https://www.listbox.com/member/archive/182191/=now > RSS Feed: > https://www.listbox.com/member/archive/rss/182191/21635000-ebd1d460 > Modify Your Subscription: > https://www.listbox.com/member/?member_id=21635000&id_secret=21635000-73dc201a > Powered by Listbox: http://www.listbox.com >
_______________________________________________ developer mailing list [email protected] http://lists.open-zfs.org/mailman/listinfo/developer
