* [RESEND PATCH 2/3] bcache: update document info @ 2016-06-22 2:12 Yijing Wang 2016-06-29 10:16 ` Coly Li 0 siblings, 1 reply; 5+ messages in thread From: Yijing Wang @ 2016-06-22 2:12 UTC (permalink / raw) To: axboe, Kent Overstreet Cc: Eric Wheeler, Coly Li, linux-bcache, linux-raid, linux-kernel, Yijing Wang There is no return in continue_at(), update the documentation. Signed-off-by: Yijing Wang <wangyijing@huawei.com> --- 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 * * 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: -- 1.7.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [RESEND PATCH 2/3] bcache: update document info 2016-06-22 2:12 [RESEND PATCH 2/3] bcache: update document info Yijing Wang @ 2016-06-29 10:16 ` Coly Li 2016-07-01 1:51 ` wangyijing 0 siblings, 1 reply; 5+ messages in thread From: Coly Li @ 2016-06-29 10:16 UTC (permalink / raw) To: Yijing Wang, axboe, Kent Overstreet Cc: Eric Wheeler, Coly Li, linux-bcache, linux-raid, linux-kernel 在 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 <wangyijing@huawei.com> > --- > 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: > -- Coly Li ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RESEND PATCH 2/3] bcache: update document info 2016-06-29 10:16 ` Coly Li @ 2016-07-01 1:51 ` wangyijing 2016-07-01 4:21 ` Coly Li 0 siblings, 1 reply; 5+ messages in thread From: wangyijing @ 2016-07-01 1:51 UTC (permalink / raw) To: Coly Li, axboe, Kent Overstreet Cc: Eric Wheeler, Coly Li, linux-bcache, linux-raid, linux-kernel 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. 在 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 <wangyijing@huawei.com> >> --- >> 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: >> > > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RESEND PATCH 2/3] bcache: update document info 2016-07-01 1:51 ` wangyijing @ 2016-07-01 4:21 ` Coly Li 2016-07-01 6:25 ` wangyijing 0 siblings, 1 reply; 5+ messages in thread From: Coly Li @ 2016-07-01 4:21 UTC (permalink / raw) To: wangyijing, Coly Li, axboe, Kent Overstreet Cc: Eric Wheeler, linux-bcache, linux-raid, linux-kernel 在 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(). 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 <wangyijing@huawei.com> >>> --- >>> 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: >>> >> >> > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RESEND PATCH 2/3] bcache: update document info 2016-07-01 4:21 ` Coly Li @ 2016-07-01 6:25 ` wangyijing 0 siblings, 0 replies; 5+ messages in thread From: wangyijing @ 2016-07-01 6:25 UTC (permalink / raw) To: Coly Li, Coly Li, axboe, Kent Overstreet Cc: Eric Wheeler, linux-bcache, linux-raid, linux-kernel 在 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 <wangyijing@huawei.com> >>>> --- >>>> 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: >>>> >>> >>> >> > > > . > ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-07-01 6:26 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-06-22 2:12 [RESEND PATCH 2/3] bcache: update document info Yijing Wang 2016-06-29 10:16 ` Coly Li 2016-07-01 1:51 ` wangyijing 2016-07-01 4:21 ` Coly Li 2016-07-01 6:25 ` wangyijing
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).