* [git pull] device-mapper fixes for 2.6.29
@ 2009-03-16 19:06 ` Alasdair G Kergon
0 siblings, 0 replies; 12+ messages in thread
From: Alasdair G Kergon @ 2009-03-16 19:06 UTC (permalink / raw)
To: Linus Torvalds
Cc: dm-devel, linux-kernel, Herbert Xu, Huang Ying, Jonathan Brassow,
Kiyoshi Ueda, Mikulas Patocka, Milan Broz, Andrew Morton
Please pull from:
master.kernel.org:/pub/scm/linux/kernel/git/agk/linux-2.6-dm.git
to get the following device-mapper fixes for 2.6.29:
Huang Ying (1):
dm crypt: fix kcryptd_async_done parameter
Mikulas Patocka (2):
dm table: rework reference counting fix
dm io: respect BIO_MAX_PAGES limit
Milan Broz (2):
dm ioctl: validate name length when renaming
dm crypt: wait for endio to complete before destruction
drivers/md/dm-crypt.c | 43 +++++++++++++++++++++++++++++++------------
drivers/md/dm-io.c | 2 ++
drivers/md/dm-ioctl.c | 7 ++++---
drivers/md/dm.c | 32 +++++++++++++++++++-------------
4 files changed, 56 insertions(+), 28 deletions(-)
Thanks,
Alasdair
--
agk@redhat.com
^ permalink raw reply [flat|nested] 12+ messages in thread
* [git pull] device-mapper fixes for 2.6.29
@ 2009-03-16 19:06 ` Alasdair G Kergon
0 siblings, 0 replies; 12+ messages in thread
From: Alasdair G Kergon @ 2009-03-16 19:06 UTC (permalink / raw)
To: Linus Torvalds
Cc: Kiyoshi Ueda, Herbert Xu, linux-kernel, dm-devel,
Mikulas Patocka, Huang Ying, Andrew Morton, Milan Broz
Please pull from:
master.kernel.org:/pub/scm/linux/kernel/git/agk/linux-2.6-dm.git
to get the following device-mapper fixes for 2.6.29:
Huang Ying (1):
dm crypt: fix kcryptd_async_done parameter
Mikulas Patocka (2):
dm table: rework reference counting fix
dm io: respect BIO_MAX_PAGES limit
Milan Broz (2):
dm ioctl: validate name length when renaming
dm crypt: wait for endio to complete before destruction
drivers/md/dm-crypt.c | 43 +++++++++++++++++++++++++++++++------------
drivers/md/dm-io.c | 2 ++
drivers/md/dm-ioctl.c | 7 ++++---
drivers/md/dm.c | 32 +++++++++++++++++++-------------
4 files changed, 56 insertions(+), 28 deletions(-)
Thanks,
Alasdair
--
agk@redhat.com
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [git pull] device-mapper fixes for 2.6.29
2009-03-16 19:06 ` Alasdair G Kergon
(?)
@ 2009-03-17 13:25 ` Mikulas Patocka
2009-03-17 20:38 ` Alasdair G Kergon
-1 siblings, 1 reply; 12+ messages in thread
From: Mikulas Patocka @ 2009-03-17 13:25 UTC (permalink / raw)
To: Alasdair G Kergon; +Cc: dm-devel, Milan Broz
On Mon, 16 Mar 2009, Alasdair G Kergon wrote:
> Please pull from:
>
> master.kernel.org:/pub/scm/linux/kernel/git/agk/linux-2.6-dm.git
>
> to get the following device-mapper fixes for 2.6.29:
>
> Huang Ying (1):
> dm crypt: fix kcryptd_async_done parameter
>
> Mikulas Patocka (2):
> dm table: rework reference counting fix
> dm io: respect BIO_MAX_PAGES limit
Hi
Also look at:
dm-io crashes when signal is sent:
http://people.redhat.com/mpatocka/patches/kernel/misc/2.6.29-rc8/dm-dmio-fix-signal-crash.patch
that partial-read dm-raid1 + dm-multipath crash:
http://people.redhat.com/mpatocka/patches/kernel/misc/2.6.29-rc8/dm-record-bio-vector.patch
http://people.redhat.com/mpatocka/patches/kernel/misc/2.6.29-rc8/dm-raid1-read-record-slab.patch
crash or data corruption in snapshots (callbacks are called concurrently
and shouldn't be):
http://people.redhat.com/mpatocka/patches/kernel/misc/2.6.29-rc8/dm-kcopyd-fix.patch
failure to check pending exception after we dropped a lock:
http://people.redhat.com/mpatocka/patches/kernel/misc/2.6.29-rc8/dm-snap-break-__find_pending_exception.patch
http://people.redhat.com/mpatocka/patches/kernel/misc/2.6.29-rc8/dm-snap-move-allocation-to-the-caller.patch
http://people.redhat.com/mpatocka/patches/kernel/misc/2.6.29-rc8/dm-snap-check-complete-exceptions.patch
--- all the patches fix crashes or data corruptions.
--- all these patches except the first one were already committed to RHEL
4.8, so you should put them upstream, at least for testing.
Mikulas
> Milan Broz (2):
> dm ioctl: validate name length when renaming
> dm crypt: wait for endio to complete before destruction
>
> drivers/md/dm-crypt.c | 43 +++++++++++++++++++++++++++++++------------
> drivers/md/dm-io.c | 2 ++
> drivers/md/dm-ioctl.c | 7 ++++---
> drivers/md/dm.c | 32 +++++++++++++++++++-------------
> 4 files changed, 56 insertions(+), 28 deletions(-)
>
> Thanks,
> Alasdair
> --
> agk@redhat.com
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Re: [git pull] device-mapper fixes for 2.6.29
2009-03-17 13:25 ` Mikulas Patocka
@ 2009-03-17 20:38 ` Alasdair G Kergon
2009-03-18 23:13 ` Mikulas Patocka
2009-03-18 23:14 ` [PATCH 1/3] kcopyd+snapshots race condition Mikulas Patocka
0 siblings, 2 replies; 12+ messages in thread
From: Alasdair G Kergon @ 2009-03-17 20:38 UTC (permalink / raw)
To: Mikulas Patocka; +Cc: dm-devel
On Tue, Mar 17, 2009 at 09:25:30AM -0400, Mikulas Patocka wrote:
> http://people.redhat.com/mpatocka/patches/kernel/misc/2.6.29-rc8/dm-kcopyd-fix.patch
Needs submitting to patchwork - split into 4 patches perhaps?
The others are now queued for linux-next and possibly still 2.6.29.
Alasdair
--
agk@redhat.com
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Re: [git pull] device-mapper fixes for 2.6.29
2009-03-17 20:38 ` Alasdair G Kergon
@ 2009-03-18 23:13 ` Mikulas Patocka
2009-03-18 23:14 ` [PATCH 1/3] kcopyd+snapshots race condition Mikulas Patocka
1 sibling, 0 replies; 12+ messages in thread
From: Mikulas Patocka @ 2009-03-18 23:13 UTC (permalink / raw)
To: Alasdair G Kergon; +Cc: dm-devel
On Tue, 17 Mar 2009, Alasdair G Kergon wrote:
> On Tue, Mar 17, 2009 at 09:25:30AM -0400, Mikulas Patocka wrote:
> > http://people.redhat.com/mpatocka/patches/kernel/misc/2.6.29-rc8/dm-kcopyd-fix.patch
>
> Needs submitting to patchwork - split into 4 patches perhaps?
>
> The others are now queued for linux-next and possibly still 2.6.29.
>
> Alasdair
> --
> agk@redhat.com
I think it is already in patchwork, but maybe under some weird name (it
didn't have [PATCH] header when I sent it). Anyway, I'm resending it,
split to 3 pieces.
Mikulas
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 1/3] kcopyd+snapshots race condition
2009-03-17 20:38 ` Alasdair G Kergon
2009-03-18 23:13 ` Mikulas Patocka
@ 2009-03-18 23:14 ` Mikulas Patocka
2009-03-18 23:14 ` [PATCH 2/3] " Mikulas Patocka
` (2 more replies)
1 sibling, 3 replies; 12+ messages in thread
From: Mikulas Patocka @ 2009-03-18 23:14 UTC (permalink / raw)
To: Alasdair G Kergon; +Cc: dm-devel
Use variable to point to kcopyd client. This will be needed for further patches.
Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
---
drivers/md/dm-kcopyd.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
Index: linux-2.6.29-rc8-devel/drivers/md/dm-kcopyd.c
===================================================================
--- linux-2.6.29-rc8-devel.orig/drivers/md/dm-kcopyd.c 2009-03-18 20:27:00.000000000 +0100
+++ linux-2.6.29-rc8-devel/drivers/md/dm-kcopyd.c 2009-03-18 20:27:22.000000000 +0100
@@ -461,6 +461,7 @@ static void segment_complete(int read_er
sector_t progress = 0;
sector_t count = 0;
struct kcopyd_job *job = (struct kcopyd_job *) context;
+ struct dm_kcopyd_client *kc = job->kc;
mutex_lock(&job->lock);
@@ -490,7 +491,7 @@ static void segment_complete(int read_er
if (count) {
int i;
- struct kcopyd_job *sub_job = mempool_alloc(job->kc->job_pool,
+ struct kcopyd_job *sub_job = mempool_alloc(kc->job_pool,
GFP_NOIO);
*sub_job = *job;
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 2/3] kcopyd+snapshots race condition
2009-03-18 23:14 ` [PATCH 1/3] kcopyd+snapshots race condition Mikulas Patocka
@ 2009-03-18 23:14 ` Mikulas Patocka
2009-04-06 19:39 ` Alasdair G Kergon
2009-03-18 23:15 ` [PATCH 3/3] " Mikulas Patocka
2009-04-06 19:39 ` [PATCH 1/3] kcopyd+snapshots race condition Alasdair G Kergon
2 siblings, 1 reply; 12+ messages in thread
From: Mikulas Patocka @ 2009-03-18 23:14 UTC (permalink / raw)
To: Alasdair G Kergon; +Cc: dm-devel
Test for job->pages being zero. This will be needed for the next patch.
Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
---
drivers/md/dm-kcopyd.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
Index: linux-2.6.29-rc8-devel/drivers/md/dm-kcopyd.c
===================================================================
--- linux-2.6.29-rc8-devel.orig/drivers/md/dm-kcopyd.c 2009-03-18 20:39:07.000000000 +0100
+++ linux-2.6.29-rc8-devel/drivers/md/dm-kcopyd.c 2009-03-18 20:39:19.000000000 +0100
@@ -297,7 +297,8 @@ static int run_complete_job(struct kcopy
dm_kcopyd_notify_fn fn = job->fn;
struct dm_kcopyd_client *kc = job->kc;
- kcopyd_put_pages(kc, job->pages);
+ if (job->pages)
+ kcopyd_put_pages(kc, job->pages);
mempool_free(job, kc->job_pool);
fn(read_err, write_err, context);
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 3/3] kcopyd+snapshots race condition
2009-03-18 23:14 ` [PATCH 1/3] kcopyd+snapshots race condition Mikulas Patocka
2009-03-18 23:14 ` [PATCH 2/3] " Mikulas Patocka
@ 2009-03-18 23:15 ` Mikulas Patocka
2009-04-04 0:57 ` [PATCH 3/3] kcopyd+snapshots race condition ... updated Mikulas Patocka
2009-04-06 19:39 ` [PATCH 1/3] kcopyd+snapshots race condition Alasdair G Kergon
2 siblings, 1 reply; 12+ messages in thread
From: Mikulas Patocka @ 2009-03-18 23:15 UTC (permalink / raw)
To: Alasdair G Kergon; +Cc: dm-devel
Under specific circumstances, kcopyd callback could be called from
the thread that called dm_kcopyd_copy not from kcopyd workqueue.
dm_kcopyd_copy -> split_job -> segment_complete -> job->fn()
This code path is taken if thread calling dm_kcopyd_copy is delayed due to
scheduling inside split_job/segment_complete and the subjobs complete before
the loop in split_job completes.
Snapshots depend on the fact that callbacks are called from the singlethreaded
kcopyd workqueue and expect that there is no racing between individual
callbacks. The racing between callbacks can lead to corruption of exception
store and it can also cause that exception store callbacks are called twice
for the same exception --- a likely reason for crashes inside
pending_complete() / remove_exception().
When I reviewed kcopyd further, I found four total problems:
1. job->fn being called from the thread that submitted the job (see above).
2. job->fn(read_err, write_err, job->context); in segment_complete
reports the error of the last subjob, not the union of all errors.
3. This comment is bogus ---- the jobs are not cancelable. It is leftover from
some time when developers thought about adding cancel possibility.
/*
* To avoid a race we must keep the job around
* until after the notify function has completed.
* Otherwise the client may try and stop the job
* after we've completed.
*/
job->fn(read_err, write_err, job->context);
mempool_free(job, job->kc->job_pool);
4. Master jobs and subjobs are allocated from the same mempool.
Completion and freeing of a master job depends on successful allocation of
all subjobs -> deadlock.
This patch moves completion of master jobs to run_complete_job (being called
from the kcopyd workqueue). The patch fixes problems 1, 2, 3.
Problem 4 will need more changes and another patch.
Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
---
drivers/md/dm-kcopyd.c | 12 ++++--------
1 file changed, 4 insertions(+), 8 deletions(-)
Index: linux-2.6.29-rc8-devel/drivers/md/dm-kcopyd.c
===================================================================
--- linux-2.6.29-rc8-devel.orig/drivers/md/dm-kcopyd.c 2009-03-18 20:39:19.000000000 +0100
+++ linux-2.6.29-rc8-devel/drivers/md/dm-kcopyd.c 2009-03-18 20:40:26.000000000 +0100
@@ -510,14 +510,8 @@ static void segment_complete(int read_er
} else if (atomic_dec_and_test(&job->sub_jobs)) {
- /*
- * To avoid a race we must keep the job around
- * until after the notify function has completed.
- * Otherwise the client may try and stop the job
- * after we've completed.
- */
- job->fn(read_err, write_err, job->context);
- mempool_free(job, job->kc->job_pool);
+ push(&kc->complete_jobs, job);
+ wake(kc);
}
}
@@ -530,6 +524,8 @@ static void split_job(struct kcopyd_job
{
int i;
+ atomic_inc(&job->kc->nr_jobs);
+
atomic_set(&job->sub_jobs, SPLIT_COUNT);
for (i = 0; i < SPLIT_COUNT; i++)
segment_complete(0, 0u, job);
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 3/3] kcopyd+snapshots race condition ... updated
2009-03-18 23:15 ` [PATCH 3/3] " Mikulas Patocka
@ 2009-04-04 0:57 ` Mikulas Patocka
2009-04-06 19:40 ` Alasdair G Kergon
0 siblings, 1 reply; 12+ messages in thread
From: Mikulas Patocka @ 2009-04-04 0:57 UTC (permalink / raw)
To: Alasdair G Kergon; +Cc: dm-devel
Under specific circumstances, kcopyd callback could be called from
the thread that called dm_kcopyd_copy not from kcopyd workqueue.
dm_kcopyd_copy -> split_job -> segment_complete -> job->fn()
This code path is taken if thread calling dm_kcopyd_copy is delayed due to
scheduling inside split_job/segment_complete and the subjobs complete before
the loop in split_job completes.
Snapshots depend on the fact that callbacks are called from the singlethreaded
kcopyd workqueue and expect that there is no racing between individual
callbacks. The racing between callbacks can lead to corruption of exception
store and it can also cause that exception store callbacks are called twice
for the same exception --- a likely reason for crashes inside
pending_complete() / remove_exception().
This patch fixes two problems:
1. job->fn being called from the thread that submitted the job (see above).
- Fix: hand over the completion callback to the kcopyd thread.
2. job->fn(read_err, write_err, job->context); in segment_complete
reports the error of the last subjob, not the union of all errors.
- Fix: pass job->write_err to the callback to report all error bits
(it is done already in run_complete_job)
Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
---
drivers/md/dm-kcopyd.c | 17 +++++++++++------
1 file changed, 11 insertions(+), 6 deletions(-)
Index: linux-2.6.29-devel/drivers/md/dm-kcopyd.c
===================================================================
--- linux-2.6.29-devel.orig/drivers/md/dm-kcopyd.c 2009-04-01 03:14:49.000000000 +0200
+++ linux-2.6.29-devel/drivers/md/dm-kcopyd.c 2009-04-04 02:05:02.000000000 +0200
@@ -511,13 +511,16 @@ static void segment_complete(int read_er
} else if (atomic_dec_and_test(&job->sub_jobs)) {
/*
- * To avoid a race we must keep the job around
- * until after the notify function has completed.
- * Otherwise the client may try and stop the job
- * after we've completed.
+ * Queue the completion callback to the kcopyd thread.
+ *
+ * Some callers assume that all the completions are called
+ * from a single thread and don't race with each other.
+ *
+ * We must not call the callback directly here because this
+ * code may not be executing in the thread.
*/
- job->fn(read_err, write_err, job->context);
- mempool_free(job, job->kc->job_pool);
+ push(&kc->complete_jobs, job);
+ wake(kc);
}
}
@@ -530,6 +533,8 @@ static void split_job(struct kcopyd_job
{
int i;
+ atomic_inc(&job->kc->nr_jobs);
+
atomic_set(&job->sub_jobs, SPLIT_COUNT);
for (i = 0; i < SPLIT_COUNT; i++)
segment_complete(0, 0u, job);
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] kcopyd+snapshots race condition
2009-03-18 23:14 ` [PATCH 1/3] kcopyd+snapshots race condition Mikulas Patocka
2009-03-18 23:14 ` [PATCH 2/3] " Mikulas Patocka
2009-03-18 23:15 ` [PATCH 3/3] " Mikulas Patocka
@ 2009-04-06 19:39 ` Alasdair G Kergon
2 siblings, 0 replies; 12+ messages in thread
From: Alasdair G Kergon @ 2009-04-06 19:39 UTC (permalink / raw)
To: dm-devel
Mered with patch 2.
Alasdair
--
agk@redhat.com
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/3] kcopyd+snapshots race condition
2009-03-18 23:14 ` [PATCH 2/3] " Mikulas Patocka
@ 2009-04-06 19:39 ` Alasdair G Kergon
0 siblings, 0 replies; 12+ messages in thread
From: Alasdair G Kergon @ 2009-04-06 19:39 UTC (permalink / raw)
To: Mikulas Patocka; +Cc: dm-devel
On Wed, Mar 18, 2009 at 07:14:42PM -0400, Mikulas Patocka wrote:
> Test for job->pages being zero. This will be needed for the next patch.
Queued to linux-next (+stable).
Alasdair
--
agk@redhat.com
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] kcopyd+snapshots race condition ... updated
2009-04-04 0:57 ` [PATCH 3/3] kcopyd+snapshots race condition ... updated Mikulas Patocka
@ 2009-04-06 19:40 ` Alasdair G Kergon
0 siblings, 0 replies; 12+ messages in thread
From: Alasdair G Kergon @ 2009-04-06 19:40 UTC (permalink / raw)
To: Mikulas Patocka; +Cc: dm-devel
Queued to linux-next (+stable).
Alasdair
--
agk@redhat.com
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2009-04-06 19:40 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-03-16 19:06 [git pull] device-mapper fixes for 2.6.29 Alasdair G Kergon
2009-03-16 19:06 ` Alasdair G Kergon
2009-03-17 13:25 ` Mikulas Patocka
2009-03-17 20:38 ` Alasdair G Kergon
2009-03-18 23:13 ` Mikulas Patocka
2009-03-18 23:14 ` [PATCH 1/3] kcopyd+snapshots race condition Mikulas Patocka
2009-03-18 23:14 ` [PATCH 2/3] " Mikulas Patocka
2009-04-06 19:39 ` Alasdair G Kergon
2009-03-18 23:15 ` [PATCH 3/3] " Mikulas Patocka
2009-04-04 0:57 ` [PATCH 3/3] kcopyd+snapshots race condition ... updated Mikulas Patocka
2009-04-06 19:40 ` Alasdair G Kergon
2009-04-06 19:39 ` [PATCH 1/3] kcopyd+snapshots race condition Alasdair G Kergon
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.