Re: [dm-devel] [PATCH 2/2] block: Avoid deadlocks with bio allocation by stacking drivers
On Mon, Sep 10, 2012 at 03:09:10PM -0700, Tejun Heo wrote: > On Mon, Sep 10, 2012 at 02:56:33PM -0700, Kent Overstreet wrote: > > commit df7e63cbffa3065fcc4ba2b9a93418d7c7312243 > > Author: Kent Overstreet > > Date: Mon Sep 10 14:33:46 2012 -0700 > > > > block: Avoid deadlocks with bio allocation by stacking drivers > > Note that this doesn't do anything for allocation from other mempools. Note that dm has several cases of this, so this patch should not be used with dm yet. Mikulas is studying those cases to see whether anything like this might be feasible/sensible or not. > I'm still a bit scared but think this is correct. > Acked-by: Tejun Heo Alasdair -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [dm-devel] [PATCH 2/2] block: Avoid deadlocks with bio allocation by stacking drivers
Hello, On Mon, Sep 10, 2012 at 3:50 PM, Alasdair G Kergon wrote: >> > Note that this doesn't do anything for allocation from other mempools. > > Note that dm has several cases of this, so this patch should not be used with > dm yet. Mikulas is studying those cases to see whether anything like this > might be feasible/sensible or not. IIUC, Kent posted a patch which converts all of them to use front-pad (there's no reason not to, really). This better come after that but it's not like this is gonna break something which isn't broken now. Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [dm-devel] [PATCH 2/2] block: Avoid deadlocks with bio allocation by stacking drivers
On Mon, Sep 10, 2012 at 11:50:57PM +0100, Alasdair G Kergon wrote:
> On Mon, Sep 10, 2012 at 03:09:10PM -0700, Tejun Heo wrote:
> > On Mon, Sep 10, 2012 at 02:56:33PM -0700, Kent Overstreet wrote:
> > > commit df7e63cbffa3065fcc4ba2b9a93418d7c7312243
> > > Author: Kent Overstreet
> > > Date: Mon Sep 10 14:33:46 2012 -0700
> > >
> > > block: Avoid deadlocks with bio allocation by stacking drivers
>
> > > Note that this doesn't do anything for allocation from other mempools.
>
> Note that dm has several cases of this, so this patch should not be used with
> dm yet.
That just means it won't affect dm one way or the other for those
allocations.
> Mikulas is studying those cases to see whether anything like this
> might be feasible/sensible or not.
I've got a patch that eliminates one of the per bio mempools in dm, and
I'll probably work on the rest after I finish off with immutable biovecs
- which is mostly done, just cleaning up/testing/pushing patches in now.
commit 8754349145edfc791450d3ad54c19f0f3715c86c
Author: Kent Overstreet
Date: Tue Sep 4 06:17:56 2012 -0700
dm: Use bioset's front_pad for dm_target_io
diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index f2eb730..3cf39b0 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -71,6 +71,7 @@ struct dm_target_io {
struct dm_io *io;
struct dm_target *ti;
union map_info info;
+ struct bio clone;
};
/*
@@ -174,7 +175,7 @@ struct mapped_device {
* io objects are allocated from here.
*/
mempool_t *io_pool;
- mempool_t *tio_pool;
+ mempool_t *rq_tio_pool;
struct bio_set *bs;
@@ -214,15 +215,8 @@ struct dm_md_mempools {
#define MIN_IOS 256
static struct kmem_cache *_io_cache;
-static struct kmem_cache *_tio_cache;
static struct kmem_cache *_rq_tio_cache;
-/*
- * Unused now, and needs to be deleted. But since io_pool is overloaded and
it's
- * still used for _io_cache, I'm leaving this for a later cleanup
- */
-static struct kmem_cache *_rq_bio_info_cache;
-
static int __init local_init(void)
{
int r = -ENOMEM;
@@ -232,22 +226,13 @@ static int __init local_init(void)
if (!_io_cache)
return r;
- /* allocate a slab for the target ios */
- _tio_cache = KMEM_CACHE(dm_target_io, 0);
- if (!_tio_cache)
- goto out_free_io_cache;
-
_rq_tio_cache = KMEM_CACHE(dm_rq_target_io, 0);
if (!_rq_tio_cache)
- goto out_free_tio_cache;
-
- _rq_bio_info_cache = KMEM_CACHE(dm_rq_clone_bio_info, 0);
- if (!_rq_bio_info_cache)
- goto out_free_rq_tio_cache;
+ goto out_free_io_cache;
r = dm_uevent_init();
if (r)
- goto out_free_rq_bio_info_cache;
+ goto out_free_rq_tio_cache;
_major = major;
r = register_blkdev(_major, _name);
@@ -261,12 +246,8 @@ static int __init local_init(void)
out_uevent_exit:
dm_uevent_exit();
-out_free_rq_bio_info_cache:
- kmem_cache_destroy(_rq_bio_info_cache);
out_free_rq_tio_cache:
kmem_cache_destroy(_rq_tio_cache);
-out_free_tio_cache:
- kmem_cache_destroy(_tio_cache);
out_free_io_cache:
kmem_cache_destroy(_io_cache);
@@ -275,9 +256,7 @@ out_free_io_cache:
static void local_exit(void)
{
- kmem_cache_destroy(_rq_bio_info_cache);
kmem_cache_destroy(_rq_tio_cache);
- kmem_cache_destroy(_tio_cache);
kmem_cache_destroy(_io_cache);
unregister_blkdev(_major, _name);
dm_uevent_exit();
@@ -461,20 +440,15 @@ static void free_io(struct mapped_device *md, struct
dm_io *io)
mempool_free(io, md->io_pool);
}
-static void free_tio(struct mapped_device *md, struct dm_target_io *tio)
-{
- mempool_free(tio, md->tio_pool);
-}
-
static struct dm_rq_target_io *alloc_rq_tio(struct mapped_device *md,
gfp_t gfp_mask)
{
- return mempool_alloc(md->tio_pool, gfp_mask);
+ return mempool_alloc(md->rq_tio_pool, gfp_mask);
}
static void free_rq_tio(struct dm_rq_target_io *tio)
{
- mempool_free(tio, tio->md->tio_pool);
+ mempool_free(tio, tio->md->rq_tio_pool);
}
static int md_in_flight(struct mapped_device *md)
@@ -658,7 +632,6 @@ static void clone_endio(struct bio *bio, int error)
int r = 0;
struct dm_target_io *tio = bio->bi_private;
struct dm_io *io = tio->io;
- struct mapped_device *md = tio->io->md;
dm_endio_fn endio = tio->ti->type->end_io;
if (!bio_flagged(bio, BIO_UPTODATE) && !error)
@@ -681,7 +654,6 @@ static void clone_endio(struct bio *bio, int error)
}
}
- free_tio(md, tio);
bio_put(bio);
dec_pending(io, error);
}
@@ -998,13 +970,16 @@ int dm_set_target_max_io_len(struct dm_target *ti,
sector_t len)
}
EXPORT_SYMBOL_GPL(dm_set_target_max_io_len);
-static void __map_bio(struct dm_t
Re: [dm-devel] [PATCH 2/2] block: Avoid deadlocks with bio allocation by stacking drivers
On Mon, Sep 10, 2012 at 04:01:01PM -0700, Tejun Heo wrote: > Hello, > > On Mon, Sep 10, 2012 at 3:50 PM, Alasdair G Kergon wrote: > >> > Note that this doesn't do anything for allocation from other > >> > mempools. > > > > Note that dm has several cases of this, so this patch should not be used > > with > > dm yet. Mikulas is studying those cases to see whether anything like this > > might be feasible/sensible or not. > > IIUC, Kent posted a patch which converts all of them to use front-pad > (there's no reason not to, really). This better come after that but > it's not like this is gonna break something which isn't broken now. Not all, I only did the easy one - you know how dm has all those crazy abstraction layers? They've got multiple per bio allocations because of that; the core dm code does one, and then some other code takes that struct dm_io* and allocates its own state pointing to that (which then points to the original bio...) So front_pad should still work, but you need to have say dm_crypt pass the amount of front pad it needs to the core dm code when it creates the bio_set, and then dm crypt can use container_of(struct dm_io) and embed like everything does that use the bio_set front pad. *I'm probably misremembering all the names. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [dm-devel] [PATCH 2/2] block: Avoid deadlocks with bio allocation by stacking drivers
On Mon, Sep 10, 2012 at 04:01:01PM -0700, Tejun Heo wrote: > IIUC, Kent posted a patch which converts all of them to use front-pad The only patch I saw posted here only handles one of the easier cases so far. The others are a bit trickier and probably involve a decision about which way to change an in-kernel dm interface. Alasdair -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [dm-devel] [PATCH 2/2] block: Avoid deadlocks with bio allocation by stacking drivers
Hello, Alasdair. On Tue, Sep 11, 2012 at 12:09:17AM +0100, Alasdair G Kergon wrote: > On Mon, Sep 10, 2012 at 04:01:01PM -0700, Tejun Heo wrote: > > IIUC, Kent posted a patch which converts all of them to use front-pad > > The only patch I saw posted here only handles one of the easier cases so far. > > The others are a bit trickier and probably involve a decision about which way > to > change an in-kernel dm interface. Ah, I see. Thought that was full conversion. I still think this is the right direction to be headed tho. Using multiple mempools from the stacking drivers is way too fragile and difficult to verify and debug. Would it be difficult to convert dm drivers to collect size requirements and use front-pad for all per-bio data? Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [dm-devel] [PATCH 2/2] block: Avoid deadlocks with bio allocation by stacking drivers
On Mon, Sep 10, 2012 at 04:35:02PM -0700, Tejun Heo wrote: > debug. Would it be difficult to convert dm drivers to collect size > requirements and use front-pad for all per-bio data? I can't give a quick answer because a single bio may require a variable number (depending on the bio content) of (sequential) mempool_allocs from the same mempool to complete its processing, so we do have to audit all this very carefully to see what can/can't be pulled out. Alasdair -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
