[patchset 0/5 version 2] osd: Stop usage of blk_rq_append_bio

[patchset 0/5 version 2] osd: Stop usage of blk_rq_append_bio

[Boaz Harrosh]

[Version 2]
- Update to axboe/linux-2.6-block.git for-2.6.31 branch
- Added 5th patch that removes the export of blk_rq_append_bio()

[Version 1]
Osd library needs to submit pre-allocated bios, form several sources.
osdblk exofs and pNFS-layout driver all have prepared bios for IO submission.
On top of that the osd library needs to append additional segments to the
IO memory, for get/set attributes and more.

All these are done today by use of a temporary hack - blk_rq_append_bio().
This is bad on few accounts.
1. blk_rq_append_bio was not meant to be exported and is very specific to its users.
2. blk_rq_append_bio does not support chained bios.
3. blk_rq_append_bio does not bounce the bio and therefore current osd implementation
   has a bug.

The proposed solution adds two new fixtures to the block layer, and a corresponding
fixing patch to osd. These are:

[PATCH 1/5] allow blk_rq_map_kern to append to requests
[PATCH 2/5] libosd: Use new blk_rq_map_kern

  This is originally a James patch. It is used, to let blk_rq_map_kern append it's buffer
  to existing bio, and therefor is able to be called multiple times in a loop, to append
  multiple segments. This API can also be useful for scsi/block targets that have segment
  information in some other memory structure (like scatterlist) and wants to set it into
  a request. Until such time that they have a proper support for mapping scatterlists directly.
  (Above called on long lists might not be good for performance)

  Here in osd it makes tons of sense.

[PATCH 3/5] New blk_make_request(), takes bio, returns a request
[PATCH 4/5] libosd: Use of new blk_make_request
  
  Here I propose a new block API, that will support proper delegation of a bio
  to a full request. Please read inside the patch descriptions for details.
  After this patch both osd and block layer will have the proper support for osdblk
  driver as well as future needs.
  These patches also eliminate the last use of blk_rq_append_bio which can be finally un-exported.

[PATCH 5/5] Un-export blk_rq_append_bio

Thank you
Boaz

[PATCH 1/5] allow blk_rq_map_kern to append to requests

[Boaz Harrosh]

From: James Bottomley <James.Bottomley@HansenPartnership.com>

Use blk_rq_append_bio() internally instead of blk_rq_bio_prep()
so blk_rq_map_kern can be called multiple times, to map multiple
buffers.

This is in the effort to un-export blk_rq_append_bio()

Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 block/blk-map.c |   12 ++++++++++--
 1 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/block/blk-map.c b/block/blk-map.c
index 56082be..caa05a6 100644
--- a/block/blk-map.c
+++ b/block/blk-map.c
@@ -282,7 +282,8 @@ EXPORT_SYMBOL(blk_rq_unmap_user);
  *
  * Description:
  *    Data will be mapped directly if possible. Otherwise a bounce
- *    buffer is used.
+ *    buffer is used. Can be called multple times to append multple
+ *    buffers.
  */
 int blk_rq_map_kern(struct request_queue *q, struct request *rq, void *kbuf,
 		    unsigned int len, gfp_t gfp_mask)
@@ -290,6 +291,7 @@ int blk_rq_map_kern(struct request_queue *q, struct request *rq, void *kbuf,
 	int reading = rq_data_dir(rq) == READ;
 	int do_copy = 0;
 	struct bio *bio;
+	int ret;
 
 	if (len > (q->max_hw_sectors << 9))
 		return -EINVAL;
@@ -311,7 +313,13 @@ int blk_rq_map_kern(struct request_queue *q, struct request *rq, void *kbuf,
 	if (do_copy)
 		rq->cmd_flags |= REQ_COPY_USER;
 
-	blk_rq_bio_prep(q, rq, bio);
+	ret = blk_rq_append_bio(q, rq, bio);
+	if (unlikely(ret)) {
+		/* request is too big */
+		bio_put(bio);
+		return ret;
+	}
+
 	blk_queue_bounce(q, &rq->bio);
 	rq->buffer = NULL;
 	return 0;
-- 
1.6.2.1

[PATCH 2/5] libosd: Use new blk_rq_map_kern

[Boaz Harrosh]

Now that blk_rq_map_kern will append the buffer onto the
request we can use it easily for adding extra segments
(eg. attributes)

This patch is dependent on a block layer patch titled:
   [BLOCK] allow blk_rq_map_kern to append to requests

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 drivers/scsi/osd/osd_initiator.c |   24 ++----------------------
 1 files changed, 2 insertions(+), 22 deletions(-)

diff --git a/drivers/scsi/osd/osd_initiator.c b/drivers/scsi/osd/osd_initiator.c
index d178a8b..bf66e30 100644
--- a/drivers/scsi/osd/osd_initiator.c
+++ b/drivers/scsi/osd/osd_initiator.c
@@ -826,26 +826,6 @@ int osd_req_add_set_attr_list(struct osd_request *or,
 }
 EXPORT_SYMBOL(osd_req_add_set_attr_list);
 
-static int _append_map_kern(struct request *req,
-	void *buff, unsigned len, gfp_t flags)
-{
-	struct bio *bio;
-	int ret;
-
-	bio = bio_map_kern(req->q, buff, len, flags);
-	if (IS_ERR(bio)) {
-		OSD_ERR("Failed bio_map_kern(%p, %d) => %ld\n", buff, len,
-			PTR_ERR(bio));
-		return PTR_ERR(bio);
-	}
-	ret = blk_rq_append_bio(req->q, req, bio);
-	if (ret) {
-		OSD_ERR("Failed blk_rq_append_bio(%p) => %d\n", bio, ret);
-		bio_put(bio);
-	}
-	return ret;
-}
-
 static int _req_append_segment(struct osd_request *or,
 	unsigned padding, struct _osd_req_data_segment *seg,
 	struct _osd_req_data_segment *last_seg, struct _osd_io_info *io)
@@ -861,14 +841,14 @@ static int _req_append_segment(struct osd_request *or,
 		else
 			pad_buff = io->pad_buff;
 
-		ret = _append_map_kern(io->req, pad_buff, padding,
+		ret = blk_rq_map_kern(io->req->q, io->req, pad_buff, padding,
 				       or->alloc_flags);
 		if (ret)
 			return ret;
 		io->total_bytes += padding;
 	}
 
-	ret = _append_map_kern(io->req, seg->buff, seg->total_bytes,
+	ret = blk_rq_map_kern(io->req->q, io->req, seg->buff, seg->total_bytes,
 			       or->alloc_flags);
 	if (ret)
 		return ret;
-- 
1.6.2.1

[PATCH 3/5] New blk_make_request(), takes bio, returns a request

[Boaz Harrosh]

New block API:
given a struct bio allocates a new request. This is the parallel of
generic_make_request for BLOCK_PC commands users.

The passed bio may be a chained-bio. The bio is bounced if needed
inside the call to this member.

This is in the effort of un-exporting blk_rq_append_bio().

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
CC: Jeff Garzik <jeff@garzik.org>
---
 block/blk-core.c       |   45 +++++++++++++++++++++++++++++++++++++++++++++
 include/linux/blkdev.h |    2 ++
 2 files changed, 47 insertions(+), 0 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index a2d97de..89261d2 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -891,6 +891,51 @@ 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.
+ *
+ * @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.
+ *
+ * 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.
+ */
+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 (unlikely(!rq))
+		return ERR_PTR(-ENOMEM);
+
+	for_each_bio(bio) {
+		struct bio *bounce_bio = bio;
+		int ret;
+
+		blk_queue_bounce(q, &bounce_bio);
+		ret = blk_rq_append_bio(q, 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/include/linux/blkdev.h b/include/linux/blkdev.h
index f9d60a7..88a83e1 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -740,6 +740,8 @@ 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_insert_request(struct request_queue *, struct request *, int, void *);
 extern void blk_requeue_request(struct request_queue *, struct request *);
 extern int blk_rq_check_limits(struct request_queue *q, struct request *rq);
-- 
1.6.2.1

[PATCH 4/5] libosd: Use of new blk_make_request

[Boaz Harrosh]

Use new blk_make_request() to allocate a request from bio
and avoid using deprecated blk_rq_append_bio().

This patch is dependent on a block layer patch titled:
    [BLOCK] New blk_make_request() takes bio returns request

This is the last usage of blk_rq_append_bio in osd, it can now
be un-exported.

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
CC: Jeff Garzik <jeff@garzik.org>
CC: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
---
 drivers/scsi/osd/osd_initiator.c |   48 ++++++++++++++++++-------------------
 1 files changed, 23 insertions(+), 25 deletions(-)

diff --git a/drivers/scsi/osd/osd_initiator.c b/drivers/scsi/osd/osd_initiator.c
index bf66e30..865ec0f 100644
--- a/drivers/scsi/osd/osd_initiator.c
+++ b/drivers/scsi/osd/osd_initiator.c
@@ -1200,6 +1200,21 @@ static int _osd_req_finalize_data_integrity(struct osd_request *or,
 /*
  * osd_finalize_request and helpers
  */
+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 (unlikely(!req))
+			return ERR_PTR(-ENOMEM);
+
+		return req;
+	}
+}
 
 static int _init_blk_request(struct osd_request *or,
 	bool has_in, bool has_out)
@@ -1208,11 +1223,13 @@ static int _init_blk_request(struct osd_request *or,
 	struct scsi_device *scsi_device = or->osd_dev->scsi_device;
 	struct request_queue *q = scsi_device->request_queue;
 	struct request *req;
-	int ret = -ENOMEM;
+	int ret;
 
-	req = blk_get_request(q, has_out, flags);
-	if (!req)
+	req = _make_request(q, has_out, has_out ? &or->out : &or->in, flags);
+	if (IS_ERR(req)) {
+		ret = PTR_ERR(req);
 		goto out;
+	}
 
 	or->request = req;
 	req->cmd_type = REQ_TYPE_BLOCK_PC;
@@ -1225,9 +1242,10 @@ static int _init_blk_request(struct osd_request *or,
 		or->out.req = req;
 		if (has_in) {
 			/* allocate bidi request */
-			req = blk_get_request(q, READ, flags);
-			if (!req) {
+			req = _make_request(q, false, &or->in, flags);
+			if (IS_ERR(req)) {
 				OSD_DEBUG("blk_get_request for bidi failed\n");
+				ret = PTR_ERR(req);
 				goto out;
 			}
 			req->cmd_type = REQ_TYPE_BLOCK_PC;
@@ -1271,26 +1289,6 @@ int osd_finalize_request(struct osd_request *or,
 		return ret;
 	}
 
-	if (or->out.bio) {
-		ret = blk_rq_append_bio(or->request->q, or->out.req,
-					or->out.bio);
-		if (ret) {
-			OSD_DEBUG("blk_rq_append_bio out failed\n");
-			return ret;
-		}
-		OSD_DEBUG("out bytes=%llu (bytes_req=%u)\n",
-			_LLU(or->out.total_bytes), blk_rq_bytes(or->out.req));
-	}
-	if (or->in.bio) {
-		ret = blk_rq_append_bio(or->request->q, or->in.req, or->in.bio);
-		if (ret) {
-			OSD_DEBUG("blk_rq_append_bio in failed\n");
-			return ret;
-		}
-		OSD_DEBUG("in bytes=%llu (bytes_req=%u)\n",
-			_LLU(or->in.total_bytes), blk_rq_bytes(or->in.req));
-	}
-
 	or->out.pad_buff = sg_out_pad_buffer;
 	or->in.pad_buff = sg_in_pad_buffer;
 
-- 
1.6.2.1

[PATCH 5/5] Un-export blk_rq_append_bio

[Boaz Harrosh]

OSD was the last in-tree user of blk_rq_append_bio(). Now
that it is fixed blk_rq_append_bio is un-exported and
is only used internally by block layer.

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 block/blk-map.c        |    1 -
 block/blk.h            |    2 ++
 include/linux/blkdev.h |    6 ------
 3 files changed, 2 insertions(+), 7 deletions(-)

diff --git a/block/blk-map.c b/block/blk-map.c
index caa05a6..ef2492a 100644
--- a/block/blk-map.c
+++ b/block/blk-map.c
@@ -24,7 +24,6 @@ int blk_rq_append_bio(struct request_queue *q, struct request *rq,
 	}
 	return 0;
 }
-EXPORT_SYMBOL(blk_rq_append_bio);
 
 static int __blk_rq_unmap_user(struct bio *bio)
 {
diff --git a/block/blk.h b/block/blk.h
index 9e0042c..c863ec2 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -13,6 +13,8 @@ extern struct kobj_type blk_queue_ktype;
 void init_request_from_bio(struct request *req, struct bio *bio);
 void blk_rq_bio_prep(struct request_queue *q, struct request *rq,
 			struct bio *bio);
+int blk_rq_append_bio(struct request_queue *q, struct request *rq,
+		      struct bio *bio);
 void blk_dequeue_request(struct request *rq);
 void __blk_queue_free_tags(struct request_queue *q);
 
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 88a83e1..564445b 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -758,12 +758,6 @@ extern int sg_scsi_ioctl(struct request_queue *, struct gendisk *, fmode_t,
 			 struct scsi_ioctl_command __user *);
 
 /*
- * Temporary export, until SCSI gets fixed up.
- */
-extern int blk_rq_append_bio(struct request_queue *q, struct request *rq,
-			     struct bio *bio);
-
-/*
  * A queue has just exitted congestion.  Note this in the global counter of
  * congested queues, and wake up anyone who was waiting for requests to be
  * put back.
-- 
1.6.2.1

Re: [PATCH 3/5] New blk_make_request(), takes bio, returns a request

[Jens Axboe]

On Sun, May 17 2009, Boaz Harrosh wrote:
> 
> New block API:
> given a struct bio allocates a new request. This is the parallel of
> generic_make_request for BLOCK_PC commands users.
> 
> The passed bio may be a chained-bio. The bio is bounced if needed
> inside the call to this member.
> 
> This is in the effort of un-exporting blk_rq_append_bio().
> 
> Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
> CC: Jeff Garzik <jeff@garzik.org>
> ---
>  block/blk-core.c       |   45 +++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/blkdev.h |    2 ++
>  2 files changed, 47 insertions(+), 0 deletions(-)
> 
> diff --git a/block/blk-core.c b/block/blk-core.c
> index a2d97de..89261d2 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -891,6 +891,51 @@ 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.
> + *
> + * @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.
> + *
> + * 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.
> + */
> +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 (unlikely(!rq))
> +		return ERR_PTR(-ENOMEM);
> +
> +	for_each_bio(bio) {
> +		struct bio *bounce_bio = bio;
> +		int ret;
> +
> +		blk_queue_bounce(q, &bounce_bio);
> +		ret = blk_rq_append_bio(q, rq, bounce_bio);
> +		if (unlikely(ret)) {
> +			blk_put_request(rq);
> +			return ERR_PTR(ret);
> +		}
> +	}
> +
> +	return rq;
> +}
> +EXPORT_SYMBOL(blk_make_request);

Generally the patchset looks good, my only worry is that interfaces like
the above may be asking for trouble. To generate a chained list of
bio's, you need to be careful with how you allocate them. In particular,
you cannot use __GFP_WAIT for anything but the first bio in the chain.
Otherwise you risk deadlocking waiting for a bio to be returned to the
pool, which it never will since you haven't submitted it yet.


> +
> +/**
>   * 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/include/linux/blkdev.h b/include/linux/blkdev.h
> index f9d60a7..88a83e1 100644
> --- a/include/linux/blkdev.h
> +++ b/include/linux/blkdev.h
> @@ -740,6 +740,8 @@ 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_insert_request(struct request_queue *, struct request *, int, void *);
>  extern void blk_requeue_request(struct request_queue *, struct request *);
>  extern int blk_rq_check_limits(struct request_queue *q, struct request *rq);
> -- 
> 1.6.2.1
> 
> 

-- 
Jens Axboe

Re: [PATCH 3/5] New blk_make_request(), takes bio, returns a request

[Boaz Harrosh]

On 05/19/2009 12:41 PM, Jens Axboe wrote:
> On Sun, May 17 2009, Boaz Harrosh wrote:
>> New block API:
>> given a struct bio allocates a new request. This is the parallel of
>> generic_make_request for BLOCK_PC commands users.
>>
>> The passed bio may be a chained-bio. The bio is bounced if needed
>> inside the call to this member.
>>
>> This is in the effort of un-exporting blk_rq_append_bio().
>>
>> Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
>> CC: Jeff Garzik <jeff@garzik.org>
>> ---
>>  block/blk-core.c       |   45 +++++++++++++++++++++++++++++++++++++++++++++
>>  include/linux/blkdev.h |    2 ++
>>  2 files changed, 47 insertions(+), 0 deletions(-)
>>
>> diff --git a/block/blk-core.c b/block/blk-core.c
>> index a2d97de..89261d2 100644
>> --- a/block/blk-core.c
>> +++ b/block/blk-core.c
>> @@ -891,6 +891,51 @@ 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.
>> + *
>> + * @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.
>> + *
>> + * 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.
>> + */
>> +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 (unlikely(!rq))
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	for_each_bio(bio) {
>> +		struct bio *bounce_bio = bio;
>> +		int ret;
>> +
>> +		blk_queue_bounce(q, &bounce_bio);
>> +		ret = blk_rq_append_bio(q, rq, bounce_bio);
>> +		if (unlikely(ret)) {
>> +			blk_put_request(rq);
>> +			return ERR_PTR(ret);
>> +		}
>> +	}
>> +
>> +	return rq;
>> +}
>> +EXPORT_SYMBOL(blk_make_request);
> 
> Generally the patchset looks good, my only worry is that interfaces like
> the above may be asking for trouble. To generate a chained list of
> bio's, you need to be careful with how you allocate them. In particular,
> you cannot use __GFP_WAIT for anything but the first bio in the chain.
> Otherwise you risk deadlocking waiting for a bio to be returned to the
> pool, which it never will since you haven't submitted it yet.
> 
> 

Thanks Jens, for your comment.

I have three sources of bio allocations.
1. bio_map_kern which uses bio_kmalloc (recently fixed by Tejun)
2. by osdblk which does a clone and will not ever wait.
   (I've fixed the code to split up the IO on allocation failure into
    smaller requests (will repost soon))
3. Future code in exofs and pNFS-Client that will only ever use bio_kmalloc.

Should we add something to the Documentation, and/or above doc_book comment
to warn off users?

Boaz

Re: [PATCH 3/5] New blk_make_request(), takes bio, returns a request

[Jens Axboe]

On Tue, May 19 2009, Boaz Harrosh wrote:
> On 05/19/2009 12:41 PM, Jens Axboe wrote:
> > On Sun, May 17 2009, Boaz Harrosh wrote:
> >> New block API:
> >> given a struct bio allocates a new request. This is the parallel of
> >> generic_make_request for BLOCK_PC commands users.
> >>
> >> The passed bio may be a chained-bio. The bio is bounced if needed
> >> inside the call to this member.
> >>
> >> This is in the effort of un-exporting blk_rq_append_bio().
> >>
> >> Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
> >> CC: Jeff Garzik <jeff@garzik.org>
> >> ---
> >>  block/blk-core.c       |   45 +++++++++++++++++++++++++++++++++++++++++++++
> >>  include/linux/blkdev.h |    2 ++
> >>  2 files changed, 47 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/block/blk-core.c b/block/blk-core.c
> >> index a2d97de..89261d2 100644
> >> --- a/block/blk-core.c
> >> +++ b/block/blk-core.c
> >> @@ -891,6 +891,51 @@ 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.
> >> + *
> >> + * @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.
> >> + *
> >> + * 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.
> >> + */
> >> +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 (unlikely(!rq))
> >> +		return ERR_PTR(-ENOMEM);
> >> +
> >> +	for_each_bio(bio) {
> >> +		struct bio *bounce_bio = bio;
> >> +		int ret;
> >> +
> >> +		blk_queue_bounce(q, &bounce_bio);
> >> +		ret = blk_rq_append_bio(q, rq, bounce_bio);
> >> +		if (unlikely(ret)) {
> >> +			blk_put_request(rq);
> >> +			return ERR_PTR(ret);
> >> +		}
> >> +	}
> >> +
> >> +	return rq;
> >> +}
> >> +EXPORT_SYMBOL(blk_make_request);
> > 
> > Generally the patchset looks good, my only worry is that interfaces like
> > the above may be asking for trouble. To generate a chained list of
> > bio's, you need to be careful with how you allocate them. In particular,
> > you cannot use __GFP_WAIT for anything but the first bio in the chain.
> > Otherwise you risk deadlocking waiting for a bio to be returned to the
> > pool, which it never will since you haven't submitted it yet.
> > 
> > 
> 
> Thanks Jens, for your comment.
> 
> I have three sources of bio allocations.
> 1. bio_map_kern which uses bio_kmalloc (recently fixed by Tejun)
> 2. by osdblk which does a clone and will not ever wait.
>    (I've fixed the code to split up the IO on allocation failure into
>     smaller requests (will repost soon))
> 3. Future code in exofs and pNFS-Client that will only ever use bio_kmalloc.

All of those are fine!

> Should we add something to the Documentation, and/or above doc_book comment
> to warn off users?

Yes I think so. I'm generally weary of adding interfaces that are easy
to misuse. This one has that potential, but it also has merits. So I'll
merge your series, if you could send a patch updating the
comment/docbook, then that would be great.

-- 
Jens Axboe

Re: [PATCH 3/5] New blk_make_request(), takes bio, returns a request

[Boaz Harrosh]

On 05/19/2009 01:13 PM, Jens Axboe wrote:
> On Tue, May 19 2009, Boaz Harrosh wrote:
<snip>
>> Thanks Jens, for your comment.
>>
>> I have three sources of bio allocations.
>> 1. bio_map_kern which uses bio_kmalloc (recently fixed by Tejun)
>> 2. by osdblk which does a clone and will not ever wait.
>>    (I've fixed the code to split up the IO on allocation failure into
>>     smaller requests (will repost soon))
>> 3. Future code in exofs and pNFS-Client that will only ever use bio_kmalloc.
> 
> All of those are fine!
> 
>> Should we add something to the Documentation, and/or above doc_book comment
>> to warn off users?
> 
> Yes I think so. I'm generally weary of adding interfaces that are easy
> to misuse. This one has that potential, but it also has merits. So I'll
> merge your series, if you could send a patch updating the
> comment/docbook, then that would be great.
> 

As my English sucks, please read proof below addition. I will repost later
today.

Should I just post this one patch, or all the 5?
(alternatively I have these on a public git tree reabased on block/for-next branch.)

Thanks Boaz
---
diff --git a/block/blk-core.c b/block/blk-core.c
index 89261d2..4dc4e32 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -910,6 +910,15 @@ EXPORT_SYMBOL(blk_get_request);
  * 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_WAIT for
+ * anything but the first bio in the chain. Otherwise you risk deadlocking,
+ * waiting for a bio to be returned to the pool, which will never return, since
+ * it was not submitted yet.
+ * Alternatively bios should be allocated using bio_kmalloc only.
+ * If possible a long 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)

Re: [PATCH 3/5] New blk_make_request(), takes bio, returns a request

[Jens Axboe]

On Tue, May 19 2009, Boaz Harrosh wrote:
> On 05/19/2009 01:13 PM, Jens Axboe wrote:
> > On Tue, May 19 2009, Boaz Harrosh wrote:
> <snip>
> >> Thanks Jens, for your comment.
> >>
> >> I have three sources of bio allocations.
> >> 1. bio_map_kern which uses bio_kmalloc (recently fixed by Tejun)
> >> 2. by osdblk which does a clone and will not ever wait.
> >>    (I've fixed the code to split up the IO on allocation failure into
> >>     smaller requests (will repost soon))
> >> 3. Future code in exofs and pNFS-Client that will only ever use bio_kmalloc.
> > 
> > All of those are fine!
> > 
> >> Should we add something to the Documentation, and/or above doc_book comment
> >> to warn off users?
> > 
> > Yes I think so. I'm generally weary of adding interfaces that are easy
> > to misuse. This one has that potential, but it also has merits. So I'll
> > merge your series, if you could send a patch updating the
> > comment/docbook, then that would be great.
> > 
> 
> As my English sucks, please read proof below addition. I will repost later
> today.
> 
> Should I just post this one patch, or all the 5?
> (alternatively I have these on a public git tree reabased on
> block/for-next branch.)

Just this one is fine, I already merged the series.

> Thanks Boaz
> ---
> diff --git a/block/blk-core.c b/block/blk-core.c
> index 89261d2..4dc4e32 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -910,6 +910,15 @@ EXPORT_SYMBOL(blk_get_request);
>   * 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_WAIT for
> + * anything but the first bio in the chain. Otherwise you risk deadlocking,
> + * waiting for a bio to be returned to the pool, which will never return, since
> + * it was not submitted yet.

Perhaps something like:

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 only.
> + * If possible a long 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)

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 ...

-- 
Jens Axboe

Re: [PATCH 3/5] New blk_make_request(), takes bio, returns a request

[Boaz Harrosh]

On 05/19/2009 03:49 PM, Jens Axboe wrote:
> On Tue, May 19 2009, Boaz Harrosh wrote:
>> + * WARNING: When allocating/cloning a bio-chain, careful consideration should be
>> + * given to how you allocate bios. In particular, you cannot use __GFP_WAIT for
>> + * anything but the first bio in the chain. Otherwise you risk deadlocking,
>> + * waiting for a bio to be returned to the pool, which will never return, since
>> + * it was not submitted yet.
> 
> Perhaps something like:
> 
> 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 only.
>> + * If possible a long 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)
> 
> 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 ...
> 

Thanks Jens makes much more sense.

I'm posting as reply to original patch (with version 2 at title).

I'm also reposting a small forgoten patch that belongs to Tejun's
batch.

Boaz

[PATCH version 2] New blk_make_request(), takes bio, returns a request

[Boaz Harrosh]

New block API:
given a struct bio allocates a new request. This is the parallel of
generic_make_request for BLOCK_PC commands users.

The passed bio may be a chained-bio. The bio is bounced if needed
inside the call to this member.

This is in the effort of un-exporting blk_rq_append_bio().

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
CC: Jeff Garzik <jeff@garzik.org>
---
 block/blk-core.c       |   54 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/blkdev.h |    2 +
 2 files changed, 56 insertions(+), 0 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index a2d97de..3145a37 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -891,6 +891,60 @@ 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.
+ *
+ * @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.
+ *
+ * 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_WAIT 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 (unlikely(!rq))
+		return ERR_PTR(-ENOMEM);
+
+	for_each_bio(bio) {
+		struct bio *bounce_bio = bio;
+		int ret;
+
+		blk_queue_bounce(q, &bounce_bio);
+		ret = blk_rq_append_bio(q, 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/include/linux/blkdev.h b/include/linux/blkdev.h
index f9d60a7..88a83e1 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -740,6 +740,8 @@ 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_insert_request(struct request_queue *, struct request *, int, void *);
 extern void blk_requeue_request(struct request_queue *, struct request *);
 extern int blk_rq_check_limits(struct request_queue *q, struct request *rq);
-- 
1.6.2.1

Re: [PATCH version 2] New blk_make_request(), takes bio, returns a request

[Jens Axboe]

On Tue, May 19 2009, Boaz Harrosh wrote:
> 
> New block API:
> given a struct bio allocates a new request. This is the parallel of
> generic_make_request for BLOCK_PC commands users.
> 
> The passed bio may be a chained-bio. The bio is bounced if needed
> inside the call to this member.
> 
> This is in the effort of un-exporting blk_rq_append_bio().

Thanks. I had already applied your previous patch, so I just copied the
new lines and added those.

-- 
Jens Axboe