-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)
{
@@ -1471,7 +1463,6 @@ static int dm_rq_bio_constructor(struct bio *bio, struct bio *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;
}
--
1.7.9.rc2
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
05-18-2012, 03:57 PM
Tejun Heo
dm: kill dm_rq_bio_destructor
On Thu, May 17, 2012 at 10:59:49PM -0400, koverstreet@google.com wrote:
> From: Kent Overstreet <koverstreet@google.com>
>
> Signed-off-by: Kent Overstreet <koverstreet@google.com>
Please explain why this is done and how it's safe. Alasdair / dm
folks, can you please ack this?
Thanks.
--
tejun
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
05-18-2012, 04:43 PM
Alasdair G Kergon
dm: kill dm_rq_bio_destructor
On Fri, May 18, 2012 at 08:57:29AM -0700, Tejun Heo wrote:
> Please explain why this is done and how it's safe. Alasdair / dm
> folks, can you please ack this?
I think it's relying on there being never more than one reference on those
bios so that that endio fn, called exactly once, always frees it and there
are no dm_puts elsewhere.
Alasdair
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
05-18-2012, 06:50 PM
Kent Overstreet
dm: kill dm_rq_bio_destructor
On Fri, May 18, 2012 at 05:43:19PM +0100, Alasdair G Kergon wrote:
> On Fri, May 18, 2012 at 08:57:29AM -0700, Tejun Heo wrote:
> > Please explain why this is done and how it's safe. Alasdair / dm
> > folks, can you please ack this?
>
> I think it's relying on there being never more than one reference on those
> bios so that that endio fn, called exactly once, always frees it and there
> are no dm_puts elsewhere.
Is that a safe assumption? From my perusal of the code it certainly
looks like it should be, but I don't know dm all that well.
Seems like it might be better to use bio set's front_pad to put it in
the same allocation as the bio, but I don't really want to get into that
myself.
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
05-22-2012, 04:29 AM
"Jun'ichi Nomura"
dm: kill dm_rq_bio_destructor
Hi,
On 05/19/12 03:50, Kent Overstreet wrote:
> On Fri, May 18, 2012 at 05:43:19PM +0100, Alasdair G Kergon wrote:
>> On Fri, May 18, 2012 at 08:57:29AM -0700, Tejun Heo wrote:
>>> Please explain why this is done and how it's safe. Alasdair / dm
>>> folks, can you please ack this?
>>
>> I think it's relying on there being never more than one reference on those
>> bios so that that endio fn, called exactly once, always frees it and there
>> are no dm_puts elsewhere.
>
> Is that a safe assumption? From my perusal of the code it certainly
> looks like it should be, but I don't know dm all that well.
Doing free_bio_info() in end_clone_bio() is safe.
But there is other problem.
This bio may be put by blk_rq_unprep_clone() and leak memory
without the destructor.
So could it be possible to keep bi_destructor available for this case?
Thanks,
--
Jun'ichi Nomura, NEC Corporation
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
The destructor may also be called from blk_rq_unprep_clone(),
which just puts bio.
So this patch will introduce a memory leak.
Please check this comment as well:
https://www.redhat.com/archives/dm-devel/2012-May/msg00216.html
Thanks,
--
Jun'ichi Nomura, NEC Corporation
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
05-24-2012, 12:39 AM
Kent Overstreet
dm: kill dm_rq_bio_destructor
On Thu, May 24, 2012 at 09:19:12AM +0900, Jun'ichi Nomura wrote:
> Hi,
>
> On 05/24/12 09:02, Kent Overstreet wrote:
> > @@ -1438,15 +1439,6 @@ 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)
> > {
> > @@ -1461,7 +1453,6 @@ static int dm_rq_bio_constructor(struct bio *bio, struct bio *bio_orig,
> > info->tio = tio;
> > bio->bi_end_io = end_clone_bio;
> > bio->bi_private = info;
> > - bio->bi_destructor = dm_rq_bio_destructor;
>
> The destructor may also be called from blk_rq_unprep_clone(),
> which just puts bio.
> So this patch will introduce a memory leak.
Well, keeping around bi_destructor solely for that reason would be
pretty lousy. Can you come up with a better solution?
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
05-24-2012, 01:16 AM
"Jun'ichi Nomura"
dm: kill dm_rq_bio_destructor
On 05/24/12 09:39, Kent Overstreet wrote:
> On Thu, May 24, 2012 at 09:19:12AM +0900, Jun'ichi Nomura wrote:
>> The destructor may also be called from blk_rq_unprep_clone(),
>> which just puts bio.
>> So this patch will introduce a memory leak.
>
> Well, keeping around bi_destructor solely for that reason would be
> pretty lousy. Can you come up with a better solution?
I don't have good one but here are some ideas:
a) Do bio_endio() rather than bio_put() in blk_rq_unprep_clone()
and let bi_end_io reap additional data.
It looks ugly.
b) Separate the constructor from blk_rq_prep_clone().
dm has to do rq_for_each_bio loop again for constructor.
Possible performance impact.
c) Open code blk_rq_prep/unprep_clone() in dm.
It exposes unnecessary block-internals to dm.
d) Pass destructor function to blk_rq_prep/unprep_clone()
for them to callback.
Umm, is "d)" better?
--
Jun'ichi Nomura, NEC Corporation
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
05-24-2012, 01:39 AM
"Jun'ichi Nomura"
dm: kill dm_rq_bio_destructor
On 05/24/12 10:16, Jun'ichi Nomura wrote:
> On 05/24/12 09:39, Kent Overstreet wrote:
>> On Thu, May 24, 2012 at 09:19:12AM +0900, Jun'ichi Nomura wrote:
>>> The destructor may also be called from blk_rq_unprep_clone(),
>>> which just puts bio.
>>> So this patch will introduce a memory leak.
>>
>> Well, keeping around bi_destructor solely for that reason would be
>> pretty lousy. Can you come up with a better solution?
>
> I don't have good one but here are some ideas:
> a) Do bio_endio() rather than bio_put() in blk_rq_unprep_clone()
> and let bi_end_io reap additional data.
> It looks ugly.
> b) Separate the constructor from blk_rq_prep_clone().
> dm has to do rq_for_each_bio loop again for constructor.
> Possible performance impact.
> c) Open code blk_rq_prep/unprep_clone() in dm.
> It exposes unnecessary block-internals to dm.
> d) Pass destructor function to blk_rq_prep/unprep_clone()
> for them to callback.
>
> Umm, is "d)" better?
I mean the patch like this:
Index: linux-3.4/block/blk-core.c
================================================== =================
--- linux-3.4.orig/block/blk-core.c 2012-05-21 07:29:13.000000000 +0900
+++ linux-3.4/block/blk-core.c 2012-05-24 11:15:40.562817784 +0900
@@ -2596,17 +2596,20 @@
/**
* blk_rq_unprep_clone - Helper function to free all bios in a cloned request
* @rq: the clone request to be cleaned up
+ * @bio_dtr: cleanup function to be called for each clone bio.
*
* Description:
* Free all bios in @rq for a cloned request.
*/
-void blk_rq_unprep_clone(struct request *rq)
+void blk_rq_unprep_clone(struct request *rq, void (*bio_dtr)(struct bio *))
{
struct bio *bio;
while ((bio = rq->bio) != NULL) {
rq->bio = bio->bi_next;
+ if (bio_dtr)
+ bio_dtr(bio);
bio_put(bio);
}
}
@@ -2636,6 +2639,7 @@
* @gfp_mask: memory allocation mask for bio
* @bio_ctr: setup function to be called for each clone bio.
* Returns %0 for success, non %0 for failure.
+ * @bio_dtr: cleanup function to be called for each clone bio.
* @data: private data to be passed to @bio_ctr
*
* Description:
@@ -2650,7 +2654,7 @@
int blk_rq_prep_clone(struct request *rq, struct request *rq_src,
struct bio_set *bs, gfp_t gfp_mask,
int (*bio_ctr)(struct bio *, struct bio *, void *),
- void *data)
+ void (*bio_dtr)(struct bio *), void *data)
{
struct bio *bio, *bio_src;
dm: kill dm_rq_bio_destructor
