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-28-2012, 01:21 AM
Tejun Heo
 
Default dm: Use bioset's front_pad for dm_rq_clone_bio_info

On Fri, May 25, 2012 at 01:25:25PM -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.

To what goal are we doing this? How is it tested?

--
tejun

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 08-08-2012, 10:06 PM
Tejun Heo
 
Default dm: Use bioset's front_pad for dm_rq_clone_bio_info

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@google.com>
> ---
> 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.

Can someone more familiar review this one? Alasdir, Mike?

Also, how was this tested?

Thanks.

--
tejun

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 08-08-2012, 11:57 PM
Kent Overstreet
 
Default dm: Use bioset's front_pad for dm_rq_clone_bio_info

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@google.com>
> > ---
> > 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

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 08-13-2012, 09:44 PM
Kent Overstreet
 
Default dm: Use bioset's front_pad for dm_rq_clone_bio_info

On Sat, Aug 11, 2012 at 03:24:45PM +1000, Joseph Glanville wrote:
> Hi Kent, Tejun
>
> On 9 August 2012 09:57, Kent Overstreet <koverstreet@google.com> wrote:
> >> 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.
>
> Multipath can be tested quite easily with a loopback scsi target, you
> don't require specialized hardware.
> The easiest way to do this would probably be the built in LIO target +
> open_iscsi initiator.
>
> I haven't attempted running this current version of the patch series
> but I haven't run into issues with bcache+multipath in the past.

Actually, I bet you have been running this code - do you think you could
check the git log for the version you're running? That'd be awesome if
you are.

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 08-15-2012, 08:46 PM
Kent Overstreet
 
Default dm: Use bioset's front_pad for dm_rq_clone_bio_info

On Tue, Aug 14, 2012 at 02:33:20PM +0900, Jun'ichi Nomura wrote:
> On 08/07/12 07:08, Kent Overstreet wrote:
> > struct dm_rq_clone_bio_info {
> > struct bio *orig;
> > struct dm_rq_target_io *tio;
> > + struct bio clone;
> > };
> ...
> > - 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;
>
> Shouldn't this be offsetof(struct dm_rq_clone_bio_info, clone)?

Ouch! Yes, it definitely should be. Good catch, and thanks.

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 08-22-2012, 06:32 PM
Tejun Heo
 
Default dm: Use bioset's front_pad for dm_rq_clone_bio_info

On Wed, Aug 22, 2012 at 10:03:59AM -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.
>
> v6: Fix comment on struct dm_rq_clone_bio_info, per Tejun
>
> Signed-off-by: Kent Overstreet <koverstreet@google.com>
> CC: Alasdair Kergon <agk@redhat.com>

Acked-by: Tejun Heo <tj@kernel.org>

It would be nice if dm people can review these patches tho.

Thanks.

--
tejun

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 08-22-2012, 09:30 PM
Vivek Goyal
 
Default dm: Use bioset's front_pad for dm_rq_clone_bio_info

On Wed, Aug 22, 2012 at 10:03:59AM -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.
>
> v6: Fix comment on struct dm_rq_clone_bio_info, per Tejun
>
> Signed-off-by: Kent Overstreet <koverstreet@google.com>
> CC: Alasdair Kergon <agk@redhat.com>
> ---
> drivers/md/dm.c | 39 +++++++++++----------------------------
> 1 file changed, 11 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/md/dm.c b/drivers/md/dm.c
> index 0c3d6dd..5ed9779 100644
> --- a/drivers/md/dm.c
> +++ b/drivers/md/dm.c
> @@ -86,12 +86,17 @@ struct dm_rq_target_io {
> };
>
> /*
> - * For request-based dm.
> - * One of these is allocated per bio.
> + * 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;
> };
>
> union map_info *dm_get_mapinfo(struct bio *bio)
> @@ -467,16 +472,6 @@ static void free_rq_tio(struct dm_rq_target_io *tio)
> mempool_free(tio, tio->md->tio_pool);
> }
>
> -static struct dm_rq_clone_bio_info *alloc_bio_info(struct mapped_device *md)
> -{
> - return mempool_alloc(md->io_pool, GFP_ATOMIC);
> -}
> -
> -static void free_bio_info(struct dm_rq_clone_bio_info *info)
> -{
> - mempool_free(info, info->tio->md->io_pool);
> -}
> -

With this change, do you still need "_rq_bio_info_cache" slab cache? I would
think that it can be cleaned up now?

Thanks
Vivek

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 08-24-2012, 07:14 AM
Kent Overstreet
 
Default dm: Use bioset's front_pad for dm_rq_clone_bio_info

On Wed, Aug 22, 2012 at 05:30:10PM -0400, Vivek Goyal wrote:
> On Wed, Aug 22, 2012 at 10:03:59AM -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.
> >
> > v6: Fix comment on struct dm_rq_clone_bio_info, per Tejun
> >
> > Signed-off-by: Kent Overstreet <koverstreet@google.com>
> > CC: Alasdair Kergon <agk@redhat.com>
> > ---
> > drivers/md/dm.c | 39 +++++++++++----------------------------
> > 1 file changed, 11 insertions(+), 28 deletions(-)
> >
> > diff --git a/drivers/md/dm.c b/drivers/md/dm.c
> > index 0c3d6dd..5ed9779 100644
> > --- a/drivers/md/dm.c
> > +++ b/drivers/md/dm.c
> > @@ -86,12 +86,17 @@ struct dm_rq_target_io {
> > };
> >
> > /*
> > - * For request-based dm.
> > - * One of these is allocated per bio.
> > + * 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;
> > };
> >
> > union map_info *dm_get_mapinfo(struct bio *bio)
> > @@ -467,16 +472,6 @@ static void free_rq_tio(struct dm_rq_target_io *tio)
> > mempool_free(tio, tio->md->tio_pool);
> > }
> >
> > -static struct dm_rq_clone_bio_info *alloc_bio_info(struct mapped_device *md)
> > -{
> > - return mempool_alloc(md->io_pool, GFP_ATOMIC);
> > -}
> > -
> > -static void free_bio_info(struct dm_rq_clone_bio_info *info)
> > -{
> > - mempool_free(info, info->tio->md->io_pool);
> > -}
> > -
>
> With this change, do you still need "_rq_bio_info_cache" slab cache? I would
> think that it can be cleaned up now?

It looks like it, but I'm hesitent to make more extensive changes to the
dm code given that I'm unfamiliar with it and I haven't been able to
personally test the request type dm target code.

That and the way io_pool is overloaded. I see too many ways I could
screw things up.

Also it looks like the equivalent change ought to be done with struct
dm_io first (then we'd have removed all the users of io_pool), but
honestly it takes me forever to do anything in the dm code so I'd rather
leave that to someone else.

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 08-24-2012, 06:40 PM
Vivek Goyal
 
Default dm: Use bioset's front_pad for dm_rq_clone_bio_info

On Fri, Aug 24, 2012 at 12:14:48AM -0700, Kent Overstreet wrote:

[..]
> > > -static struct dm_rq_clone_bio_info *alloc_bio_info(struct mapped_device *md)
> > > -{
> > > - return mempool_alloc(md->io_pool, GFP_ATOMIC);
> > > -}
> > > -
> > > -static void free_bio_info(struct dm_rq_clone_bio_info *info)
> > > -{
> > > - mempool_free(info, info->tio->md->io_pool);
> > > -}
> > > -
> >
> > With this change, do you still need "_rq_bio_info_cache" slab cache? I would
> > think that it can be cleaned up now?
>
> It looks like it, but I'm hesitent to make more extensive changes to the
> dm code given that I'm unfamiliar with it and I haven't been able to
> personally test the request type dm target code.
>
> That and the way io_pool is overloaded. I see too many ways I could
> screw things up.

I understand your concern but still if you leave it behind, job is
half done. You moved rq_bio_info in bio front padding but left the
associated cache and mempool behind. I would say we need to clean
it up and then get ACK from dm/md folks.

I am looking at the code and one thing which is not clear to me is
__bind_mempools() which assumes that md->io_pool is always set. With
your change md->io_pool is set only for BIO based targets and not
request based targets. So that will need some tidying up.

Testing of request based target should be easy. Just enable multipath
for your sata disk.


>
> Also it looks like the equivalent change ought to be done with struct
> dm_io first (then we'd have removed all the users of io_pool), but
> honestly it takes me forever to do anything in the dm code so I'd rather
> leave that to someone else.

I think we can leave io_pool behind. Just that it remains null for
request based targets.

Thanks
Vivek

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 09-06-2012, 10:28 PM
Kent Overstreet
 
Default dm: Use bioset's front_pad for dm_rq_clone_bio_info

On Thu, Sep 06, 2012 at 12:21:15PM +0900, Jun'ichi Nomura wrote:
> On 09/06/12 05:27, Kent Overstreet wrote:
> > @@ -2718,7 +2705,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, clone));
> > if (!pools->bs)
> > goto free_tio_pool_and_out;
>
> frontpad is not necessary if type is DM_TYPE_BIO_BASED.
>
> Other pool creation in that function do something like:
> pools->bs = (type == DM_TYPE_BIO_BASED) ?
> bioset_create(pool_size, 0) :
> bioset_create(pool_size, offsetof(struct dm_rq_clone_bio_info, clone));
>

Eh, it doesn't really matter considering it's two pointers of padding
and struct bio + the inline vecs are something like 200 bytes, but I can
do it if it makes you happy. Can I get someone's acked-by?

--
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 09:10 AM.

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