Hello,
I see a problem with this patch:
Ming Lei <ming.lei@xxxxxxxxxx> writes:
> From: Jens Axboe <axboe@xxxxxxxxx>
>
> Commit caa4b02476e3(blk-map: call blk_queue_bounce from blk_rq_append_bio)
> moves blk_queue_bounce() into blk_rq_append_bio(), but don't consider
> that the bounced bio becomes invisiable to caller since the parameter type is
> 'struct bio *', which should have been 'struct bio **'.
>
> This patch fixes this issue by passing 'struct bio **' to
> blk_rq_append_bio(), then the bounced bio can be returned to caller.
>
> Also failure handling is considered too.
>
> Fixes: caa4b02476e3 ("blk-map: call blk_queue_bounce from blk_rq_append_bio")
> Cc: Christoph Hellwig <hch@xxxxxx>
> Reported-by: Michele Ballabio <barra_cuda@xxxxxxxxxxxx>
> (handling failure of blk_rq_append_bio(), only call bio_get() after
> blk_rq_append_bio() returns OK)
> Signed-off-by: Ming Lei <ming.lei@xxxxxxxxxx>
> ---
> block/blk-map.c | 38 ++++++++++++++++++++++----------------
> drivers/scsi/osd/osd_initiator.c | 4 +++-
> drivers/target/target_core_pscsi.c | 4 ++--
> include/linux/blkdev.h | 2 +-
> 4 files changed, 28 insertions(+), 20 deletions(-)
>
> diff --git a/block/blk-map.c b/block/blk-map.c
> index b21f8e86f120..d3a94719f03f 100644
> --- a/block/blk-map.c
> +++ b/block/blk-map.c
> @@ -12,22 +12,29 @@
> #include "blk.h"
>
> /*
> - * Append a bio to a passthrough request. Only works can be merged into
> - * the request based on the driver constraints.
> + * Append a bio to a passthrough request. Only works if the bio can be merged
> + * into the request based on the driver constraints.
> */
> -int blk_rq_append_bio(struct request *rq, struct bio *bio)
> +int blk_rq_append_bio(struct request *rq, struct bio **bio)
> {
> - blk_queue_bounce(rq->q, &bio);
> + struct bio *orig_bio = *bio;
> +
> + blk_queue_bounce(rq->q, bio);
>
> if (!rq->bio) {
> - blk_rq_bio_prep(rq->q, rq, bio);
> + blk_rq_bio_prep(rq->q, rq, *bio);
> } else {
> - if (!ll_back_merge_fn(rq->q, rq, bio))
> + if (!ll_back_merge_fn(rq->q, rq, *bio)) {
> + if (orig_bio != *bio) {
> + bio_put(*bio);
> + *bio = orig_bio;
1) Suppose the bio *does* bounce, which allocates some pages from the
DMA pool, and subsequently this test fails. bio_put won't deallocate
those pages. Prior to caa4b02476e3, it would be done by bio_endio, which
is IMHO the correct way to do it.
2) What if the bio bounces and goes into the splitting path of
__blk_queue_bounce. That would submit the request, which would be a
problem should this test or some other later fail, cancelling the whole
request.
For 1, I will send a patch. As for 2, it would be a problem for a long
time so I have a hunch it doesn't actually happen, however, I'd like to
know if it is somehow guaranteed.
Regards
Jiri Palecek
