| Message ID | 20190322131346.20169-4-jthumshirn@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | add flag for tracking bio allocation | expand |
Looks fine,
Reviewed-by: Christoph Hellwig <hch@lst.de>
But how do you manage to get the tiny on-stack bios split? What kind
of setup is this?
On 3/22/19 3:02 PM, Christoph Hellwig wrote: > Looks fine, > > Reviewed-by: Christoph Hellwig <hch@lst.de> > > But how do you manage to get the tiny on-stack bios split? What kind > of setup is this? > It's not tiny if you send a 2M file via direct-io, _and_ have a non-zero MDTS setting... Cheers, Hannes
On 3/22/19 2:13 PM, Johannes Thumshirn wrote: > When we're submitting a bio from stack and this ends up being split, we > call bio_put(). bio_put() will eventually call bio_free() if the reference > count drops to 0. But freeing the bio is wrong, as it was never allocated > out of the bio's mempool. > > Flag each normally allocated bio as 'BIO_ALLOCATED' and skip freeing if the > flag isn't set. > > Fixes: 189ce2b9dcc3 ("block: fast-path for small and simple direct I/O requests") > Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de> > --- > block/bio.c | 4 ++++ > include/linux/blk_types.h | 1 + > 2 files changed, 5 insertions(+) > Reviewed-by: Hannes Reinecke <hare@suse.com> Although I do question the 'Fixes' tag here; without the first two this patch would be pretty pointless, so I'd rather have all 3 marked with 'Fixes'. Cheers, Hannes
On Fri, Mar 22, 2019 at 03:05:46PM +0100, Hannes Reinecke wrote: > On 3/22/19 3:02 PM, Christoph Hellwig wrote: > > But how do you manage to get the tiny on-stack bios split? What kind > > of setup is this? > > > It's not tiny if you send a 2M file via direct-io, _and_ have a non-zero > MDTS setting... I see a larger request can create a bio chain (though I think 1M is the max that goes through _simple), but how does the stack bio get used in a bio_put()? The chained bios are allocated through a bio_set, and their bio_end_io() calls bio_put() on themselves through bio_chain_endio(), but that's it. The original's endio is never modified from blkdev_bio_end_io_simple(), so who's calling bio_put() on the on-stack bio?
On Sat, Mar 23, 2019 at 5:30 AM Keith Busch <kbusch@kernel.org> wrote: > > On Fri, Mar 22, 2019 at 03:05:46PM +0100, Hannes Reinecke wrote: > > On 3/22/19 3:02 PM, Christoph Hellwig wrote: > > > But how do you manage to get the tiny on-stack bios split? What kind > > > of setup is this? > > > > > It's not tiny if you send a 2M file via direct-io, _and_ have a non-zero > > MDTS setting... > > I see a larger request can create a bio chain (though I think 1M > is the max that goes through _simple), but how does the stack bio multi-page bvec is merged now, 1M isn't the max size any more. > get used in a bio_put()? The chained bios are allocated through a > bio_set, and their bio_end_io() calls bio_put() on themselves through > bio_chain_endio(), but that's it. The original's endio is never modified > from blkdev_bio_end_io_simple(), so who's calling bio_put() on the > on-stack bio? I have the same concern, blkdev_bio_end_io_simple() doesn't release the bio, and this bio shouldn't be released until __blkdev_direct_IO_simple() returns. How can any underlying driver or block layer free such bio coming from upper layer? thanks, Ming Lei
diff --git a/block/bio.c b/block/bio.c index 87a515e93bee..ba6949f111b7 100644 --- a/block/bio.c +++ b/block/bio.c @@ -255,6 +255,9 @@ static void bio_free(struct bio *bio) bio_uninit(bio); + if (!bio_flagged(bio, BIO_ALLOCED)) + return; + if (bs) { bvec_free(&bs->bvec_pool, bio->bi_io_vec, BVEC_POOL_IDX(bio)); @@ -523,6 +526,7 @@ struct bio *bio_alloc_bioset(gfp_t gfp_mask, unsigned int nr_iovecs, bvl = bio->bi_inline_vecs; } + bio_set_flag(bio, BIO_ALLOCED); bio->bi_pool = bs; bio->bi_max_vecs = nr_iovecs; bio->bi_io_vec = bvl; diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h index 1d28992a20f0..19d7402a9af3 100644 --- a/include/linux/blk_types.h +++ b/include/linux/blk_types.h @@ -229,6 +229,7 @@ enum { * of this bio. */ BIO_QUEUE_ENTERED, /* can use blk_queue_enter_live() */ BIO_TRACKED, /* set if bio goes through the rq_qos path */ + BIO_ALLOCED, /* bio allocated by bio_alloc_bioset */ BIO_FLAG_LAST };
When we're submitting a bio from stack and this ends up being split, we call bio_put(). bio_put() will eventually call bio_free() if the reference count drops to 0. But freeing the bio is wrong, as it was never allocated out of the bio's mempool. Flag each normally allocated bio as 'BIO_ALLOCATED' and skip freeing if the flag isn't set. Fixes: 189ce2b9dcc3 ("block: fast-path for small and simple direct I/O requests") Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de> --- block/bio.c | 4 ++++ include/linux/blk_types.h | 1 + 2 files changed, 5 insertions(+)