From: Alex Elder <elder@inktank.com>
To: Sage Weil <sage@inktank.com>
Cc: ceph-devel@vger.kernel.org
Subject: Re: [PATCH 07/13] libceph: embed ceph connection structure in mon_client
Date: Fri, 01 Jun 2012 07:12:13 -0500 [thread overview]
Message-ID: <4FC8B19D.2080204@inktank.com> (raw)
In-Reply-To: <Pine.LNX.4.64.1205312108540.2166@cobra.newdream.net>
On 05/31/2012 11:24 PM, Sage Weil wrote:
> On Wed, 30 May 2012, Alex Elder wrote:
>> A monitor client has a pointer to a ceph connection structure in it.
>> This is the only one of the three ceph client types that do it this
>> way; the OSD and MDS clients embed the connection into their main
>> structures. There is always exactly one ceph connection for a
>> monitor client, so there is no need to allocate it separate from the
>> monitor client structure.
>>
>> So switch the ceph_mon_client structure to embed its
>> ceph_connection structure.
>>
>> Signed-off-by: Alex Elder<elder@inktank.com>
>> ---
>> include/linux/ceph/mon_client.h | 2 +-
>> net/ceph/mon_client.c | 47 ++++++++++++++++----------------------
>> 2 files changed, 21 insertions(+), 28 deletions(-)
>>
>> diff --git a/include/linux/ceph/mon_client.h b/include/linux/ceph/mon_client.h
>> index 545f859..2113e38 100644
>> --- a/include/linux/ceph/mon_client.h
>> +++ b/include/linux/ceph/mon_client.h
>> @@ -70,7 +70,7 @@ struct ceph_mon_client {
>> bool hunting;
>> int cur_mon; /* last monitor i contacted */
>> unsigned long sub_sent, sub_renew_after;
>> - struct ceph_connection *con;
>> + struct ceph_connection con;
>> bool have_fsid;
>>
>> /* pending generic requests */
>> diff --git a/net/ceph/mon_client.c b/net/ceph/mon_client.c
>> index 704dc95..ac4d6b1 100644
>> --- a/net/ceph/mon_client.c
>> +++ b/net/ceph/mon_client.c
>> @@ -106,9 +106,9 @@ static void __send_prepared_auth_request(struct
>> ceph_mon_client *monc, int len)
>> monc->pending_auth = 1;
>> monc->m_auth->front.iov_len = len;
>> monc->m_auth->hdr.front_len = cpu_to_le32(len);
>> - ceph_con_revoke(monc->con, monc->m_auth);
>> + ceph_con_revoke(&monc->con, monc->m_auth);
>> ceph_msg_get(monc->m_auth); /* keep our ref */
>> - ceph_con_send(monc->con, monc->m_auth);
>> + ceph_con_send(&monc->con, monc->m_auth);
>> }
>>
>> /*
>> @@ -117,8 +117,8 @@ static void __send_prepared_auth_request(struct
>> ceph_mon_client *monc, int len)
>> static void __close_session(struct ceph_mon_client *monc)
>> {
>> dout("__close_session closing mon%d\n", monc->cur_mon);
>> - ceph_con_revoke(monc->con, monc->m_auth);
>> - ceph_con_close(monc->con);
>> + ceph_con_revoke(&monc->con, monc->m_auth);
>> + ceph_con_close(&monc->con);
>> monc->cur_mon = -1;
>> monc->pending_auth = 0;
>> ceph_auth_reset(monc->auth);
>> @@ -142,9 +142,9 @@ static int __open_session(struct ceph_mon_client *monc)
>> monc->want_next_osdmap = !!monc->want_next_osdmap;
>>
>> dout("open_session mon%d opening\n", monc->cur_mon);
>> - monc->con->peer_name.type = CEPH_ENTITY_TYPE_MON;
>> - monc->con->peer_name.num = cpu_to_le64(monc->cur_mon);
>> - ceph_con_open(monc->con,
>> + monc->con.peer_name.type = CEPH_ENTITY_TYPE_MON;
>> + monc->con.peer_name.num = cpu_to_le64(monc->cur_mon);
>> + ceph_con_open(&monc->con,
>> &monc->monmap->mon_inst[monc->cur_mon].addr);
>>
>> /* initiatiate authentication handshake */
>> @@ -226,8 +226,8 @@ static void __send_subscribe(struct ceph_mon_client *monc)
>>
>> msg->front.iov_len = p - msg->front.iov_base;
>> msg->hdr.front_len = cpu_to_le32(msg->front.iov_len);
>> - ceph_con_revoke(monc->con, msg);
>> - ceph_con_send(monc->con, ceph_msg_get(msg));
>> + ceph_con_revoke(&monc->con, msg);
>> + ceph_con_send(&monc->con, ceph_msg_get(msg));
>>
>> monc->sub_sent = jiffies | 1; /* never 0 */
>> }
>> @@ -247,7 +247,7 @@ static void handle_subscribe_ack(struct ceph_mon_client
>> *monc,
>> if (monc->hunting) {
>> pr_info("mon%d %s session established\n",
>> monc->cur_mon,
>> - ceph_pr_addr(&monc->con->peer_addr.in_addr));
>> + ceph_pr_addr(&monc->con.peer_addr.in_addr));
>> monc->hunting = false;
>> }
>> dout("handle_subscribe_ack after %d seconds\n", seconds);
>> @@ -461,7 +461,7 @@ static int do_generic_request(struct ceph_mon_client
>> *monc,
>> req->request->hdr.tid = cpu_to_le64(req->tid);
>> __insert_generic_request(monc, req);
>> monc->num_generic_requests++;
>> - ceph_con_send(monc->con, ceph_msg_get(req->request));
>> + ceph_con_send(&monc->con, ceph_msg_get(req->request));
>> mutex_unlock(&monc->mutex);
>>
>> err = wait_for_completion_interruptible(&req->completion);
>> @@ -684,8 +684,8 @@ static void __resend_generic_request(struct
>> ceph_mon_client *monc)
>>
>> for (p = rb_first(&monc->generic_request_tree); p; p = rb_next(p)) {
>> req = rb_entry(p, struct ceph_mon_generic_request, node);
>> - ceph_con_revoke(monc->con, req->request);
>> - ceph_con_send(monc->con, ceph_msg_get(req->request));
>> + ceph_con_revoke(&monc->con, req->request);
>> + ceph_con_send(&monc->con, ceph_msg_get(req->request));
>> }
>> }
>>
>> @@ -705,7 +705,7 @@ static void delayed_work(struct work_struct *work)
>> __close_session(monc);
>> __open_session(monc); /* continue hunting */
>> } else {
>> - ceph_con_keepalive(monc->con);
>> + ceph_con_keepalive(&monc->con);
>>
>> __validate_auth(monc);
>>
>> @@ -760,19 +760,16 @@ int ceph_monc_init(struct ceph_mon_client *monc, struct
>> ceph_client *cl)
>> goto out;
>>
>> /* connection */
>> - monc->con = kmalloc(sizeof(*monc->con), GFP_KERNEL);
>> - if (!monc->con)
>> - goto out_monmap;
>> - ceph_con_init(&monc->client->msgr, monc->con);
>> - monc->con->private = monc;
>> - monc->con->ops =&mon_con_ops;
>> + ceph_con_init(&monc->client->msgr,&monc->con);
>> + monc->con.private = monc;
>> + monc->con.ops =&mon_con_ops;
>>
>> /* authentication */
>> monc->auth = ceph_auth_init(cl->options->name,
>> cl->options->key);
>> if (IS_ERR(monc->auth)) {
>> err = PTR_ERR(monc->auth);
>> - goto out_con;
>> + goto out_monmap;
>> }
>> monc->auth->want_keys =
>> CEPH_ENTITY_TYPE_AUTH | CEPH_ENTITY_TYPE_MON |
>> @@ -824,8 +821,6 @@ out_subscribe_ack:
>> ceph_msg_put(monc->m_subscribe_ack);
>> out_auth:
>> ceph_auth_destroy(monc->auth);
>> -out_con:
>> - monc->con->ops->put(monc->con);
>
> AH!
>
> This reminds me, these connections need to be refcounted. There's a
> ->get() and ->put() op defined so that you can refcount the containing
> structure. That means that this patch needs to alo change
>
> static const struct ceph_connection_operations mon_con_ops = {
> .get = ceph_con_get,
> .put = ceph_con_put,
>
> in mon_client.c. Hopefully the mon_client itself is refcounted, *or* we
> can ensure that it won't go away before the msgr workqueue is drained and
> the get/put ops can turn to no-ops.
Earlier I looked at the ref counting stuff a bit and stopped myself
from going off on that tangent. But it didn't look like it was used
consistently and made a note to myself to revisit it.
> Also: when poking around, I noticed that ceph_con_get() and put() are
> called directly from osd_client.c... that's a bug! Those connections have
> a get and put op defined that twiddles the containing ceph_osd struct's
> ref count.
>
> I pushed several patches to your latest (wip-messenger-2) branch that fix
> these issues. Compile tested only! The first should probably be folded
> into this one, the others follow.
I'll look at your patches and incorporate them as appropriate. But at
the moment I don't see them; whenever you are back online again perhaps
you'll send me a link.
-Alex
>
>> out_monmap:
>> kfree(monc->monmap);
>> out:
>> @@ -841,9 +836,7 @@ void ceph_monc_stop(struct ceph_mon_client *monc)
>> mutex_lock(&monc->mutex);
>> __close_session(monc);
>>
>> - monc->con->private = NULL;
>> - monc->con->ops->put(monc->con);
>> - monc->con = NULL;
>> + monc->con.private = NULL;
>>
>> mutex_unlock(&monc->mutex);
>>
>> @@ -1021,7 +1014,7 @@ static void mon_fault(struct ceph_connection *con)
>> if (!monc->hunting)
>> pr_info("mon%d %s session lost, "
>> "hunting for new mon\n", monc->cur_mon,
>> - ceph_pr_addr(&monc->con->peer_addr.in_addr));
>> + ceph_pr_addr(&monc->con.peer_addr.in_addr));
>>
>> __close_session(monc);
>> if (!monc->hunting) {
>> --
>> 1.7.5.4
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>>
>
next prev parent reply other threads:[~2012-06-01 12:12 UTC|newest]
Thread overview: 44+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-05-30 19:24 [PATCH 00/13] libceph: cleanups preparing for state cleanup Alex Elder
2012-05-30 19:34 ` [PATCH 01/13] libceph: eliminate connection state "DEAD" Alex Elder
2012-05-31 16:20 ` Yehuda Sadeh
2012-05-30 19:34 ` [PATCH 02/13] libceph: kill bad_proto ceph connection op Alex Elder
2012-05-31 16:30 ` Yehuda Sadeh
2012-05-30 19:34 ` [PATCH 03/13] libceph: delete useless SOCK_CLOSED manipulations Alex Elder
2012-06-01 18:47 ` Alex Elder
2012-05-30 19:34 ` [PATCH 04/13] libceph: rename socket callbacks Alex Elder
2012-05-31 16:33 ` Yehuda Sadeh Weinraub
2012-06-01 4:02 ` Sage Weil
2012-05-30 19:34 ` [PATCH 05/13] libceph: rename kvec_reset and kvec_add functions Alex Elder
2012-05-31 16:34 ` Yehuda Sadeh
2012-06-01 4:02 ` Sage Weil
2012-05-30 19:34 ` [PATCH 06/13] libceph: embed ceph messenger structure in ceph_client Alex Elder
2012-05-31 16:44 ` Yehuda Sadeh
2012-06-01 4:04 ` Sage Weil
2012-05-30 19:34 ` [PATCH 07/13] libceph: embed ceph connection structure in mon_client Alex Elder
2012-06-01 4:24 ` Sage Weil
2012-06-01 12:12 ` Alex Elder [this message]
2012-06-01 13:30 ` Alex Elder
2012-06-01 16:20 ` Sage Weil
2012-06-01 16:32 ` Alex Elder
2012-06-01 16:39 ` Sage Weil
2012-06-01 17:09 ` Alex Elder
2012-06-01 17:10 ` Sage Weil
2012-05-30 19:35 ` [PATCH 08/13] libceph: start separating connection flags from state Alex Elder
2012-06-01 4:25 ` Sage Weil
2012-06-01 12:13 ` Alex Elder
2012-05-30 19:35 ` [PATCH 09/13] libceph: start tracking connection socket state Alex Elder
2012-06-01 4:28 ` Sage Weil
2012-06-01 12:15 ` Alex Elder
2012-06-12 4:52 ` Yan, Zheng
2012-06-12 5:00 ` Sage Weil
2012-06-12 5:02 ` Yan, Zheng
2012-06-12 16:58 ` Alex Elder
2012-06-13 1:50 ` Yan, Zheng
2012-05-30 19:35 ` [PATCH 10/13] libceph: provide osd number when creating osd Alex Elder
2012-06-01 4:29 ` Sage Weil
2012-05-30 19:35 ` [PATCH 11/13] libceph: init monitor connection when opening Alex Elder
2012-06-01 4:30 ` Sage Weil
2012-05-30 19:35 ` [PATCH 12/13] libceph: fully initialize connection in con_init() Alex Elder
2012-06-01 4:31 ` Sage Weil
2012-05-30 19:35 ` [PATCH 13/13] libceph: set CLOSED state bit in con_init Alex Elder
2012-06-01 4:32 ` Sage Weil
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4FC8B19D.2080204@inktank.com \
--to=elder@inktank.com \
--cc=ceph-devel@vger.kernel.org \
--cc=sage@inktank.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.