On Wed, May 13, 2009 at 05:42:23PM -0500, Steve Wise wrote: > Hey Bruce, > > J. Bruce Fields wrote: >> On Wed, Apr 29, 2009 at 02:14:00PM -0500, Steve Wise wrote: >> >>> These fixes resolved crashes due to resource leak BUG_ON checks. The >>> resource leaks were detected by introducing asynchronous transport errors. >>> >> >> Thanks, applied for 2.6.30. (And also appropriate for stable (2.6.29), >> I assume?) >> >> But, could someone take a closer look at the error paths here? Questions: >> >> - svc_rdma_post_recv() does a svc_rdma_put_context() on error-- >> are you sure its caller needs to as well? >> > > The svc_rdma_put_context() call inside svc_rdma_post_recv() is for the > recv context that was allocated inside that function. The caller, in > this case send_reply() also does a svc_rdma_put_context(), but that is > for the send context. So I think this is correct. > >> - In send_reply, some of the cleanout is shared between the >> first return -ENOTCONN and the final err: cleanup. Could we >> add another err: label and share some of that cleanup? >> > > The only common logic I see is the svc_rdma_put_context() call that > could be shared. But one case calls it with free_pages == 1 after the > pages have been mapped, and the other with 0 since no pages are mapped > at that point (when the call to svc_rdma_post_recv() fails). So I'm > not sure its worth doing? No, I think you're probably right about both of these. Thanks for taking a look. --b. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
