Re: [PATCH 2/2] block: Avoid deadlocks with bio allocation by stacking drivers

2012-09-11 Thread Muthu Kumar
Kent, On Tue, Sep 11, 2012 at 12:31 PM, Kent Overstreet wrote: > On Tue, Sep 11, 2012 at 11:58:05AM -0700, Muthu Kumar wrote: >> On Tue, Sep 11, 2012 at 11:45 AM, Tejun Heo wrote: >> > Hello, >> > >> > On Tue, Sep 11, 2012 at 11:36:28AM -0700, Muthu Kumar wrote: >> >> Does this preserve the CPU

Re: [PATCH 2/2] block: Avoid deadlocks with bio allocation by stacking drivers

2012-09-11 Thread Kent Overstreet
On Tue, Sep 11, 2012 at 11:58:05AM -0700, Muthu Kumar wrote: > On Tue, Sep 11, 2012 at 11:45 AM, Tejun Heo wrote: > > Hello, > > > > On Tue, Sep 11, 2012 at 11:36:28AM -0700, Muthu Kumar wrote: > >> Does this preserve the CPU from which the bio was submitted > >> originally. Not familiar with

Re: [PATCH 2/2] block: Avoid deadlocks with bio allocation by stacking drivers

2012-09-11 Thread Muthu Kumar
On Tue, Sep 11, 2012 at 11:45 AM, Tejun Heo wrote: > Hello, > > On Tue, Sep 11, 2012 at 11:36:28AM -0700, Muthu Kumar wrote: >> Does this preserve the CPU from which the bio was submitted >> originally. Not familiar with cmwq, may be Tejun can clarify. >> >> Tejun - the question is, do we honor

Re: [PATCH 2/2] block: Avoid deadlocks with bio allocation by stacking drivers

2012-09-11 Thread Tejun Heo
Hello, On Tue, Sep 11, 2012 at 11:36:28AM -0700, Muthu Kumar wrote: > Does this preserve the CPU from which the bio was submitted > originally. Not familiar with cmwq, may be Tejun can clarify. > > Tejun - the question is, do we honor the rq_affinity with the above > rescue worker

Re: [PATCH 2/2] block: Avoid deadlocks with bio allocation by stacking drivers

2012-09-11 Thread Muthu Kumar
Kent, On Mon, Sep 10, 2012 at 2:56 PM, Kent Overstreet wrote: .. .. > > +static void bio_alloc_rescue(struct work_struct *work) > +{ > + struct bio_set *bs = container_of(work, struct bio_set, rescue_work); > + struct bio *bio; > + > + while (1) { > +

Re: [PATCH 2/2] block: Avoid deadlocks with bio allocation by stacking drivers

2012-09-11 Thread Muthu Kumar
Kent, On Mon, Sep 10, 2012 at 2:56 PM, Kent Overstreet koverstr...@google.com wrote: .. snip .. +static void bio_alloc_rescue(struct work_struct *work) +{ + struct bio_set *bs = container_of(work, struct bio_set, rescue_work); + struct bio *bio; + + while (1) { +

Re: [PATCH 2/2] block: Avoid deadlocks with bio allocation by stacking drivers

2012-09-11 Thread Tejun Heo
Hello, On Tue, Sep 11, 2012 at 11:36:28AM -0700, Muthu Kumar wrote: Does this preserve the CPU from which the bio was submitted originally. Not familiar with cmwq, may be Tejun can clarify. Tejun - the question is, do we honor the rq_affinity with the above rescue worker implementation?

Re: [PATCH 2/2] block: Avoid deadlocks with bio allocation by stacking drivers

2012-09-11 Thread Muthu Kumar
On Tue, Sep 11, 2012 at 11:45 AM, Tejun Heo t...@kernel.org wrote: Hello, On Tue, Sep 11, 2012 at 11:36:28AM -0700, Muthu Kumar wrote: Does this preserve the CPU from which the bio was submitted originally. Not familiar with cmwq, may be Tejun can clarify. Tejun - the question is, do we

Re: [PATCH 2/2] block: Avoid deadlocks with bio allocation by stacking drivers

2012-09-11 Thread Kent Overstreet
On Tue, Sep 11, 2012 at 11:58:05AM -0700, Muthu Kumar wrote: On Tue, Sep 11, 2012 at 11:45 AM, Tejun Heo t...@kernel.org wrote: Hello, On Tue, Sep 11, 2012 at 11:36:28AM -0700, Muthu Kumar wrote: Does this preserve the CPU from which the bio was submitted originally. Not familiar with

Re: [PATCH 2/2] block: Avoid deadlocks with bio allocation by stacking drivers

2012-09-11 Thread Muthu Kumar
Kent, On Tue, Sep 11, 2012 at 12:31 PM, Kent Overstreet koverstr...@google.com wrote: On Tue, Sep 11, 2012 at 11:58:05AM -0700, Muthu Kumar wrote: On Tue, Sep 11, 2012 at 11:45 AM, Tejun Heo t...@kernel.org wrote: Hello, On Tue, Sep 11, 2012 at 11:36:28AM -0700, Muthu Kumar wrote: Does

Re: [dm-devel] [PATCH 2/2] block: Avoid deadlocks with bio allocation by stacking drivers

2012-09-10 Thread Alasdair G Kergon
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

Re: [dm-devel] [PATCH 2/2] block: Avoid deadlocks with bio allocation by stacking drivers

2012-09-10 Thread Tejun Heo
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

Re: [PATCH 2/2] block: Avoid deadlocks with bio allocation by stacking drivers

2012-09-10 Thread Tejun Heo
Hello, again. On Mon, Sep 10, 2012 at 03:09:10PM -0700, Tejun Heo wrote: > I'm still a bit scared but think this is correct. > > Acked-by: Tejun Heo > > One last thing is that we may want to add @name on bioset creation so > that we can name the workqueue properly but that's for another

Re: [dm-devel] [PATCH 2/2] block: Avoid deadlocks with bio allocation by stacking drivers

2012-09-10 Thread Alasdair G Kergon
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

Re: [dm-devel] [PATCH 2/2] block: Avoid deadlocks with bio allocation by stacking drivers

2012-09-10 Thread Kent Overstreet
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

Re: [dm-devel] [PATCH 2/2] block: Avoid deadlocks with bio allocation by stacking drivers

2012-09-10 Thread Kent Overstreet
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:

Re: [dm-devel] [PATCH 2/2] block: Avoid deadlocks with bio allocation by stacking drivers

2012-09-10 Thread Tejun Heo
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

Re: [dm-devel] [PATCH 2/2] block: Avoid deadlocks with bio allocation by stacking drivers

2012-09-10 Thread Alasdair G Kergon
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

Re: [PATCH 2/2] block: Avoid deadlocks with bio allocation by stacking drivers

2012-09-10 Thread Tejun Heo
Hello, Kent. 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 > > Previously, if

Re: [PATCH 2/2] block: Avoid deadlocks with bio allocation by stacking drivers

2012-09-10 Thread Kent Overstreet
On Mon, Sep 10, 2012 at 02:37:10PM -0700, Tejun Heo wrote: > On Mon, Sep 10, 2012 at 02:33:49PM -0700, Kent Overstreet wrote: > > "Simpler" isn't really an objective thing though. To me the goto version > > is more obvious/idiomatic. > > > > Eh. I'll do it your way, but consider this a formal

Re: [PATCH 2/2] block: Avoid deadlocks with bio allocation by stacking drivers

2012-09-10 Thread Tejun Heo
On Mon, Sep 10, 2012 at 02:33:49PM -0700, Kent Overstreet wrote: > "Simpler" isn't really an objective thing though. To me the goto version > is more obvious/idiomatic. > > Eh. I'll do it your way, but consider this a formal objection :p Thanks. :) -- tejun -- To unsubscribe from this list:

Re: [PATCH 2/2] block: Avoid deadlocks with bio allocation by stacking drivers

2012-09-10 Thread Kent Overstreet
On Mon, Sep 10, 2012 at 01:40:10PM -0700, Tejun Heo wrote: > Hello, Kent. > > On Mon, Sep 10, 2012 at 01:24:35PM -0700, Kent Overstreet wrote: > > And at that point, why duplicate that line of code? It doesn't matter that > > much, but IMO a goto retry better labels what's actually going on (it's

Re: [PATCH 2/2] block: Avoid deadlocks with bio allocation by stacking drivers

2012-09-10 Thread Tejun Heo
Hello, Kent. On Mon, Sep 10, 2012 at 01:24:35PM -0700, Kent Overstreet wrote: > And at that point, why duplicate that line of code? It doesn't matter that > much, but IMO a goto retry better labels what's actually going on (it's > something that's not uncommon in the kernel and if I see a retry

Re: [PATCH 2/2] block: Avoid deadlocks with bio allocation by stacking drivers

2012-09-10 Thread Kent Overstreet
On Mon, Sep 10, 2012 at 10:22:10AM -0700, Tejun Heo wrote: > Hello, Kent. > > On Sun, Sep 09, 2012 at 05:28:10PM -0700, Kent Overstreet wrote: > > > > + while ((bio = bio_list_pop(current->bio_list))) > > > > + bio_list_add(bio->bi_pool == bs ? : , bio); > > > > + > > > > +

Re: [PATCH 2/2] block: Avoid deadlocks with bio allocation by stacking drivers

2012-09-10 Thread Tejun Heo
Hello, Kent. On Sun, Sep 09, 2012 at 05:28:10PM -0700, Kent Overstreet wrote: > > > + while ((bio = bio_list_pop(current->bio_list))) > > > + bio_list_add(bio->bi_pool == bs ? : , bio); > > > + > > > + *current->bio_list = nopunt; > > > > Why this is necessary needs explanation and it's

Re: [PATCH 2/2] block: Avoid deadlocks with bio allocation by stacking drivers

2012-09-10 Thread Vivek Goyal
On Sun, Sep 09, 2012 at 05:28:10PM -0700, Kent Overstreet wrote: [..] > > > +retry: > > > p = mempool_alloc(bs->bio_pool, gfp_mask); > > > front_pad = bs->front_pad; > > > inline_vecs = BIO_INLINE_VECS; > > > } > > > > Wouldn't the following be better? > > > >

Re: [PATCH 2/2] block: Avoid deadlocks with bio allocation by stacking drivers

2012-09-10 Thread Vivek Goyal
On Sun, Sep 09, 2012 at 05:28:10PM -0700, Kent Overstreet wrote: [..] +retry: p = mempool_alloc(bs-bio_pool, gfp_mask); front_pad = bs-front_pad; inline_vecs = BIO_INLINE_VECS; } Wouldn't the following be better? p =

Re: [PATCH 2/2] block: Avoid deadlocks with bio allocation by stacking drivers

2012-09-10 Thread Tejun Heo
Hello, Kent. On Sun, Sep 09, 2012 at 05:28:10PM -0700, Kent Overstreet wrote: + while ((bio = bio_list_pop(current-bio_list))) + bio_list_add(bio-bi_pool == bs ? punt : nopunt, bio); + + *current-bio_list = nopunt; Why this is necessary needs explanation and it's done in

Re: [PATCH 2/2] block: Avoid deadlocks with bio allocation by stacking drivers

2012-09-10 Thread Kent Overstreet
On Mon, Sep 10, 2012 at 10:22:10AM -0700, Tejun Heo wrote: Hello, Kent. On Sun, Sep 09, 2012 at 05:28:10PM -0700, Kent Overstreet wrote: + while ((bio = bio_list_pop(current-bio_list))) + bio_list_add(bio-bi_pool == bs ? punt : nopunt, bio); + +

Re: [PATCH 2/2] block: Avoid deadlocks with bio allocation by stacking drivers

2012-09-10 Thread Tejun Heo
Hello, Kent. On Mon, Sep 10, 2012 at 01:24:35PM -0700, Kent Overstreet wrote: And at that point, why duplicate that line of code? It doesn't matter that much, but IMO a goto retry better labels what's actually going on (it's something that's not uncommon in the kernel and if I see a retry

Re: [PATCH 2/2] block: Avoid deadlocks with bio allocation by stacking drivers

2012-09-10 Thread Kent Overstreet
On Mon, Sep 10, 2012 at 01:40:10PM -0700, Tejun Heo wrote: Hello, Kent. On Mon, Sep 10, 2012 at 01:24:35PM -0700, Kent Overstreet wrote: And at that point, why duplicate that line of code? It doesn't matter that much, but IMO a goto retry better labels what's actually going on (it's

Re: [PATCH 2/2] block: Avoid deadlocks with bio allocation by stacking drivers

2012-09-10 Thread Tejun Heo
On Mon, Sep 10, 2012 at 02:33:49PM -0700, Kent Overstreet wrote: Simpler isn't really an objective thing though. To me the goto version is more obvious/idiomatic. Eh. I'll do it your way, but consider this a formal objection :p Thanks. :) -- tejun -- To unsubscribe from this list: send the

Re: [PATCH 2/2] block: Avoid deadlocks with bio allocation by stacking drivers

2012-09-10 Thread Kent Overstreet
On Mon, Sep 10, 2012 at 02:37:10PM -0700, Tejun Heo wrote: On Mon, Sep 10, 2012 at 02:33:49PM -0700, Kent Overstreet wrote: Simpler isn't really an objective thing though. To me the goto version is more obvious/idiomatic. Eh. I'll do it your way, but consider this a formal objection :p

Re: [PATCH 2/2] block: Avoid deadlocks with bio allocation by stacking drivers

2012-09-10 Thread Tejun Heo
Hello, Kent. On Mon, Sep 10, 2012 at 02:56:33PM -0700, Kent Overstreet wrote: commit df7e63cbffa3065fcc4ba2b9a93418d7c7312243 Author: Kent Overstreet koverstr...@google.com Date: Mon Sep 10 14:33:46 2012 -0700 block: Avoid deadlocks with bio allocation by stacking drivers

Re: [dm-devel] [PATCH 2/2] block: Avoid deadlocks with bio allocation by stacking drivers

2012-09-10 Thread Alasdair G Kergon
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 koverstr...@google.com Date: Mon Sep 10 14:33:46 2012 -0700 block: Avoid deadlocks

Re: [dm-devel] [PATCH 2/2] block: Avoid deadlocks with bio allocation by stacking drivers

2012-09-10 Thread Tejun Heo
Hello, On Mon, Sep 10, 2012 at 3:50 PM, Alasdair G Kergon a...@redhat.com 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

Re: [dm-devel] [PATCH 2/2] block: Avoid deadlocks with bio allocation by stacking drivers

2012-09-10 Thread Kent Overstreet
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 koverstr...@google.com

Re: [dm-devel] [PATCH 2/2] block: Avoid deadlocks with bio allocation by stacking drivers

2012-09-10 Thread Kent Overstreet
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 a...@redhat.com 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

Re: [dm-devel] [PATCH 2/2] block: Avoid deadlocks with bio allocation by stacking drivers

2012-09-10 Thread Alasdair G Kergon
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

Re: [PATCH 2/2] block: Avoid deadlocks with bio allocation by stacking drivers

2012-09-10 Thread Tejun Heo
Hello, again. On Mon, Sep 10, 2012 at 03:09:10PM -0700, Tejun Heo wrote: I'm still a bit scared but think this is correct. Acked-by: Tejun Heo t...@kernel.org One last thing is that we may want to add @name on bioset creation so that we can name the workqueue properly but that's for

Re: [dm-devel] [PATCH 2/2] block: Avoid deadlocks with bio allocation by stacking drivers

2012-09-10 Thread Tejun Heo
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.

Re: [dm-devel] [PATCH 2/2] block: Avoid deadlocks with bio allocation by stacking drivers

2012-09-10 Thread Alasdair G Kergon
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

Re: [PATCH 2/2] block: Avoid deadlocks with bio allocation by stacking drivers

2012-09-09 Thread Kent Overstreet
On Sat, Sep 08, 2012 at 12:36:41PM -0700, Tejun Heo wrote: > (Restoring cc list from the previous discussion. Please retain the cc > list of the people who discussed in the previous postings.) > > On Fri, Sep 07, 2012 at 03:12:53PM -0700, Kent Overstreet wrote: > > But this is tricky and not a

Re: [PATCH 2/2] block: Avoid deadlocks with bio allocation by stacking drivers

2012-09-09 Thread Kent Overstreet
On Sat, Sep 08, 2012 at 12:36:41PM -0700, Tejun Heo wrote: (Restoring cc list from the previous discussion. Please retain the cc list of the people who discussed in the previous postings.) On Fri, Sep 07, 2012 at 03:12:53PM -0700, Kent Overstreet wrote: But this is tricky and not a generic

Re: [PATCH 2/2] block: Avoid deadlocks with bio allocation by stacking drivers

2012-09-08 Thread Tejun Heo
(Restoring cc list from the previous discussion. Please retain the cc list of the people who discussed in the previous postings.) On Fri, Sep 07, 2012 at 03:12:53PM -0700, Kent Overstreet wrote: > But this is tricky and not a generic solution. This patch solves it for > all users by inverting

Re: [PATCH 2/2] block: Avoid deadlocks with bio allocation by stacking drivers

2012-09-08 Thread Tejun Heo
(Restoring cc list from the previous discussion. Please retain the cc list of the people who discussed in the previous postings.) On Fri, Sep 07, 2012 at 03:12:53PM -0700, Kent Overstreet wrote: But this is tricky and not a generic solution. This patch solves it for all users by inverting the

[PATCH 2/2] block: Avoid deadlocks with bio allocation by stacking drivers

2012-09-07 Thread Kent Overstreet
Previously, if we ever try to allocate more than once from the same bio set while running under generic_make_request() (i.e. a stacking block driver), we risk deadlock. This is because of the code in generic_make_request() that converts recursion to iteration; any bios we submit won't actually be

[PATCH 2/2] block: Avoid deadlocks with bio allocation by stacking drivers

2012-09-07 Thread Kent Overstreet
Previously, if we ever try to allocate more than once from the same bio set while running under generic_make_request() (i.e. a stacking block driver), we risk deadlock. This is because of the code in generic_make_request() that converts recursion to iteration; any bios we submit won't actually be