@@ -663,7 +663,10 @@ struct bio *bio_clone_fast(struct bio *bio, gfp_t
gfp_mask, struct bio_set *bs)
{
struct bio *b;
- b = bio_alloc_bioset(gfp_mask, 0, bs);
+ if (bs)
+ b = bio_alloc_bioset(gfp_mask, 0, bs);
+ else
+ b = bio_kmalloc(gfp_mask, 0);
if (!b)
return NULL;
P.S.This is purely based on the code inspection and it may not solve your
issue. Proceed with the caution as it may *break* your system.
@@ -217,6 +217,7 @@ static void bounce_end_io_read_isa(struct bio *bio)staticstructbio*bounce_clone_bio(structbio*bio_src,gfp_tgfp_mask,structbio_set*bs){+unsignedintnr_iovecs=bio_segments(bio_src);structbvec_iteriter;structbio_vecbv;structbio*bio;
@@ -243,7 +244,11 @@ static struct bio *bounce_clone_bio(struct bio
*bio_src, gfp_t gfp_mask,
* __bio_clone_fast() anyways.
*/
- bio = bio_alloc_bioset(gfp_mask, bio_segments(bio_src), bs);
+ if (bs)
+ bio = bio_alloc_bioset(gfp_mask, nr_iovecs, bs);
+ else
+ bio = bio_kmalloc(gfp_mask, nr_iovecs);
+
if (!bio)
return NULL;
bio->bi_bdev = bio_src->bi_bdev;
Since __blk_queue_bounce() passes the NULL for the passthru case as a
bio_set value ?
@@ -663,7 +663,10 @@ struct bio *bio_clone_fast(struct bio *bio, gfp_t
gfp_mask, struct bio_set *bs)
{
struct bio *b;
- b = bio_alloc_bioset(gfp_mask, 0, bs);
+ if (bs)
+ b = bio_alloc_bioset(gfp_mask, 0, bs);
+ else
+ b = bio_kmalloc(gfp_mask, 0);
if (!b)
return NULL;
P.S.This is purely based on the code inspection and it may not solve your
issue. Proceed with the caution as it may *break* your system.
So with an initial quick test, this patch (along with the follow-on
one you sent) seems to avoid the issue.
I'm wondering if given there are multiple call sites, that in
bio_alloc_bioset() would something like the following make more sense?
(apologies, copy pasted so this is whitespace corrupted)
thanks
-john
@@ -402,6 +402,9 @@ struct bio *bio_alloc_bioset(gfp_t gfp_mask,
unsigned short nr_iovecs,
struct bio *bio;
void *p;
+ if(!bs)
+ return bio_kmalloc(gfp_mask, 0);
+
/* should not use nobvec bioset for nr_iovecs > 0 */
if (WARN_ON_ONCE(!mempool_initialized(&bs->bvec_pool) && nr_iovecs > 0))
return NULL;
@@ -663,7 +663,10 @@ struct bio *bio_clone_fast(struct bio *bio, gfp_t
gfp_mask, struct bio_set *bs)
{
struct bio *b;
- b = bio_alloc_bioset(gfp_mask, 0, bs);
+ if (bs)
+ b = bio_alloc_bioset(gfp_mask, 0, bs);
+ else
+ b = bio_kmalloc(gfp_mask, 0);
if (!b)
return NULL;
P.S.This is purely based on the code inspection and it may not solve your
issue. Proceed with the caution as it may *break* your system.
So with an initial quick test, this patch (along with the follow-on
one you sent) seems to avoid the issue.
Thanks for bisecting, the patch you are referring to is with the similar
fix for bio bounce ?
I'm wondering if given there are multiple call sites, that in
bio_alloc_bioset() would something like the following make more sense?
(apologies, copy pasted so this is whitespace corrupted)
thanks
-john
I've asked Christoph about how we can go about this issue, based on his
reply I can send a patch.
I think having kmalloc is what we want to avoid in the fast path, with
that in
mind in my opinion we need to fix the call sites with kmalloc call if number
is small, which I think it is. Let's wait.
Meanwhile it will be great if you can share the result of the thorough
testing.
From: Christoph Hellwig <hch@lst.de> Date: 2021-02-23 07:05:06
The problem is that the blk-crypto fallback code calls bio_split
with a NULL bioset. That was aready broken before, as the mempool
needed to guarantee forward progress was missing, but is not fatal.
Satya, can you look into adding a mempool that can guarantees forward
progress here?
From: Christoph Hellwig <hch@lst.de> Date: 2021-02-23 07:12:56
On Mon, Feb 22, 2021 at 08:22:09PM -0800, John Stultz wrote:
I'm wondering if given there are multiple call sites, that in
bio_alloc_bioset() would something like the following make more sense?
(apologies, copy pasted so this is whitespace corrupted)
thanks
No. The fallback from the mempool backing to plain kmalloc is highly
dangerous, which is one of the reasons it was removed. The other being
that we don't want to disturb the fast path with this slow path.
From: Christoph Hellwig <hch@lst.de> Date: 2021-02-23 07:24:01
On Tue, Feb 23, 2021 at 08:04:08AM +0100, Christoph Hellwig wrote:
The problem is that the blk-crypto fallback code calls bio_split
with a NULL bioset. That was aready broken before, as the mempool
needed to guarantee forward progress was missing, but is not fatal.
Satya, can you look into adding a mempool that can guarantees forward
progress here?
Something like this would be the minimum viable fix:
On Tue, Feb 23, 2021 at 03:51:23AM +0000, Chaitanya Kulkarni wrote:
quoted
Looking at the other call sites do we need something like following ?
Since __blk_queue_bounce() passes the NULL for the passthru case as a
bio_set value ?
Well, that is a somewhat odd calling convention. What about the patch below
instead? That being we really need to kill this bouncing code off..
I assume you are sending this patch, let me know otherwise.
If you do please add, looks good.
Reviewed-by: Chaitanya Kulkarni <redacted>
From: Christoph Hellwig <hch@lst.de> Date: 2021-02-23 15:09:50
On Tue, Feb 23, 2021 at 07:37:52AM +0000, Chaitanya Kulkarni wrote:
On 2/22/21 23:10, Christoph Hellwig wrote:
quoted
On Tue, Feb 23, 2021 at 03:51:23AM +0000, Chaitanya Kulkarni wrote:
quoted
Looking at the other call sites do we need something like following ?
Since __blk_queue_bounce() passes the NULL for the passthru case as a
bio_set value ?
Well, that is a somewhat odd calling convention. What about the patch below
instead? That being we really need to kill this bouncing code off..
I assume you are sending this patch, let me know otherwise.
If you do please add, looks good.
I'll split the gfp_mask cleanup out, and will submit it with your as
the author if that is ok. I'll need a signoff, though.
From: Christoph Hellwig <hch@lst.de> Date: 2021-02-23 15:14:29
On Tue, Feb 23, 2021 at 04:08:52PM +0100, Christoph Hellwig wrote:
On Tue, Feb 23, 2021 at 07:37:52AM +0000, Chaitanya Kulkarni wrote:
quoted
On 2/22/21 23:10, Christoph Hellwig wrote:
quoted
On Tue, Feb 23, 2021 at 03:51:23AM +0000, Chaitanya Kulkarni wrote:
quoted
Looking at the other call sites do we need something like following ?
Since __blk_queue_bounce() passes the NULL for the passthru case as a
bio_set value ?
Well, that is a somewhat odd calling convention. What about the patch below
instead? That being we really need to kill this bouncing code off..
I assume you are sending this patch, let me know otherwise.
If you do please add, looks good.
I'll split the gfp_mask cleanup out, and will submit it with your as
the author if that is ok. I'll need a signoff, though.
Actually, I ended up reworking it once more as there is no point for
the parameter either.
I assume you are sending this patch, let me know otherwise.
If you do please add, looks good.
I'll split the gfp_mask cleanup out, and will submit it with your as
the author if that is ok. I'll need a signoff, though.
Okay, here it is :-
Signed-off-by: Chaitanya Kulkarni <redacted>.
I'll review the patch(es) once you post again.
From: John Stultz <hidden> Date: 2021-02-23 23:01:33
On Mon, Feb 22, 2021 at 11:22 PM Christoph Hellwig [off-list ref] wrote:
On Tue, Feb 23, 2021 at 08:04:08AM +0100, Christoph Hellwig wrote:
quoted
The problem is that the blk-crypto fallback code calls bio_split
with a NULL bioset. That was aready broken before, as the mempool
needed to guarantee forward progress was missing, but is not fatal.
Satya, can you look into adding a mempool that can guarantees forward
progress here?
Something like this would be the minimum viable fix:
This seems to work for me!
Tested-by: John Stultz <redacted>
thanks
-john