* [PATCH] dlm: remove unnecessary error check @ 2015-06-09 9:46 Guoqing Jiang 2015-06-09 12:09 ` [Cluster-devel] " Bob Peterson 0 siblings, 1 reply; 11+ messages in thread From: Guoqing Jiang @ 2015-06-09 9:46 UTC (permalink / raw) To: ccaulfie, teigland; +Cc: cluster-devel, linux-kernel, Guoqing Jiang We don't need the redundant logic since send_message always returns 0. Signed-off-by: Guoqing Jiang <gqjiang@suse.com> --- fs/dlm/lock.c | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/fs/dlm/lock.c b/fs/dlm/lock.c index 35502d4..6fc3de9 100644 --- a/fs/dlm/lock.c +++ b/fs/dlm/lock.c @@ -3656,10 +3656,7 @@ static int send_common(struct dlm_rsb *r, struct dlm_lkb *lkb, int mstype) send_args(r, lkb, ms); - error = send_message(mh, ms); - if (error) - goto fail; - return 0; + return send_message(mh, ms); fail: remove_from_waiters(lkb, msg_reply_type(mstype)); @@ -3763,10 +3760,7 @@ static int send_lookup(struct dlm_rsb *r, struct dlm_lkb *lkb) send_args(r, lkb, ms); - error = send_message(mh, ms); - if (error) - goto fail; - return 0; + return send_message(mh, ms); fail: remove_from_waiters(lkb, DLM_MSG_LOOKUP_REPLY); -- 1.7.12.4 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [Cluster-devel] [PATCH] dlm: remove unnecessary error check 2015-06-09 9:46 [PATCH] dlm: remove unnecessary error check Guoqing Jiang @ 2015-06-09 12:09 ` Bob Peterson 2015-06-10 2:39 ` Guoqing Jiang 0 siblings, 1 reply; 11+ messages in thread From: Bob Peterson @ 2015-06-09 12:09 UTC (permalink / raw) To: Guoqing Jiang; +Cc: ccaulfie, teigland, cluster-devel, linux-kernel ----- Original Message ----- > We don't need the redundant logic since send_message always returns 0. > > Signed-off-by: Guoqing Jiang <gqjiang@suse.com> > --- > fs/dlm/lock.c | 10 ++-------- > 1 file changed, 2 insertions(+), 8 deletions(-) > > diff --git a/fs/dlm/lock.c b/fs/dlm/lock.c > index 35502d4..6fc3de9 100644 > --- a/fs/dlm/lock.c > +++ b/fs/dlm/lock.c > @@ -3656,10 +3656,7 @@ static int send_common(struct dlm_rsb *r, struct > dlm_lkb *lkb, int mstype) > > send_args(r, lkb, ms); > > - error = send_message(mh, ms); > - if (error) > - goto fail; > - return 0; > + return send_message(mh, ms); > > fail: > remove_from_waiters(lkb, msg_reply_type(mstype)); > @@ -3763,10 +3760,7 @@ static int send_lookup(struct dlm_rsb *r, struct > dlm_lkb *lkb) > > send_args(r, lkb, ms); > > - error = send_message(mh, ms); > - if (error) > - goto fail; > - return 0; > + return send_message(mh, ms); > > fail: > remove_from_waiters(lkb, DLM_MSG_LOOKUP_REPLY); > -- > 1.7.12.4 Hi, The patch looks okay, but if remove_from_waiters() always returns 0, wouldn't it be better to change the function from int to void and return 0 here? The advantage is that code spelunkers wouldn't need to back-track one more level (not to mention the instruction or two it might save). Regards, Bob Peterson Red Hat File Systems ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Cluster-devel] [PATCH] dlm: remove unnecessary error check 2015-06-09 12:09 ` [Cluster-devel] " Bob Peterson @ 2015-06-10 2:39 ` Guoqing Jiang 2015-06-10 2:50 ` Bob Peterson 0 siblings, 1 reply; 11+ messages in thread From: Guoqing Jiang @ 2015-06-10 2:39 UTC (permalink / raw) To: Bob Peterson; +Cc: ccaulfie, teigland, cluster-devel, linux-kernel Hi Bob, Bob Peterson wrote: > ----- Original Message ----- > >> We don't need the redundant logic since send_message always returns 0. >> >> Signed-off-by: Guoqing Jiang <gqjiang@suse.com> >> --- >> fs/dlm/lock.c | 10 ++-------- >> 1 file changed, 2 insertions(+), 8 deletions(-) >> >> diff --git a/fs/dlm/lock.c b/fs/dlm/lock.c >> index 35502d4..6fc3de9 100644 >> --- a/fs/dlm/lock.c >> +++ b/fs/dlm/lock.c >> @@ -3656,10 +3656,7 @@ static int send_common(struct dlm_rsb *r, struct >> dlm_lkb *lkb, int mstype) >> >> send_args(r, lkb, ms); >> >> - error = send_message(mh, ms); >> - if (error) >> - goto fail; >> - return 0; >> + return send_message(mh, ms); >> >> fail: >> remove_from_waiters(lkb, msg_reply_type(mstype)); >> @@ -3763,10 +3760,7 @@ static int send_lookup(struct dlm_rsb *r, struct >> dlm_lkb *lkb) >> >> send_args(r, lkb, ms); >> >> - error = send_message(mh, ms); >> - if (error) >> - goto fail; >> - return 0; >> + return send_message(mh, ms); >> >> fail: >> remove_from_waiters(lkb, DLM_MSG_LOOKUP_REPLY); >> -- >> 1.7.12.4 >> > > Hi, > > The patch looks okay, but if remove_from_waiters() always returns 0, > wouldn't it be better to change the function from int to void and > return 0 here? The advantage is that code spelunkers wouldn't need > to back-track one more level (not to mention the instruction or two > it might save). > > Seems remove_from_waiters is not always returns 0, the return value could be -1 or 0 which depends on _remove_from_waiters. BTW, I found that there are no big difference between send_common and send_lookup, since the send_common can also be use to send lookup message, I guess send_lookup can be removed as well. Thanks, Guoqing ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Cluster-devel] [PATCH] dlm: remove unnecessary error check 2015-06-10 2:39 ` Guoqing Jiang @ 2015-06-10 2:50 ` Bob Peterson 2015-06-10 3:10 ` Guoqing Jiang 0 siblings, 1 reply; 11+ messages in thread From: Bob Peterson @ 2015-06-10 2:50 UTC (permalink / raw) To: Guoqing Jiang; +Cc: ccaulfie, teigland, cluster-devel, linux-kernel ----- Original Message ----- > Hi Bob, > > Bob Peterson wrote: > > ----- Original Message ----- > > > >> We don't need the redundant logic since send_message always returns 0. > >> > >> Signed-off-by: Guoqing Jiang <gqjiang@suse.com> > >> --- > >> fs/dlm/lock.c | 10 ++-------- > >> 1 file changed, 2 insertions(+), 8 deletions(-) > >> > >> diff --git a/fs/dlm/lock.c b/fs/dlm/lock.c > >> index 35502d4..6fc3de9 100644 > >> --- a/fs/dlm/lock.c > >> +++ b/fs/dlm/lock.c > >> @@ -3656,10 +3656,7 @@ static int send_common(struct dlm_rsb *r, struct > >> dlm_lkb *lkb, int mstype) > >> > >> send_args(r, lkb, ms); > >> > >> - error = send_message(mh, ms); > >> - if (error) > >> - goto fail; > >> - return 0; > >> + return send_message(mh, ms); > >> > >> fail: > >> remove_from_waiters(lkb, msg_reply_type(mstype)); > >> @@ -3763,10 +3760,7 @@ static int send_lookup(struct dlm_rsb *r, struct > >> dlm_lkb *lkb) > >> > >> send_args(r, lkb, ms); > >> > >> - error = send_message(mh, ms); > >> - if (error) > >> - goto fail; > >> - return 0; > >> + return send_message(mh, ms); > >> > >> fail: > >> remove_from_waiters(lkb, DLM_MSG_LOOKUP_REPLY); > >> -- > >> 1.7.12.4 > >> > > > > Hi, > > > > The patch looks okay, but if remove_from_waiters() always returns 0, > > wouldn't it be better to change the function from int to void and > > return 0 here? The advantage is that code spelunkers wouldn't need > > to back-track one more level (not to mention the instruction or two > > it might save). > > > > > Seems remove_from_waiters is not always returns 0, the return value > could be -1 or 0 which depends on _remove_from_waiters. > > BTW, I found that there are no big difference between send_common > and send_lookup, since the send_common can also be use to send > lookup message, I guess send_lookup can be removed as well. > > Thanks, > Guoqing Hi Guoqing, If remove_from_waiters can return -1, then the patch would prevent the code from calling remove_from_waiters. So the patch still doesn't look right to me. Regards, Bob Peterson Red Hat File Systems ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Cluster-devel] [PATCH] dlm: remove unnecessary error check 2015-06-10 2:50 ` Bob Peterson @ 2015-06-10 3:10 ` Guoqing Jiang 2015-06-10 12:26 ` Bob Peterson 2015-06-10 15:24 ` David Teigland 0 siblings, 2 replies; 11+ messages in thread From: Guoqing Jiang @ 2015-06-10 3:10 UTC (permalink / raw) To: Bob Peterson; +Cc: ccaulfie, teigland, cluster-devel, linux-kernel Bob Peterson wrote: > ----- Original Message ----- > >> Hi Bob, >> >> Bob Peterson wrote: >> >>> ----- Original Message ----- >>> >>> >>>> We don't need the redundant logic since send_message always returns 0. >>>> >>>> Signed-off-by: Guoqing Jiang <gqjiang@suse.com> >>>> --- >>>> fs/dlm/lock.c | 10 ++-------- >>>> 1 file changed, 2 insertions(+), 8 deletions(-) >>>> >>>> diff --git a/fs/dlm/lock.c b/fs/dlm/lock.c >>>> index 35502d4..6fc3de9 100644 >>>> --- a/fs/dlm/lock.c >>>> +++ b/fs/dlm/lock.c >>>> @@ -3656,10 +3656,7 @@ static int send_common(struct dlm_rsb *r, struct >>>> dlm_lkb *lkb, int mstype) >>>> >>>> send_args(r, lkb, ms); >>>> >>>> - error = send_message(mh, ms); >>>> - if (error) >>>> - goto fail; >>>> - return 0; >>>> + return send_message(mh, ms); >>>> >>>> fail: >>>> remove_from_waiters(lkb, msg_reply_type(mstype)); >>>> @@ -3763,10 +3760,7 @@ static int send_lookup(struct dlm_rsb *r, struct >>>> dlm_lkb *lkb) >>>> >>>> send_args(r, lkb, ms); >>>> >>>> - error = send_message(mh, ms); >>>> - if (error) >>>> - goto fail; >>>> - return 0; >>>> + return send_message(mh, ms); >>>> >>>> fail: >>>> remove_from_waiters(lkb, DLM_MSG_LOOKUP_REPLY); >>>> -- >>>> 1.7.12.4 >>>> >>>> >>> Hi, >>> >>> The patch looks okay, but if remove_from_waiters() always returns 0, >>> wouldn't it be better to change the function from int to void and >>> return 0 here? The advantage is that code spelunkers wouldn't need >>> to back-track one more level (not to mention the instruction or two >>> it might save). >>> >>> >>> >> Seems remove_from_waiters is not always returns 0, the return value >> could be -1 or 0 which depends on _remove_from_waiters. >> >> BTW, I found that there are no big difference between send_common >> and send_lookup, since the send_common can also be use to send >> lookup message, I guess send_lookup can be removed as well. >> >> Thanks, >> Guoqing >> > > Hi Guoqing, > > If remove_from_waiters can return -1, then the patch would prevent the > code from calling remove_from_waiters. So the patch still doesn't look > right to me. > > Hi Bob, The remove_from_waiters could only be invoked after failed to create_message, right? Since send_message always returns 0, this patch doesn't touch anything about the failure path, and it also doesn't change the original semantic. Thanks, Guoqing ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Cluster-devel] [PATCH] dlm: remove unnecessary error check 2015-06-10 3:10 ` Guoqing Jiang @ 2015-06-10 12:26 ` Bob Peterson 2015-06-11 2:39 ` Guoqing Jiang 2015-06-10 15:24 ` David Teigland 1 sibling, 1 reply; 11+ messages in thread From: Bob Peterson @ 2015-06-10 12:26 UTC (permalink / raw) To: Guoqing Jiang; +Cc: ccaulfie, teigland, cluster-devel, linux-kernel ----- Original Message ----- > Bob Peterson wrote: > > ----- Original Message ----- > > > >> Hi Bob, > >> > >> Bob Peterson wrote: > >> > >>> ----- Original Message ----- > >>> > >>> > >>>> We don't need the redundant logic since send_message always returns 0. > >>>> > >>>> Signed-off-by: Guoqing Jiang <gqjiang@suse.com> > >>>> --- > >>>> fs/dlm/lock.c | 10 ++-------- > >>>> 1 file changed, 2 insertions(+), 8 deletions(-) > >>>> > >>>> diff --git a/fs/dlm/lock.c b/fs/dlm/lock.c > >>>> index 35502d4..6fc3de9 100644 > >>>> --- a/fs/dlm/lock.c > >>>> +++ b/fs/dlm/lock.c > >>>> @@ -3656,10 +3656,7 @@ static int send_common(struct dlm_rsb *r, struct > >>>> dlm_lkb *lkb, int mstype) > >>>> > >>>> send_args(r, lkb, ms); > >>>> > >>>> - error = send_message(mh, ms); > >>>> - if (error) > >>>> - goto fail; > >>>> - return 0; > >>>> + return send_message(mh, ms); Hi Guoqing, Sorry, I was momentarily confused. I think you misunderstood what I was saying. What I meant was: Instead of doing: + return send_message(mh, ms); ...where send_message returns 0, it might be better to have: static void send_message(struct dlm_mhandle *mh, struct dlm_message *ms) { dlm_message_out(ms); dlm_lowcomms_commit_buffer(mh); } ...And in send_common, do (in both places): + send_message(mh, ms); + return 0; Since it's so short, it might even be better to code send_message as a macro, or at least an "inline" function. Regards, Bob Peterson Red Hat File Systems ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Cluster-devel] [PATCH] dlm: remove unnecessary error check 2015-06-10 12:26 ` Bob Peterson @ 2015-06-11 2:39 ` Guoqing Jiang 0 siblings, 0 replies; 11+ messages in thread From: Guoqing Jiang @ 2015-06-11 2:39 UTC (permalink / raw) To: Bob Peterson; +Cc: ccaulfie, teigland, cluster-devel, linux-kernel Bob Peterson wrote: > >>>> >>>> >>>>> ----- Original Message ----- >>>>> >>>>> >>>>> >>>>>> We don't need the redundant logic since send_message always returns 0. >>>>>> >>>>>> Signed-off-by: Guoqing Jiang <gqjiang@suse.com> >>>>>> --- >>>>>> fs/dlm/lock.c | 10 ++-------- >>>>>> 1 file changed, 2 insertions(+), 8 deletions(-) >>>>>> >>>>>> diff --git a/fs/dlm/lock.c b/fs/dlm/lock.c >>>>>> index 35502d4..6fc3de9 100644 >>>>>> --- a/fs/dlm/lock.c >>>>>> +++ b/fs/dlm/lock.c >>>>>> @@ -3656,10 +3656,7 @@ static int send_common(struct dlm_rsb *r, struct >>>>>> dlm_lkb *lkb, int mstype) >>>>>> >>>>>> send_args(r, lkb, ms); >>>>>> >>>>>> - error = send_message(mh, ms); >>>>>> - if (error) >>>>>> - goto fail; >>>>>> - return 0; >>>>>> + return send_message(mh, ms); >>>>>> > > Hi Guoqing, > > Sorry, I was momentarily confused. I think you misunderstood what I was saying. > What I meant was: Instead of doing: > > + return send_message(mh, ms); > ...where send_message returns 0, it might be better to have: > > static void send_message(struct dlm_mhandle *mh, struct dlm_message *ms) > { > dlm_message_out(ms); > dlm_lowcomms_commit_buffer(mh); > } > > ...And in send_common, do (in both places): > + send_message(mh, ms); > + return 0; > > Since it's so short, it might even be better to code send_message as a macro, > or at least an "inline" function. > > Hi Bob, Got it, thanks. It is a better solution but it is not a bug fix or similar thing, so maybe just leave it as it is. Regards, Guoqing ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Cluster-devel] [PATCH] dlm: remove unnecessary error check 2015-06-10 3:10 ` Guoqing Jiang 2015-06-10 12:26 ` Bob Peterson @ 2015-06-10 15:24 ` David Teigland 2015-06-11 9:47 ` Guoqing Jiang 1 sibling, 1 reply; 11+ messages in thread From: David Teigland @ 2015-06-10 15:24 UTC (permalink / raw) To: Guoqing Jiang; +Cc: Bob Peterson, ccaulfie, cluster-devel, linux-kernel On Wed, Jun 10, 2015 at 11:10:44AM +0800, Guoqing Jiang wrote: > The remove_from_waiters could only be invoked after failed to > create_message, right? > Since send_message always returns 0, this patch doesn't touch anything > about the failure > path, and it also doesn't change the original semantic. I'm not inclined to take any patches unless there's a problem identified. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Cluster-devel] [PATCH] dlm: remove unnecessary error check 2015-06-10 15:24 ` David Teigland @ 2015-06-11 9:47 ` Guoqing Jiang 2015-06-11 16:18 ` David Teigland 0 siblings, 1 reply; 11+ messages in thread From: Guoqing Jiang @ 2015-06-11 9:47 UTC (permalink / raw) To: David Teigland; +Cc: Bob Peterson, ccaulfie, cluster-devel, linux-kernel Hi David, David Teigland wrote: > On Wed, Jun 10, 2015 at 11:10:44AM +0800, Guoqing Jiang wrote: > >> The remove_from_waiters could only be invoked after failed to >> create_message, right? >> Since send_message always returns 0, this patch doesn't touch anything >> about the failure >> path, and it also doesn't change the original semantic. >> > > I'm not inclined to take any patches unless there's a problem identified. > > . > Do you consider take the following clean up? If yes, I will send a formal patch, otherwise pls ignore it. diff --git a/fs/dlm/lock.c b/fs/dlm/lock.c index 35502d4..7c822f7 100644 --- a/fs/dlm/lock.c +++ b/fs/dlm/lock.c @@ -3747,30 +3747,7 @@ static int send_bast(struct dlm_rsb *r, struct dlm_lkb *lkb, int mode) static int send_lookup(struct dlm_rsb *r, struct dlm_lkb *lkb) { - struct dlm_message *ms; - struct dlm_mhandle *mh; - int to_nodeid, error; - - to_nodeid = dlm_dir_nodeid(r); - - error = add_to_waiters(lkb, DLM_MSG_LOOKUP, to_nodeid); - if (error) - return error; - - error = create_message(r, NULL, to_nodeid, DLM_MSG_LOOKUP, &ms, &mh); - if (error) - goto fail; - - send_args(r, lkb, ms); - - error = send_message(mh, ms); - if (error) - goto fail; - return 0; - - fail: - remove_from_waiters(lkb, DLM_MSG_LOOKUP_REPLY); - return error; + return send_common(r, lkb, DLM_MSG_LOOKUP); } Thanks, Guoqing ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [Cluster-devel] [PATCH] dlm: remove unnecessary error check 2015-06-11 9:47 ` Guoqing Jiang @ 2015-06-11 16:18 ` David Teigland 2015-06-17 2:14 ` Guoqing Jiang 0 siblings, 1 reply; 11+ messages in thread From: David Teigland @ 2015-06-11 16:18 UTC (permalink / raw) To: Guoqing Jiang; +Cc: Bob Peterson, ccaulfie, cluster-devel, linux-kernel On Thu, Jun 11, 2015 at 05:47:28PM +0800, Guoqing Jiang wrote: > Do you consider take the following clean up? If yes, I will send a > formal patch, otherwise pls ignore it. On first glance, the old and new code do not appear to do the same thing, so let's leave it as it is. > - to_nodeid = dlm_dir_nodeid(r); ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Cluster-devel] [PATCH] dlm: remove unnecessary error check 2015-06-11 16:18 ` David Teigland @ 2015-06-17 2:14 ` Guoqing Jiang 0 siblings, 0 replies; 11+ messages in thread From: Guoqing Jiang @ 2015-06-17 2:14 UTC (permalink / raw) To: David Teigland; +Cc: Bob Peterson, ccaulfie, cluster-devel, linux-kernel Hi David, David Teigland wrote: > On Thu, Jun 11, 2015 at 05:47:28PM +0800, Guoqing Jiang wrote: > >> Do you consider take the following clean up? If yes, I will send a >> formal patch, otherwise pls ignore it. >> > > On first glance, the old and new code do not appear to do the same thing, > so let's leave it as it is. > > >> - to_nodeid = dlm_dir_nodeid(r); >> Sorry, seems it is the only different thing, if combines previous change with below modification, then the behavior is same. @@ -3644,7 +3644,10 @@ static int send_common(struct dlm_rsb *r, struct dlm_lkb *lkb, int mstype) struct dlm_mhandle *mh; int to_nodeid, error; - to_nodeid = r->res_nodeid; + if (mstype == DLM_MSG_LOOKUP) + to_nodeid = dlm_dir_nodeid(r); + else + to_nodeid = r->res_nodeid; And for create_message, the second parameter (lkb) is not effective to create three type msgs (REQUEST/LOOKUP/REMOVE). Thanks, Guoqing ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2015-06-17 2:17 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-06-09 9:46 [PATCH] dlm: remove unnecessary error check Guoqing Jiang 2015-06-09 12:09 ` [Cluster-devel] " Bob Peterson 2015-06-10 2:39 ` Guoqing Jiang 2015-06-10 2:50 ` Bob Peterson 2015-06-10 3:10 ` Guoqing Jiang 2015-06-10 12:26 ` Bob Peterson 2015-06-11 2:39 ` Guoqing Jiang 2015-06-10 15:24 ` David Teigland 2015-06-11 9:47 ` Guoqing Jiang 2015-06-11 16:18 ` David Teigland 2015-06-17 2:14 ` Guoqing Jiang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).