> On Mar 2, 2016, at 9:10 AM, Ilya Dryomov <idryomov@xxxxxxxxx> wrote: > > On Wed, Mar 2, 2016 at 2:59 PM, Douglas Fuller <dfuller@xxxxxxxxxx> wrote: >> >>> On Mar 2, 2016, at 6:26 AM, Ilya Dryomov <idryomov@xxxxxxxxx> wrote: >>> >> >>> I also spotted two memory leaks on the NOTIFY_COMPLETE path in >>> __do_event(). The event one is trivial, >> >> We cannot free the event here because CEPH_WATCH_EVENT_NOTIFY_COMPLETE fills values needed by the caller that may be waiting on it. The waiter has to call ceph_osdc_cancel_event when done with it. > > My ceph_osdc_put_event() doesn't free the event, it just puts a ref > that was acquired above with get_event(). That ref should be put by a call to ceph_osdc_cancel_event in that particular case. It looks like the event ref counting may need attention, though. I’ll look further. >>> the page vector one I have >>> a question about. The data item is allocated in alloc_msg() and the >>> actual buffer is then passed into __do_event() and eventually into >>> rbd_send_async_notify(), but not further up the stack. Is anything >>> going to use it? If not, we should remove it entirely. >> >> The incoming message may contain the data item. In the case of CEPH_WATCH_EVENT_NOTIFY_COMPLETE, rbd does not expect any data from the OSD, only the return code. The protocol allows it to be present, though, so osd_client should handle this case. > > If there won't be any users of it on the kernel client side, there's no > point in allocating the data item, extracting that data and propagating > it half way up, only to discard the data and free the data item. alloc_msg will have to allocate and the data item anyway, because CEPH_MSG_WATCH_NOTIFY might contain one. We only know that krbd is not expecting one iff the notify opcode is CEPH_WATCH_EVENT_NOTIFY_COMPLETE, but we do not decode that until later. We could do as you suggest in your patch and free the data item there. It should be empty anyway for these cases, so this will not save memory. We could shrink the ceph_osd_event struct by a pointer and a size_t, though. >> Yes. I thought you promised not to look at the garbage/junk commits, where that line is. > > I looked at the entire diff - it was blowing past 1G of memory, after > all ;) Your test was, or the diff was? ;) -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
