Good point, Phil. Before Sam's bug fix, the free(op-entry) was in the complete-op function. However, he didn't have the unregister() and I don't know why. So, maybe, putting the unregister() and free() outside of complete-op() IS the better way to handle it. It seems that there never was an unregister() and I'm wondering if that could have been causing problems as well, i.e., one reason why the bug fix was needed in the first place.
Becky -- Becky Ligon HPC Admin Staff PVFS/OrangeFS Developer Clemson University/Omnibond.com OrangeFS Support 864-650-4065 > That may be. The thing that is a little tricky is that technically the > mgmt code can be run in a handful of different modes (thread pool, > thread per op, polling etc.) and we weren't entirely sure if one of the > other modes would need the entry to survive longer than the > complete_op() function or not. This patch specifically fixes the leak > for the threaded-queue mode that trove-directio uses and ignores the > other cases since we have no convenient way to test them. > > I kind of doubt anyone will ever use the other modes, though. Maybe it > is more practical in the long term to do the cleanup inside of > complete_op, but then also get rid of the code for the other N modes to > simplify things and make it so we don't have to worry about the other > paths? > > -Phil > > On 07/07/2011 06:12 PM, Becky Ligon wrote: >> Yes. I agree. That's exactly what I had found, too, but I think that >> code belongs inside of the PINT_manager_complete_op function. Thoughts? >> >> Becky >> >> On Thu, Jul 7, 2011 at 5:06 PM, Benjamin Severs >> <[email protected] <mailto:[email protected]>> wrote: >> >> Thanks to some direction by Phil Carns, we've managed to correct >> the memory >> leak that was affecting directio operations. Below is the patch >> that addresses >> the issue. The problem was that once all the *fast* calls were >> switched to >> *safe* calls, the proper cleanup routines were never being called. >> We simply >> placed the cleanup logic that was present in the >> PINT_context_test_* methods >> after the PINT_manager_complete_op call. >> >> diff --git a/src/common/mgmt/pint-worker-threaded-queues.c >> b/src/common/mgmt/pint-worker-threaded-queues.c >> index 1359768..a088e3f 100644 >> --- src/common/mgmt/pint-worker-threaded-queues.c >> +++ src/common/mgmt/pint-worker-threaded-queues.c >> @@ -423,6 +423,7 @@ static void >> *PINT_worker_queues_thread_function(void * >> ptr) >> { >> for(i = 0; i < op_count; ++i) >> { >> + struct PINT_op_entry *op_entry; >> op = PINT_op_from_qentry(qentries[i]); >> /* service the operation */ >> ret = PINT_manager_service_op( >> @@ -440,6 +441,14 @@ static void >> *PINT_worker_queues_thread_function(void * >> ptr) >> /* fatal if we can't complete an op */ >> goto free_ops; >> } >> + >> + op_entry = id_gen_safe_lookup(op->id); >> + if(op_entry) >> + { >> + id_gen_safe_unregister(op_entry->op.id >> <http://op.id>); >> + free(op_entry); >> + op_entry = NULL; >> + } >> } >> } >> } >> >> -- >> Benjamin Severs >> _______________________________________________ >> Pvfs2-developers mailing list >> [email protected] >> <mailto:[email protected]> >> http://www.beowulf-underground.org/mailman/listinfo/pvfs2-developers >> >> >> >> >> -- >> Becky Ligon >> OrangeFS Support and Development >> Omnibond Systems >> Anderson, South Carolina >> >> >> >> _______________________________________________ >> Pvfs2-developers mailing list >> [email protected] >> http://www.beowulf-underground.org/mailman/listinfo/pvfs2-developers > > _______________________________________________ > Pvfs2-developers mailing list > [email protected] > http://www.beowulf-underground.org/mailman/listinfo/pvfs2-developers > _______________________________________________ Pvfs2-developers mailing list [email protected] http://www.beowulf-underground.org/mailman/listinfo/pvfs2-developers
