| Message ID | 1453259619-5347-3-git-send-email-junxiao.bi@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 20 Jan 2016 11:13:35 +0800 Junxiao Bi <junxiao.bi@oracle.com> wrote: > This message is sent to master node when non-master nodes's > negotiate timer expired. Master node records these nodes in > a bitmap which is used to do write timeout timer re-queue > decision. > > ... > > +static int o2hb_nego_timeout_handler(struct o2net_msg *msg, u32 len, void *data, > + void **ret_data) > +{ > + struct o2hb_region *reg = (struct o2hb_region *)data; It's best not to typecast a void*. It's unneeded clutter and the cast can actually hide bugs - if someone changes `data' to a different type or if there's a different "data" in scope, etc. > + struct o2hb_nego_msg *nego_msg; > > + nego_msg = (struct o2hb_nego_msg *)msg->buf; > + if (nego_msg->node_num < O2NM_MAX_NODES) > + set_bit(nego_msg->node_num, reg->hr_nego_node_bitmap); > + else > + mlog(ML_ERROR, "got nego timeout message from bad node.\n"); > + > + return 0; > }
On 01/22/2016 07:47 AM, Andrew Morton wrote: > On Wed, 20 Jan 2016 11:13:35 +0800 Junxiao Bi <junxiao.bi@oracle.com> wrote: > >> This message is sent to master node when non-master nodes's >> negotiate timer expired. Master node records these nodes in >> a bitmap which is used to do write timeout timer re-queue >> decision. >> >> ... >> >> +static int o2hb_nego_timeout_handler(struct o2net_msg *msg, u32 len, void *data, >> + void **ret_data) >> +{ >> + struct o2hb_region *reg = (struct o2hb_region *)data; > > It's best not to typecast a void*. It's unneeded clutter and the cast > can actually hide bugs - if someone changes `data' to a different type > or if there's a different "data" in scope, etc. There are many kinds of messages in ocfs2 and each one needs a different type of "data", so it is made type void*. Thanks, Junxiao. > >> + struct o2hb_nego_msg *nego_msg; >> >> + nego_msg = (struct o2hb_nego_msg *)msg->buf; >> + if (nego_msg->node_num < O2NM_MAX_NODES) >> + set_bit(nego_msg->node_num, reg->hr_nego_node_bitmap); >> + else >> + mlog(ML_ERROR, "got nego timeout message from bad node.\n"); >> + >> + return 0; >> } >
On Fri, 22 Jan 2016 13:12:26 +0800 Junxiao Bi <junxiao.bi@oracle.com> wrote: > On 01/22/2016 07:47 AM, Andrew Morton wrote: > > On Wed, 20 Jan 2016 11:13:35 +0800 Junxiao Bi <junxiao.bi@oracle.com> wrote: > > > >> This message is sent to master node when non-master nodes's > >> negotiate timer expired. Master node records these nodes in > >> a bitmap which is used to do write timeout timer re-queue > >> decision. > >> > >> ... > >> > >> +static int o2hb_nego_timeout_handler(struct o2net_msg *msg, u32 len, void *data, > >> + void **ret_data) > >> +{ > >> + struct o2hb_region *reg = (struct o2hb_region *)data; > > > > It's best not to typecast a void*. It's unneeded clutter and the cast > > can actually hide bugs - if someone changes `data' to a different type > > or if there's a different "data" in scope, etc. > There are many kinds of messages in ocfs2 and each one needs a different > type of "data", so it is made type void*. What I mean is to do this: struct o2hb_region *reg = data; and not struct o2hb_region *reg = (struct o2hb_region *)data; Because the typecast is unneeded and is actually harmful. Imagine if someone goofed and had `int data;': no warning, runtime failure.
On 01/22/2016 01:45 PM, Andrew Morton wrote: > On Fri, 22 Jan 2016 13:12:26 +0800 Junxiao Bi <junxiao.bi@oracle.com> wrote: > >> On 01/22/2016 07:47 AM, Andrew Morton wrote: >>> On Wed, 20 Jan 2016 11:13:35 +0800 Junxiao Bi <junxiao.bi@oracle.com> wrote: >>> >>>> This message is sent to master node when non-master nodes's >>>> negotiate timer expired. Master node records these nodes in >>>> a bitmap which is used to do write timeout timer re-queue >>>> decision. >>>> >>>> ... >>>> >>>> +static int o2hb_nego_timeout_handler(struct o2net_msg *msg, u32 len, void *data, >>>> + void **ret_data) >>>> +{ >>>> + struct o2hb_region *reg = (struct o2hb_region *)data; >>> >>> It's best not to typecast a void*. It's unneeded clutter and the cast >>> can actually hide bugs - if someone changes `data' to a different type >>> or if there's a different "data" in scope, etc. >> There are many kinds of messages in ocfs2 and each one needs a different >> type of "data", so it is made type void*. > > What I mean is to do this: > > struct o2hb_region *reg = data; > > and not > > struct o2hb_region *reg = (struct o2hb_region *)data; > > Because the typecast is unneeded and is actually harmful. Imagine if someone > goofed and had `int data;': no warning, runtime failure. Oh, I see. Thank you. Will update this in V2. Thanks, Junxiao. >
On Wed, Jan 20, 2016 at 11:13:35AM +0800, Junxiao Bi wrote: > This message is sent to master node when non-master nodes's > negotiate timer expired. Master node records these nodes in > a bitmap which is used to do write timeout timer re-queue > decision. > > Signed-off-by: Junxiao Bi <junxiao.bi@oracle.com> > Reviewed-by: Ryan Ding <ryan.ding@oracle.com> > --- > fs/ocfs2/cluster/heartbeat.c | 66 +++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 65 insertions(+), 1 deletion(-) > > diff --git a/fs/ocfs2/cluster/heartbeat.c b/fs/ocfs2/cluster/heartbeat.c > index b601ee95de50..ecf8a5e21c38 100644 > --- a/fs/ocfs2/cluster/heartbeat.c > +++ b/fs/ocfs2/cluster/heartbeat.c > @@ -280,6 +280,10 @@ struct o2hb_region { > * being checked because we temporarily have to zero out the > * crc field. */ > struct o2hb_disk_heartbeat_block *hr_tmp_block; > + > + /* Message key for negotiate timeout message. */ > + unsigned int hr_key; > + struct list_head hr_handler_list; > }; > > struct o2hb_bio_wait_ctxt { > @@ -288,6 +292,14 @@ struct o2hb_bio_wait_ctxt { > int wc_error; > }; > > +enum { > + O2HB_NEGO_TIMEOUT_MSG = 1, > +}; > + > +struct o2hb_nego_msg { > + u8 node_num; > +}; > + > static void o2hb_write_timeout(struct work_struct *work) > { > int failed, quorum; > @@ -356,6 +368,24 @@ static void o2hb_disarm_timeout(struct o2hb_region *reg) > cancel_delayed_work_sync(®->hr_nego_timeout_work); > } > > +static int o2hb_send_nego_msg(int key, int type, u8 target) > +{ > + struct o2hb_nego_msg msg; > + int status, ret; > + > + msg.node_num = o2nm_this_node(); > +again: > + ret = o2net_send_message(type, key, &msg, sizeof(msg), > + target, &status); > + > + if (ret == -EAGAIN || ret == -ENOMEM) { > + msleep(100); > + goto again; > + } > + > + return ret; > +} > + > static void o2hb_nego_timeout(struct work_struct *work) > { > struct o2hb_region *reg = > @@ -384,8 +414,24 @@ static void o2hb_nego_timeout(struct work_struct *work) > /* approve negotiate timeout request. */ > } else { > /* negotiate timeout with master node. */ > + o2hb_send_nego_msg(reg->hr_key, O2HB_NEGO_TIMEOUT_MSG, > + master_node); > } > +} > + > +static int o2hb_nego_timeout_handler(struct o2net_msg *msg, u32 len, void *data, > + void **ret_data) > +{ > + struct o2hb_region *reg = (struct o2hb_region *)data; > + struct o2hb_nego_msg *nego_msg; > > + nego_msg = (struct o2hb_nego_msg *)msg->buf; > + if (nego_msg->node_num < O2NM_MAX_NODES) > + set_bit(nego_msg->node_num, reg->hr_nego_node_bitmap); > + else > + mlog(ML_ERROR, "got nego timeout message from bad node.\n"); > + > + return 0; > } > > static inline void o2hb_bio_wait_init(struct o2hb_bio_wait_ctxt *wc) > @@ -1493,6 +1539,7 @@ static void o2hb_region_release(struct config_item *item) > list_del(®->hr_all_item); > spin_unlock(&o2hb_live_lock); > > + o2net_unregister_handler_list(®->hr_handler_list); > kfree(reg); > } > > @@ -2039,13 +2086,30 @@ static struct config_item *o2hb_heartbeat_group_make_item(struct config_group *g > > config_item_init_type_name(®->hr_item, name, &o2hb_region_type); > > + /* this is the same way to generate msg key as dlm, for local heartbeat, > + * name is also the same, so make initial crc value different to avoid > + * message key conflict. > + */ > + reg->hr_key = crc32_le(reg->hr_region_num + O2NM_MAX_REGIONS, > + name, strlen(name)); > + INIT_LIST_HEAD(®->hr_handler_list); Looks no need to initilize ->hr_handler_list here? Thanks, Eric > + ret = o2net_register_handler(O2HB_NEGO_TIMEOUT_MSG, reg->hr_key, > + sizeof(struct o2hb_nego_msg), > + o2hb_nego_timeout_handler, > + reg, NULL, ®->hr_handler_list); > + if (ret) > + goto free; > + > ret = o2hb_debug_region_init(reg, o2hb_debug_dir); > if (ret) { > config_item_put(®->hr_item); > - goto free; > + goto free_handler; > } > > return ®->hr_item; > + > +free_handler: > + o2net_unregister_handler_list(®->hr_handler_list); > free: > kfree(reg); > return ERR_PTR(ret); > -- > 1.7.9.5 > > > _______________________________________________ > Ocfs2-devel mailing list > Ocfs2-devel@oss.oracle.com > https://oss.oracle.com/mailman/listinfo/ocfs2-devel >
On 01/25/2016 11:18 AM, Eric Ren wrote: >> >> > @@ -2039,13 +2086,30 @@ static struct config_item *o2hb_heartbeat_group_make_item(struct config_group *g >> > >> > config_item_init_type_name(®->hr_item, name, &o2hb_region_type); >> > >> > + /* this is the same way to generate msg key as dlm, for local heartbeat, >> > + * name is also the same, so make initial crc value different to avoid >> > + * message key conflict. >> > + */ >> > + reg->hr_key = crc32_le(reg->hr_region_num + O2NM_MAX_REGIONS, >> > + name, strlen(name)); >> > + INIT_LIST_HEAD(®->hr_handler_list); > Looks no need to initilize ->hr_handler_list here? Why? It is list head. Thanks, Junxiao.
On Mon, Jan 25, 2016 at 12:28:08PM +0800, Junxiao Bi wrote: > On 01/25/2016 11:18 AM, Eric Ren wrote: > >> > >> > @@ -2039,13 +2086,30 @@ static struct config_item *o2hb_heartbeat_group_make_item(struct config_group *g > >> > > >> > config_item_init_type_name(®->hr_item, name, &o2hb_region_type); > >> > > >> > + /* this is the same way to generate msg key as dlm, for local heartbeat, > >> > + * name is also the same, so make initial crc value different to avoid > >> > + * message key conflict. > >> > + */ > >> > + reg->hr_key = crc32_le(reg->hr_region_num + O2NM_MAX_REGIONS, > >> > + name, strlen(name)); > >> > + INIT_LIST_HEAD(®->hr_handler_list); > > Looks no need to initilize ->hr_handler_list here? > Why? It is list head. Oh, sorry, it should. Another trivial, the label name "free_handler" sounds little strange. How about just "unregister" or "unregister_handler"? Thanks, Eric > > Thanks, > Junxiao. > > _______________________________________________ > Ocfs2-devel mailing list > Ocfs2-devel@oss.oracle.com > https://oss.oracle.com/mailman/listinfo/ocfs2-devel >
diff --git a/fs/ocfs2/cluster/heartbeat.c b/fs/ocfs2/cluster/heartbeat.c index b601ee95de50..ecf8a5e21c38 100644 --- a/fs/ocfs2/cluster/heartbeat.c +++ b/fs/ocfs2/cluster/heartbeat.c @@ -280,6 +280,10 @@ struct o2hb_region { * being checked because we temporarily have to zero out the * crc field. */ struct o2hb_disk_heartbeat_block *hr_tmp_block; + + /* Message key for negotiate timeout message. */ + unsigned int hr_key; + struct list_head hr_handler_list; }; struct o2hb_bio_wait_ctxt { @@ -288,6 +292,14 @@ struct o2hb_bio_wait_ctxt { int wc_error; }; +enum { + O2HB_NEGO_TIMEOUT_MSG = 1, +}; + +struct o2hb_nego_msg { + u8 node_num; +}; + static void o2hb_write_timeout(struct work_struct *work) { int failed, quorum; @@ -356,6 +368,24 @@ static void o2hb_disarm_timeout(struct o2hb_region *reg) cancel_delayed_work_sync(®->hr_nego_timeout_work); } +static int o2hb_send_nego_msg(int key, int type, u8 target) +{ + struct o2hb_nego_msg msg; + int status, ret; + + msg.node_num = o2nm_this_node(); +again: + ret = o2net_send_message(type, key, &msg, sizeof(msg), + target, &status); + + if (ret == -EAGAIN || ret == -ENOMEM) { + msleep(100); + goto again; + } + + return ret; +} + static void o2hb_nego_timeout(struct work_struct *work) { struct o2hb_region *reg = @@ -384,8 +414,24 @@ static void o2hb_nego_timeout(struct work_struct *work) /* approve negotiate timeout request. */ } else { /* negotiate timeout with master node. */ + o2hb_send_nego_msg(reg->hr_key, O2HB_NEGO_TIMEOUT_MSG, + master_node); } +} + +static int o2hb_nego_timeout_handler(struct o2net_msg *msg, u32 len, void *data, + void **ret_data) +{ + struct o2hb_region *reg = (struct o2hb_region *)data; + struct o2hb_nego_msg *nego_msg; + nego_msg = (struct o2hb_nego_msg *)msg->buf; + if (nego_msg->node_num < O2NM_MAX_NODES) + set_bit(nego_msg->node_num, reg->hr_nego_node_bitmap); + else + mlog(ML_ERROR, "got nego timeout message from bad node.\n"); + + return 0; } static inline void o2hb_bio_wait_init(struct o2hb_bio_wait_ctxt *wc) @@ -1493,6 +1539,7 @@ static void o2hb_region_release(struct config_item *item) list_del(®->hr_all_item); spin_unlock(&o2hb_live_lock); + o2net_unregister_handler_list(®->hr_handler_list); kfree(reg); } @@ -2039,13 +2086,30 @@ static struct config_item *o2hb_heartbeat_group_make_item(struct config_group *g config_item_init_type_name(®->hr_item, name, &o2hb_region_type); + /* this is the same way to generate msg key as dlm, for local heartbeat, + * name is also the same, so make initial crc value different to avoid + * message key conflict. + */ + reg->hr_key = crc32_le(reg->hr_region_num + O2NM_MAX_REGIONS, + name, strlen(name)); + INIT_LIST_HEAD(®->hr_handler_list); + ret = o2net_register_handler(O2HB_NEGO_TIMEOUT_MSG, reg->hr_key, + sizeof(struct o2hb_nego_msg), + o2hb_nego_timeout_handler, + reg, NULL, ®->hr_handler_list); + if (ret) + goto free; + ret = o2hb_debug_region_init(reg, o2hb_debug_dir); if (ret) { config_item_put(®->hr_item); - goto free; + goto free_handler; } return ®->hr_item; + +free_handler: + o2net_unregister_handler_list(®->hr_handler_list); free: kfree(reg); return ERR_PTR(ret);