On 2020-03-10 11:03, Ilya Dryomov wrote:
On Tue, Mar 10, 2020 at 10:09 AM Roman Penyaev <rpenyaev@xxxxxxx>
wrote:
OSD client allocates a message with a page vector for OSD_MAP,
OSD_BACKOFF
and WATCH_NOTIFY message types (see alloc_msg_with_page_vector()
caller),
but pages vector release is never called.
Signed-off-by: Roman Penyaev <rpenyaev@xxxxxxx>
Cc: Ilya Dryomov <idryomov@xxxxxxxxx>
Cc: Jeff Layton <jlayton@xxxxxxxxxx>
Cc: Sage Weil <sage@xxxxxxxxxx>
Cc: ceph-devel@xxxxxxxxxxxxxxx
---
net/ceph/messenger.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c
index 5b4bd8261002..28cbd55ec2e3 100644
--- a/net/ceph/messenger.c
+++ b/net/ceph/messenger.c
@@ -3248,8 +3248,15 @@ static struct ceph_msg_data
*ceph_msg_data_add(struct ceph_msg *msg)
static void ceph_msg_data_destroy(struct ceph_msg_data *data)
{
- if (data->type == CEPH_MSG_DATA_PAGELIST)
+ if (data->type == CEPH_MSG_DATA_PAGES) {
+ int num_pages;
+
+ num_pages = calc_pages_for(data->alignment,
+ data->length);
+ ceph_release_page_vector(data->pages, num_pages);
+ } else if (data->type == CEPH_MSG_DATA_PAGELIST) {
ceph_pagelist_release(data->pagelist);
+ }
}
void ceph_msg_data_add_pages(struct ceph_msg *msg, struct page
**pages,
Hi Roman,
I don't think there is a leak here.
osdmap and backoff messages don't have data.
I tried to be symmetric on this path: allocation path
exists, but there is no deallocation.
watch_notify message may or may not have data and this is dealt
with in handle_watch_notify(). The pages are either released in
handle_watch_notify() or transferred to ceph_osdc_notify() through
lreq. The caller of ceph_osdc_notify() is then responsible for
them:
* @preply_{pages,len} are initialized both on success and error.
* The caller is responsible for:
*
* ceph_release_page_vector(...)
*/
int ceph_osdc_notify(struct ceph_osd_client *osdc,
Thanks for taking a look. Then there is a rare and unobvious
leak, please check ceph_con_in_msg_alloc() in messenger.c.
Message can be allocated for osd client (->alloc_msg) and then
can be immediately put by the following path:
if (con->state != CON_STATE_OPEN) {
if (msg)
ceph_msg_put(msg);
(also few lines below where con->in_msg is put)
without reaching the dispatch and set of handle_* functions,
which you've referred.
And seems if the ownership of the ->pages is transferred to
the handle_watch_notify() and freed there, then it should be
fixed by having release in one place: here or there.
--
Roman
