block: Kill bi_destructor
 

From: Kent Overstreet <koverstreet@google.com>

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

Signed-off-by: Kent Overstreet <koverstreet@google.com>
---
Documentation/block/biodoc.txt | 5 -----
fs/bio.c | 15 +++++----------
include/linux/blk_types.h | 3 ---
3 files changed, 5 insertions(+), 18 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/fs/bio.c b/fs/bio.c
index 90e4c3a..ecc9088 100644
--- a/fs/bio.c
+++ b/fs/bio.c
@@ -343,13 +343,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, fs_bio_set);
- kfree(bio);
-}
-
/**
* bio_kmalloc - allocate a bio for I/O using kmalloc()
* @gfp_mask: the GFP_ mask given to the slab allocator
@@ -376,7 +369,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;
}
@@ -417,8 +409,11 @@ void bio_put(struct bio *bio)

if (bio->bi_pool)
bio_free(bio, bio->bi_pool);
- else
- bio->bi_destructor(bio);
+ else {
+ if (bio_integrity(bio))
+ bio_integrity_free(bio, fs_bio_set);
+ kfree(bio);
+ }
}
}
EXPORT_SYMBOL(bio_put);
diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
index 4a47783..bd9a610 100644
--- a/include/linux/blk_types.h
+++ b/include/linux/blk_types.h
@@ -74,11 +74,8 @@ 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 */
-
/*
* 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.9.rc2

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

block: Kill bi_destructor
 

Hello,

> @@ -417,8 +409,11 @@ void bio_put(struct bio *bio)
>
> if (bio->bi_pool)
> bio_free(bio, bio->bi_pool);
> - else
> - bio->bi_destructor(bio);
> + else {
> + if (bio_integrity(bio))
> + bio_integrity_free(bio, fs_bio_set);
> + kfree(bio);

if {
} else {
}

And wouldn't it be better to make bio_free() handle kfreeing too?

Overall, I really like this change. I hate how ->bi_destructor() has
been used.

Thanks!

--
tejun

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

block: Kill bi_destructor
 

On Fri, May 18, 2012 at 09:21:42AM -0700, Tejun Heo wrote:
> Hello,
>
> > @@ -417,8 +409,11 @@ void bio_put(struct bio *bio)
> >
> > if (bio->bi_pool)
> > bio_free(bio, bio->bi_pool);
> > - else
> > - bio->bi_destructor(bio);
> > + else {
> > + if (bio_integrity(bio))
> > + bio_integrity_free(bio, fs_bio_set);
> > + kfree(bio);
>
> if {
> } else {
> }
>
> And wouldn't it be better to make bio_free() handle kfreeing too?

That'd kind of change the semantics of bio_free() - but I suppose it'd
make sense.

I'm not terribly happy with how the bio integrity data belongs to
fs_bio_set for kmalloced bios. Maybe I'll change that at some point.

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

block: Kill bi_destructor
 

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

Signed-off-by: Kent Overstreet <koverstreet@google.com>
Change-Id: Iade939e76ff329cb03eef9125e4377ba1e4265c5
---
Documentation/block/biodoc.txt | 5 -----
fs/bio.c | 25 +++++++++----------------
include/linux/blk_types.h | 3 ---
3 files changed, 9 insertions(+), 24 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/fs/bio.c b/fs/bio.c
index 7d8c29d..fc4a168 100644
--- a/fs/bio.c
+++ b/fs/bio.c
@@ -234,6 +234,13 @@ void bio_free(struct bio *bio, struct bio_set *bs)
{
void *p;

+ if (!bs) {
+ if (bio_integrity(bio))
+ bio_integrity_free(bio, fs_bio_set);
+ kfree(bio);
+ return;
+ }
+
if (bio_has_allocated_vec(bio))
bvec_free_bs(bs, bio->bi_io_vec, BIO_POOL_IDX(bio));

@@ -343,13 +350,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, fs_bio_set);
- kfree(bio);
-}
-
/**
* bio_kmalloc - allocate a bio for I/O using kmalloc()
* @gfp_mask: the GFP_ mask given to the slab allocator
@@ -376,7 +376,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;
}
@@ -412,14 +411,8 @@ void bio_put(struct bio *bio)
/*
* last put frees it
*/
- if (atomic_dec_and_test(&bio->bi_cnt)) {
- bio->bi_next = NULL;
-
- 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, bio->bi_pool);
}
EXPORT_SYMBOL(bio_put);

diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
index 6b7daf3..b6ddbf1 100644
--- a/include/linux/blk_types.h
+++ b/include/linux/blk_types.h
@@ -74,11 +74,8 @@ 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 */
-
/*
* 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.9.3.327.g2980b

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

block: Kill bi_destructor
 

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

[..]
> @@ -234,6 +234,13 @@ void bio_free(struct bio *bio, struct bio_set *bs)
> {
> void *p;
>
> + if (!bs) {
> + if (bio_integrity(bio))
> + bio_integrity_free(bio, fs_bio_set);
> + kfree(bio);
> + return;
> + }
> +

Ok, this seems to be the code which will take care of freeing kmalloced
bio. I think putting little comment about the explicit assumption is not
a bad idea.

Somehow we need to integrate two patches so that we don't have memory leak
in bisection and reading code becomes easier.

Also then what's the need of bio_reset() in previous patch. That seems to
be independent from getting rid of pkt_bio_destructor(). I would think
that keep we can split the patch and keep bio_reset() logic in a separate
patch. In fact I am not even sure that for one driver we should introduce
bio_reset() in generic block layer. So to me we should get rid of bio_reset()
and let all the gory details remain in driver.

Thanks
Vivek

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

block: Kill bi_destructor
 

On Thu, May 24, 2012 at 03:52:02PM -0400, Vivek Goyal wrote:
> On Wed, May 23, 2012 at 05:02:45PM -0700, Kent Overstreet wrote:
>
> [..]
> > @@ -234,6 +234,13 @@ void bio_free(struct bio *bio, struct bio_set *bs)
> > {
> > void *p;
> >
> > + if (!bs) {
> > + if (bio_integrity(bio))
> > + bio_integrity_free(bio, fs_bio_set);
> > + kfree(bio);
> > + return;
> > + }
> > +
>
> Ok, this seems to be the code which will take care of freeing kmalloced
> bio. I think putting little comment about the explicit assumption is not
> a bad idea.
>
> Somehow we need to integrate two patches so that we don't have memory leak
> in bisection and reading code becomes easier.
>
> Also then what's the need of bio_reset() in previous patch. That seems to
> be independent from getting rid of pkt_bio_destructor(). I would think
> that keep we can split the patch and keep bio_reset() logic in a separate
> patch. In fact I am not even sure that for one driver we should introduce
> bio_reset() in generic block layer. So to me we should get rid of bio_reset()
> and let all the gory details remain in driver.

Just noticed bio_kmalloc_destructor() will take care of freeing bio_kmalloc()
bios and it has been dropped in this patch. So memory leak point is moot.
bio_reset() question still remains though.

Thanks
Vivek

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

block: Kill bi_destructor
 

On Thu, May 24, 2012 at 03:52:02PM -0400, Vivek Goyal wrote:
> On Wed, May 23, 2012 at 05:02:45PM -0700, Kent Overstreet wrote:
>
> [..]
> > @@ -234,6 +234,13 @@ void bio_free(struct bio *bio, struct bio_set *bs)
> > {
> > void *p;
> >
> > + if (!bs) {
> > + if (bio_integrity(bio))
> > + bio_integrity_free(bio, fs_bio_set);
> > + kfree(bio);
> > + return;
> > + }
> > +
>
> Ok, this seems to be the code which will take care of freeing kmalloced
> bio. I think putting little comment about the explicit assumption is not
> a bad idea.

Yeah, it's changing the semantics of bio_free(). I'll document that.

> Somehow we need to integrate two patches so that we don't have memory leak
> in bisection and reading code becomes easier.

I don't think there's any memory leak issues with this patch... there
are various annoyances with the dm code, though.

> Also then what's the need of bio_reset() in previous patch. That seems to
> be independent from getting rid of pkt_bio_destructor(). I would think
> that keep we can split the patch and keep bio_reset() logic in a separate
> patch. In fact I am not even sure that for one driver we should introduce
> bio_reset() in generic block layer. So to me we should get rid of bio_reset()
> and let all the gory details remain in driver.

Well, it would be possible to kill bi_destructor without introducing
bio_reset() - but that'd mean the kill bi_destructor patch would have to
muck around in the pktcdvd code. IMO introducing bio_reset() makes the
rest of the patch series much cleaner.

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

block: Kill bi_destructor
 

On 05/24/2012 10:52 PM, Vivek Goyal wrote:

> In fact I am not even sure that for one driver we should introduce
> bio_reset() in generic block layer. So to me we should get rid of bio_reset()
> and let all the gory details remain in driver.
>


I disagree. The kind of trick we do where up to BIO_RESET_BYTES
the struct is memset **must** stay at header, very close to the
structure.

I would though make bio_reset() inline at header, it's a single
memset. Even the memset is inlined by the compiler.

> Thanks
> Vivek


Thanks
Boaz

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

block: Kill bi_destructor
 

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

This also changes the semantics of bio_free() a bit - it now also frees
bios allocated by bio_kmalloc(). It's also no longer exported, as
without bi_destructor there should be no need for it to be called
anywhere else.

Signed-off-by: Kent Overstreet <koverstreet@google.com>
Change-Id: Iade939e76ff329cb03eef9125e4377ba1e4265c5
---
Documentation/block/biodoc.txt | 5 -----
fs/bio.c | 33 +++++++++++++++------------------
include/linux/bio.h | 1 -
include/linux/blk_types.h | 3 ---
4 files changed, 15 insertions(+), 27 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/fs/bio.c b/fs/bio.c
index 240da21..4075171 100644
--- a/fs/bio.c
+++ b/fs/bio.c
@@ -230,10 +230,19 @@ fallback:
return bvl;
}

-void bio_free(struct bio *bio, struct bio_set *bs)
+static void bio_free(struct bio *bio)
{
+ struct bio_set *bs = bio->bi_pool;
void *p;

+ if (!bs) {
+ /* Bio was allocated by bio_kmalloc() */
+ if (bio_integrity(bio))
+ bio_integrity_free(bio, fs_bio_set);
+ kfree(bio);
+ return;
+ }
+
if (bio_has_allocated_vec(bio))
bvec_free_bs(bs, bio->bi_io_vec, BIO_POOL_IDX(bio));

@@ -249,7 +258,6 @@ void bio_free(struct bio *bio, struct bio_set *bs)

mempool_free(p, bs->bio_pool);
}
-EXPORT_SYMBOL(bio_free);

void bio_init(struct bio *bio)
{
@@ -343,13 +351,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, fs_bio_set);
- kfree(bio);
-}
-
/**
* bio_kmalloc - allocate a bio for I/O using kmalloc()
* @gfp_mask: the GFP_ mask given to the slab allocator
@@ -376,7 +377,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;
}
@@ -411,15 +411,12 @@ void bio_put(struct bio *bio)

/*
* last put frees it
+ *
+ * bio->bi_pool will be NULL if the bio was allocated by bio_kmalloc() -
+ * bio_free() handles that case too.
*/
- if (atomic_dec_and_test(&bio->bi_cnt)) {
- bio->bi_next = NULL;
-
- 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 e5c2504..242447c 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 6b7daf3..b6ddbf1 100644
--- a/include/linux/blk_types.h
+++ b/include/linux/blk_types.h
@@ -74,11 +74,8 @@ 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 */
-
/*
* 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.9.3.327.g2980b

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

block: Kill bi_destructor
 

On Fri, May 25, 2012 at 01:25:28PM -0700, Kent Overstreet wrote:
> Now that we've got generic code for freeing bios allocated from bio
> pools, this isn't needed anymore.
>
> This also changes the semantics of bio_free() a bit - it now also frees
> bios allocated by bio_kmalloc(). It's also no longer exported, as
> without bi_destructor there should be no need for it to be called
> anywhere else.

I like this patch but I'd *really* like to see the patch description
giving some background and explains *why* this is a good change.

> diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
> index 6b7daf3..b6ddbf1 100644
> --- a/include/linux/blk_types.h
> +++ b/include/linux/blk_types.h
> @@ -74,11 +74,8 @@ 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;

Maybe explain that %NULL bi_pool indicates kmalloc backed allocation?

--
tejun

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