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 03-18-2009, 10:14 PM
Mikulas Patocka
 
Default kcopyd+snapshots race condition

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;

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 03-18-2009, 10:14 PM
Mikulas Patocka
 
Default kcopyd+snapshots race condition

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);


--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 03-18-2009, 10:15 PM
Mikulas Patocka
 
Default kcopyd+snapshots race condition

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);

--
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:46 PM.

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