diff mbox

[v2,4/4] lockdep: Assign a lock_class per gendisk used for wait_for_completion()

Message ID 20171020144451.GA16793@infradead.org (mailing list archive)
State Deferred, archived
Headers show

Commit Message

Christoph Hellwig Oct. 20, 2017, 2:44 p.m. UTC 
The Subject prefix for this should be "block:".

> @@ -945,7 +945,7 @@ int submit_bio_wait(struct bio *bio)
>  {
>  	struct submit_bio_ret ret;
>  
> -	init_completion(&ret.event);
> +	init_completion_with_map(&ret.event, &bio->bi_disk->lockdep_map);

FYI, I have an outstanding patch to simplify this a lot, which
switches this to DECLARE_COMPLETION_ONSTACK.  I can delay this or let
you pick it up with your series, but we'll need a variant of
DECLARE_COMPLETION_ONSTACK with the lockdep annotations.

Patch below for reference:

---
From d65b89843c9f82c0744643515ba51dd10e66e67b Mon Sep 17 00:00:00 2001
From: Christoph Hellwig <hch@lst.de>
Date: Thu, 5 Oct 2017 18:31:02 +0200
Subject: block: use DECLARE_COMPLETION_ONSTACK in submit_bio_wait

Simplify the code by getting rid of the submit_bio_ret structure.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/bio.c | 19 +++++--------------
 1 file changed, 5 insertions(+), 14 deletions(-)

Comments

Byungchul Park Oct. 22, 2017, 11:53 p.m. UTC | #1 
On Fri, Oct 20, 2017 at 07:44:51AM -0700, Christoph Hellwig wrote:
> The Subject prefix for this should be "block:".
> 
> > @@ -945,7 +945,7 @@ int submit_bio_wait(struct bio *bio)
> >  {
> >  	struct submit_bio_ret ret;
> >  
> > -	init_completion(&ret.event);
> > +	init_completion_with_map(&ret.event, &bio->bi_disk->lockdep_map);
> 
> FYI, I have an outstanding patch to simplify this a lot, which
> switches this to DECLARE_COMPLETION_ONSTACK.  I can delay this or let
> you pick it up with your series, but we'll need a variant of
> DECLARE_COMPLETION_ONSTACK with the lockdep annotations.

Hello,

I'm sorry for late.

I think your patch makes block code simpler and better. I like it.

But, I just wonder if it's related to my series. Is it proper to add
your patch into my series?

Thanks,
Byungchul

> Patch below for reference:
> 
> ---
> >From d65b89843c9f82c0744643515ba51dd10e66e67b Mon Sep 17 00:00:00 2001
> From: Christoph Hellwig <hch@lst.de>
> Date: Thu, 5 Oct 2017 18:31:02 +0200
> Subject: block: use DECLARE_COMPLETION_ONSTACK in submit_bio_wait
> 
> Simplify the code by getting rid of the submit_bio_ret structure.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  block/bio.c | 19 +++++--------------
>  1 file changed, 5 insertions(+), 14 deletions(-)
> 
> diff --git a/block/bio.c b/block/bio.c
> index 8338304ea256..4e18e959fc0a 100644
> --- a/block/bio.c
> +++ b/block/bio.c
> @@ -917,17 +917,9 @@ int bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter)
>  }
>  EXPORT_SYMBOL_GPL(bio_iov_iter_get_pages);
>  
> -struct submit_bio_ret {
> -	struct completion event;
> -	int error;
> -};
> -
>  static void submit_bio_wait_endio(struct bio *bio)
>  {
> -	struct submit_bio_ret *ret = bio->bi_private;
> -
> -	ret->error = blk_status_to_errno(bio->bi_status);
> -	complete(&ret->event);
> +	complete(bio->bi_private);
>  }
>  
>  /**
> @@ -943,16 +935,15 @@ static void submit_bio_wait_endio(struct bio *bio)
>   */
>  int submit_bio_wait(struct bio *bio)
>  {
> -	struct submit_bio_ret ret;
> +	DECLARE_COMPLETION_ONSTACK(done);
>  
> -	init_completion(&ret.event);
> -	bio->bi_private = &ret;
> +	bio->bi_private = &done;
>  	bio->bi_end_io = submit_bio_wait_endio;
>  	bio->bi_opf |= REQ_SYNC;
>  	submit_bio(bio);
> -	wait_for_completion_io(&ret.event);
> +	wait_for_completion_io(&done);
>  
> -	return ret.error;
> +	return blk_status_to_errno(bio->bi_status);
>  }
>  EXPORT_SYMBOL(submit_bio_wait);
>  
> -- 
> 2.14.2
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Christoph Hellwig Oct. 23, 2017, 6:36 a.m. UTC | #2 
On Mon, Oct 23, 2017 at 08:53:35AM +0900, Byungchul Park wrote:
> On Fri, Oct 20, 2017 at 07:44:51AM -0700, Christoph Hellwig wrote:
> > The Subject prefix for this should be "block:".
> > 
> > > @@ -945,7 +945,7 @@ int submit_bio_wait(struct bio *bio)
> > >  {
> > >  	struct submit_bio_ret ret;
> > >  
> > > -	init_completion(&ret.event);
> > > +	init_completion_with_map(&ret.event, &bio->bi_disk->lockdep_map);
> > 
> > FYI, I have an outstanding patch to simplify this a lot, which
> > switches this to DECLARE_COMPLETION_ONSTACK.  I can delay this or let
> > you pick it up with your series, but we'll need a variant of
> > DECLARE_COMPLETION_ONSTACK with the lockdep annotations.
> 
> Hello,
> 
> I'm sorry for late.
> 
> I think your patch makes block code simpler and better. I like it.
> 
> But, I just wonder if it's related to my series.

Because it shows that we also need a version of DECLARE_COMPLETION_ONSTACK
the gets passed an explicit lockdep map.  And because if it was merged
through a different tree it would create a conflict.

> Is it proper to add
> your patch into my series?

Sure.
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Byungchul Park Oct. 23, 2017, 7:04 a.m. UTC | #3 
On Sun, Oct 22, 2017 at 11:36:30PM -0700, Christoph Hellwig wrote:
> On Mon, Oct 23, 2017 at 08:53:35AM +0900, Byungchul Park wrote:
> > On Fri, Oct 20, 2017 at 07:44:51AM -0700, Christoph Hellwig wrote:
> > > The Subject prefix for this should be "block:".
> > > 
> > > > @@ -945,7 +945,7 @@ int submit_bio_wait(struct bio *bio)
> > > >  {
> > > >  	struct submit_bio_ret ret;
> > > >  
> > > > -	init_completion(&ret.event);
> > > > +	init_completion_with_map(&ret.event, &bio->bi_disk->lockdep_map);
> > > 
> > > FYI, I have an outstanding patch to simplify this a lot, which
> > > switches this to DECLARE_COMPLETION_ONSTACK.  I can delay this or let
> > > you pick it up with your series, but we'll need a variant of
> > > DECLARE_COMPLETION_ONSTACK with the lockdep annotations.
> > 
> > Hello,
> > 
> > I'm sorry for late.
> > 
> > I think your patch makes block code simpler and better. I like it.
> > 
> > But, I just wonder if it's related to my series.
> 
> Because it shows that we also need a version of DECLARE_COMPLETION_ONSTACK
> the gets passed an explicit lockdep map.  And because if it was merged
> through a different tree it would create a conflict.
> 
> > Is it proper to add
> > your patch into my series?
> 
> Sure.

I will add yours at the next spin.

Thank you.

BTW, to all...

Any additional opinions about these patches?
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/block/bio.c b/block/bio.c
index 8338304ea256..4e18e959fc0a 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -917,17 +917,9 @@  int bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter)
 }
 EXPORT_SYMBOL_GPL(bio_iov_iter_get_pages);
 
-struct submit_bio_ret {
-	struct completion event;
-	int error;
-};
-
 static void submit_bio_wait_endio(struct bio *bio)
 {
-	struct submit_bio_ret *ret = bio->bi_private;
-
-	ret->error = blk_status_to_errno(bio->bi_status);
-	complete(&ret->event);
+	complete(bio->bi_private);
 }
 
 /**
@@ -943,16 +935,15 @@  static void submit_bio_wait_endio(struct bio *bio)
  */
 int submit_bio_wait(struct bio *bio)
 {
-	struct submit_bio_ret ret;
+	DECLARE_COMPLETION_ONSTACK(done);
 
-	init_completion(&ret.event);
-	bio->bi_private = &ret;
+	bio->bi_private = &done;
 	bio->bi_end_io = submit_bio_wait_endio;
 	bio->bi_opf |= REQ_SYNC;
 	submit_bio(bio);
-	wait_for_completion_io(&ret.event);
+	wait_for_completion_io(&done);
 
-	return ret.error;
+	return blk_status_to_errno(bio->bi_status);
 }
 EXPORT_SYMBOL(submit_bio_wait);