[PATCH 0/2] block: two cleanup patches

[PATCH 0/2] block: two cleanup patches

[Shaohua Li]

From: Shaohua Li <shli@fb.com>

Hi,
Cleanup the bio_uninit staff as Christoph suggested. I don't remove
bio_disassociate_task from bio_free, as bio_associate_blkcg is an exported
symbol and can be called anywhere.

Thanks,
Shaohua

Shaohua Li (2):
  block: delete bio_uninit in blockdev
  block: delete bio_uninit

 block/bio.c         | 17 +++--------------
 fs/block_dev.c      |  4 +---
 include/linux/bio.h |  1 -
 3 files changed, 4 insertions(+), 18 deletions(-)

-- 
2.9.3

[PATCH 1/2] block: delete bio_uninit in blockdev

[Shaohua Li]

From: Shaohua Li <shli@fb.com>

This is to partially revert commit 9ae3b3f52c62 (block: provide
bio_uninit() free freeing integrity/task associations). With commit
b222dd2 (block: call bio_uninit in bio_endio) and 7c20f11(bio-integrity:
stop abusing bi_end_io), integrity/cgroup info is freed in bio_endio. We
don't need to call bio_unit in other places

Cc: Christoph Hellwig <hch@lst.de>
Signed-off-by: Shaohua Li <shli@fb.com>
---
 block/bio.c         | 8 +-------
 fs/block_dev.c      | 4 +---
 include/linux/bio.h | 1 -
 3 files changed, 2 insertions(+), 11 deletions(-)

diff --git a/block/bio.c b/block/bio.c
index 9a63597..ce078fb 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -240,11 +240,10 @@ struct bio_vec *bvec_alloc(gfp_t gfp_mask, int nr, unsigned long *idx,
 	return bvl;
 }
 
-void bio_uninit(struct bio *bio)
+static void bio_uninit(struct bio *bio)
 {
 	bio_disassociate_task(bio);
 }
-EXPORT_SYMBOL(bio_uninit);
 
 static void bio_free(struct bio *bio)
 {
@@ -269,11 +268,6 @@ static void bio_free(struct bio *bio)
 	}
 }
 
-/*
- * Users of this function have their own bio allocation. Subsequently,
- * they must remember to pair any call to bio_init() with bio_uninit()
- * when IO has completed, or when the bio is released.
- */
 void bio_init(struct bio *bio, struct bio_vec *table,
 	      unsigned short max_vecs)
 {
diff --git a/fs/block_dev.c b/fs/block_dev.c
index 9941dc8..673536e 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -264,9 +264,7 @@ __blkdev_direct_IO_simple(struct kiocb *iocb, struct iov_iter *iter,
 		kfree(vecs);
 
 	if (unlikely(bio.bi_status))
-		ret = blk_status_to_errno(bio.bi_status);
-
-	bio_uninit(&bio);
+		return blk_status_to_errno(bio.bi_status);
 
 	return ret;
 }
diff --git a/include/linux/bio.h b/include/linux/bio.h
index 7b1cf4b..8a56a50 100644
--- a/include/linux/bio.h
+++ b/include/linux/bio.h
@@ -444,7 +444,6 @@ extern void bio_advance(struct bio *, unsigned);
 
 extern void bio_init(struct bio *bio, struct bio_vec *table,
 		     unsigned short max_vecs);
-extern void bio_uninit(struct bio *);
 extern void bio_reset(struct bio *);
 void bio_chain(struct bio *, struct bio *);
 
-- 
2.9.3

[PATCH 2/2] block: delete bio_uninit

[Shaohua Li]

From: Shaohua Li <shli@fb.com>

bio_uninit only calls bio_disassociate_task now. It's meaningless to
have a wrap.

Cc: Christoph Hellwig <hch@lst.de>
Signed-off-by: Shaohua Li <shli@fb.com>
---
 block/bio.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/block/bio.c b/block/bio.c
index ce078fb..8773d1b 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -240,17 +240,12 @@ struct bio_vec *bvec_alloc(gfp_t gfp_mask, int nr, unsigned long *idx,
 	return bvl;
 }
 
-static void bio_uninit(struct bio *bio)
-{
-	bio_disassociate_task(bio);
-}
-
 static void bio_free(struct bio *bio)
 {
 	struct bio_set *bs = bio->bi_pool;
 	void *p;
 
-	bio_uninit(bio);
+	bio_disassociate_task(bio);
 
 	if (bs) {
 		bvec_free(bs->bvec_pool, bio->bi_io_vec, BVEC_POOL_IDX(bio));
@@ -294,7 +289,7 @@ void bio_reset(struct bio *bio)
 {
 	unsigned long flags = bio->bi_flags & (~0UL << BIO_RESET_BITS);
 
-	bio_uninit(bio);
+	bio_disassociate_task(bio);
 
 	memset(bio, 0, BIO_RESET_BYTES);
 	bio->bi_flags = flags;
@@ -1828,7 +1823,7 @@ void bio_endio(struct bio *bio)
 
 	blk_throtl_bio_endio(bio);
 	/* release cgroup info */
-	bio_uninit(bio);
+	bio_disassociate_task(bio);
 	if (bio->bi_end_io)
 		bio->bi_end_io(bio);
 }
-- 
2.9.3

Re: [PATCH 2/2] block: delete bio_uninit

[Christoph Hellwig]

>  static void bio_free(struct bio *bio)
>  {
>  	struct bio_set *bs = bio->bi_pool;
>  	void *p;
>  
> -	bio_uninit(bio);
> +	bio_disassociate_task(bio);

As said in the last mail I think there is no point in having this call..

> @@ -294,7 +289,7 @@ void bio_reset(struct bio *bio)
>  {
>  	unsigned long flags = bio->bi_flags & (~0UL << BIO_RESET_BITS);
>  
> -	bio_uninit(bio);
> +	bio_disassociate_task(bio);

.. and this one.  And I suspect it would make sense to have all these
changes in a single patch.

Re: [PATCH 2/2] block: delete bio_uninit

[Shaohua Li]

On Thu, Jul 13, 2017 at 09:45:19AM +0200, Christoph Hellwig wrote:
> >  static void bio_free(struct bio *bio)
> >  {
> >  	struct bio_set *bs = bio->bi_pool;
> >  	void *p;
> >  
> > -	bio_uninit(bio);
> > +	bio_disassociate_task(bio);
> 
> As said in the last mail I think there is no point in having this call..

I'm hesitant to do this. bio_associate_blkcg/bio_associate_current can be
called in any time for a bio, so we not just attach cgroup info to info in bio
submit (maybe the bio_associate_blkcg/bio_associate_current callers do sumbit
always, but I didn't audit yet). The other reason is I'd like
blk_throtl_bio_endio is only called once for the whole bio not for splitted
bio, so this depends on bio_free to free cgroup info for chained bio which
always does bio_put.

> > @@ -294,7 +289,7 @@ void bio_reset(struct bio *bio)
> >  {
> >  	unsigned long flags = bio->bi_flags & (~0UL << BIO_RESET_BITS);
> >  
> > -	bio_uninit(bio);
> > +	bio_disassociate_task(bio);
> 
> .. and this one.  And I suspect it would make sense to have all these
> changes in a single patch.

This one is likely ok, but again I didn't audit yet. I'm ok these changes are
in a single patch.

Thanks,
Shaohua

Re: [PATCH 2/2] block: delete bio_uninit

[Christoph Hellwig]

On Thu, Jul 13, 2017 at 09:50:23AM -0700, Shaohua Li wrote:
> > As said in the last mail I think there is no point in having this call..
> 
> I'm hesitant to do this. bio_associate_blkcg/bio_associate_current can be
> called in any time for a bio, so we not just attach cgroup info to info in bio
> submit (maybe the bio_associate_blkcg/bio_associate_current callers do sumbit
> always, but I didn't audit yet).

bio_associate_current is only called from blk_throtl_assoc_bio, which
is only called from blk_throtl_bio, which is only called from
blkcg_bio_issue_check, which is only called from
generic_make_request_checks, which is only called from
generic_make_request, which at that point consumes the bio from the
callers perspective.

bio_associate_blkcg might be a different story, but then we should
treat both very differently.

> The other reason is I'd like
> blk_throtl_bio_endio is only called once for the whole bio not for splitted
> bio, so this depends on bio_free to free cgroup info for chained bio which
> always does bio_put.

Then we'll need to fix that.  We really should not require every
caller of bio_init to pair it with a new uninit call, which would be
a whole lot more work.