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);
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
