FAQ Search Today's Posts Mark Forums Read
» Video Reviews

» Linux Archive

Linux-archive is a website aiming to archive linux email lists and to make them easily accessible for linux users/developers.


» Sponsor

» Partners

» Sponsor

Go Back   Linux Archive > Redhat > Device-mapper Development

 
 
LinkBack Thread Tools
 
Old 05-18-2012, 02:59 AM
 
Default block: Add bio_clone_bioset()

From: Kent Overstreet <koverstreet@google.com>

This consolidates some code, and will help in a later patch changing how
bio cloning works.

Signed-off-by: Kent Overstreet <koverstreet@google.com>
---
block/blk-core.c | 8 +-------
drivers/md/dm.c | 27 ++++++++++-----------------
drivers/md/md.c | 18 +-----------------
fs/bio.c | 16 ++++++++++++----
include/linux/bio.h | 1 +
5 files changed, 25 insertions(+), 45 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 1f61b74..91617eb 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -2660,16 +2660,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, bs))
- goto free_and_out;
-
if (bio_ctr && bio_ctr(bio, bio_src, data))
goto free_and_out;

diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index 3cc2169..3e33039 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -1072,26 +1072,19 @@ static struct bio *split_bvec(struct bio *bio, sector_t sector,
* Creates a bio that consists of range of complete bvecs.
*/
static struct bio *clone_bio(struct bio *bio, sector_t sector,
- unsigned short idx, unsigned short bv_count,
+ unsigned short bv_count,
unsigned int len, struct bio_set *bs)
{
struct bio *clone;

- clone = bio_alloc_bioset(GFP_NOIO, bio->bi_max_vecs, bs);
- __bio_clone(clone, bio);
+ clone = bio_clone_bioset(bio, GFP_NOIO, bs);
clone->bi_sector = sector;
- clone->bi_idx = idx;
- clone->bi_vcnt = idx + bv_count;
+ clone->bi_vcnt = bv_count;
clone->bi_size = to_bytes(len);
- clone->bi_flags &= ~(1 << BIO_SEG_VALID);

- if (bio_integrity(bio)) {
- bio_integrity_clone(clone, bio, GFP_NOIO, bs);
-
- if (idx != bio->bi_idx || clone->bi_size < bio->bi_size)
- bio_integrity_trim(clone,
- bio_sector_offset(bio, idx, 0), len);
- }
+ if (bio_integrity(bio) &&
+ clone->bi_size < bio->bi_size)
+ bio_integrity_trim(clone, 0, len);

return clone;
}
@@ -1121,8 +1114,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);
@@ -1161,7 +1154,7 @@ static void __clone_and_map_simple(struct clone_info *ci, struct dm_target *ti)
struct dm_target_io *tio;

tio = alloc_tio(ci, ti);
- clone = clone_bio(bio, ci->sector, ci->idx,
+ clone = clone_bio(bio, ci->sector,
bio->bi_vcnt - ci->idx, ci->sector_count,
ci->md->bs);
__map_bio(ti, clone, tio);
@@ -1240,7 +1233,7 @@ static int __clone_and_map(struct clone_info *ci)
}

tio = alloc_tio(ci, ti);
- clone = clone_bio(bio, ci->sector, ci->idx, i - ci->idx, len,
+ clone = clone_bio(bio, ci->sector, i - ci->idx, len,
ci->md->bs);
__map_bio(ti, clone, tio);

diff --git a/drivers/md/md.c b/drivers/md/md.c
index a5a524e..47605e7 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -178,23 +178,7 @@ struct bio *bio_clone_mddev(struct bio *bio, gfp_t gfp_mask,
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, mddev->bio_set);
-
- if (ret < 0) {
- bio_put(b);
- return NULL;
- }
- }
-
- return b;
+ return bio_clone_bioset(gfp_mask, bio_segments(bio), mddev->bio_set);
}
EXPORT_SYMBOL_GPL(bio_clone_mddev);

diff --git a/fs/bio.c b/fs/bio.c
index 3667cef..4cb621f 100644
--- a/fs/bio.c
+++ b/fs/bio.c
@@ -453,15 +453,17 @@ 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 = bio_alloc_bioset(gfp_mask, bio->bi_max_vecs, bs);

if (!b)
return NULL;
@@ -471,7 +473,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, bs);

if (ret < 0) {
bio_put(b);
@@ -481,6 +483,12 @@ struct bio *bio_clone(struct bio *bio, gfp_t gfp_mask)

return b;
}
+EXPORT_SYMBOL(bio_clone_bioset);
+
+struct bio *bio_clone(struct bio *bio, gfp_t gfp_mask)
+{
+ return bio_clone_bioset(bio, gfp_mask, fs_bio_set);
+}
EXPORT_SYMBOL(bio_clone);

/**
diff --git a/include/linux/bio.h b/include/linux/bio.h
index 4d94eb8..e068a68 100644
--- a/include/linux/bio.h
+++ b/include/linux/bio.h
@@ -223,6 +223,7 @@ 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_bioset(struct bio *, gfp_t, struct bio_set *bs);
extern struct bio *bio_clone(struct bio *, gfp_t);

extern void bio_init(struct bio *);
--
1.7.9.rc2

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 05-18-2012, 04:05 PM
Tejun Heo
 
Default block: Add bio_clone_bioset()

Hey, Kent.

On Thu, May 17, 2012 at 10:59:50PM -0400, koverstreet@google.com wrote:
> From: Kent Overstreet <koverstreet@google.com>
>
> This consolidates some code, and will help in a later patch changing how
> bio cloning works.
>
> Signed-off-by: Kent Overstreet <koverstreet@google.com>

I'd prefer a bit more explanation on what's going on generally and why
and how dm conversion is different.

> diff --git a/drivers/md/dm.c b/drivers/md/dm.c
> index 3cc2169..3e33039 100644
> --- a/drivers/md/dm.c
> +++ b/drivers/md/dm.c
> @@ -1072,26 +1072,19 @@ static struct bio *split_bvec(struct bio *bio, sector_t sector,
> * Creates a bio that consists of range of complete bvecs.
> */
> static struct bio *clone_bio(struct bio *bio, sector_t sector,
> - unsigned short idx, unsigned short bv_count,
> + unsigned short bv_count,
> unsigned int len, struct bio_set *bs)
> {
> struct bio *clone;
>
> - clone = bio_alloc_bioset(GFP_NOIO, bio->bi_max_vecs, bs);
> - __bio_clone(clone, bio);
> + clone = bio_clone_bioset(bio, GFP_NOIO, bs);
> clone->bi_sector = sector;
> - clone->bi_idx = idx;
> - clone->bi_vcnt = idx + bv_count;
> + clone->bi_vcnt = bv_count;
> clone->bi_size = to_bytes(len);
> - clone->bi_flags &= ~(1 << BIO_SEG_VALID);

Maybe removal of @idx deserves a separate patch?

> - if (bio_integrity(bio)) {
> - bio_integrity_clone(clone, bio, GFP_NOIO, bs);
> -
> - if (idx != bio->bi_idx || clone->bi_size < bio->bi_size)
> - bio_integrity_trim(clone,
> - bio_sector_offset(bio, idx, 0), len);
> - }
> + if (bio_integrity(bio) &&
> + clone->bi_size < bio->bi_size)

Unnecessary line break.

Other than that, looks good to me.

Thanks.

--
tejun

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 05-18-2012, 04:11 PM
Vivek Goyal
 
Default block: Add bio_clone_bioset()

On Thu, May 17, 2012 at 10:59:50PM -0400, koverstreet@google.com wrote:

[..]
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index a5a524e..47605e7 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -178,23 +178,7 @@ struct bio *bio_clone_mddev(struct bio *bio, gfp_t gfp_mask,
> 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, mddev->bio_set);
> -
> - if (ret < 0) {
> - bio_put(b);
> - return NULL;
> - }
> - }
> -
> - return b;
> + return bio_clone_bioset(gfp_mask, bio_segments(bio), mddev->bio_set);

Are we passing wrong arguments to bio_clone_bioset()?

Thanks
Vivek

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 05-18-2012, 06:55 PM
Kent Overstreet
 
Default block: Add bio_clone_bioset()

On Fri, May 18, 2012 at 12:11:30PM -0400, Vivek Goyal wrote:
> On Thu, May 17, 2012 at 10:59:50PM -0400, koverstreet@google.com wrote:
>
> [..]
> > diff --git a/drivers/md/md.c b/drivers/md/md.c
> > index a5a524e..47605e7 100644
> > --- a/drivers/md/md.c
> > +++ b/drivers/md/md.c
> > @@ -178,23 +178,7 @@ struct bio *bio_clone_mddev(struct bio *bio, gfp_t gfp_mask,
> > 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, mddev->bio_set);
> > -
> > - if (ret < 0) {
> > - bio_put(b);
> > - return NULL;
> > - }
> > - }
> > -
> > - return b;
> > + return bio_clone_bioset(gfp_mask, bio_segments(bio), mddev->bio_set);
>
> Are we passing wrong arguments to bio_clone_bioset()?

Uh - whoops, yes. Good catch, thanks.

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 05-18-2012, 08:31 PM
Kent Overstreet
 
Default block: Add bio_clone_bioset()

On Fri, May 18, 2012 at 09:05:11AM -0700, Tejun Heo wrote:
> Hey, Kent.
>
> On Thu, May 17, 2012 at 10:59:50PM -0400, koverstreet@google.com wrote:
> > From: Kent Overstreet <koverstreet@google.com>
> >
> > This consolidates some code, and will help in a later patch changing how
> > bio cloning works.
> >
> > Signed-off-by: Kent Overstreet <koverstreet@google.com>
>
> I'd prefer a bit more explanation on what's going on generally and why
> and how dm conversion is different.

Well, besides the removal of idx it's all just removing stuff that
bio_clone_bioset() does for it.

> > diff --git a/drivers/md/dm.c b/drivers/md/dm.c
> > index 3cc2169..3e33039 100644
> > --- a/drivers/md/dm.c
> > +++ b/drivers/md/dm.c
> > @@ -1072,26 +1072,19 @@ static struct bio *split_bvec(struct bio *bio, sector_t sector,
> > * Creates a bio that consists of range of complete bvecs.
> > */
> > static struct bio *clone_bio(struct bio *bio, sector_t sector,
> > - unsigned short idx, unsigned short bv_count,
> > + unsigned short bv_count,
> > unsigned int len, struct bio_set *bs)
> > {
> > struct bio *clone;
> >
> > - clone = bio_alloc_bioset(GFP_NOIO, bio->bi_max_vecs, bs);
> > - __bio_clone(clone, bio);
> > + clone = bio_clone_bioset(bio, GFP_NOIO, bs);
> > clone->bi_sector = sector;
> > - clone->bi_idx = idx;
> > - clone->bi_vcnt = idx + bv_count;
> > + clone->bi_vcnt = bv_count;
> > clone->bi_size = to_bytes(len);
> > - clone->bi_flags &= ~(1 << BIO_SEG_VALID);
>
> Maybe removal of @idx deserves a separate patch?

I'm gonna back that out, I'm not sure it was correct now.

> > - if (bio_integrity(bio)) {
> > - bio_integrity_clone(clone, bio, GFP_NOIO, bs);
> > -
> > - if (idx != bio->bi_idx || clone->bi_size < bio->bi_size)
> > - bio_integrity_trim(clone,
> > - bio_sector_offset(bio, idx, 0), len);
> > - }
> > + if (bio_integrity(bio) &&
> > + clone->bi_size < bio->bi_size)
>
> Unnecessary line break.
>
> Other than that, looks good to me.
>
> Thanks.
>
> --
> tejun

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 05-24-2012, 12:02 AM
Kent Overstreet
 
Default block: Add bio_clone_bioset()

This consolidates some code, and will help in a later patch changing how
bio cloning works.

Signed-off-by: Kent Overstreet <koverstreet@google.com>
Change-Id: If6cfd840db9cf90a798c6a5862e3314465c83000
---
block/blk-core.c | 8 +-------
drivers/md/dm.c | 27 ++++++++++-----------------
drivers/md/md.c | 20 +-------------------
fs/bio.c | 16 ++++++++++++----
include/linux/bio.h | 1 +
5 files changed, 25 insertions(+), 47 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 1f61b74..91617eb 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -2660,16 +2660,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, bs))
- goto free_and_out;
-
if (bio_ctr && bio_ctr(bio, bio_src, data))
goto free_and_out;

diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index e6e7b19..e5076f2 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -1062,26 +1062,19 @@ static struct bio *split_bvec(struct bio *bio, sector_t sector,
* Creates a bio that consists of range of complete bvecs.
*/
static struct bio *clone_bio(struct bio *bio, sector_t sector,
- unsigned short idx, unsigned short bv_count,
+ unsigned short bv_count,
unsigned int len, struct bio_set *bs)
{
struct bio *clone;

- clone = bio_alloc_bioset(GFP_NOIO, bio->bi_max_vecs, bs);
- __bio_clone(clone, bio);
+ clone = bio_clone_bioset(bio, GFP_NOIO, bs);
clone->bi_sector = sector;
- clone->bi_idx = idx;
- clone->bi_vcnt = idx + bv_count;
+ clone->bi_vcnt = bv_count;
clone->bi_size = to_bytes(len);
- clone->bi_flags &= ~(1 << BIO_SEG_VALID);

- if (bio_integrity(bio)) {
- bio_integrity_clone(clone, bio, GFP_NOIO, bs);
-
- if (idx != bio->bi_idx || clone->bi_size < bio->bi_size)
- bio_integrity_trim(clone,
- bio_sector_offset(bio, idx, 0), len);
- }
+ if (bio_integrity(bio) &&
+ clone->bi_size < bio->bi_size)
+ bio_integrity_trim(clone, 0, len);

return clone;
}
@@ -1111,8 +1104,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);
@@ -1151,7 +1144,7 @@ static void __clone_and_map_simple(struct clone_info *ci, struct dm_target *ti)
struct dm_target_io *tio;

tio = alloc_tio(ci, ti);
- clone = clone_bio(bio, ci->sector, ci->idx,
+ clone = clone_bio(bio, ci->sector,
bio->bi_vcnt - ci->idx, ci->sector_count,
ci->md->bs);
__map_bio(ti, clone, tio);
@@ -1230,7 +1223,7 @@ static int __clone_and_map(struct clone_info *ci)
}

tio = alloc_tio(ci, ti);
- clone = clone_bio(bio, ci->sector, ci->idx, i - ci->idx, len,
+ clone = clone_bio(bio, ci->sector, i - ci->idx, len,
ci->md->bs);
__map_bio(ti, clone, tio);

diff --git a/drivers/md/md.c b/drivers/md/md.c
index a5a524e..508ae81 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, mddev->bio_set);
-
- 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 3667cef..4cb621f 100644
--- a/fs/bio.c
+++ b/fs/bio.c
@@ -453,15 +453,17 @@ 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 = bio_alloc_bioset(gfp_mask, bio->bi_max_vecs, bs);

if (!b)
return NULL;
@@ -471,7 +473,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, bs);

if (ret < 0) {
bio_put(b);
@@ -481,6 +483,12 @@ struct bio *bio_clone(struct bio *bio, gfp_t gfp_mask)

return b;
}
+EXPORT_SYMBOL(bio_clone_bioset);
+
+struct bio *bio_clone(struct bio *bio, gfp_t gfp_mask)
+{
+ return bio_clone_bioset(bio, gfp_mask, fs_bio_set);
+}
EXPORT_SYMBOL(bio_clone);

/**
diff --git a/include/linux/bio.h b/include/linux/bio.h
index 4d94eb8..e068a68 100644
--- a/include/linux/bio.h
+++ b/include/linux/bio.h
@@ -223,6 +223,7 @@ 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_bioset(struct bio *, gfp_t, struct bio_set *bs);
extern struct bio *bio_clone(struct bio *, gfp_t);

extern void bio_init(struct bio *);
--
1.7.9.3.327.g2980b

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 05-24-2012, 04:38 PM
Tejun Heo
 
Default block: Add bio_clone_bioset()

On Wed, May 23, 2012 at 05:02:40PM -0700, Kent Overstreet wrote:
> This consolidates some code, and will help in a later patch changing how
> bio cloning works.

Which consolidations are happening and which drivers are being
affected how?

> diff --git a/drivers/md/dm.c b/drivers/md/dm.c
> index e6e7b19..e5076f2 100644
> --- a/drivers/md/dm.c
> +++ b/drivers/md/dm.c
> @@ -1062,26 +1062,19 @@ static struct bio *split_bvec(struct bio *bio, sector_t sector,
> * Creates a bio that consists of range of complete bvecs.
> */
> static struct bio *clone_bio(struct bio *bio, sector_t sector,
> - unsigned short idx, unsigned short bv_count,
> + unsigned short bv_count,
> unsigned int len, struct bio_set *bs)
> {
> struct bio *clone;
>
> - clone = bio_alloc_bioset(GFP_NOIO, bio->bi_max_vecs, bs);
> - __bio_clone(clone, bio);
> + clone = bio_clone_bioset(bio, GFP_NOIO, bs);
> clone->bi_sector = sector;
> - clone->bi_idx = idx;
> - clone->bi_vcnt = idx + bv_count;
> + clone->bi_vcnt = bv_count;
> clone->bi_size = to_bytes(len);
> - clone->bi_flags &= ~(1 << BIO_SEG_VALID);

Why is this safe? If it's only bioset related API changes, why are
there other changes at all? If the new clone interface can handle
bioset fine, do we still need to expose __bio_clone()?

> - if (bio_integrity(bio)) {
> - bio_integrity_clone(clone, bio, GFP_NOIO, bs);
> -
> - if (idx != bio->bi_idx || clone->bi_size < bio->bi_size)
> - bio_integrity_trim(clone,
> - bio_sector_offset(bio, idx, 0), len);
> - }
> + if (bio_integrity(bio) &&
> + clone->bi_size < bio->bi_size)
> + bio_integrity_trim(clone, 0, len);

Why is idx != bi_idx test dropped?

I'm gonna stop here on this series. It doesn't seem like the issues
pointed out before have been addressed. I recommend spending more
effort on patch descriptions. Writing descriptions is not only
important for reviewing and history but it's a good step in ensuring
the patches are sane and properly split. If you can't explain each
change in the patch, it generally means either the changes themselves
are wrong or wrongly split.

Thanks.

--
tejun

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 05-24-2012, 06:45 PM
Vivek Goyal
 
Default block: Add bio_clone_bioset()

On Wed, May 23, 2012 at 05:02:40PM -0700, Kent Overstreet wrote:

[..]
> --- a/drivers/md/dm.c
> +++ b/drivers/md/dm.c
> @@ -1062,26 +1062,19 @@ static struct bio *split_bvec(struct bio *bio, sector_t sector,
> * Creates a bio that consists of range of complete bvecs.
> */
> static struct bio *clone_bio(struct bio *bio, sector_t sector,
> - unsigned short idx, unsigned short bv_count,
> + unsigned short bv_count,
> unsigned int len, struct bio_set *bs)
> {
> struct bio *clone;
>
> - clone = bio_alloc_bioset(GFP_NOIO, bio->bi_max_vecs, bs);
> - __bio_clone(clone, bio);
> + clone = bio_clone_bioset(bio, GFP_NOIO, bs);
> clone->bi_sector = sector;
> - clone->bi_idx = idx;
> - clone->bi_vcnt = idx + bv_count;
> + clone->bi_vcnt = bv_count;

In last posting you said that you are going to backout this idx change
because you were not sure if it is correct.

Thanks
Vivek

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 05-24-2012, 11:35 PM
Kent Overstreet
 
Default block: Add bio_clone_bioset()

On Thu, May 24, 2012 at 02:45:07PM -0400, Vivek Goyal wrote:
> On Wed, May 23, 2012 at 05:02:40PM -0700, Kent Overstreet wrote:
>
> [..]
> > --- a/drivers/md/dm.c
> > +++ b/drivers/md/dm.c
> > @@ -1062,26 +1062,19 @@ static struct bio *split_bvec(struct bio *bio, sector_t sector,
> > * Creates a bio that consists of range of complete bvecs.
> > */
> > static struct bio *clone_bio(struct bio *bio, sector_t sector,
> > - unsigned short idx, unsigned short bv_count,
> > + unsigned short bv_count,
> > unsigned int len, struct bio_set *bs)
> > {
> > struct bio *clone;
> >
> > - clone = bio_alloc_bioset(GFP_NOIO, bio->bi_max_vecs, bs);
> > - __bio_clone(clone, bio);
> > + clone = bio_clone_bioset(bio, GFP_NOIO, bs);
> > clone->bi_sector = sector;
> > - clone->bi_idx = idx;
> > - clone->bi_vcnt = idx + bv_count;
> > + clone->bi_vcnt = bv_count;
>
> In last posting you said that you are going to backout this idx change
> because you were not sure if it is correct.

Yeah, I distinctly remember doing that but clearly something got lost.
Sorry.

Anyways, that change shouldn't have been part of that patch, but
something has to change there for the "Only clone bio vecs that are in
use" patch.

Gonna take a stab at reworking that code, it really ought to be possible
to just replace it with my bio splitting code but the way dm is doing
the splitting is... confusing.

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 05-25-2012, 08:25 PM
Kent Overstreet
 
Default block: Add bio_clone_bioset()

This consolidates some code, and will help in a later patch changing how
bio cloning works.

Signed-off-by: Kent Overstreet <koverstreet@google.com>
Change-Id: If6cfd840db9cf90a798c6a5862e3314465c83000
---
block/blk-core.c | 8 +-------
drivers/md/dm.c | 4 ++--
drivers/md/md.c | 20 +-------------------
fs/bio.c | 16 ++++++++++++----
include/linux/bio.h | 1 +
5 files changed, 17 insertions(+), 32 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 1f61b74..91617eb 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -2660,16 +2660,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, bs))
- goto free_and_out;
-
if (bio_ctr && bio_ctr(bio, bio_src, data))
goto free_and_out;

diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index 4014696..3f3c26e 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -1101,8 +1101,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 a5a524e..508ae81 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, mddev->bio_set);
-
- 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 d212ee2..a62b5b6 100644
--- a/fs/bio.c
+++ b/fs/bio.c
@@ -459,15 +459,17 @@ 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 = bio_alloc_bioset(gfp_mask, bio->bi_max_vecs, bs);

if (!b)
return NULL;
@@ -477,7 +479,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, bs);

if (ret < 0) {
bio_put(b);
@@ -487,6 +489,12 @@ struct bio *bio_clone(struct bio *bio, gfp_t gfp_mask)

return b;
}
+EXPORT_SYMBOL(bio_clone_bioset);
+
+struct bio *bio_clone(struct bio *bio, gfp_t gfp_mask)
+{
+ return bio_clone_bioset(bio, gfp_mask, fs_bio_set);
+}
EXPORT_SYMBOL(bio_clone);

struct bio *bio_clone_kmalloc(struct bio *bio, gfp_t gfp_mask)
diff --git a/include/linux/bio.h b/include/linux/bio.h
index 8f280f7..0880a71 100644
--- a/include/linux/bio.h
+++ b/include/linux/bio.h
@@ -219,6 +219,7 @@ 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_bioset(struct bio *, gfp_t, struct bio_set *bs);
extern struct bio *bio_clone(struct bio *, gfp_t);
struct bio *bio_clone_kmalloc(struct bio *, gfp_t);

--
1.7.9.3.327.g2980b

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 

Thread Tools




All times are GMT. The time now is 07:27 AM.

VBulletin, Copyright ©2000 - 2014, Jelsoft Enterprises Ltd.
Content Relevant URLs by vBSEO ©2007, Crawlability, Inc.
Copyright 2007 - 2008, www.linux-archive.org