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 dm: kill dm_rq_bio_destructor

From: Kent Overstreet <koverstreet@google.com>

Signed-off-by: Kent Overstreet <koverstreet@google.com>
---
drivers/md/dm.c | 11 +----------
1 files changed, 1 insertions(+), 10 deletions(-)

diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index 2114ec1..3cc2169 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -701,6 +701,7 @@ static void end_clone_bio(struct bio *clone, int error)
struct bio *bio = info->orig;
unsigned int nr_bytes = info->orig->bi_size;

+ free_bio_info(info);
bio_put(clone);

if (tio->error)
@@ -1448,15 +1449,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)
{
@@ -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
 
Old 05-18-2012, 03:57 PM
Tejun Heo
 
Default 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
 
Old 05-18-2012, 04:43 PM
Alasdair G Kergon
 
Default 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
 
Old 05-18-2012, 06:50 PM
Kent Overstreet
 
Default 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
 
Old 05-22-2012, 04:29 AM
"Jun'ichi Nomura"
 
Default 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
 
Old 05-24-2012, 12:02 AM
Kent Overstreet
 
Default dm: kill dm_rq_bio_destructor

Signed-off-by: Kent Overstreet <koverstreet@google.com>
---
drivers/md/dm.c | 11 +----------
1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index 40b7735..e6e7b19 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -696,6 +696,7 @@ static void end_clone_bio(struct bio *clone, int error)
struct bio *bio = info->orig;
unsigned int nr_bytes = info->orig->bi_size;

+ free_bio_info(info);
bio_put(clone);

if (tio->error)
@@ -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;

return 0;
}
--
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, 12:19 AM
"Jun'ichi Nomura"
 
Default dm: kill dm_rq_bio_destructor

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.

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
 
Old 05-24-2012, 12:39 AM
Kent Overstreet
 
Default 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
 
Old 05-24-2012, 01:16 AM
"Jun'ichi Nomura"
 
Default 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
 
Old 05-24-2012, 01:39 AM
"Jun'ichi Nomura"
 
Default 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;

@@ -2687,7 +2691,7 @@
free_and_out:
if (bio)
bio_free(bio, bs);
- blk_rq_unprep_clone(rq);
+ blk_rq_unprep_clone(rq, bio_dtr);

return -ENOMEM;
}
Index: linux-3.4/drivers/md/dm.c
================================================== =================
--- linux-3.4.orig/drivers/md/dm.c 2012-05-24 11:12:52.000000000 +0900
+++ linux-3.4/drivers/md/dm.c 2012-05-24 11:24:06.325803254 +0900
@@ -701,6 +701,7 @@
struct bio *bio = info->orig;
unsigned int nr_bytes = info->orig->bi_size;

+ free_bio_info(info);
bio_put(clone);

if (tio->error)
@@ -763,11 +764,12 @@
dm_put(md);
}

+static void dm_rq_bio_destructor(struct bio *bio);
static void free_rq_clone(struct request *clone)
{
struct dm_rq_target_io *tio = clone->end_io_data;

- blk_rq_unprep_clone(clone);
+ blk_rq_unprep_clone(clone, dm_rq_bio_destructor);
free_rq_tio(tio);
}

@@ -1461,10 +1463,8 @@
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,
@@ -1481,7 +1481,6 @@
info->tio = tio;
bio->bi_end_io = end_clone_bio;
bio->bi_private = info;
- bio->bi_destructor = dm_rq_bio_destructor;

return 0;
}
@@ -1492,7 +1491,7 @@
int r;

r = blk_rq_prep_clone(clone, rq, tio->md->bs, GFP_ATOMIC,
- dm_rq_bio_constructor, tio);
+ dm_rq_bio_constructor, dm_rq_bio_destructor, tio);
if (r)
return r;

Index: linux-3.4/include/linux/blkdev.h
================================================== =================
--- linux-3.4.orig/include/linux/blkdev.h 2012-05-21 07:29:13.000000000 +0900
+++ linux-3.4/include/linux/blkdev.h 2012-05-24 11:20:53.455808958 +0900
@@ -678,8 +678,8 @@
extern 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);
-extern void blk_rq_unprep_clone(struct request *rq);
+ void (*bio_dtr)(struct bio *), void *data);
+extern void blk_rq_unprep_clone(struct request *rq, void (*bio_dtr)(struct bio *));
extern int blk_insert_cloned_request(struct request_queue *q,
struct request *rq);
extern void blk_delay_queue(struct request_queue *, unsigned long);

--
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 05:55 AM.

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