From mboxrd@z Thu Jan 1 00:00:00 1970 From: wangyijing Subject: Re: [RESEND PATCH 2/3] bcache: update document info Date: Fri, 1 Jul 2016 14:25:49 +0800 Message-ID: <57760CED.6090901@huawei.com> References: <1466561534-17595-1-git-send-email-wangyijing@huawei.com> <5775CCA4.9070805@huawei.com> Mime-Version: 1.0 Content-Type: text/plain; charset=gbk Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: In-Reply-To: Sender: linux-bcache-owner@vger.kernel.org To: Coly Li , Coly Li , axboe@fb.com, Kent Overstreet Cc: Eric Wheeler , linux-bcache@vger.kernel.org, linux-raid@vger.kernel.org, linux-kernel@vger.kernel.org List-Id: linux-raid.ids =D4=DA 2016/7/1 12:21, Coly Li =D0=B4=B5=C0: > =D4=DA 16/7/1 =C9=CF=CE=E79:51, wangyijing =D0=B4=B5=C0: >> Hi Coly, thanks to your review and comments. >> >> Commit 77b5a08427e875 ("bcache: don't embed 'return' statements in c= losure macros") >> remove the return in continue_at(), so I think we should update the = document info >> about continue_at(). >> >> Thanks! >> Yijing. >=20 > Hi Yijing, >=20 > The original version of continue_at() returns to caller function insi= de > the macro, Jens thinks this macro breaks code execution flow implicit= ly, > so he moves 'return' out of continue_at() and to follow continue_at()= at > the location where continue_at() is referenced. >=20 > So as I suggested, the original author means the code should return t= o > the calling function. >=20 > But YES, I agree that the comments should be updated, because there i= s > no 'return' inside macro continue_at(). We should explicitly point ou= t > that there should be a 'return' immediately following macro continue_= at(). Yes, you are right, it's better to explicitly point out a return needed= to follow continue_at() than remove this document info, I will update this patch, thanks very m= uch! >=20 > Thanks. >=20 > Coly >=20 >=20 >> =D4=DA 2016/6/29 18:16, Coly Li =D0=B4=B5=C0: >>> =D4=DA 16/6/22 =C9=CF=CE=E710:12, Yijing Wang =D0=B4=B5=C0: >>>> There is no return in continue_at(), update the documentation. >>>> >>> >>> There are 2 modification of this patch. The first one is about a ty= po, >>> it is correct. >>> >>> But I doubt your second modification is proper. The line removed in= your >>> patch is, >>>> - * continue_at() also, critically, is a macro that returns the >>> calling function. >>>> - * There's good reason for this. >>>> - * >>> >>> I think this is exactly what original author wants to say. It does = not >>> mean return a value, it means return to the calling function. And t= he >>> bellowed lines explains the reason. >>> >>>> Signed-off-by: Yijing Wang >>>> --- >>>> drivers/md/bcache/closure.c | 2 +- >>>> drivers/md/bcache/closure.h | 3 --- >>>> 2 files changed, 1 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/drivers/md/bcache/closure.c b/drivers/md/bcache/closu= re.c >>>> index 9eaf1d6..864e673 100644 >>>> --- a/drivers/md/bcache/closure.c >>>> +++ b/drivers/md/bcache/closure.c >>>> @@ -112,7 +112,7 @@ bool closure_wait(struct closure_waitlist *wai= tlist, struct closure *cl) >>>> EXPORT_SYMBOL(closure_wait); >>>> =20 >>>> /** >>>> - * closure_sync - sleep until a closure a closure has nothing lef= t to wait on >>>> + * closure_sync - sleep until a closure has nothing left to wait = on >>> >>> Yes, this modification is good. >>> >>>> * >>>> * Sleeps until the refcount hits 1 - the thread that's running t= he closure owns >>>> * the last refcount. >>>> diff --git a/drivers/md/bcache/closure.h b/drivers/md/bcache/closu= re.h >>>> index 782cc2c..f51188d 100644 >>>> --- a/drivers/md/bcache/closure.h >>>> +++ b/drivers/md/bcache/closure.h >>>> @@ -31,9 +31,6 @@ >>>> * passing it, as you might expect, the function to run when noth= ing is pending >>>> * and the workqueue to run that function out of. >>>> * >>>> - * continue_at() also, critically, is a macro that returns the ca= lling function. >>>> - * There's good reason for this. >>>> - * >>>> * To use safely closures asynchronously, they must always have a= refcount while >>>> * they are running owned by the thread that is running them. Oth= erwise, suppose >>>> * you submit some bios and wish to have a function run when they= all complete: >>>> >>> >>> >> >=20 >=20 > . >=20 From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752199AbcGAG0y (ORCPT ); Fri, 1 Jul 2016 02:26:54 -0400 Received: from szxga04-in.huawei.com ([119.145.14.52]:63804 "EHLO szxga04-in.huawei.com" rhost-flags-OK-FAIL-OK-FAIL) by vger.kernel.org with ESMTP id S1752043AbcGAG0x (ORCPT ); Fri, 1 Jul 2016 02:26:53 -0400 Subject: Re: [RESEND PATCH 2/3] bcache: update document info To: Coly Li , Coly Li , , "Kent Overstreet" References: <1466561534-17595-1-git-send-email-wangyijing@huawei.com> <5775CCA4.9070805@huawei.com> CC: Eric Wheeler , , , From: wangyijing Message-ID: <57760CED.6090901@huawei.com> Date: Fri, 1 Jul 2016 14:25:49 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101 Thunderbird/38.5.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="gbk" Content-Transfer-Encoding: 8bit X-Originating-IP: [10.177.23.4] X-CFilter-Loop: Reflected X-Mirapoint-Virus-RAPID-Raw: score=unknown(0), refid=str=0001.0A090205.57760CF7.000A,ss=1,re=0.000,recu=0.000,reip=0.000,cl=1,cld=1,fgs=0, ip=0.0.0.0, so=2014-11-16 11:51:01, dmn=2013-03-21 17:37:32 X-Mirapoint-Loop-Id: 0a66785b8aeac1b1d0a7fc4517b6ae84 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 在 2016/7/1 12:21, Coly Li 写道: > 在 16/7/1 上午9:51, wangyijing 写道: >> Hi Coly, thanks to your review and comments. >> >> Commit 77b5a08427e875 ("bcache: don't embed 'return' statements in closure macros") >> remove the return in continue_at(), so I think we should update the document info >> about continue_at(). >> >> Thanks! >> Yijing. > > Hi Yijing, > > The original version of continue_at() returns to caller function inside > the macro, Jens thinks this macro breaks code execution flow implicitly, > so he moves 'return' out of continue_at() and to follow continue_at() at > the location where continue_at() is referenced. > > So as I suggested, the original author means the code should return to > the calling function. > > But YES, I agree that the comments should be updated, because there is > no 'return' inside macro continue_at(). We should explicitly point out > that there should be a 'return' immediately following macro continue_at(). Yes, you are right, it's better to explicitly point out a return needed to follow continue_at() than remove this document info, I will update this patch, thanks very much! > > Thanks. > > Coly > > >> 在 2016/6/29 18:16, Coly Li 写道: >>> 在 16/6/22 上午10:12, Yijing Wang 写道: >>>> There is no return in continue_at(), update the documentation. >>>> >>> >>> There are 2 modification of this patch. The first one is about a typo, >>> it is correct. >>> >>> But I doubt your second modification is proper. The line removed in your >>> patch is, >>>> - * continue_at() also, critically, is a macro that returns the >>> calling function. >>>> - * There's good reason for this. >>>> - * >>> >>> I think this is exactly what original author wants to say. It does not >>> mean return a value, it means return to the calling function. And the >>> bellowed lines explains the reason. >>> >>>> Signed-off-by: Yijing Wang >>>> --- >>>> drivers/md/bcache/closure.c | 2 +- >>>> drivers/md/bcache/closure.h | 3 --- >>>> 2 files changed, 1 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/drivers/md/bcache/closure.c b/drivers/md/bcache/closure.c >>>> index 9eaf1d6..864e673 100644 >>>> --- a/drivers/md/bcache/closure.c >>>> +++ b/drivers/md/bcache/closure.c >>>> @@ -112,7 +112,7 @@ bool closure_wait(struct closure_waitlist *waitlist, struct closure *cl) >>>> EXPORT_SYMBOL(closure_wait); >>>> >>>> /** >>>> - * closure_sync - sleep until a closure a closure has nothing left to wait on >>>> + * closure_sync - sleep until a closure has nothing left to wait on >>> >>> Yes, this modification is good. >>> >>>> * >>>> * Sleeps until the refcount hits 1 - the thread that's running the closure owns >>>> * the last refcount. >>>> diff --git a/drivers/md/bcache/closure.h b/drivers/md/bcache/closure.h >>>> index 782cc2c..f51188d 100644 >>>> --- a/drivers/md/bcache/closure.h >>>> +++ b/drivers/md/bcache/closure.h >>>> @@ -31,9 +31,6 @@ >>>> * passing it, as you might expect, the function to run when nothing is pending >>>> * and the workqueue to run that function out of. >>>> * >>>> - * continue_at() also, critically, is a macro that returns the calling function. >>>> - * There's good reason for this. >>>> - * >>>> * To use safely closures asynchronously, they must always have a refcount while >>>> * they are running owned by the thread that is running them. Otherwise, suppose >>>> * you submit some bios and wish to have a function run when they all complete: >>>> >>> >>> >> > > > . >