* [PATCH 0/2] block: two cleanup patches @ 2017-07-12 18:00 Shaohua Li 2017-07-12 18:00 ` [PATCH 1/2] block: delete bio_uninit in blockdev Shaohua Li 2017-07-12 18:00 ` [PATCH 2/2] block: delete bio_uninit Shaohua Li 0 siblings, 2 replies; 6+ messages in thread From: Shaohua Li @ 2017-07-12 18:00 UTC (permalink / raw) To: linux-block; +Cc: axboe, 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] block: delete bio_uninit in blockdev 2017-07-12 18:00 [PATCH 0/2] block: two cleanup patches Shaohua Li @ 2017-07-12 18:00 ` Shaohua Li 2017-07-12 18:00 ` [PATCH 2/2] block: delete bio_uninit Shaohua Li 1 sibling, 0 replies; 6+ messages in thread From: Shaohua Li @ 2017-07-12 18:00 UTC (permalink / raw) To: linux-block; +Cc: axboe, Shaohua Li, Christoph Hellwig 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] block: delete bio_uninit 2017-07-12 18:00 [PATCH 0/2] block: two cleanup patches Shaohua Li 2017-07-12 18:00 ` [PATCH 1/2] block: delete bio_uninit in blockdev Shaohua Li @ 2017-07-12 18:00 ` Shaohua Li 2017-07-13 7:45 ` Christoph Hellwig 1 sibling, 1 reply; 6+ messages in thread From: Shaohua Li @ 2017-07-12 18:00 UTC (permalink / raw) To: linux-block; +Cc: axboe, Shaohua Li, Christoph Hellwig 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] block: delete bio_uninit 2017-07-12 18:00 ` [PATCH 2/2] block: delete bio_uninit Shaohua Li @ 2017-07-13 7:45 ` Christoph Hellwig 2017-07-13 16:50 ` Shaohua Li 0 siblings, 1 reply; 6+ messages in thread From: Christoph Hellwig @ 2017-07-13 7:45 UTC (permalink / raw) To: Shaohua Li; +Cc: linux-block, axboe, Shaohua Li, 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] block: delete bio_uninit 2017-07-13 7:45 ` Christoph Hellwig @ 2017-07-13 16:50 ` Shaohua Li 2017-07-13 17:23 ` Christoph Hellwig 0 siblings, 1 reply; 6+ messages in thread From: Shaohua Li @ 2017-07-13 16:50 UTC (permalink / raw) To: Christoph Hellwig; +Cc: linux-block, axboe, 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] block: delete bio_uninit 2017-07-13 16:50 ` Shaohua Li @ 2017-07-13 17:23 ` Christoph Hellwig 0 siblings, 0 replies; 6+ messages in thread From: Christoph Hellwig @ 2017-07-13 17:23 UTC (permalink / raw) To: Shaohua Li; +Cc: Christoph Hellwig, linux-block, axboe, Shaohua Li 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-07-13 17:23 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-07-12 18:00 [PATCH 0/2] block: two cleanup patches Shaohua Li 2017-07-12 18:00 ` [PATCH 1/2] block: delete bio_uninit in blockdev Shaohua Li 2017-07-12 18:00 ` [PATCH 2/2] block: delete bio_uninit Shaohua Li 2017-07-13 7:45 ` Christoph Hellwig 2017-07-13 16:50 ` Shaohua Li 2017-07-13 17:23 ` Christoph Hellwig
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).