[6/7] scsi/osd: open code blk_make_request
diff mbox

Message ID 1465831278-23653-7-git-send-email-hch@lst.de
State New
Headers show

Commit Message

Christoph Hellwig June 13, 2016, 3:21 p.m. UTC
I wish the OSD code could simply use blk_rq_map_* helpers like
everyone else, but the complex nature of deciding if we have
DATA IN and/or DATA OUT buffers might make this impossible
(at least for a mere human like me).

But using blk_rq_append_bio at least allows sharing the setup code
between request with or without dat a buffers, and given that this
is the last user of blk_make_request it allows getting rid of that
somewhat awkward interface.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/blk-core.c                 | 57 ----------------------------------------
 drivers/scsi/osd/osd_initiator.c | 25 +++++++++++-------
 include/linux/blkdev.h           |  2 --
 3 files changed, 16 insertions(+), 68 deletions(-)

Comments

Boaz Harrosh June 15, 2016, 10:42 a.m. UTC | #1
On 06/13/2016 06:21 PM, Christoph Hellwig wrote:
> I wish the OSD code could simply use blk_rq_map_* helpers like
> everyone else, but the complex nature of deciding if we have
> DATA IN and/or DATA OUT buffers might make this impossible
> (at least for a mere human like me).
> 
> But using blk_rq_append_bio at least allows sharing the setup code
> between request with or without dat a buffers, and given that this
> is the last user of blk_make_request it allows getting rid of that
> somewhat awkward interface.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

I tested this and it works. But I have some comments. I think there
might be a bug in the original code

ACK-by: Boaz Harrosh <ooo@electrozaur.com>

> ---
>  block/blk-core.c                 | 57 ----------------------------------------
>  drivers/scsi/osd/osd_initiator.c | 25 +++++++++++-------
>  include/linux/blkdev.h           |  2 --
>  3 files changed, 16 insertions(+), 68 deletions(-)
> 
> diff --git a/block/blk-core.c b/block/blk-core.c
> index ceefa48..22b72ab 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -1319,63 +1319,6 @@ struct request *blk_get_request(struct request_queue *q, int rw, gfp_t gfp_mask)
>  EXPORT_SYMBOL(blk_get_request);
>  
>  /**
> - * blk_make_request - given a bio, allocate a corresponding struct request.
> - * @q: target request queue
> - * @bio:  The bio describing the memory mappings that will be submitted for IO.
> - *        It may be a chained-bio properly constructed by block/bio layer.
> - * @gfp_mask: gfp flags to be used for memory allocation
> - *
> - * blk_make_request is the parallel of generic_make_request for BLOCK_PC
> - * type commands. Where the struct request needs to be farther initialized by
> - * the caller. It is passed a &struct bio, which describes the memory info of
> - * the I/O transfer.
> - *
> - * The caller of blk_make_request must make sure that bi_io_vec
> - * are set to describe the memory buffers. That bio_data_dir() will return
> - * the needed direction of the request. (And all bio's in the passed bio-chain
> - * are properly set accordingly)
> - *
> - * If called under none-sleepable conditions, mapped bio buffers must not
> - * need bouncing, by calling the appropriate masked or flagged allocator,
> - * suitable for the target device. Otherwise the call to blk_queue_bounce will
> - * BUG.
> - *
> - * WARNING: When allocating/cloning a bio-chain, careful consideration should be
> - * given to how you allocate bios. In particular, you cannot use
> - * __GFP_DIRECT_RECLAIM for anything but the first bio in the chain. Otherwise
> - * you risk waiting for IO completion of a bio that hasn't been submitted yet,
> - * thus resulting in a deadlock. Alternatively bios should be allocated using
> - * bio_kmalloc() instead of bio_alloc(), as that avoids the mempool deadlock.
> - * If possible a big IO should be split into smaller parts when allocation
> - * fails. Partial allocation should not be an error, or you risk a live-lock.
> - */
> -struct request *blk_make_request(struct request_queue *q, struct bio *bio,
> -				 gfp_t gfp_mask)
> -{
> -	struct request *rq = blk_get_request(q, bio_data_dir(bio), gfp_mask);
> -
> -	if (IS_ERR(rq))
> -		return rq;
> -
> -	rq->cmd_type = REQ_TYPE_BLOCK_PC;
> -
> -	for_each_bio(bio) {
> -		struct bio *bounce_bio = bio;
> -		int ret;
> -
> -		blk_queue_bounce(q, &bounce_bio);
> -		ret = blk_rq_append_bio(rq, bounce_bio);
> -		if (unlikely(ret)) {
> -			blk_put_request(rq);
> -			return ERR_PTR(ret);
> -		}
> -	}
> -
> -	return rq;
> -}
> -EXPORT_SYMBOL(blk_make_request);
> -
> -/**
>   * blk_requeue_request - put a request back on queue
>   * @q:		request queue where request should be inserted
>   * @rq:		request to be inserted
> diff --git a/drivers/scsi/osd/osd_initiator.c b/drivers/scsi/osd/osd_initiator.c
> index 868ae29d..95f456f 100644
> --- a/drivers/scsi/osd/osd_initiator.c
> +++ b/drivers/scsi/osd/osd_initiator.c
> @@ -1558,18 +1558,25 @@ static int _osd_req_finalize_data_integrity(struct osd_request *or,
>  static struct request *_make_request(struct request_queue *q, bool has_write,
>  			      struct _osd_io_info *oii, gfp_t flags)
>  {
> -	if (oii->bio)
> -		return blk_make_request(q, oii->bio, flags);
> -	else {
> -		struct request *req;
> -
> -		req = blk_get_request(q, has_write ? WRITE : READ, flags);
> -		if (IS_ERR(req))
> -			return req;
> +	struct request *req;
> +	struct bio *bio = oii->bio;
> +	int ret;
>  
> -		req->cmd_type = REQ_TYPE_BLOCK_PC;
> +	req = blk_get_request(q, has_write ? WRITE : READ, flags);
> +	if (IS_ERR(req))
>  		return req;
> +	req->cmd_type = REQ_TYPE_BLOCK_PC;
> +
> +	for_each_bio(bio) {
> +		struct bio *bounce_bio = bio;
> +
> +		blk_queue_bounce(req->q, &bounce_bio);
> +		ret = blk_rq_append_bio(req, bounce_bio);

So you know how blk_rq_append_bio() puts the new bio on tail->bi_next
and then for_each_bio() goes to look for bio->bi_next. This is just crap
code that sets the bi_next pointers to what they were before.

But in the case where blk_queue_bounce() decides to return a new cloned
bio, we will get a short list and not hang all BIOs on the request list.

BUT since this is an old BUG not introduced here I don't care.
(See ACK above)

The only case where this can hit with current bidi supporting devices
is in 32bit & highmem. But I suspect last I tested many years ago that
32bit is not supported because of some other bugs.

Open coding for_each_bio() or introducing for_each_bio_safe would fix
this:
	while(bio) {
		struct bio *bounce_bio = bio;

		blk_queue_bounce(req->q, &bounce_bio);
		ret = blk_rq_append_bio(req, bounce_bio);
		if (ret)
			return ERR_PTR(ret);

		bio = bio->bi_next;
	}

> +		if (ret)
> +			return ERR_PTR(ret);
>  	}
> +
> +	return req;
>  }
>  

Thanks
Boaz

>  static int _init_blk_request(struct osd_request *or,
> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> index 998fbe0..8a984a7 100644
> --- a/include/linux/blkdev.h
> +++ b/include/linux/blkdev.h
> @@ -787,8 +787,6 @@ extern void blk_rq_init(struct request_queue *q, struct request *rq);
>  extern void blk_put_request(struct request *);
>  extern void __blk_put_request(struct request_queue *, struct request *);
>  extern struct request *blk_get_request(struct request_queue *, int, gfp_t);
> -extern struct request *blk_make_request(struct request_queue *, struct bio *,
> -					gfp_t);
>  extern void blk_requeue_request(struct request_queue *, struct request *);
>  extern void blk_add_request_payload(struct request *rq, struct page *page,
>  		int offset, unsigned int len);
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-block" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Boaz Harrosh June 15, 2016, 10:52 a.m. UTC | #2
On 06/15/2016 01:42 PM, Boaz Harrosh wrote:
> On 06/13/2016 06:21 PM, Christoph Hellwig wrote:
>> I wish the OSD code could simply use blk_rq_map_* helpers like
>> everyone else, but the complex nature of deciding if we have
>> DATA IN and/or DATA OUT buffers might make this impossible
>> (at least for a mere human like me).
>>
>> But using blk_rq_append_bio at least allows sharing the setup code
>> between request with or without dat a buffers, and given that this
>> is the last user of blk_make_request it allows getting rid of that
>> somewhat awkward interface.
>>
>> Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> I tested this and it works. But I have some comments. I think there
> might be a bug in the original code
> 
> ACK-by: Boaz Harrosh <ooo@electrozaur.com>
> 
>> ---
>>  block/blk-core.c                 | 57 ----------------------------------------
>>  drivers/scsi/osd/osd_initiator.c | 25 +++++++++++-------
>>  include/linux/blkdev.h           |  2 --
>>  3 files changed, 16 insertions(+), 68 deletions(-)
>>
>> diff --git a/block/blk-core.c b/block/blk-core.c
>> index ceefa48..22b72ab 100644
>> --- a/block/blk-core.c
>> +++ b/block/blk-core.c
>> @@ -1319,63 +1319,6 @@ struct request *blk_get_request(struct request_queue *q, int rw, gfp_t gfp_mask)
>>  EXPORT_SYMBOL(blk_get_request);
>>  
>>  /**
>> - * blk_make_request - given a bio, allocate a corresponding struct request.
>> - * @q: target request queue
>> - * @bio:  The bio describing the memory mappings that will be submitted for IO.
>> - *        It may be a chained-bio properly constructed by block/bio layer.
>> - * @gfp_mask: gfp flags to be used for memory allocation
>> - *
>> - * blk_make_request is the parallel of generic_make_request for BLOCK_PC
>> - * type commands. Where the struct request needs to be farther initialized by
>> - * the caller. It is passed a &struct bio, which describes the memory info of
>> - * the I/O transfer.
>> - *
>> - * The caller of blk_make_request must make sure that bi_io_vec
>> - * are set to describe the memory buffers. That bio_data_dir() will return
>> - * the needed direction of the request. (And all bio's in the passed bio-chain
>> - * are properly set accordingly)
>> - *
>> - * If called under none-sleepable conditions, mapped bio buffers must not
>> - * need bouncing, by calling the appropriate masked or flagged allocator,
>> - * suitable for the target device. Otherwise the call to blk_queue_bounce will
>> - * BUG.
>> - *
>> - * WARNING: When allocating/cloning a bio-chain, careful consideration should be
>> - * given to how you allocate bios. In particular, you cannot use
>> - * __GFP_DIRECT_RECLAIM for anything but the first bio in the chain. Otherwise
>> - * you risk waiting for IO completion of a bio that hasn't been submitted yet,
>> - * thus resulting in a deadlock. Alternatively bios should be allocated using
>> - * bio_kmalloc() instead of bio_alloc(), as that avoids the mempool deadlock.
>> - * If possible a big IO should be split into smaller parts when allocation
>> - * fails. Partial allocation should not be an error, or you risk a live-lock.
>> - */
>> -struct request *blk_make_request(struct request_queue *q, struct bio *bio,
>> -				 gfp_t gfp_mask)
>> -{
>> -	struct request *rq = blk_get_request(q, bio_data_dir(bio), gfp_mask);
>> -
>> -	if (IS_ERR(rq))
>> -		return rq;
>> -
>> -	rq->cmd_type = REQ_TYPE_BLOCK_PC;
>> -
>> -	for_each_bio(bio) {
>> -		struct bio *bounce_bio = bio;
>> -		int ret;
>> -
>> -		blk_queue_bounce(q, &bounce_bio);
>> -		ret = blk_rq_append_bio(rq, bounce_bio);
>> -		if (unlikely(ret)) {
>> -			blk_put_request(rq);
>> -			return ERR_PTR(ret);
>> -		}
>> -	}
>> -
>> -	return rq;
>> -}
>> -EXPORT_SYMBOL(blk_make_request);
>> -
>> -/**
>>   * blk_requeue_request - put a request back on queue
>>   * @q:		request queue where request should be inserted
>>   * @rq:		request to be inserted
>> diff --git a/drivers/scsi/osd/osd_initiator.c b/drivers/scsi/osd/osd_initiator.c
>> index 868ae29d..95f456f 100644
>> --- a/drivers/scsi/osd/osd_initiator.c
>> +++ b/drivers/scsi/osd/osd_initiator.c
>> @@ -1558,18 +1558,25 @@ static int _osd_req_finalize_data_integrity(struct osd_request *or,
>>  static struct request *_make_request(struct request_queue *q, bool has_write,
>>  			      struct _osd_io_info *oii, gfp_t flags)
>>  {
>> -	if (oii->bio)
>> -		return blk_make_request(q, oii->bio, flags);
>> -	else {
>> -		struct request *req;
>> -
>> -		req = blk_get_request(q, has_write ? WRITE : READ, flags);
>> -		if (IS_ERR(req))
>> -			return req;
>> +	struct request *req;
>> +	struct bio *bio = oii->bio;
>> +	int ret;
>>  
>> -		req->cmd_type = REQ_TYPE_BLOCK_PC;
>> +	req = blk_get_request(q, has_write ? WRITE : READ, flags);
>> +	if (IS_ERR(req))
>>  		return req;
>> +	req->cmd_type = REQ_TYPE_BLOCK_PC;
>> +
>> +	for_each_bio(bio) {
>> +		struct bio *bounce_bio = bio;
>> +
>> +		blk_queue_bounce(req->q, &bounce_bio);
>> +		ret = blk_rq_append_bio(req, bounce_bio);
> 
> So you know how blk_rq_append_bio() puts the new bio on tail->bi_next
> and then for_each_bio() goes to look for bio->bi_next. This is just crap
> code that sets the bi_next pointers to what they were before.
> 
> But in the case where blk_queue_bounce() decides to return a new cloned
> bio, we will get a short list and not hang all BIOs on the request list.
> 
> BUT since this is an old BUG not introduced here I don't care.
> (See ACK above)
> 
> The only case where this can hit with current bidi supporting devices
> is in 32bit & highmem. But I suspect last I tested many years ago that
> 32bit is not supported because of some other bugs.
> 
> Open coding for_each_bio() or introducing for_each_bio_safe would fix
> this:
> 	while(bio) {
> 		struct bio *bounce_bio = bio;
> 
> 		blk_queue_bounce(req->q, &bounce_bio);
> 		ret = blk_rq_append_bio(req, bounce_bio);
> 		if (ret)
> 			return ERR_PTR(ret);
> 
> 		bio = bio->bi_next;

OK sorry is exactly the same. No emails before coffee right?

Sorry for the noise it works just fine
Thanks - Boaz

> 	}
> 
>> +		if (ret)
>> +			return ERR_PTR(ret);
>>  	}
>> +
>> +	return req;
>>  }
>>  
> 
> Thanks
> Boaz
> 
>>  static int _init_blk_request(struct osd_request *or,
>> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
>> index 998fbe0..8a984a7 100644
>> --- a/include/linux/blkdev.h
>> +++ b/include/linux/blkdev.h
>> @@ -787,8 +787,6 @@ extern void blk_rq_init(struct request_queue *q, struct request *rq);
>>  extern void blk_put_request(struct request *);
>>  extern void __blk_put_request(struct request_queue *, struct request *);
>>  extern struct request *blk_get_request(struct request_queue *, int, gfp_t);
>> -extern struct request *blk_make_request(struct request_queue *, struct bio *,
>> -					gfp_t);
>>  extern void blk_requeue_request(struct request_queue *, struct request *);
>>  extern void blk_add_request_payload(struct request *rq, struct page *page,
>>  		int offset, unsigned int len);
>>
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-block" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

diff --git a/block/blk-core.c b/block/blk-core.c
index ceefa48..22b72ab 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -1319,63 +1319,6 @@  struct request *blk_get_request(struct request_queue *q, int rw, gfp_t gfp_mask)
 EXPORT_SYMBOL(blk_get_request);
 
 /**
- * blk_make_request - given a bio, allocate a corresponding struct request.
- * @q: target request queue
- * @bio:  The bio describing the memory mappings that will be submitted for IO.
- *        It may be a chained-bio properly constructed by block/bio layer.
- * @gfp_mask: gfp flags to be used for memory allocation
- *
- * blk_make_request is the parallel of generic_make_request for BLOCK_PC
- * type commands. Where the struct request needs to be farther initialized by
- * the caller. It is passed a &struct bio, which describes the memory info of
- * the I/O transfer.
- *
- * The caller of blk_make_request must make sure that bi_io_vec
- * are set to describe the memory buffers. That bio_data_dir() will return
- * the needed direction of the request. (And all bio's in the passed bio-chain
- * are properly set accordingly)
- *
- * If called under none-sleepable conditions, mapped bio buffers must not
- * need bouncing, by calling the appropriate masked or flagged allocator,
- * suitable for the target device. Otherwise the call to blk_queue_bounce will
- * BUG.
- *
- * WARNING: When allocating/cloning a bio-chain, careful consideration should be
- * given to how you allocate bios. In particular, you cannot use
- * __GFP_DIRECT_RECLAIM for anything but the first bio in the chain. Otherwise
- * you risk waiting for IO completion of a bio that hasn't been submitted yet,
- * thus resulting in a deadlock. Alternatively bios should be allocated using
- * bio_kmalloc() instead of bio_alloc(), as that avoids the mempool deadlock.
- * If possible a big IO should be split into smaller parts when allocation
- * fails. Partial allocation should not be an error, or you risk a live-lock.
- */
-struct request *blk_make_request(struct request_queue *q, struct bio *bio,
-				 gfp_t gfp_mask)
-{
-	struct request *rq = blk_get_request(q, bio_data_dir(bio), gfp_mask);
-
-	if (IS_ERR(rq))
-		return rq;
-
-	rq->cmd_type = REQ_TYPE_BLOCK_PC;
-
-	for_each_bio(bio) {
-		struct bio *bounce_bio = bio;
-		int ret;
-
-		blk_queue_bounce(q, &bounce_bio);
-		ret = blk_rq_append_bio(rq, bounce_bio);
-		if (unlikely(ret)) {
-			blk_put_request(rq);
-			return ERR_PTR(ret);
-		}
-	}
-
-	return rq;
-}
-EXPORT_SYMBOL(blk_make_request);
-
-/**
  * blk_requeue_request - put a request back on queue
  * @q:		request queue where request should be inserted
  * @rq:		request to be inserted
diff --git a/drivers/scsi/osd/osd_initiator.c b/drivers/scsi/osd/osd_initiator.c
index 868ae29d..95f456f 100644
--- a/drivers/scsi/osd/osd_initiator.c
+++ b/drivers/scsi/osd/osd_initiator.c
@@ -1558,18 +1558,25 @@  static int _osd_req_finalize_data_integrity(struct osd_request *or,
 static struct request *_make_request(struct request_queue *q, bool has_write,
 			      struct _osd_io_info *oii, gfp_t flags)
 {
-	if (oii->bio)
-		return blk_make_request(q, oii->bio, flags);
-	else {
-		struct request *req;
-
-		req = blk_get_request(q, has_write ? WRITE : READ, flags);
-		if (IS_ERR(req))
-			return req;
+	struct request *req;
+	struct bio *bio = oii->bio;
+	int ret;
 
-		req->cmd_type = REQ_TYPE_BLOCK_PC;
+	req = blk_get_request(q, has_write ? WRITE : READ, flags);
+	if (IS_ERR(req))
 		return req;
+	req->cmd_type = REQ_TYPE_BLOCK_PC;
+
+	for_each_bio(bio) {
+		struct bio *bounce_bio = bio;
+
+		blk_queue_bounce(req->q, &bounce_bio);
+		ret = blk_rq_append_bio(req, bounce_bio);
+		if (ret)
+			return ERR_PTR(ret);
 	}
+
+	return req;
 }
 
 static int _init_blk_request(struct osd_request *or,
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 998fbe0..8a984a7 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -787,8 +787,6 @@  extern void blk_rq_init(struct request_queue *q, struct request *rq);
 extern void blk_put_request(struct request *);
 extern void __blk_put_request(struct request_queue *, struct request *);
 extern struct request *blk_get_request(struct request_queue *, int, gfp_t);
-extern struct request *blk_make_request(struct request_queue *, struct bio *,
-					gfp_t);
 extern void blk_requeue_request(struct request_queue *, struct request *);
 extern void blk_add_request_payload(struct request *rq, struct page *page,
 		int offset, unsigned int len);