Re: [PATCH v5 02/12] dm: Use bioset's front_pad for dm_rq_clone_bio_info

[Kent Overstreet <koverstreet-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>]

On Wed, Aug 08, 2012 at 03:06:12PM -0700, Tejun Heo wrote:
> Hello,
> 
> On Mon, Aug 06, 2012 at 03:08:31PM -0700, Kent Overstreet wrote:
> > Previously, dm_rq_clone_bio_info needed to be freed by the bio's
> > destructor to avoid a memory leak in the blk_rq_prep_clone() error path.
> > This gets rid of a memory allocation and means we can kill
> > dm_rq_bio_destructor.
> > 
> > Signed-off-by: Kent Overstreet <koverstreet-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> > ---
> >  drivers/md/dm.c |   31 +++++--------------------------
> >  1 files changed, 5 insertions(+), 26 deletions(-)
> > 
> > diff --git a/drivers/md/dm.c b/drivers/md/dm.c
> > index 40b7735..4014696 100644
> > --- a/drivers/md/dm.c
> > +++ b/drivers/md/dm.c
> > @@ -92,6 +92,7 @@ struct dm_rq_target_io {
> >  struct dm_rq_clone_bio_info {
> >  	struct bio *orig;
> >  	struct dm_rq_target_io *tio;
> > +	struct bio clone;
> >  };
> ...
> > @@ -2696,7 +2674,8 @@ struct dm_md_mempools *dm_alloc_md_mempools(unsigned type, unsigned integrity)
> >  	if (!pools->tio_pool)
> >  		goto free_io_pool_and_out;
> >  
> > -	pools->bs = bioset_create(pool_size, 0);
> > +	pools->bs = bioset_create(pool_size,
> > +				  offsetof(struct dm_rq_clone_bio_info, orig));
> >  	if (!pools->bs)
> >  		goto free_tio_pool_and_out;
> 
> I do like this approach much better but this isn't something
> super-obvious.  Can we please explain what's going on?  Especially,
> the comment above dm_rq_clone_bio_info is outright misleading now.

This look better to you?

/*
 * For request-based dm - the bio clones we allocate are embedded in these
 * structs.
 *
 * We allocate these with bio_alloc_bioset, using the front_pad parameter when
 * the bioset is created - this means the bio has to come at the end of the
 * struct.
 */
struct dm_rq_clone_bio_info {
	struct bio *orig;
	struct dm_rq_target_io *tio;
	struct bio clone;
};

> Can someone more familiar review this one?  Alasdir, Mike?
> 
> Also, how was this tested?

Well, AFAICT the only request based dm target is multipath, and from the
documentation I've seen it doesn't appear to work without multipath
hardware, or at least I haven't seen it documented how. So, unless
there's another user I missed it's not been tested.

> 
> Thanks.
> 
> -- 
> tejun