[PATCH v10 0/8] Block cleanups

[PATCH v10 0/8] Block cleanups

[Kent Overstreet]

Screwed up the bio_reset() patch in the last patch series when I went to edit
the description, fixed that here.

Only other change is the dm patch - made the front_pad conditional on
DM_TYPE_BIO_BASED.

Kent Overstreet (8):
  block: Generalized bio pool freeing
  block: Ues bi_pool for bio_integrity_alloc()
  dm: Use bioset's front_pad for dm_rq_clone_bio_info
  block: Add bio_reset()
  pktcdvd: Switch to bio_kmalloc()
  block: Kill bi_destructor
  block: Consolidate bio_alloc_bioset(), bio_kmalloc()
  block: Add bio_clone_bioset(), bio_clone_kmalloc()

 Documentation/block/biodoc.txt      |   5 -
 block/blk-core.c                    |  10 +-
 drivers/block/drbd/drbd_main.c      |  13 +--
 drivers/block/osdblk.c              |   3 +-
 drivers/block/pktcdvd.c             |  52 ++-------
 drivers/md/dm-crypt.c               |  16 +--
 drivers/md/dm-io.c                  |  11 --
 drivers/md/dm.c                     |  74 ++++---------
 drivers/md/md.c                     |  44 +-------
 drivers/target/target_core_iblock.c |   9 --
 fs/bio-integrity.c                  |  44 +++-----
 fs/bio.c                            | 206 ++++++++++++++++--------------------
 fs/exofs/ore.c                      |   5 +-
 include/linux/bio.h                 |  44 +++++---
 include/linux/blk_types.h           |  27 +++--
 15 files changed, 195 insertions(+), 368 deletions(-)

-- 
1.7.12

[PATCH v10 1/8] block: Generalized bio pool freeing

[Kent Overstreet]

With the old code, when you allocate a bio from a bio pool you have to
implement your own destructor that knows how to find the bio pool the
bio was originally allocated from.

This adds a new field to struct bio (bi_pool) and changes
bio_alloc_bioset() to use it. This makes various bio destructors
unnecessary, so they're then deleted.

v6: Explain the temporary if statement in bio_put

Signed-off-by: Kent Overstreet <koverstreet@google.com>
CC: Jens Axboe <axboe@kernel.dk>
CC: NeilBrown <neilb@suse.de>
CC: Alasdair Kergon <agk@redhat.com>
CC: Nicholas Bellinger <nab@linux-iscsi.org>
CC: Lars Ellenberg <lars.ellenberg@linbit.com>
Acked-by: Tejun Heo <tj@kernel.org>
Acked-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/block/drbd/drbd_main.c      | 13 +------------
 drivers/md/dm-crypt.c               |  9 ---------
 drivers/md/dm-io.c                  | 11 -----------
 drivers/md/dm.c                     | 20 --------------------
 drivers/md/md.c                     | 28 ++++------------------------
 drivers/target/target_core_iblock.c |  9 ---------
 fs/bio.c                            | 31 +++++++++++++------------------
 include/linux/blk_types.h           |  3 +++
 8 files changed, 21 insertions(+), 103 deletions(-)

diff --git a/drivers/block/drbd/drbd_main.c b/drivers/block/drbd/drbd_main.c
index f93a032..f55683a 100644
--- a/drivers/block/drbd/drbd_main.c
+++ b/drivers/block/drbd/drbd_main.c
@@ -162,23 +162,12 @@ static const struct block_device_operations drbd_ops = {
 	.release = drbd_release,
 };
 
-static void bio_destructor_drbd(struct bio *bio)
-{
-	bio_free(bio, drbd_md_io_bio_set);
-}
-
 struct bio *bio_alloc_drbd(gfp_t gfp_mask)
 {
-	struct bio *bio;
-
 	if (!drbd_md_io_bio_set)
 		return bio_alloc(gfp_mask, 1);
 
-	bio = bio_alloc_bioset(gfp_mask, 1, drbd_md_io_bio_set);
-	if (!bio)
-		return NULL;
-	bio->bi_destructor = bio_destructor_drbd;
-	return bio;
+	return bio_alloc_bioset(gfp_mask, 1, drbd_md_io_bio_set);
 }
 
 #ifdef __CHECKER__
diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index 664743d..3c0acba 100644
--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -798,14 +798,6 @@ static int crypt_convert(struct crypt_config *cc,
 	return 0;
 }
 
-static void dm_crypt_bio_destructor(struct bio *bio)
-{
-	struct dm_crypt_io *io = bio->bi_private;
-	struct crypt_config *cc = io->cc;
-
-	bio_free(bio, cc->bs);
-}
-
 /*
  * Generate a new unfragmented bio with the given size
  * This should never violate the device limitations
@@ -974,7 +966,6 @@ static void clone_init(struct dm_crypt_io *io, struct bio *clone)
 	clone->bi_end_io  = crypt_endio;
 	clone->bi_bdev    = cc->dev->bdev;
 	clone->bi_rw      = io->base_bio->bi_rw;
-	clone->bi_destructor = dm_crypt_bio_destructor;
 }
 
 static int kcryptd_io_read(struct dm_crypt_io *io, gfp_t gfp)
diff --git a/drivers/md/dm-io.c b/drivers/md/dm-io.c
index ea5dd28..1c46f97 100644
--- a/drivers/md/dm-io.c
+++ b/drivers/md/dm-io.c
@@ -249,16 +249,6 @@ static void vm_dp_init(struct dpages *dp, void *data)
 	dp->context_ptr = data;
 }
 
-static void dm_bio_destructor(struct bio *bio)
-{
-	unsigned region;
-	struct io *io;
-
-	retrieve_io_and_region_from_bio(bio, &io, &region);
-
-	bio_free(bio, io->client->bios);
-}
-
 /*
  * Functions for getting the pages from kernel memory.
  */
@@ -317,7 +307,6 @@ static void do_region(int rw, unsigned region, struct dm_io_region *where,
 		bio->bi_sector = where->sector + (where->count - remaining);
 		bio->bi_bdev = where->bdev;
 		bio->bi_end_io = endio;
-		bio->bi_destructor = dm_bio_destructor;
 		store_io_and_region_in_bio(bio, io, region);
 
 		if (rw & REQ_DISCARD) {
diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index 4e09b6f..0c3d6dd 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -681,11 +681,6 @@ static void clone_endio(struct bio *bio, int error)
 		}
 	}
 
-	/*
-	 * Store md for cleanup instead of tio which is about to get freed.
-	 */
-	bio->bi_private = md->bs;
-
 	free_tio(md, tio);
 	bio_put(bio);
 	dec_pending(io, error);
@@ -1032,11 +1027,6 @@ static void __map_bio(struct dm_target *ti, struct bio *clone,
 		/* error the io and bail out, or requeue it if needed */
 		md = tio->io->md;
 		dec_pending(tio->io, r);
-		/*
-		 * Store bio_set for cleanup.
-		 */
-		clone->bi_end_io = NULL;
-		clone->bi_private = md->bs;
 		bio_put(clone);
 		free_tio(md, tio);
 	} else if (r) {
@@ -1055,13 +1045,6 @@ struct clone_info {
 	unsigned short idx;
 };
 
-static void dm_bio_destructor(struct bio *bio)
-{
-	struct bio_set *bs = bio->bi_private;
-
-	bio_free(bio, bs);
-}
-
 /*
  * Creates a little bio that just does part of a bvec.
  */
@@ -1073,7 +1056,6 @@ static struct bio *split_bvec(struct bio *bio, sector_t sector,
 	struct bio_vec *bv = bio->bi_io_vec + idx;
 
 	clone = bio_alloc_bioset(GFP_NOIO, 1, bs);
-	clone->bi_destructor = dm_bio_destructor;
 	*clone->bi_io_vec = *bv;
 
 	clone->bi_sector = sector;
@@ -1105,7 +1087,6 @@ static struct bio *clone_bio(struct bio *bio, sector_t sector,
 
 	clone = bio_alloc_bioset(GFP_NOIO, bio->bi_max_vecs, bs);
 	__bio_clone(clone, bio);
-	clone->bi_destructor = dm_bio_destructor;
 	clone->bi_sector = sector;
 	clone->bi_idx = idx;
 	clone->bi_vcnt = idx + bv_count;
@@ -1150,7 +1131,6 @@ static void __issue_target_request(struct clone_info *ci, struct dm_target *ti,
 	 */
 	clone = bio_alloc_bioset(GFP_NOIO, ci->bio->bi_max_vecs, ci->md->bs);
 	__bio_clone(clone, ci->bio);
-	clone->bi_destructor = dm_bio_destructor;
 	if (len) {
 		clone->bi_sector = ci->sector;
 		clone->bi_size = to_bytes(len);
diff --git a/drivers/md/md.c b/drivers/md/md.c
index 3f6203a..b8eebe3 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -155,32 +155,17 @@ static int start_readonly;
  * like bio_clone, but with a local bio set
  */
 
-static void mddev_bio_destructor(struct bio *bio)
-{
-	struct mddev *mddev, **mddevp;
-
-	mddevp = (void*)bio;
-	mddev = mddevp[-1];
-
-	bio_free(bio, mddev->bio_set);
-}
-
 struct bio *bio_alloc_mddev(gfp_t gfp_mask, int nr_iovecs,
 			    struct mddev *mddev)
 {
 	struct bio *b;
-	struct mddev **mddevp;
 
 	if (!mddev || !mddev->bio_set)
 		return bio_alloc(gfp_mask, nr_iovecs);
 
-	b = bio_alloc_bioset(gfp_mask, nr_iovecs,
-			     mddev->bio_set);
+	b = bio_alloc_bioset(gfp_mask, nr_iovecs, mddev->bio_set);
 	if (!b)
 		return NULL;
-	mddevp = (void*)b;
-	mddevp[-1] = mddev;
-	b->bi_destructor = mddev_bio_destructor;
 	return b;
 }
 EXPORT_SYMBOL_GPL(bio_alloc_mddev);
@@ -189,18 +174,14 @@ struct bio *bio_clone_mddev(struct bio *bio, gfp_t gfp_mask,
 			    struct mddev *mddev)
 {
 	struct bio *b;
-	struct mddev **mddevp;
 
 	if (!mddev || !mddev->bio_set)
 		return bio_clone(bio, gfp_mask);
 
-	b = bio_alloc_bioset(gfp_mask, bio->bi_max_vecs,
-			     mddev->bio_set);
+	b = bio_alloc_bioset(gfp_mask, bio->bi_max_vecs, mddev->bio_set);
 	if (!b)
 		return NULL;
-	mddevp = (void*)b;
-	mddevp[-1] = mddev;
-	b->bi_destructor = mddev_bio_destructor;
+
 	__bio_clone(b, bio);
 	if (bio_integrity(bio)) {
 		int ret;
@@ -5006,8 +4987,7 @@ int md_run(struct mddev *mddev)
 	}
 
 	if (mddev->bio_set == NULL)
-		mddev->bio_set = bioset_create(BIO_POOL_SIZE,
-					       sizeof(struct mddev *));
+		mddev->bio_set = bioset_create(BIO_POOL_SIZE, 0);
 
 	spin_lock(&pers_lock);
 	pers = find_pers(mddev->level, mddev->clevel);
diff --git a/drivers/target/target_core_iblock.c b/drivers/target/target_core_iblock.c
index 76db75e..e58cd7d 100644
--- a/drivers/target/target_core_iblock.c
+++ b/drivers/target/target_core_iblock.c
@@ -543,14 +543,6 @@ static void iblock_complete_cmd(struct se_cmd *cmd)
 	kfree(ibr);
 }
 
-static void iblock_bio_destructor(struct bio *bio)
-{
-	struct se_cmd *cmd = bio->bi_private;
-	struct iblock_dev *ib_dev = cmd->se_dev->dev_ptr;
-
-	bio_free(bio, ib_dev->ibd_bio_set);
-}
-
 static struct bio *
 iblock_get_bio(struct se_cmd *cmd, sector_t lba, u32 sg_num)
 {
@@ -572,7 +564,6 @@ iblock_get_bio(struct se_cmd *cmd, sector_t lba, u32 sg_num)
 
 	bio->bi_bdev = ib_dev->ibd_bd;
 	bio->bi_private = cmd;
-	bio->bi_destructor = iblock_bio_destructor;
 	bio->bi_end_io = &iblock_bio_done;
 	bio->bi_sector = lba;
 	return bio;
diff --git a/fs/bio.c b/fs/bio.c
index 71072ab..e017f7a 100644
--- a/fs/bio.c
+++ b/fs/bio.c
@@ -272,10 +272,6 @@ EXPORT_SYMBOL(bio_init);
  *   bio_alloc_bioset will try its own mempool to satisfy the allocation.
  *   If %__GFP_WAIT is set then we will block on the internal pool waiting
  *   for a &struct bio to become free.
- *
- *   Note that the caller must set ->bi_destructor on successful return
- *   of a bio, to do the appropriate freeing of the bio once the reference
- *   count drops to zero.
  **/
 struct bio *bio_alloc_bioset(gfp_t gfp_mask, int nr_iovecs, struct bio_set *bs)
 {
@@ -290,6 +286,7 @@ struct bio *bio_alloc_bioset(gfp_t gfp_mask, int nr_iovecs, struct bio_set *bs)
 	bio = p + bs->front_pad;
 
 	bio_init(bio);
+	bio->bi_pool = bs;
 
 	if (unlikely(!nr_iovecs))
 		goto out_set;
@@ -316,11 +313,6 @@ err_free:
 }
 EXPORT_SYMBOL(bio_alloc_bioset);
 
-static void bio_fs_destructor(struct bio *bio)
-{
-	bio_free(bio, fs_bio_set);
-}
-
 /**
  *	bio_alloc - allocate a new bio, memory pool backed
  *	@gfp_mask: allocation mask to use
@@ -342,12 +334,7 @@ static void bio_fs_destructor(struct bio *bio)
  */
 struct bio *bio_alloc(gfp_t gfp_mask, unsigned int nr_iovecs)
 {
-	struct bio *bio = bio_alloc_bioset(gfp_mask, nr_iovecs, fs_bio_set);
-
-	if (bio)
-		bio->bi_destructor = bio_fs_destructor;
-
-	return bio;
+	return bio_alloc_bioset(gfp_mask, nr_iovecs, fs_bio_set);
 }
 EXPORT_SYMBOL(bio_alloc);
 
@@ -423,7 +410,16 @@ void bio_put(struct bio *bio)
 	if (atomic_dec_and_test(&bio->bi_cnt)) {
 		bio_disassociate_task(bio);
 		bio->bi_next = NULL;
-		bio->bi_destructor(bio);
+
+		/*
+		 * This if statement is temporary - bi_pool is replacing
+		 * bi_destructor, but bi_destructor will be taken out in another
+		 * patch.
+		 */
+		if (bio->bi_pool)
+			bio_free(bio, bio->bi_pool);
+		else
+			bio->bi_destructor(bio);
 	}
 }
 EXPORT_SYMBOL(bio_put);
@@ -474,12 +470,11 @@ EXPORT_SYMBOL(__bio_clone);
  */
 struct bio *bio_clone(struct bio *bio, gfp_t gfp_mask)
 {
-	struct bio *b = bio_alloc_bioset(gfp_mask, bio->bi_max_vecs, fs_bio_set);
+	struct bio *b = bio_alloc(gfp_mask, bio->bi_max_vecs);
 
 	if (!b)
 		return NULL;
 
-	b->bi_destructor = bio_fs_destructor;
 	__bio_clone(b, bio);
 
 	if (bio_integrity(bio)) {
diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
index 7b7ac9c..af9dd9d 100644
--- a/include/linux/blk_types.h
+++ b/include/linux/blk_types.h
@@ -80,6 +80,9 @@ struct bio {
 	struct bio_integrity_payload *bi_integrity;  /* data integrity */
 #endif
 
+	/* If bi_pool is non NULL, bi_destructor is not called */
+	struct bio_set		*bi_pool;
+
 	bio_destructor_t	*bi_destructor;	/* destructor */
 
 	/*
-- 
1.7.12

Re: [dm-devel] [PATCH v10 1/8] block: Generalized bio pool freeing

[Alasdair G Kergon]

On Thu, Sep 06, 2012 at 03:34:55PM -0700, Kent Overstreet wrote:
> With the old code, when you allocate a bio from a bio pool you have to
> implement your own destructor that knows how to find the bio pool the
> bio was originally allocated from.
> 
> This adds a new field to struct bio (bi_pool) and changes
> bio_alloc_bioset() to use it. This makes various bio destructors
> unnecessary, so they're then deleted.
> 
> v6: Explain the temporary if statement in bio_put
 
This patch also silently reverts 
commit 4d7b38b7d944a79da3793b6c92d38682f3905ac9
"dm: clear bi_end_io on remapping failure"

Why?

If it's intentional, please explain it in your patch header and
copy Hannes to reconsider the matter.

If it wasn't intentional, please reinstate it.

Alasdair

Re: [dm-devel] [PATCH v10 1/8] block: Generalized bio pool freeing

[Kent Overstreet]

On Fri, Sep 14, 2012 at 07:28:28PM +0100, Alasdair G Kergon wrote:
> On Thu, Sep 06, 2012 at 03:34:55PM -0700, Kent Overstreet wrote:
> > With the old code, when you allocate a bio from a bio pool you have to
> > implement your own destructor that knows how to find the bio pool the
> > bio was originally allocated from.
> > 
> > This adds a new field to struct bio (bi_pool) and changes
> > bio_alloc_bioset() to use it. This makes various bio destructors
> > unnecessary, so they're then deleted.
> > 
> > v6: Explain the temporary if statement in bio_put
>  
> This patch also silently reverts 
> commit 4d7b38b7d944a79da3793b6c92d38682f3905ac9
> "dm: clear bi_end_io on remapping failure"
> 
> Why?
> 
> If it's intentional, please explain it in your patch header and
> copy Hannes to reconsider the matter.

Never noticed that was introduced in its own patch until you pointed it
out.

That isn't a very good patch - it says it's clearing bi_end_io as a
precaution, but as a precaution to what?

As far as I can tell, it was never necessary. The bio is about to be
freed - there shouldn't be any other references on it (__bio_map() is
called on freshly allocated bios, and bio_get() is never called in dm.c)
Nothing else should've been looking at bi_end_io, certainly the
destructor didn't.

Now that there's no destructor, it makes even less sense to have it -
after that bio_put() that bio isn't being touched by dm code anymore.

Re: [dm-devel] [PATCH v10 1/8] block: Generalized bio pool freeing

[Alasdair G Kergon]

On Mon, Sep 17, 2012 at 01:51:39PM -0700, Kent Overstreet wrote:
> That isn't a very good patch - it says it's clearing bi_end_io as a
> precaution, but as a precaution to what?
 
As a precaution against the endio function being called an extra time by
mistake if a stray reference remains after the bio has been freed.
Wipe this field when dm frees it.  Defensive programming, basically,
in response to a tricky bug he had to deal with.

Alasdair

Re: [dm-devel] [PATCH v10 1/8] block: Generalized bio pool freeing

[Alasdair G Kergon]

On Thu, Sep 06, 2012 at 03:34:55PM -0700, Kent Overstreet wrote:
> With the old code, when you allocate a bio from a bio pool you have to
> implement your own destructor that knows how to find the bio pool the
> bio was originally allocated from.
> This adds a new field to struct bio (bi_pool) and changes
> bio_alloc_bioset() to use it. This makes various bio destructors
> unnecessary, so they're then deleted.
 
If you reinstate commit 4d7b38b7d944a79da3793b6c92d38682f3905ac9
"dm: clear bi_end_io on remapping failure",

Acked-by: Alasdair Kergon <agk@redhat.com>

for the device-mapper parts.

Alasdair

[PATCH v10 2/8] block: Ues bi_pool for bio_integrity_alloc()

[Kent Overstreet]

Now that bios keep track of where they were allocated from,
bio_integrity_alloc_bioset() becomes redundant.

Remove bio_integrity_alloc_bioset() and drop bio_set argument from the
related functions and make them use bio->bi_pool.

Signed-off-by: Kent Overstreet <koverstreet@google.com>
CC: Jens Axboe <axboe@kernel.dk>
CC: Martin K. Petersen <martin.petersen@oracle.com>
Acked-by: Tejun Heo <tj@kernel.org>
---
 block/blk-core.c    |  2 +-
 drivers/md/dm.c     |  4 ++--
 drivers/md/md.c     |  2 +-
 fs/bio-integrity.c  | 44 +++++++++++++++-----------------------------
 fs/bio.c            |  6 +++---
 include/linux/bio.h |  9 ++++-----
 6 files changed, 26 insertions(+), 41 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 4b4dbdf..95c4935 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -2788,7 +2788,7 @@ int blk_rq_prep_clone(struct request *rq, struct request *rq_src,
 		__bio_clone(bio, bio_src);
 
 		if (bio_integrity(bio_src) &&
-		    bio_integrity_clone(bio, bio_src, gfp_mask, bs))
+		    bio_integrity_clone(bio, bio_src, gfp_mask))
 			goto free_and_out;
 
 		if (bio_ctr && bio_ctr(bio, bio_src, data))
diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index 0c3d6dd..f43aaf6 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -1068,7 +1068,7 @@ static struct bio *split_bvec(struct bio *bio, sector_t sector,
 	clone->bi_flags |= 1 << BIO_CLONED;
 
 	if (bio_integrity(bio)) {
-		bio_integrity_clone(clone, bio, GFP_NOIO, bs);
+		bio_integrity_clone(clone, bio, GFP_NOIO);
 		bio_integrity_trim(clone,
 				   bio_sector_offset(bio, idx, offset), len);
 	}
@@ -1094,7 +1094,7 @@ static struct bio *clone_bio(struct bio *bio, sector_t sector,
 	clone->bi_flags &= ~(1 << BIO_SEG_VALID);
 
 	if (bio_integrity(bio)) {
-		bio_integrity_clone(clone, bio, GFP_NOIO, bs);
+		bio_integrity_clone(clone, bio, GFP_NOIO);
 
 		if (idx != bio->bi_idx || clone->bi_size < bio->bi_size)
 			bio_integrity_trim(clone,
diff --git a/drivers/md/md.c b/drivers/md/md.c
index b8eebe3..457ca84 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -186,7 +186,7 @@ struct bio *bio_clone_mddev(struct bio *bio, gfp_t gfp_mask,
 	if (bio_integrity(bio)) {
 		int ret;
 
-		ret = bio_integrity_clone(b, bio, gfp_mask, mddev->bio_set);
+		ret = bio_integrity_clone(b, bio, gfp_mask);
 
 		if (ret < 0) {
 			bio_put(b);
diff --git a/fs/bio-integrity.c b/fs/bio-integrity.c
index e85c04b..a3f28f3 100644
--- a/fs/bio-integrity.c
+++ b/fs/bio-integrity.c
@@ -70,23 +70,25 @@ static inline int use_bip_pool(unsigned int idx)
 }
 
 /**
- * bio_integrity_alloc_bioset - Allocate integrity payload and attach it to bio
+ * bio_integrity_alloc - Allocate integrity payload and attach it to bio
  * @bio:	bio to attach integrity metadata to
  * @gfp_mask:	Memory allocation mask
  * @nr_vecs:	Number of integrity metadata scatter-gather elements
- * @bs:		bio_set to allocate from
  *
  * Description: This function prepares a bio for attaching integrity
  * metadata.  nr_vecs specifies the maximum number of pages containing
  * integrity metadata that can be attached.
  */
-struct bio_integrity_payload *bio_integrity_alloc_bioset(struct bio *bio,
-							 gfp_t gfp_mask,
-							 unsigned int nr_vecs,
-							 struct bio_set *bs)
+struct bio_integrity_payload *bio_integrity_alloc(struct bio *bio,
+						  gfp_t gfp_mask,
+						  unsigned int nr_vecs)
 {
 	struct bio_integrity_payload *bip;
 	unsigned int idx = vecs_to_idx(nr_vecs);
+	struct bio_set *bs = bio->bi_pool;
+
+	if (!bs)
+		bs = fs_bio_set;
 
 	BUG_ON(bio == NULL);
 	bip = NULL;
@@ -114,37 +116,22 @@ struct bio_integrity_payload *bio_integrity_alloc_bioset(struct bio *bio,
 
 	return bip;
 }
-EXPORT_SYMBOL(bio_integrity_alloc_bioset);
-
-/**
- * bio_integrity_alloc - Allocate integrity payload and attach it to bio
- * @bio:	bio to attach integrity metadata to
- * @gfp_mask:	Memory allocation mask
- * @nr_vecs:	Number of integrity metadata scatter-gather elements
- *
- * Description: This function prepares a bio for attaching integrity
- * metadata.  nr_vecs specifies the maximum number of pages containing
- * integrity metadata that can be attached.
- */
-struct bio_integrity_payload *bio_integrity_alloc(struct bio *bio,
-						  gfp_t gfp_mask,
-						  unsigned int nr_vecs)
-{
-	return bio_integrity_alloc_bioset(bio, gfp_mask, nr_vecs, fs_bio_set);
-}
 EXPORT_SYMBOL(bio_integrity_alloc);
 
 /**
  * bio_integrity_free - Free bio integrity payload
  * @bio:	bio containing bip to be freed
- * @bs:		bio_set this bio was allocated from
  *
  * Description: Used to free the integrity portion of a bio. Usually
  * called from bio_free().
  */
-void bio_integrity_free(struct bio *bio, struct bio_set *bs)
+void bio_integrity_free(struct bio *bio)
 {
 	struct bio_integrity_payload *bip = bio->bi_integrity;
+	struct bio_set *bs = bio->bi_pool;
+
+	if (!bs)
+		bs = fs_bio_set;
 
 	BUG_ON(bip == NULL);
 
@@ -730,19 +717,18 @@ EXPORT_SYMBOL(bio_integrity_split);
  * @bio:	New bio
  * @bio_src:	Original bio
  * @gfp_mask:	Memory allocation mask
- * @bs:		bio_set to allocate bip from
  *
  * Description:	Called to allocate a bip when cloning a bio
  */
 int bio_integrity_clone(struct bio *bio, struct bio *bio_src,
-			gfp_t gfp_mask, struct bio_set *bs)
+			gfp_t gfp_mask)
 {
 	struct bio_integrity_payload *bip_src = bio_src->bi_integrity;
 	struct bio_integrity_payload *bip;
 
 	BUG_ON(bip_src == NULL);
 
-	bip = bio_integrity_alloc_bioset(bio, gfp_mask, bip_src->bip_vcnt, bs);
+	bip = bio_integrity_alloc(bio, gfp_mask, bip_src->bip_vcnt);
 
 	if (bip == NULL)
 		return -EIO;
diff --git a/fs/bio.c b/fs/bio.c
index e017f7a..b14f71a 100644
--- a/fs/bio.c
+++ b/fs/bio.c
@@ -241,7 +241,7 @@ void bio_free(struct bio *bio, struct bio_set *bs)
 		bvec_free_bs(bs, bio->bi_io_vec, BIO_POOL_IDX(bio));
 
 	if (bio_integrity(bio))
-		bio_integrity_free(bio, bs);
+		bio_integrity_free(bio);
 
 	/*
 	 * If we have front padding, adjust the bio pointer before freeing
@@ -341,7 +341,7 @@ EXPORT_SYMBOL(bio_alloc);
 static void bio_kmalloc_destructor(struct bio *bio)
 {
 	if (bio_integrity(bio))
-		bio_integrity_free(bio, fs_bio_set);
+		bio_integrity_free(bio);
 	kfree(bio);
 }
 
@@ -480,7 +480,7 @@ struct bio *bio_clone(struct bio *bio, gfp_t gfp_mask)
 	if (bio_integrity(bio)) {
 		int ret;
 
-		ret = bio_integrity_clone(b, bio, gfp_mask, fs_bio_set);
+		ret = bio_integrity_clone(b, bio, gfp_mask);
 
 		if (ret < 0) {
 			bio_put(b);
diff --git a/include/linux/bio.h b/include/linux/bio.h
index 2643589..a11f74b 100644
--- a/include/linux/bio.h
+++ b/include/linux/bio.h
@@ -505,9 +505,8 @@ static inline struct bio *bio_list_get(struct bio_list *bl)
 
 #define bio_integrity(bio) (bio->bi_integrity != NULL)
 
-extern struct bio_integrity_payload *bio_integrity_alloc_bioset(struct bio *, gfp_t, unsigned int, struct bio_set *);
 extern struct bio_integrity_payload *bio_integrity_alloc(struct bio *, gfp_t, unsigned int);
-extern void bio_integrity_free(struct bio *, struct bio_set *);
+extern void bio_integrity_free(struct bio *);
 extern int bio_integrity_add_page(struct bio *, struct page *, unsigned int, unsigned int);
 extern int bio_integrity_enabled(struct bio *bio);
 extern int bio_integrity_set_tag(struct bio *, void *, unsigned int);
@@ -517,7 +516,7 @@ extern void bio_integrity_endio(struct bio *, int);
 extern void bio_integrity_advance(struct bio *, unsigned int);
 extern void bio_integrity_trim(struct bio *, unsigned int, unsigned int);
 extern void bio_integrity_split(struct bio *, struct bio_pair *, int);
-extern int bio_integrity_clone(struct bio *, struct bio *, gfp_t, struct bio_set *);
+extern int bio_integrity_clone(struct bio *, struct bio *, gfp_t);
 extern int bioset_integrity_create(struct bio_set *, int);
 extern void bioset_integrity_free(struct bio_set *);
 extern void bio_integrity_init(void);
@@ -549,13 +548,13 @@ static inline int bio_integrity_prep(struct bio *bio)
 	return 0;
 }
 
-static inline void bio_integrity_free(struct bio *bio, struct bio_set *bs)
+static inline void bio_integrity_free(struct bio *bio)
 {
 	return;
 }
 
 static inline int bio_integrity_clone(struct bio *bio, struct bio *bio_src,
-				      gfp_t gfp_mask, struct bio_set *bs)
+				      gfp_t gfp_mask)
 {
 	return 0;
 }
-- 
1.7.12

[PATCH v10 3/8] dm: Use bioset's front_pad for dm_rq_clone_bio_info

[Kent Overstreet]

Previously, dm_rq_clone_bio_info needed to be freed by the bio's
destructor to avoid a memory leak in the blk_rq_prep_clone() error path.
This gets rid of a memory allocation and means we can kill
dm_rq_bio_destructor.

The _rq_bio_info_cache kmem cache is unused now and needs to be deleted,
but due to the way io_pool is used and overloaded this looks not quite
trivial so I'm leaving it for a later patch.

v6: Fix comment on struct dm_rq_clone_bio_info, per Tejun

Signed-off-by: Kent Overstreet <koverstreet@google.com>
CC: Alasdair Kergon <agk@redhat.com>
Acked-by: Tejun Heo <tj@kernel.org>
---
 drivers/md/dm.c | 46 ++++++++++++++++++----------------------------
 1 file changed, 18 insertions(+), 28 deletions(-)

diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index f43aaf6..33470f0 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -86,12 +86,17 @@ struct dm_rq_target_io {
 };
 
 /*
- * For request-based dm.
- * One of these is allocated per bio.
+ * For request-based dm - the bio clones we allocate are embedded in these
+ * structs.
+ *
+ * We allocate these with bio_alloc_bioset, using the front_pad parameter when
+ * the bioset is created - this means the bio has to come at the end of the
+ * struct.
  */
 struct dm_rq_clone_bio_info {
 	struct bio *orig;
 	struct dm_rq_target_io *tio;
+	struct bio clone;
 };
 
 union map_info *dm_get_mapinfo(struct bio *bio)
@@ -211,6 +216,11 @@ struct dm_md_mempools {
 static struct kmem_cache *_io_cache;
 static struct kmem_cache *_tio_cache;
 static struct kmem_cache *_rq_tio_cache;
+
+/*
+ * Unused now, and needs to be deleted. But since io_pool is overloaded and it's
+ * still used for _io_cache, I'm leaving this for a later cleanup
+ */
 static struct kmem_cache *_rq_bio_info_cache;
 
 static int __init local_init(void)
@@ -467,16 +477,6 @@ static void free_rq_tio(struct dm_rq_target_io *tio)
 	mempool_free(tio, tio->md->tio_pool);
 }
 
-static struct dm_rq_clone_bio_info *alloc_bio_info(struct mapped_device *md)
-{
-	return mempool_alloc(md->io_pool, GFP_ATOMIC);
-}
-
-static void free_bio_info(struct dm_rq_clone_bio_info *info)
-{
-	mempool_free(info, info->tio->md->io_pool);
-}
-
 static int md_in_flight(struct mapped_device *md)
 {
 	return atomic_read(&md->pending[READ]) +
@@ -1460,30 +1460,17 @@ void dm_dispatch_request(struct request *rq)
 }
 EXPORT_SYMBOL_GPL(dm_dispatch_request);
 
-static void dm_rq_bio_destructor(struct bio *bio)
-{
-	struct dm_rq_clone_bio_info *info = bio->bi_private;
-	struct mapped_device *md = info->tio->md;
-
-	free_bio_info(info);
-	bio_free(bio, md->bs);
-}
-
 static int dm_rq_bio_constructor(struct bio *bio, struct bio *bio_orig,
 				 void *data)
 {
 	struct dm_rq_target_io *tio = data;
-	struct mapped_device *md = tio->md;
-	struct dm_rq_clone_bio_info *info = alloc_bio_info(md);
-
-	if (!info)
-		return -ENOMEM;
+	struct dm_rq_clone_bio_info *info =
+		container_of(bio, struct dm_rq_clone_bio_info, clone);
 
 	info->orig = bio_orig;
 	info->tio = tio;
 	bio->bi_end_io = end_clone_bio;
 	bio->bi_private = info;
-	bio->bi_destructor = dm_rq_bio_destructor;
 
 	return 0;
 }
@@ -2718,7 +2705,10 @@ struct dm_md_mempools *dm_alloc_md_mempools(unsigned type, unsigned integrity)
 	if (!pools->tio_pool)
 		goto free_io_pool_and_out;
 
-	pools->bs = bioset_create(pool_size, 0);
+	pools->bs = (type == DM_TYPE_BIO_BASED) ?
+		bioset_create(pool_size, 0) :
+		bioset_create(pool_size,
+			      offsetof(struct dm_rq_clone_bio_info, clone));
 	if (!pools->bs)
 		goto free_tio_pool_and_out;
 
-- 
1.7.12

[PATCH v10 4/8] block: Add bio_reset()

[Kent Overstreet]

Reusing bios is something that's been highly frowned upon in the past,
but driver code keeps doing it anyways. If it's going to happen anyways,
we should provide a generic method.

This'll help with getting rid of bi_destructor - drivers/block/pktcdvd.c
was open coding it, by doing a bio_init() and resetting bi_destructor.

This required reordering struct bio, but the block layer is not yet
nearly fast enough for any cacheline effects to matter here.

v5: Add a define BIO_RESET_BITS, to be very explicit about what parts of
bio->bi_flags are saved.
v6: Further commenting verbosity, per Tejun
v9: Add a function comment

Signed-off-by: Kent Overstreet <koverstreet@google.com>
CC: Jens Axboe <axboe@kernel.dk>
Acked-by: Tejun Heo <tj@kernel.org>
---
 fs/bio.c                  | 24 ++++++++++++++++++++++++
 include/linux/bio.h       |  1 +
 include/linux/blk_types.h | 25 +++++++++++++++++++------
 3 files changed, 44 insertions(+), 6 deletions(-)

diff --git a/fs/bio.c b/fs/bio.c
index b14f71a..919ee9a 100644
--- a/fs/bio.c
+++ b/fs/bio.c
@@ -263,6 +263,30 @@ void bio_init(struct bio *bio)
 EXPORT_SYMBOL(bio_init);
 
 /**
+ * bio_reset - reinitialize a bio
+ * @bio:	bio to reset
+ *
+ * Description:
+ *   After calling bio_reset(), @bio will be in the same state as a freshly
+ *   allocated bio returned bio bio_alloc_bioset() - the only fields that are
+ *   preserved are the ones that are initialized by bio_alloc_bioset(). See
+ *   comment in struct bio.
+ */
+void bio_reset(struct bio *bio)
+{
+	unsigned long flags = bio->bi_flags & (~0UL << BIO_RESET_BITS);
+
+	if (bio_integrity(bio))
+		bio_integrity_free(bio);
+
+	bio_disassociate_task(bio);
+
+	memset(bio, 0, BIO_RESET_BYTES);
+	bio->bi_flags = flags|(1 << BIO_UPTODATE);
+}
+EXPORT_SYMBOL(bio_reset);
+
+/**
  * bio_alloc_bioset - allocate a bio for I/O
  * @gfp_mask:   the GFP_ mask given to the slab allocator
  * @nr_iovecs:	number of iovecs to pre-allocate
diff --git a/include/linux/bio.h b/include/linux/bio.h
index a11f74b..76f6c25 100644
--- a/include/linux/bio.h
+++ b/include/linux/bio.h
@@ -226,6 +226,7 @@ extern void __bio_clone(struct bio *, struct bio *);
 extern struct bio *bio_clone(struct bio *, gfp_t);
 
 extern void bio_init(struct bio *);
+extern void bio_reset(struct bio *);
 
 extern int bio_add_page(struct bio *, struct page *, unsigned int,unsigned int);
 extern int bio_add_pc_page(struct request_queue *, struct bio *, struct page *,
diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
index af9dd9d..1b607c2 100644
--- a/include/linux/blk_types.h
+++ b/include/linux/blk_types.h
@@ -59,12 +59,6 @@ struct bio {
 	unsigned int		bi_seg_front_size;
 	unsigned int		bi_seg_back_size;
 
-	unsigned int		bi_max_vecs;	/* max bvl_vecs we can hold */
-
-	atomic_t		bi_cnt;		/* pin count */
-
-	struct bio_vec		*bi_io_vec;	/* the actual vec list */
-
 	bio_end_io_t		*bi_end_io;
 
 	void			*bi_private;
@@ -80,6 +74,16 @@ struct bio {
 	struct bio_integrity_payload *bi_integrity;  /* data integrity */
 #endif
 
+	/*
+	 * Everything starting with bi_max_vecs will be preserved by bio_reset()
+	 */
+
+	unsigned int		bi_max_vecs;	/* max bvl_vecs we can hold */
+
+	atomic_t		bi_cnt;		/* pin count */
+
+	struct bio_vec		*bi_io_vec;	/* the actual vec list */
+
 	/* If bi_pool is non NULL, bi_destructor is not called */
 	struct bio_set		*bi_pool;
 
@@ -93,6 +97,8 @@ struct bio {
 	struct bio_vec		bi_inline_vecs[0];
 };
 
+#define BIO_RESET_BYTES		offsetof(struct bio, bi_max_vecs)
+
 /*
  * bio flags
  */
@@ -108,6 +114,13 @@ struct bio {
 #define BIO_FS_INTEGRITY 9	/* fs owns integrity data, not block layer */
 #define BIO_QUIET	10	/* Make BIO Quiet */
 #define BIO_MAPPED_INTEGRITY 11/* integrity metadata has been remapped */
+
+/*
+ * Flags starting here get preserved by bio_reset() - this includes
+ * BIO_POOL_IDX()
+ */
+#define BIO_RESET_BITS	12
+
 #define bio_flagged(bio, flag)	((bio)->bi_flags & (1 << (flag)))
 
 /*
-- 
1.7.12

Re: [PATCH v10 4/8] block: Add bio_reset()

[Jens Axboe]

On 2012-09-06 16:34, Kent Overstreet wrote:
> Reusing bios is something that's been highly frowned upon in the past,
> but driver code keeps doing it anyways. If it's going to happen anyways,
> we should provide a generic method.
> 
> This'll help with getting rid of bi_destructor - drivers/block/pktcdvd.c
> was open coding it, by doing a bio_init() and resetting bi_destructor.
> 
> This required reordering struct bio, but the block layer is not yet
> nearly fast enough for any cacheline effects to matter here.

That's an odd and misplaced comment. Was just doing testing today at 5M
IOPS, and even years back we've had cache effects for O_DIRECT in higher
speed setups.

That said, we haven't done cache analysis in a long time. So moving
members around isn't necessarily a huge deal.

Lastly, this isn't a great commit message for other reasons. Anyone can
see that it moves members around. It'd be a lot better to explain _why_
it is reordering the struct.

BTW, I looked over the rest of the patches, and it looks OK to me.
However, you didn't CC me on the full series, so I'm missing bits 3 and
5.

-- 
Jens Axboe

Re: [PATCH v10 4/8] block: Add bio_reset()

[Kent Overstreet]

On Thu, Sep 06, 2012 at 07:34:18PM -0600, Jens Axboe wrote:
> On 2012-09-06 16:34, Kent Overstreet wrote:
> > Reusing bios is something that's been highly frowned upon in the past,
> > but driver code keeps doing it anyways. If it's going to happen anyways,
> > we should provide a generic method.
> > 
> > This'll help with getting rid of bi_destructor - drivers/block/pktcdvd.c
> > was open coding it, by doing a bio_init() and resetting bi_destructor.
> > 
> > This required reordering struct bio, but the block layer is not yet
> > nearly fast enough for any cacheline effects to matter here.
> 
> That's an odd and misplaced comment. Was just doing testing today at 5M
> IOPS, and even years back we've had cache effects for O_DIRECT in higher
> speed setups.

Ah, I wasn't aware that you were pushing that many iops through the
block layer - most I've tested myself was around 1M. It wouldn't
surprise me if cache effects in struct bio mattered around 5M...

> That said, we haven't done cache analysis in a long time. So moving
> members around isn't necessarily a huge deal.

Ok, good to know. I've got another patch coming later that reorders
struct bio a bit more, for immutable bvecs (bi_sector, bi_size, bi_idx
go into a struct bvec_iter together).

> Lastly, this isn't a great commit message for other reasons. Anyone can
> see that it moves members around. It'd be a lot better to explain _why_
> it is reordering the struct.

Yeah, I suppose so. Will keep that in mind for the next patch.

> 
> BTW, I looked over the rest of the patches, and it looks OK to me.

Resent them. Thanks!

Re: [PATCH v10 4/8] block: Add bio_reset()

[Jens Axboe]

On 2012-09-07 14:58, Kent Overstreet wrote:
> On Thu, Sep 06, 2012 at 07:34:18PM -0600, Jens Axboe wrote:
>> On 2012-09-06 16:34, Kent Overstreet wrote:
>>> Reusing bios is something that's been highly frowned upon in the past,
>>> but driver code keeps doing it anyways. If it's going to happen anyways,
>>> we should provide a generic method.
>>>
>>> This'll help with getting rid of bi_destructor - drivers/block/pktcdvd.c
>>> was open coding it, by doing a bio_init() and resetting bi_destructor.
>>>
>>> This required reordering struct bio, but the block layer is not yet
>>> nearly fast enough for any cacheline effects to matter here.
>>
>> That's an odd and misplaced comment. Was just doing testing today at 5M
>> IOPS, and even years back we've had cache effects for O_DIRECT in higher
>> speed setups.
> 
> Ah, I wasn't aware that you were pushing that many iops through the
> block layer - most I've tested myself was around 1M. It wouldn't
> surprise me if cache effects in struct bio mattered around 5M...

5M is nothing, just did 13.5M :-)

But we can reshuffle for now. As mentioned, we're way overdue for a
decent look at cache profiling in any case.

>> That said, we haven't done cache analysis in a long time. So moving
>> members around isn't necessarily a huge deal.
> 
> Ok, good to know. I've got another patch coming later that reorders
> struct bio a bit more, for immutable bvecs (bi_sector, bi_size, bi_idx
> go into a struct bvec_iter together).

OK

>> Lastly, this isn't a great commit message for other reasons. Anyone can
>> see that it moves members around. It'd be a lot better to explain _why_
>> it is reordering the struct.
> 
> Yeah, I suppose so. Will keep that in mind for the next patch.

Thanks.

>> BTW, I looked over the rest of the patches, and it looks OK to me.
> 
> Resent them. Thanks!

Got it.

-- 
Jens Axboe

Re: [PATCH v10 4/8] block: Add bio_reset()

[Jens Axboe]

On 2012-09-07 15:55, Jens Axboe wrote:
> On 2012-09-07 14:58, Kent Overstreet wrote:
>> On Thu, Sep 06, 2012 at 07:34:18PM -0600, Jens Axboe wrote:
>>> On 2012-09-06 16:34, Kent Overstreet wrote:
>>>> Reusing bios is something that's been highly frowned upon in the past,
>>>> but driver code keeps doing it anyways. If it's going to happen anyways,
>>>> we should provide a generic method.
>>>>
>>>> This'll help with getting rid of bi_destructor - drivers/block/pktcdvd.c
>>>> was open coding it, by doing a bio_init() and resetting bi_destructor.
>>>>
>>>> This required reordering struct bio, but the block layer is not yet
>>>> nearly fast enough for any cacheline effects to matter here.
>>>
>>> That's an odd and misplaced comment. Was just doing testing today at 5M
>>> IOPS, and even years back we've had cache effects for O_DIRECT in higher
>>> speed setups.
>>
>> Ah, I wasn't aware that you were pushing that many iops through the
>> block layer - most I've tested myself was around 1M. It wouldn't
>> surprise me if cache effects in struct bio mattered around 5M...
> 
> 5M is nothing, just did 13.5M :-)
> 
> But we can reshuffle for now. As mentioned, we're way overdue for a
> decent look at cache profiling in any case.

No ill effects seen so far, fwiw:

  read : io=1735.8GB, bw=53690MB/s, iops=13745K, runt= 33104msec

-- 
Jens Axboe

Re: [PATCH v10 4/8] block: Add bio_reset()

[Kent Overstreet]

On Fri, Sep 07, 2012 at 04:06:45PM -0600, Jens Axboe wrote:
> On 2012-09-07 15:55, Jens Axboe wrote:
> > On 2012-09-07 14:58, Kent Overstreet wrote:
> >> On Thu, Sep 06, 2012 at 07:34:18PM -0600, Jens Axboe wrote:
> >>> On 2012-09-06 16:34, Kent Overstreet wrote:
> >>>> Reusing bios is something that's been highly frowned upon in the past,
> >>>> but driver code keeps doing it anyways. If it's going to happen anyways,
> >>>> we should provide a generic method.
> >>>>
> >>>> This'll help with getting rid of bi_destructor - drivers/block/pktcdvd.c
> >>>> was open coding it, by doing a bio_init() and resetting bi_destructor.
> >>>>
> >>>> This required reordering struct bio, but the block layer is not yet
> >>>> nearly fast enough for any cacheline effects to matter here.
> >>>
> >>> That's an odd and misplaced comment. Was just doing testing today at 5M
> >>> IOPS, and even years back we've had cache effects for O_DIRECT in higher
> >>> speed setups.
> >>
> >> Ah, I wasn't aware that you were pushing that many iops through the
> >> block layer - most I've tested myself was around 1M. It wouldn't
> >> surprise me if cache effects in struct bio mattered around 5M...
> > 
> > 5M is nothing, just did 13.5M :-)
> > 
> > But we can reshuffle for now. As mentioned, we're way overdue for a
> > decent look at cache profiling in any case.
> 
> No ill effects seen so far, fwiw:
> 
>   read : io=1735.8GB, bw=53690MB/s, iops=13745K, runt= 33104msec

Cool!

I'd be really curious to see a profile. Of the patches I've got queued
up I don't think anything's going to significantly affect performance
yet, but I'm hoping the cleanups/immutable bvec stuff/efficient bio
splitting enables some performance gains.

Well, it certainly will for stacking drivers, but I'm less sure what
it's going to look like running on just a raw flash device.

My end goal is making generic_make_request handle arbitrary sized bios,
and have (efficient) splitting happen as required. This'll get rid of a
bunch of code and complexity in the upper layers, in bio_add_page() and
elsewhere. More in the stacking drivers - merge_bvec_fn is horrendous to
support.

I think I might be able to efficiently get rid of the
segments-after-merging precalculating, and just have segments merged
once. That'd get rid of a couple fields in struct bio, and get it under
2 cachelines last I counted.

Course, all this doesn't matter as much for 4k bios so it may just be a
wash for you.

Re: [PATCH v10 4/8] block: Add bio_reset()

[Jens Axboe]

On 2012-09-07 16:25, Kent Overstreet wrote:
> On Fri, Sep 07, 2012 at 04:06:45PM -0600, Jens Axboe wrote:
>> On 2012-09-07 15:55, Jens Axboe wrote:
>>> On 2012-09-07 14:58, Kent Overstreet wrote:
>>>> On Thu, Sep 06, 2012 at 07:34:18PM -0600, Jens Axboe wrote:
>>>>> On 2012-09-06 16:34, Kent Overstreet wrote:
>>>>>> Reusing bios is something that's been highly frowned upon in the past,
>>>>>> but driver code keeps doing it anyways. If it's going to happen anyways,
>>>>>> we should provide a generic method.
>>>>>>
>>>>>> This'll help with getting rid of bi_destructor - drivers/block/pktcdvd.c
>>>>>> was open coding it, by doing a bio_init() and resetting bi_destructor.
>>>>>>
>>>>>> This required reordering struct bio, but the block layer is not yet
>>>>>> nearly fast enough for any cacheline effects to matter here.
>>>>>
>>>>> That's an odd and misplaced comment. Was just doing testing today at 5M
>>>>> IOPS, and even years back we've had cache effects for O_DIRECT in higher
>>>>> speed setups.
>>>>
>>>> Ah, I wasn't aware that you were pushing that many iops through the
>>>> block layer - most I've tested myself was around 1M. It wouldn't
>>>> surprise me if cache effects in struct bio mattered around 5M...
>>>
>>> 5M is nothing, just did 13.5M :-)
>>>
>>> But we can reshuffle for now. As mentioned, we're way overdue for a
>>> decent look at cache profiling in any case.
>>
>> No ill effects seen so far, fwiw:
>>
>>   read : io=1735.8GB, bw=53690MB/s, iops=13745K, runt= 33104msec
> 
> Cool!
> 
> I'd be really curious to see a profile. Of the patches I've got queued
> up I don't think anything's going to significantly affect performance
> yet, but I'm hoping the cleanups/immutable bvec stuff/efficient bio
> splitting enables some performance gains.

Got more work to do, but certainly not a problem sharing.

> Well, it certainly will for stacking drivers, but I'm less sure what
> it's going to look like running on just a raw flash device.
> 
> My end goal is making generic_make_request handle arbitrary sized bios,
> and have (efficient) splitting happen as required. This'll get rid of a
> bunch of code and complexity in the upper layers, in bio_add_page() and
> elsewhere. More in the stacking drivers - merge_bvec_fn is horrendous to
> support.

It is a nasty interface, in retrospect probably a mistake. As long as we
don't split ever on non-stacking drivers, I don't care too much. And it
would get rid of complexity in those drivers, so that's a nice win.
merge_bvec_fn not only a bad interface, it's also pretty slow...

> I think I might be able to efficiently get rid of the
> segments-after-merging precalculating, and just have segments merged
> once. That'd get rid of a couple fields in struct bio, and get it under
> 2 cachelines last I counted.

It's 2 cachelines now, but reducing is always a great thing. Getting rid
of the repeated recalculate after merge would be a nice win.

> Course, all this doesn't matter as much for 4k bios so it may just be a
> wash for you.

Right, for me it doesn't matter. As long as you don't slow me down :-)

-- 
Jens Axboe

Re: [dm-devel] [PATCH v10 4/8] block: Add bio_reset()

[Alasdair G Kergon]

As I indicated already in this discussion, dm started to use
merge_bvec_fn as a cheap way of avoiding splitting and this improved
overall efficiency.  Often it's better to pay the small price of calling
that function to ensure the bio is created the right size in the first
place so it won't have to get split later.

I'm as yet unconvinced that removing merge_bvec_fn would be an overall
win.  Some of Kent's other changes that make splitting cheaper will
improve the balance in some situations, but that might be handled by
simplifying the merge_bvec_fn calculations in those situations.
(Or changing the mechanism to avoid repeating performing the mapping
when it hasn't changed.)

IOW Any proposal to remove merge_bvec_fn from dm needs careful 
evaluation to ensure it doesn't introduce any significant
performance regressions for some sets of users.

Alasdair

Re: [dm-devel] [PATCH v10 4/8] block: Add bio_reset()

[Kent Overstreet]

On Sat, Sep 08, 2012 at 12:14:33AM +0100, Alasdair G Kergon wrote:
> As I indicated already in this discussion, dm started to use
> merge_bvec_fn as a cheap way of avoiding splitting and this improved
> overall efficiency.  Often it's better to pay the small price of calling
> that function to ensure the bio is created the right size in the first
> place so it won't have to get split later.

When I say cheap, I mean _cheap_:

split = bio_clone_bioset(bio, gfp_flags, bs);

bio_advance(bio, sectors << 9);
split->bi_iter.bi_size = sectors << 9;

And the clone doesn't copy the bvecs - split->bi_io_vec ==
bio->bi_io_vec.

> I'm as yet unconvinced that removing merge_bvec_fn would be an overall
> win.  Some of Kent's other changes that make splitting cheaper will
> improve the balance in some situations, but that might be handled by
> simplifying the merge_bvec_fn calculations in those situations.
> (Or changing the mechanism to avoid repeating performing the mapping
> when it hasn't changed.)

The current situation is what causes you to repeatedly do the mapping
lookup, since you'll often get contiguous bios that don't need to be
split at the mapping level (because of other requirements of the
underlying devices or because implementing merge_bvec_fn correctly was
too hard).

Splitting only when required is going to _improve_ that.

> IOW Any proposal to remove merge_bvec_fn from dm needs careful 
> evaluation to ensure it doesn't introduce any significant
> performance regressions for some sets of users.

There's also the 1000+ lines of deleted code to consider. In my
immutable bvec branch I've deleted over 400 lines of code, and that's
without actually trying to delete code. Getting rid of merge_bvec_fn
deletes another 800 lines of code on top of that.

CPU wise, there won't be any performance regressions. The only cause for
concern I can think of is where the upper layer could've made use of
partial completions - i.e. it submitted a 1 mb bio instead of a bunch of
128k bios, but it could've made use of that first 128k if it went to a
different device and completed sooner.

Only thing I know of that'd be affected by that though is readahead, and
I have a couple ideas for easily solving that if it actually becomes an
issue.

[PATCH v10 5/8] pktcdvd: Switch to bio_kmalloc()

[Kent Overstreet]

This is prep work for killing bi_destructor - previously, pktcdvd had
its own pkt_bio_alloc which was basically duplication bio_kmalloc(),
necessitating its own bi_destructor implementation.

v5: Un-reorder some functions, to make the patch easier to review

Signed-off-by: Kent Overstreet <koverstreet@google.com>
Acked-by: Jiri Kosina <jkosina@suse.cz>
---
 drivers/block/pktcdvd.c | 52 +++++++------------------------------------------
 1 file changed, 7 insertions(+), 45 deletions(-)

diff --git a/drivers/block/pktcdvd.c b/drivers/block/pktcdvd.c
index ba66e44..2e7de7a 100644
--- a/drivers/block/pktcdvd.c
+++ b/drivers/block/pktcdvd.c
@@ -522,38 +522,6 @@ static void pkt_bio_finished(struct pktcdvd_device *pd)
 	}
 }
 
-static void pkt_bio_destructor(struct bio *bio)
-{
-	kfree(bio->bi_io_vec);
-	kfree(bio);
-}
-
-static struct bio *pkt_bio_alloc(int nr_iovecs)
-{
-	struct bio_vec *bvl = NULL;
-	struct bio *bio;
-
-	bio = kmalloc(sizeof(struct bio), GFP_KERNEL);
-	if (!bio)
-		goto no_bio;
-	bio_init(bio);
-
-	bvl = kcalloc(nr_iovecs, sizeof(struct bio_vec), GFP_KERNEL);
-	if (!bvl)
-		goto no_bvl;
-
-	bio->bi_max_vecs = nr_iovecs;
-	bio->bi_io_vec = bvl;
-	bio->bi_destructor = pkt_bio_destructor;
-
-	return bio;
-
- no_bvl:
-	kfree(bio);
- no_bio:
-	return NULL;
-}
-
 /*
  * Allocate a packet_data struct
  */
@@ -567,7 +535,7 @@ static struct packet_data *pkt_alloc_packet_data(int frames)
 		goto no_pkt;
 
 	pkt->frames = frames;
-	pkt->w_bio = pkt_bio_alloc(frames);
+	pkt->w_bio = bio_kmalloc(GFP_KERNEL, frames);
 	if (!pkt->w_bio)
 		goto no_bio;
 
@@ -581,9 +549,10 @@ static struct packet_data *pkt_alloc_packet_data(int frames)
 	bio_list_init(&pkt->orig_bios);
 
 	for (i = 0; i < frames; i++) {
-		struct bio *bio = pkt_bio_alloc(1);
+		struct bio *bio = bio_kmalloc(GFP_KERNEL, 1);
 		if (!bio)
 			goto no_rd_bio;
+
 		pkt->r_bios[i] = bio;
 	}
 
@@ -1111,21 +1080,17 @@ static void pkt_gather_data(struct pktcdvd_device *pd, struct packet_data *pkt)
 	 * Schedule reads for missing parts of the packet.
 	 */
 	for (f = 0; f < pkt->frames; f++) {
-		struct bio_vec *vec;
-
 		int p, offset;
+
 		if (written[f])
 			continue;
+
 		bio = pkt->r_bios[f];
-		vec = bio->bi_io_vec;
-		bio_init(bio);
-		bio->bi_max_vecs = 1;
+		bio_reset(bio);
 		bio->bi_sector = pkt->sector + f * (CD_FRAMESIZE >> 9);
 		bio->bi_bdev = pd->bdev;
 		bio->bi_end_io = pkt_end_io_read;
 		bio->bi_private = pkt;
-		bio->bi_io_vec = vec;
-		bio->bi_destructor = pkt_bio_destructor;
 
 		p = (f * CD_FRAMESIZE) / PAGE_SIZE;
 		offset = (f * CD_FRAMESIZE) % PAGE_SIZE;
@@ -1418,14 +1383,11 @@ static void pkt_start_write(struct pktcdvd_device *pd, struct packet_data *pkt)
 	}
 
 	/* Start the write request */
-	bio_init(pkt->w_bio);
-	pkt->w_bio->bi_max_vecs = PACKET_MAX_SIZE;
+	bio_reset(pkt->w_bio);
 	pkt->w_bio->bi_sector = pkt->sector;
 	pkt->w_bio->bi_bdev = pd->bdev;
 	pkt->w_bio->bi_end_io = pkt_end_io_packet_write;
 	pkt->w_bio->bi_private = pkt;
-	pkt->w_bio->bi_io_vec = bvec;
-	pkt->w_bio->bi_destructor = pkt_bio_destructor;
 	for (f = 0; f < pkt->frames; f++)
 		if (!bio_add_page(pkt->w_bio, bvec[f].bv_page, CD_FRAMESIZE, bvec[f].bv_offset))
 			BUG();
-- 
1.7.12

[PATCH v10 6/8] block: Kill bi_destructor

[Kent Overstreet]

Now that we've got generic code for freeing bios allocated from bio
pools, this isn't needed anymore.

This patch also makes bio_free() static, since without bi_destructor
there should be no need for it to be called anywhere else.

bio_free() is now only called from bio_put, so we can refactor those a
bit - move some code from bio_put() to bio_free() and kill the redundant
bio->bi_next = NULL.

v5: Switch to BIO_KMALLOC_POOL ((void *)~0), per Boaz
v6: BIO_KMALLOC_POOL now NULL, drop bio_free's EXPORT_SYMBOL
v7: No #define BIO_KMALLOC_POOL anymore

Signed-off-by: Kent Overstreet <koverstreet@google.com>
CC: Jens Axboe <axboe@kernel.dk>
---
 Documentation/block/biodoc.txt |  5 ----
 block/blk-core.c               |  2 +-
 fs/bio.c                       | 64 +++++++++++++++++-------------------------
 include/linux/bio.h            |  1 -
 include/linux/blk_types.h      |  3 --
 5 files changed, 27 insertions(+), 48 deletions(-)

diff --git a/Documentation/block/biodoc.txt b/Documentation/block/biodoc.txt
index e418dc0..8df5e8e 100644
--- a/Documentation/block/biodoc.txt
+++ b/Documentation/block/biodoc.txt
@@ -465,7 +465,6 @@ struct bio {
        bio_end_io_t	*bi_end_io;  /* bi_end_io (bio) */
        atomic_t		bi_cnt;	     /* pin count: free when it hits zero */
        void             *bi_private;
-       bio_destructor_t *bi_destructor; /* bi_destructor (bio) */
 };
 
 With this multipage bio design:
@@ -647,10 +646,6 @@ for a non-clone bio. There are the 6 pools setup for different size biovecs,
 so bio_alloc(gfp_mask, nr_iovecs) will allocate a vec_list of the
 given size from these slabs.
 
-The bi_destructor() routine takes into account the possibility of the bio
-having originated from a different source (see later discussions on
-n/w to block transfers and kvec_cb)
-
 The bio_get() routine may be used to hold an extra reference on a bio prior
 to i/o submission, if the bio fields are likely to be accessed after the
 i/o is issued (since the bio may otherwise get freed in case i/o completion
diff --git a/block/blk-core.c b/block/blk-core.c
index 95c4935..b776cc9 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -2807,7 +2807,7 @@ int blk_rq_prep_clone(struct request *rq, struct request *rq_src,
 
 free_and_out:
 	if (bio)
-		bio_free(bio, bs);
+		bio_put(bio);
 	blk_rq_unprep_clone(rq);
 
 	return -ENOMEM;
diff --git a/fs/bio.c b/fs/bio.c
index 919ee9a..736ef12 100644
--- a/fs/bio.c
+++ b/fs/bio.c
@@ -233,26 +233,37 @@ fallback:
 	return bvl;
 }
 
-void bio_free(struct bio *bio, struct bio_set *bs)
+static void __bio_free(struct bio *bio)
 {
-	void *p;
-
-	if (bio_has_allocated_vec(bio))
-		bvec_free_bs(bs, bio->bi_io_vec, BIO_POOL_IDX(bio));
+	bio_disassociate_task(bio);
 
 	if (bio_integrity(bio))
 		bio_integrity_free(bio);
+}
 
-	/*
-	 * If we have front padding, adjust the bio pointer before freeing
-	 */
-	p = bio;
-	if (bs->front_pad)
+static void bio_free(struct bio *bio)
+{
+	struct bio_set *bs = bio->bi_pool;
+	void *p;
+
+	__bio_free(bio);
+
+	if (bs) {
+		if (bio_has_allocated_vec(bio))
+			bvec_free_bs(bs, bio->bi_io_vec, BIO_POOL_IDX(bio));
+
+		/*
+		 * If we have front padding, adjust the bio pointer before freeing
+		 */
+		p = bio;
 		p -= bs->front_pad;
 
-	mempool_free(p, bs->bio_pool);
+		mempool_free(p, bs->bio_pool);
+	} else {
+		/* Bio was allocated by bio_kmalloc() */
+		kfree(bio);
+	}
 }
-EXPORT_SYMBOL(bio_free);
 
 void bio_init(struct bio *bio)
 {
@@ -276,10 +287,7 @@ void bio_reset(struct bio *bio)
 {
 	unsigned long flags = bio->bi_flags & (~0UL << BIO_RESET_BITS);
 
-	if (bio_integrity(bio))
-		bio_integrity_free(bio);
-
-	bio_disassociate_task(bio);
+	__bio_free(bio);
 
 	memset(bio, 0, BIO_RESET_BYTES);
 	bio->bi_flags = flags|(1 << BIO_UPTODATE);
@@ -362,13 +370,6 @@ struct bio *bio_alloc(gfp_t gfp_mask, unsigned int nr_iovecs)
 }
 EXPORT_SYMBOL(bio_alloc);
 
-static void bio_kmalloc_destructor(struct bio *bio)
-{
-	if (bio_integrity(bio))
-		bio_integrity_free(bio);
-	kfree(bio);
-}
-
 /**
  * bio_kmalloc - allocate a bio for I/O using kmalloc()
  * @gfp_mask:   the GFP_ mask given to the slab allocator
@@ -395,7 +396,6 @@ struct bio *bio_kmalloc(gfp_t gfp_mask, unsigned int nr_iovecs)
 	bio->bi_flags |= BIO_POOL_NONE << BIO_POOL_OFFSET;
 	bio->bi_max_vecs = nr_iovecs;
 	bio->bi_io_vec = bio->bi_inline_vecs;
-	bio->bi_destructor = bio_kmalloc_destructor;
 
 	return bio;
 }
@@ -431,20 +431,8 @@ void bio_put(struct bio *bio)
 	/*
 	 * last put frees it
 	 */
-	if (atomic_dec_and_test(&bio->bi_cnt)) {
-		bio_disassociate_task(bio);
-		bio->bi_next = NULL;
-
-		/*
-		 * This if statement is temporary - bi_pool is replacing
-		 * bi_destructor, but bi_destructor will be taken out in another
-		 * patch.
-		 */
-		if (bio->bi_pool)
-			bio_free(bio, bio->bi_pool);
-		else
-			bio->bi_destructor(bio);
-	}
+	if (atomic_dec_and_test(&bio->bi_cnt))
+		bio_free(bio);
 }
 EXPORT_SYMBOL(bio_put);
 
diff --git a/include/linux/bio.h b/include/linux/bio.h
index 76f6c25..04944c9 100644
--- a/include/linux/bio.h
+++ b/include/linux/bio.h
@@ -216,7 +216,6 @@ extern struct bio *bio_alloc(gfp_t, unsigned int);
 extern struct bio *bio_kmalloc(gfp_t, unsigned int);
 extern struct bio *bio_alloc_bioset(gfp_t, int, struct bio_set *);
 extern void bio_put(struct bio *);
-extern void bio_free(struct bio *, struct bio_set *);
 
 extern void bio_endio(struct bio *, int);
 struct request_queue;
diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
index 1b607c2..3eefbb2 100644
--- a/include/linux/blk_types.h
+++ b/include/linux/blk_types.h
@@ -84,11 +84,8 @@ struct bio {
 
 	struct bio_vec		*bi_io_vec;	/* the actual vec list */
 
-	/* If bi_pool is non NULL, bi_destructor is not called */
 	struct bio_set		*bi_pool;
 
-	bio_destructor_t	*bi_destructor;	/* destructor */
-
 	/*
 	 * We can inline a number of vecs at the end of the bio, to avoid
 	 * double allocations for a small number of bio_vecs. This member
-- 
1.7.12

Re: [PATCH v10 6/8] block: Kill bi_destructor

[Tejun Heo]

On Thu, Sep 06, 2012 at 03:35:00PM -0700, Kent Overstreet wrote:
> Now that we've got generic code for freeing bios allocated from bio
> pools, this isn't needed anymore.
> 
> This patch also makes bio_free() static, since without bi_destructor
> there should be no need for it to be called anywhere else.
> 
> bio_free() is now only called from bio_put, so we can refactor those a
> bit - move some code from bio_put() to bio_free() and kill the redundant
> bio->bi_next = NULL.
> 
> v5: Switch to BIO_KMALLOC_POOL ((void *)~0), per Boaz
> v6: BIO_KMALLOC_POOL now NULL, drop bio_free's EXPORT_SYMBOL
> v7: No #define BIO_KMALLOC_POOL anymore
> 
> Signed-off-by: Kent Overstreet <koverstreet@google.com>
> CC: Jens Axboe <axboe@kernel.dk>

Acked-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun

[PATCH v10 7/8] block: Consolidate bio_alloc_bioset(), bio_kmalloc()

[Kent Overstreet]

Previously, bio_kmalloc() and bio_alloc_bioset() behaved slightly
different because there was some almost-duplicated code - this fixes
some of that.

The important change is that previously bio_kmalloc() always set
bi_io_vec = bi_inline_vecs, even if nr_iovecs == 0 - unlike
bio_alloc_bioset(). This would cause bio_has_data() to return true; I
don't know if this resulted in any actual bugs but it was certainly
wrong.

bio_kmalloc() and bio_alloc_bioset() also have different arbitrary
limits on nr_iovecs - 1024 (UIO_MAXIOV) for bio_kmalloc(), 256
(BIO_MAX_PAGES) for bio_alloc_bioset(). This patch doesn't fix that, but
at least they're enforced closer together and hopefully they will be
fixed in a later patch.

This'll also help with some future cleanups - there are a fair number of
functions that allocate bios (e.g. bio_clone()), and now they don't have
to be duplicated for bio_alloc(), bio_alloc_bioset(), and bio_kmalloc().

Signed-off-by: Kent Overstreet <koverstreet@google.com>
CC: Jens Axboe <axboe@kernel.dk>
v7: Re-add dropped comments, improv patch description
---
 fs/bio.c            | 110 ++++++++++++++++++----------------------------------
 include/linux/bio.h |  16 ++++++--
 2 files changed, 49 insertions(+), 77 deletions(-)

diff --git a/fs/bio.c b/fs/bio.c
index 736ef12..191b9b8 100644
--- a/fs/bio.c
+++ b/fs/bio.c
@@ -55,6 +55,7 @@ static struct biovec_slab bvec_slabs[BIOVEC_NR_POOLS] __read_mostly = {
  * IO code that does not need private memory pools.
  */
 struct bio_set *fs_bio_set;
+EXPORT_SYMBOL(fs_bio_set);
 
 /*
  * Our slab pool management
@@ -301,39 +302,58 @@ EXPORT_SYMBOL(bio_reset);
  * @bs:		the bio_set to allocate from.
  *
  * Description:
- *   bio_alloc_bioset will try its own mempool to satisfy the allocation.
- *   If %__GFP_WAIT is set then we will block on the internal pool waiting
- *   for a &struct bio to become free.
- **/
+ *   If @bs is NULL, uses kmalloc() to allocate the bio; else the allocation is
+ *   backed by the @bs's mempool.
+ *
+ *   When @bs is not NULL, if %__GFP_WAIT is set then bio_alloc will always be
+ *   able to allocate a bio. This is due to the mempool guarantees. To make this
+ *   work, callers must never allocate more than 1 bio at a time from this pool.
+ *   Callers that need to allocate more than 1 bio must always submit the
+ *   previously allocated bio for IO before attempting to allocate a new one.
+ *   Failure to do so can cause deadlocks under memory pressure.
+ *
+ *   RETURNS:
+ *   Pointer to new bio on success, NULL on failure.
+ */
 struct bio *bio_alloc_bioset(gfp_t gfp_mask, int nr_iovecs, struct bio_set *bs)
 {
+	unsigned front_pad;
+	unsigned inline_vecs;
 	unsigned long idx = BIO_POOL_NONE;
 	struct bio_vec *bvl = NULL;
 	struct bio *bio;
 	void *p;
 
-	p = mempool_alloc(bs->bio_pool, gfp_mask);
+	if (!bs) {
+		if (nr_iovecs > UIO_MAXIOV)
+			return NULL;
+
+		p = kmalloc(sizeof(struct bio) +
+			    nr_iovecs * sizeof(struct bio_vec),
+			    gfp_mask);
+		front_pad = 0;
+		inline_vecs = nr_iovecs;
+	} else {
+		p = mempool_alloc(bs->bio_pool, gfp_mask);
+		front_pad = bs->front_pad;
+		inline_vecs = BIO_INLINE_VECS;
+	}
+
 	if (unlikely(!p))
 		return NULL;
-	bio = p + bs->front_pad;
 
+	bio = p + front_pad;
 	bio_init(bio);
-	bio->bi_pool = bs;
-
-	if (unlikely(!nr_iovecs))
-		goto out_set;
 
-	if (nr_iovecs <= BIO_INLINE_VECS) {
-		bvl = bio->bi_inline_vecs;
-		nr_iovecs = BIO_INLINE_VECS;
-	} else {
+	if (nr_iovecs > inline_vecs) {
 		bvl = bvec_alloc_bs(gfp_mask, nr_iovecs, &idx, bs);
 		if (unlikely(!bvl))
 			goto err_free;
-
-		nr_iovecs = bvec_nr_vecs(idx);
+	} else if (nr_iovecs) {
+		bvl = bio->bi_inline_vecs;
 	}
-out_set:
+
+	bio->bi_pool = bs;
 	bio->bi_flags |= idx << BIO_POOL_OFFSET;
 	bio->bi_max_vecs = nr_iovecs;
 	bio->bi_io_vec = bvl;
@@ -345,62 +365,6 @@ err_free:
 }
 EXPORT_SYMBOL(bio_alloc_bioset);
 
-/**
- *	bio_alloc - allocate a new bio, memory pool backed
- *	@gfp_mask: allocation mask to use
- *	@nr_iovecs: number of iovecs
- *
- *	bio_alloc will allocate a bio and associated bio_vec array that can hold
- *	at least @nr_iovecs entries. Allocations will be done from the
- *	fs_bio_set. Also see @bio_alloc_bioset and @bio_kmalloc.
- *
- *	If %__GFP_WAIT is set, then bio_alloc will always be able to allocate
- *	a bio. This is due to the mempool guarantees. To make this work, callers
- *	must never allocate more than 1 bio at a time from this pool. Callers
- *	that need to allocate more than 1 bio must always submit the previously
- *	allocated bio for IO before attempting to allocate a new one. Failure to
- *	do so can cause livelocks under memory pressure.
- *
- *	RETURNS:
- *	Pointer to new bio on success, NULL on failure.
- */
-struct bio *bio_alloc(gfp_t gfp_mask, unsigned int nr_iovecs)
-{
-	return bio_alloc_bioset(gfp_mask, nr_iovecs, fs_bio_set);
-}
-EXPORT_SYMBOL(bio_alloc);
-
-/**
- * bio_kmalloc - allocate a bio for I/O using kmalloc()
- * @gfp_mask:   the GFP_ mask given to the slab allocator
- * @nr_iovecs:	number of iovecs to pre-allocate
- *
- * Description:
- *   Allocate a new bio with @nr_iovecs bvecs.  If @gfp_mask contains
- *   %__GFP_WAIT, the allocation is guaranteed to succeed.
- *
- **/
-struct bio *bio_kmalloc(gfp_t gfp_mask, unsigned int nr_iovecs)
-{
-	struct bio *bio;
-
-	if (nr_iovecs > UIO_MAXIOV)
-		return NULL;
-
-	bio = kmalloc(sizeof(struct bio) + nr_iovecs * sizeof(struct bio_vec),
-		      gfp_mask);
-	if (unlikely(!bio))
-		return NULL;
-
-	bio_init(bio);
-	bio->bi_flags |= BIO_POOL_NONE << BIO_POOL_OFFSET;
-	bio->bi_max_vecs = nr_iovecs;
-	bio->bi_io_vec = bio->bi_inline_vecs;
-
-	return bio;
-}
-EXPORT_SYMBOL(bio_kmalloc);
-
 void zero_fill_bio(struct bio *bio)
 {
 	unsigned long flags;
diff --git a/include/linux/bio.h b/include/linux/bio.h
index 04944c9..fbe35b1 100644
--- a/include/linux/bio.h
+++ b/include/linux/bio.h
@@ -212,11 +212,21 @@ extern void bio_pair_release(struct bio_pair *dbio);
 extern struct bio_set *bioset_create(unsigned int, unsigned int);
 extern void bioset_free(struct bio_set *);
 
-extern struct bio *bio_alloc(gfp_t, unsigned int);
-extern struct bio *bio_kmalloc(gfp_t, unsigned int);
 extern struct bio *bio_alloc_bioset(gfp_t, int, struct bio_set *);
 extern void bio_put(struct bio *);
 
+extern struct bio_set *fs_bio_set;
+
+static inline struct bio *bio_alloc(gfp_t gfp_mask, unsigned int nr_iovecs)
+{
+	return bio_alloc_bioset(gfp_mask, nr_iovecs, fs_bio_set);
+}
+
+static inline struct bio *bio_kmalloc(gfp_t gfp_mask, unsigned int nr_iovecs)
+{
+	return bio_alloc_bioset(gfp_mask, nr_iovecs, NULL);
+}
+
 extern void bio_endio(struct bio *, int);
 struct request_queue;
 extern int bio_phys_segments(struct request_queue *, struct bio *);
@@ -304,8 +314,6 @@ struct biovec_slab {
 	struct kmem_cache *slab;
 };
 
-extern struct bio_set *fs_bio_set;
-
 /*
  * a small number of entries is fine, not going to be performance critical.
  * basically we just need to survive
-- 
1.7.12

Re: [PATCH v10 7/8] block: Consolidate bio_alloc_bioset(), bio_kmalloc()

[Tejun Heo]

On Thu, Sep 06, 2012 at 03:35:01PM -0700, Kent Overstreet wrote:
> Previously, bio_kmalloc() and bio_alloc_bioset() behaved slightly
> different because there was some almost-duplicated code - this fixes
> some of that.
> 
> The important change is that previously bio_kmalloc() always set
> bi_io_vec = bi_inline_vecs, even if nr_iovecs == 0 - unlike
> bio_alloc_bioset(). This would cause bio_has_data() to return true; I
> don't know if this resulted in any actual bugs but it was certainly
> wrong.
> 
> bio_kmalloc() and bio_alloc_bioset() also have different arbitrary
> limits on nr_iovecs - 1024 (UIO_MAXIOV) for bio_kmalloc(), 256
> (BIO_MAX_PAGES) for bio_alloc_bioset(). This patch doesn't fix that, but
> at least they're enforced closer together and hopefully they will be
> fixed in a later patch.
> 
> This'll also help with some future cleanups - there are a fair number of
> functions that allocate bios (e.g. bio_clone()), and now they don't have
> to be duplicated for bio_alloc(), bio_alloc_bioset(), and bio_kmalloc().
> 
> Signed-off-by: Kent Overstreet <koverstreet@google.com>
> CC: Jens Axboe <axboe@kernel.dk>
> v7: Re-add dropped comments, improv patch description

Acked-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun

[PATCH v10 8/8] block: Add bio_clone_bioset(), bio_clone_kmalloc()

[Kent Overstreet]

Previously, there was bio_clone() but it only allocated from the fs bio
set; as a result various users were open coding it and using
__bio_clone().

This changes bio_clone() to become bio_clone_bioset(), and then we add
bio_clone() and bio_clone_kmalloc() as wrappers around it, making use of
the functionality the last patch adedd.

This will also help in a later patch changing how bio cloning works.

Signed-off-by: Kent Overstreet <koverstreet@google.com>
CC: Jens Axboe <axboe@kernel.dk>
CC: NeilBrown <neilb@suse.de>
CC: Alasdair Kergon <agk@redhat.com>
CC: Boaz Harrosh <bharrosh@panasas.com>
CC: Jeff Garzik <jeff@garzik.org>
Acked-by: Jeff Garzik <jgarzik@redhat.com>
---
 block/blk-core.c       |  8 +-------
 drivers/block/osdblk.c |  3 +--
 drivers/md/dm-crypt.c  |  7 +------
 drivers/md/dm.c        |  4 ++--
 drivers/md/md.c        | 20 +-------------------
 fs/bio.c               | 11 +++++++----
 fs/exofs/ore.c         |  5 ++---
 include/linux/bio.h    | 17 ++++++++++++++---
 8 files changed, 29 insertions(+), 46 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index b776cc9..82aab28 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -2781,16 +2781,10 @@ int blk_rq_prep_clone(struct request *rq, struct request *rq_src,
 	blk_rq_init(NULL, rq);
 
 	__rq_for_each_bio(bio_src, rq_src) {
-		bio = bio_alloc_bioset(gfp_mask, bio_src->bi_max_vecs, bs);
+		bio = bio_clone_bioset(bio_src, gfp_mask, bs);
 		if (!bio)
 			goto free_and_out;
 
-		__bio_clone(bio, bio_src);
-
-		if (bio_integrity(bio_src) &&
-		    bio_integrity_clone(bio, bio_src, gfp_mask))
-			goto free_and_out;
-
 		if (bio_ctr && bio_ctr(bio, bio_src, data))
 			goto free_and_out;
 
diff --git a/drivers/block/osdblk.c b/drivers/block/osdblk.c
index 87311eb..1bbc681 100644
--- a/drivers/block/osdblk.c
+++ b/drivers/block/osdblk.c
@@ -266,11 +266,10 @@ static struct bio *bio_chain_clone(struct bio *old_chain, gfp_t gfpmask)
 	struct bio *tmp, *new_chain = NULL, *tail = NULL;
 
 	while (old_chain) {
-		tmp = bio_kmalloc(gfpmask, old_chain->bi_max_vecs);
+		tmp = bio_clone_kmalloc(old_chain, gfpmask);
 		if (!tmp)
 			goto err_out;
 
-		__bio_clone(tmp, old_chain);
 		tmp->bi_bdev = NULL;
 		gfpmask &= ~__GFP_WAIT;
 		tmp->bi_next = NULL;
diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index 3c0acba..bbf459b 100644
--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -979,19 +979,14 @@ static int kcryptd_io_read(struct dm_crypt_io *io, gfp_t gfp)
 	 * copy the required bvecs because we need the original
 	 * one in order to decrypt the whole bio data *afterwards*.
 	 */
-	clone = bio_alloc_bioset(gfp, bio_segments(base_bio), cc->bs);
+	clone = bio_clone_bioset(base_bio, gfp, cc->bs);
 	if (!clone)
 		return 1;
 
 	crypt_inc_pending(io);
 
 	clone_init(io, clone);
-	clone->bi_idx = 0;
-	clone->bi_vcnt = bio_segments(base_bio);
-	clone->bi_size = base_bio->bi_size;
 	clone->bi_sector = cc->start + io->sector;
-	memcpy(clone->bi_io_vec, bio_iovec(base_bio),
-	       sizeof(struct bio_vec) * clone->bi_vcnt);
 
 	generic_make_request(clone);
 	return 0;
diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index 33470f0..8378797 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -1129,8 +1129,8 @@ static void __issue_target_request(struct clone_info *ci, struct dm_target *ti,
 	 * ci->bio->bi_max_vecs is BIO_INLINE_VECS anyway, for both flush
 	 * and discard, so no need for concern about wasted bvec allocations.
 	 */
-	clone = bio_alloc_bioset(GFP_NOIO, ci->bio->bi_max_vecs, ci->md->bs);
-	__bio_clone(clone, ci->bio);
+	clone = bio_clone_bioset(ci->bio, GFP_NOIO, ci->md->bs);
+
 	if (len) {
 		clone->bi_sector = ci->sector;
 		clone->bi_size = to_bytes(len);
diff --git a/drivers/md/md.c b/drivers/md/md.c
index 457ca84..7a2b079 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -173,28 +173,10 @@ EXPORT_SYMBOL_GPL(bio_alloc_mddev);
 struct bio *bio_clone_mddev(struct bio *bio, gfp_t gfp_mask,
 			    struct mddev *mddev)
 {
-	struct bio *b;
-
 	if (!mddev || !mddev->bio_set)
 		return bio_clone(bio, gfp_mask);
 
-	b = bio_alloc_bioset(gfp_mask, bio->bi_max_vecs, mddev->bio_set);
-	if (!b)
-		return NULL;
-
-	__bio_clone(b, bio);
-	if (bio_integrity(bio)) {
-		int ret;
-
-		ret = bio_integrity_clone(b, bio, gfp_mask);
-
-		if (ret < 0) {
-			bio_put(b);
-			return NULL;
-		}
-	}
-
-	return b;
+	return bio_clone_bioset(bio, gfp_mask, mddev->bio_set);
 }
 EXPORT_SYMBOL_GPL(bio_clone_mddev);
 
diff --git a/fs/bio.c b/fs/bio.c
index 191b9b8..13e9567 100644
--- a/fs/bio.c
+++ b/fs/bio.c
@@ -438,16 +438,19 @@ void __bio_clone(struct bio *bio, struct bio *bio_src)
 EXPORT_SYMBOL(__bio_clone);
 
 /**
- *	bio_clone	-	clone a bio
+ *	bio_clone_bioset -	clone a bio
  *	@bio: bio to clone
  *	@gfp_mask: allocation priority
+ *	@bs: bio_set to allocate from
  *
  * 	Like __bio_clone, only also allocates the returned bio
  */
-struct bio *bio_clone(struct bio *bio, gfp_t gfp_mask)
+struct bio *bio_clone_bioset(struct bio *bio, gfp_t gfp_mask,
+			     struct bio_set *bs)
 {
-	struct bio *b = bio_alloc(gfp_mask, bio->bi_max_vecs);
+	struct bio *b;
 
+	b = bio_alloc_bioset(gfp_mask, bio->bi_max_vecs, bs);
 	if (!b)
 		return NULL;
 
@@ -466,7 +469,7 @@ struct bio *bio_clone(struct bio *bio, gfp_t gfp_mask)
 
 	return b;
 }
-EXPORT_SYMBOL(bio_clone);
+EXPORT_SYMBOL(bio_clone_bioset);
 
 /**
  *	bio_get_nr_vecs		- return approx number of vecs
diff --git a/fs/exofs/ore.c b/fs/exofs/ore.c
index 1585db1..f936cb5 100644
--- a/fs/exofs/ore.c
+++ b/fs/exofs/ore.c
@@ -814,8 +814,8 @@ static int _write_mirror(struct ore_io_state *ios, int cur_comp)
 			struct bio *bio;
 
 			if (per_dev != master_dev) {
-				bio = bio_kmalloc(GFP_KERNEL,
-						  master_dev->bio->bi_max_vecs);
+				bio = bio_clone_kmalloc(master_dev->bio,
+							GFP_KERNEL);
 				if (unlikely(!bio)) {
 					ORE_DBGMSG(
 					      "Failed to allocate BIO size=%u\n",
@@ -824,7 +824,6 @@ static int _write_mirror(struct ore_io_state *ios, int cur_comp)
 					goto out;
 				}
 
-				__bio_clone(bio, master_dev->bio);
 				bio->bi_bdev = NULL;
 				bio->bi_next = NULL;
 				per_dev->offset = master_dev->offset;
diff --git a/include/linux/bio.h b/include/linux/bio.h
index fbe35b1..52b9cbc 100644
--- a/include/linux/bio.h
+++ b/include/linux/bio.h
@@ -215,6 +215,9 @@ extern void bioset_free(struct bio_set *);
 extern struct bio *bio_alloc_bioset(gfp_t, int, struct bio_set *);
 extern void bio_put(struct bio *);
 
+extern void __bio_clone(struct bio *, struct bio *);
+extern struct bio *bio_clone_bioset(struct bio *, gfp_t, struct bio_set *bs);
+
 extern struct bio_set *fs_bio_set;
 
 static inline struct bio *bio_alloc(gfp_t gfp_mask, unsigned int nr_iovecs)
@@ -222,18 +225,26 @@ static inline struct bio *bio_alloc(gfp_t gfp_mask, unsigned int nr_iovecs)
 	return bio_alloc_bioset(gfp_mask, nr_iovecs, fs_bio_set);
 }
 
+static inline struct bio *bio_clone(struct bio *bio, gfp_t gfp_mask)
+{
+	return bio_clone_bioset(bio, gfp_mask, fs_bio_set);
+}
+
 static inline struct bio *bio_kmalloc(gfp_t gfp_mask, unsigned int nr_iovecs)
 {
 	return bio_alloc_bioset(gfp_mask, nr_iovecs, NULL);
 }
 
+static inline struct bio *bio_clone_kmalloc(struct bio *bio, gfp_t gfp_mask)
+{
+	return bio_clone_bioset(bio, gfp_mask, NULL);
+
+}
+
 extern void bio_endio(struct bio *, int);
 struct request_queue;
 extern int bio_phys_segments(struct request_queue *, struct bio *);
 
-extern void __bio_clone(struct bio *, struct bio *);
-extern struct bio *bio_clone(struct bio *, gfp_t);
-
 extern void bio_init(struct bio *);
 extern void bio_reset(struct bio *);
 
-- 
1.7.12

Re: [PATCH v10 8/8] block: Add bio_clone_bioset(), bio_clone_kmalloc()

[Tejun Heo]

On Thu, Sep 06, 2012 at 03:35:02PM -0700, Kent Overstreet wrote:
> Previously, there was bio_clone() but it only allocated from the fs bio
> set; as a result various users were open coding it and using
> __bio_clone().
> 
> This changes bio_clone() to become bio_clone_bioset(), and then we add
> bio_clone() and bio_clone_kmalloc() as wrappers around it, making use of
> the functionality the last patch adedd.
> 
> This will also help in a later patch changing how bio cloning works.
> 
> Signed-off-by: Kent Overstreet <koverstreet@google.com>
> CC: Jens Axboe <axboe@kernel.dk>
> CC: NeilBrown <neilb@suse.de>
> CC: Alasdair Kergon <agk@redhat.com>
> CC: Boaz Harrosh <bharrosh@panasas.com>
> CC: Jeff Garzik <jeff@garzik.org>
> Acked-by: Jeff Garzik <jgarzik@redhat.com>

Acked-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun

Re: [dm-devel] [PATCH v10 8/8] block: Add bio_clone_bioset(), bio_clone_kmalloc()

[Alasdair G Kergon]

On Thu, Sep 06, 2012 at 03:35:02PM -0700, Kent Overstreet wrote:
> Previously, there was bio_clone() but it only allocated from the fs bio
> set; as a result various users were open coding it and using
> __bio_clone().

Explain in the header the reasoning behind the change to dm-crypt so that 
it no longer resets bi_idx to 0 too?

> -	clone->bi_idx = 0;

Alasdair

Re: [dm-devel] [PATCH v10 8/8] block: Add bio_clone_bioset(), bio_clone_kmalloc()

[Kent Overstreet]

On Fri, Sep 14, 2012 at 10:50:59PM +0100, Alasdair G Kergon wrote:
> On Thu, Sep 06, 2012 at 03:35:02PM -0700, Kent Overstreet wrote:
> > Previously, there was bio_clone() but it only allocated from the fs bio
> > set; as a result various users were open coding it and using
> > __bio_clone().
> 
> Explain in the header the reasoning behind the change to dm-crypt so that 
> it no longer resets bi_idx to 0 too?
> 
> > -	clone->bi_idx = 0;

Previously, it was open coding __bio_clone(), that's what the setting
bi_idx to 0 was from. With the change to bio_clone_bioset() that's no
longer necessary (and dangerous, since bi_idx needs to be consistent
with bi_sector/bi_size).

Re: [dm-devel] [PATCH v10 8/8] block: Add bio_clone_bioset(), bio_clone_kmalloc()

[Alasdair G Kergon]

On Mon, Sep 17, 2012 at 01:42:27PM -0700, Kent Overstreet wrote:
> Previously, it was open coding __bio_clone(), that's what the setting
> bi_idx to 0 was from. With the change to bio_clone_bioset() that's no
> longer necessary (and dangerous, since bi_idx needs to be consistent
> with bi_sector/bi_size).
 
It was performing a partial clone, deleting any no-longer-needed entries
from the array.  This patch undoes that which is presumably OK because
you propose to eliminate them again in a later one of your patches.

Alasdair

Re: [PATCH v10 0/8] Block cleanups

[Tejun Heo]

Hello, guys.

(cc'ing Jens, Alasdair and Neil)

On Thu, Sep 06, 2012 at 03:34:54PM -0700, Kent Overstreet wrote:
> Screwed up the bio_reset() patch in the last patch series when I went to edit
> the description, fixed that here.
> 
> Only other change is the dm patch - made the front_pad conditional on
> DM_TYPE_BIO_BASED.
> 
> Kent Overstreet (8):
>   block: Generalized bio pool freeing
>   block: Ues bi_pool for bio_integrity_alloc()
>   dm: Use bioset's front_pad for dm_rq_clone_bio_info
>   block: Add bio_reset()
>   pktcdvd: Switch to bio_kmalloc()
>   block: Kill bi_destructor
>   block: Consolidate bio_alloc_bioset(), bio_kmalloc()
>   block: Add bio_clone_bioset(), bio_clone_kmalloc()

This series looks good to me now.  If someone can ack the dm patch, I
think it's good to go.  Jens, what do you think?

Thanks.

-- 
tejun

Re: [PATCH v10 0/8] Block cleanups

[Jens Axboe]

On 2012-09-06 17:48, Tejun Heo wrote:
> Hello, guys.
> 
> (cc'ing Jens, Alasdair and Neil)
> 
> On Thu, Sep 06, 2012 at 03:34:54PM -0700, Kent Overstreet wrote:
>> Screwed up the bio_reset() patch in the last patch series when I went to edit
>> the description, fixed that here.
>>
>> Only other change is the dm patch - made the front_pad conditional on
>> DM_TYPE_BIO_BASED.
>>
>> Kent Overstreet (8):
>>   block: Generalized bio pool freeing
>>   block: Ues bi_pool for bio_integrity_alloc()
>>   dm: Use bioset's front_pad for dm_rq_clone_bio_info
>>   block: Add bio_reset()
>>   pktcdvd: Switch to bio_kmalloc()
>>   block: Kill bi_destructor
>>   block: Consolidate bio_alloc_bioset(), bio_kmalloc()
>>   block: Add bio_clone_bioset(), bio_clone_kmalloc()
> 
> This series looks good to me now.  If someone can ack the dm patch, I
> think it's good to go.  Jens, what do you think?

Looks OK to me too. I'll run a quick perf test on this tomorrow, and get
it queued up for 3.7 if it looks fine.

Kent, are to resend 3+5 to me?

-- 
Jens Axboe

Re: [PATCH v10 0/8] Block cleanups

[Kent Overstreet]

On Thu, Sep 06, 2012 at 07:37:10PM -0600, Jens Axboe wrote:
> On 2012-09-06 17:48, Tejun Heo wrote:
> > Hello, guys.
> > 
> > (cc'ing Jens, Alasdair and Neil)
> > 
> > On Thu, Sep 06, 2012 at 03:34:54PM -0700, Kent Overstreet wrote:
> >> Screwed up the bio_reset() patch in the last patch series when I went to edit
> >> the description, fixed that here.
> >>
> >> Only other change is the dm patch - made the front_pad conditional on
> >> DM_TYPE_BIO_BASED.
> >>
> >> Kent Overstreet (8):
> >>   block: Generalized bio pool freeing
> >>   block: Ues bi_pool for bio_integrity_alloc()
> >>   dm: Use bioset's front_pad for dm_rq_clone_bio_info
> >>   block: Add bio_reset()
> >>   pktcdvd: Switch to bio_kmalloc()
> >>   block: Kill bi_destructor
> >>   block: Consolidate bio_alloc_bioset(), bio_kmalloc()
> >>   block: Add bio_clone_bioset(), bio_clone_kmalloc()
> > 
> > This series looks good to me now.  If someone can ack the dm patch, I
> > think it's good to go.  Jens, what do you think?
> 
> Looks OK to me too. I'll run a quick perf test on this tomorrow, and get
> it queued up for 3.7 if it looks fine.

Thanks!

> Kent, are to resend 3+5 to me?

You should have it now.

Re: [PATCH v10 0/8] Block cleanups

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 1157 bytes --]

On Thu, 6 Sep 2012 16:48:05 -0700 Tejun Heo <tj@kernel.org> wrote:

> Hello, guys.
> 
> (cc'ing Jens, Alasdair and Neil)
> 
> On Thu, Sep 06, 2012 at 03:34:54PM -0700, Kent Overstreet wrote:
> > Screwed up the bio_reset() patch in the last patch series when I went to edit
> > the description, fixed that here.
> > 
> > Only other change is the dm patch - made the front_pad conditional on
> > DM_TYPE_BIO_BASED.
> > 
> > Kent Overstreet (8):
> >   block: Generalized bio pool freeing
> >   block: Ues bi_pool for bio_integrity_alloc()
> >   dm: Use bioset's front_pad for dm_rq_clone_bio_info
> >   block: Add bio_reset()
> >   pktcdvd: Switch to bio_kmalloc()
> >   block: Kill bi_destructor
> >   block: Consolidate bio_alloc_bioset(), bio_kmalloc()
> >   block: Add bio_clone_bioset(), bio_clone_kmalloc()
> 
> This series looks good to me now.  If someone can ack the dm patch, I
> think it's good to go.  Jens, what do you think?
> 
> Thanks.
> 

I'm very happy with them from the perspective of 'md'.  Great work!

 Acked-by: NeilBrown <neilb@suse.de>

for the few patches which touch drivers/md/md.c


NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]