osd: Stop usage of blk_rq_append_bio

PATCH In review Boaz Harrosh 2 participants 14 messages May 17, 2009 – May 19, 2009

Subsystems (2)

SCSI Block layer

lore

Share & tools

lore

Boaz Harrosh

author

2009-05-17 15:53 UTC #0 lore raw

cover osd: Stop usage of blk_rq_append_bio

[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

Boaz Harrosh

author

2009-05-17 15:55 UTC #1 lore raw

PATCH 1/5 allow blk_rq_map_kern to append to requests

From: James Bottomley <[email protected]>

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 <[email protected]>

Signed-off-by Boaz Harrosh <[email protected]>

---
 block/blk-map.c |   12 ++++++++++--
 1 files changed, 10 insertions(+), 2 deletions(-)

block/blk-map.c +10 -2

index 56082be..caa05a6 100644
@@ -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;

Signature · 1 line

1.6.2.1

Boaz Harrosh

author

2009-05-17 15:56 UTC #2 lore raw

PATCH 2/5 libosd: Use new blk_rq_map_kern

In reply to 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 <[email protected]>

---
 drivers/scsi/osd/osd_initiator.c |   24 ++----------------------
 1 files changed, 2 insertions(+), 22 deletions(-)

drivers/scsi/osd/osd_initiator.c +2 ~~-22~~

index d178a8b..bf66e30 100644
@@ -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;

Signature · 1 line

1.6.2.1

Boaz Harrosh

author

2009-05-17 15:57 UTC #3 lore raw

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

In reply to 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 <[email protected]>

CC Jeff Garzik <[email protected]>

---
 block/blk-core.c       |   45 +++++++++++++++++++++++++++++++++++++++++++++
 include/linux/blkdev.h |    2 ++
 2 files changed, 47 insertions(+), 0 deletions(-)

block/blk-core.c +45 -0

index a2d97de..89261d2 100644
@@ -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

include/linux/blkdev.h +2 -0

index f9d60a7..88a83e1 100644
@@ -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);

Signature · 1 line

1.6.2.1

Jens Axboe

2009-05-19 09:41 UTC #4 lore raw

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

Quoted 58 lines · 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 <[email protected]>

CC Jeff Garzik <[email protected]>

---
 block/blk-core.c       |   45 +++++++++++++++++++++++++++++++++++++++++++++
 include/linux/blkdev.h |    2 ++
 2 files changed, 47 insertions(+), 0 deletions(-)

block/blk-core.c +33 -0

index a2d97de..89261d2 100644
@@ -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);

block/blk-core.c struct request *blk_get_request(struct request_queue *q, int rw, gfp_t gfp_mask) · line 936

Quoted 10 lines

block/blk-core.c +10 -0

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

Quoted 22 lines

block/blk-core.c +2 -0

+
+/**
  * blk_requeue_request - put a request back on queue
  * @q:		request queue where request should be inserted
  * @rq:		request to be inserted

include/linux/blkdev.h +2 -0

index f9d60a7..88a83e1 100644
@@ -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);

Signature · 1 line

1.6.2.1

Signature · 1 line

Jens Axboe

Boaz Harrosh

author

2009-05-19 10:07 UTC #5 lore raw

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

Quoted 67 lines · Jens Axboe

Quoted 66 lines · 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 <[email protected]>

CC Jeff Garzik <[email protected]>

---
 block/blk-core.c       |   45 +++++++++++++++++++++++++++++++++++++++++++++
 include/linux/blkdev.h |    2 ++
 2 files changed, 47 insertions(+), 0 deletions(-)

block/blk-core.c +42 -0

index a2d97de..89261d2 100644
@@ -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;
+}

Quoted 10 lines

block/blk-core.c struct request *blk_get_request(struct request_queue *q, int rw, gfp_t gfp_mask) · line 936
Quoted 1 line
block/blk-core.c +1 -0
+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

Jens Axboe

2009-05-19 10:13 UTC #6 lore raw

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

Quoted 79 lines · Boaz Harrosh

Quoted 67 lines · Jens Axboe
Quoted 66 lines · 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 <[email protected]>
CC Jeff Garzik <[email protected]>
---
 block/blk-core.c       |   45 +++++++++++++++++++++++++++++++++++++++++++++
 include/linux/blkdev.h |    2 ++
 2 files changed, 47 insertions(+), 0 deletions(-)
block/blk-core.c +42 -0
index a2d97de..89261d2 100644
@@ -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;
+}
Quoted 10 lines
block/blk-core.c struct request *blk_get_request(struct request_queue *q, int rw, gfp_t gfp_mask) · line 936
Quoted 1 line
block/blk-core.c +1 -0
+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.

Quoted 8 lines

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!

Quoted 2 lines

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.

Signature · 1 line

Jens Axboe

Boaz Harrosh

author

2009-05-19 12:27 UTC #7 lore raw

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

Quoted 1 line · Jens Axboe

On Tue, May 19 2009, Boaz Harrosh wrote:

<snip>

Quoted 11 lines

Quoted 8 lines
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!

Quoted 8 lines

Quoted 2 lines
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
---

block/blk-core.c +9 -0

index 89261d2..4dc4e32 100644
@@ -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)

Jens Axboe

2009-05-19 12:49 UTC #8 lore raw

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

Quoted 23 lines · Boaz Harrosh

Quoted 1 line · Jens Axboe
On Tue, May 19 2009, Boaz Harrosh wrote:
<snip>
Quoted 11 lines
Quoted 8 lines
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!
Quoted 8 lines
Quoted 2 lines
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.

Quoted 6 lines

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.

Quoted 7 lines

Thanks Boaz
---
block/blk-core.c +0 -0
index 89261d2..4dc4e32 100644

block/blk-core.c EXPORT_SYMBOL(blk_get_request); · line 918

Quoted 9 lines

block/blk-core.c +6 -0

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

block/blk-core.c EXPORT_SYMBOL(blk_get_request); · line 924

Quoted 6 lines

block/blk-core.c +3 -0

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

Signature · 1 line

Jens Axboe

Boaz Harrosh

author

2009-05-19 13:33 UTC #9 lore raw

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

Quoted 19 lines · Jens Axboe

Quoted 5 lines · Boaz Harrosh
+ * 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.
Quoted 6 lines
+ * 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)

Quoted 4 lines

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

Boaz Harrosh

author

2009-05-19 13:35 UTC #10 lore raw

PATCH 1/1 New blk_make_request(), takes bio, returns a request

In reply to 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 <[email protected]>

CC Jeff Garzik <[email protected]>

---
 block/blk-core.c       |   54 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/blkdev.h |    2 +
 2 files changed, 56 insertions(+), 0 deletions(-)

block/blk-core.c +54 -0

index a2d97de..3145a37 100644
@@ -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

include/linux/blkdev.h +2 -0

index f9d60a7..88a83e1 100644
@@ -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);

Signature · 1 line

1.6.2.1

Jens Axboe

2009-05-19 17:53 UTC #11 lore raw

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

Quoted 9 lines · 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().

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

Signature · 1 line

Jens Axboe

Boaz Harrosh

author

2009-05-17 15:58 UTC #12 lore raw

PATCH 4/5 libosd: Use of new blk_make_request

In reply to 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 <[email protected]>

CC Jeff Garzik <[email protected]>

CC FUJITA Tomonori <[email protected]>

---
 drivers/scsi/osd/osd_initiator.c |   48 ++++++++++++++++++-------------------
 1 files changed, 23 insertions(+), 25 deletions(-)

drivers/scsi/osd/osd_initiator.c +23 ~~-25~~

index bf66e30..865ec0f 100644
@@ -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;

Signature · 1 line

1.6.2.1

Boaz Harrosh

author

2009-05-17 16:00 UTC #13 lore raw

PATCH 5/5 Un-export blk_rq_append_bio

In reply to 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 <[email protected]>

---
 block/blk-map.c        |    1 -
 block/blk.h            |    2 ++
 include/linux/blkdev.h |    6 ------
 3 files changed, 2 insertions(+), 7 deletions(-)

block/blk-map.c +0 -1

index caa05a6..ef2492a 100644
@@ -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)
 {

block/blk.h +2 -0

index 9e0042c..c863ec2 100644
@@ -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);

include/linux/blkdev.h +0 -6

index 88a83e1..564445b 100644
@@ -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.

Signature · 1 line

1.6.2.1