From patchwork Wed Mar 18 23:15:20 2009 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Mikulas Patocka X-Patchwork-Id: 12989 X-Patchwork-Delegate: agk@redhat.com Received: from hormel.redhat.com (hormel1.redhat.com [209.132.177.33]) by demeter.kernel.org (8.14.2/8.14.2) with ESMTP id n2INFNOO022697 for ; Wed, 18 Mar 2009 23:15:24 GMT Received: from listman.util.phx.redhat.com (listman.util.phx.redhat.com [10.8.4.110]) by hormel.redhat.com (Postfix) with ESMTP id 7666161A538; Wed, 18 Mar 2009 19:15:24 -0400 (EDT) Received: from int-mx1.corp.redhat.com (int-mx1.corp.redhat.com [172.16.52.254]) by listman.util.phx.redhat.com (8.13.1/8.13.1) with ESMTP id n2INFLcI032041 for ; Wed, 18 Mar 2009 19:15:21 -0400 Received: from hs20-bc2-1.build.redhat.com (hs20-bc2-1.build.redhat.com [10.10.28.34]) by int-mx1.corp.redhat.com (8.13.1/8.13.1) with ESMTP id n2INFLvD014581; Wed, 18 Mar 2009 19:15:21 -0400 Received: from hs20-bc2-1.build.redhat.com (localhost.localdomain [127.0.0.1]) by hs20-bc2-1.build.redhat.com (8.13.1/8.13.1) with ESMTP id n2INFKPs026562; Wed, 18 Mar 2009 19:15:20 -0400 Received: from localhost (mpatocka@localhost) by hs20-bc2-1.build.redhat.com (8.13.1/8.13.1/Submit) with ESMTP id n2INFKBk026556; Wed, 18 Mar 2009 19:15:20 -0400 X-Authentication-Warning: hs20-bc2-1.build.redhat.com: mpatocka owned process doing -bs Date: Wed, 18 Mar 2009 19:15:20 -0400 (EDT) From: Mikulas Patocka X-X-Sender: mpatocka@hs20-bc2-1.build.redhat.com To: Alasdair G Kergon In-Reply-To: Message-ID: References: <20090316190642.GH5098@agk.fab.redhat.com> <20090317203838.GQ3063@agk.fab.redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.58 on 172.16.52.254 X-loop: dm-devel@redhat.com Cc: dm-devel@redhat.com Subject: [dm-devel] [PATCH 3/3] kcopyd+snapshots race condition X-BeenThere: dm-devel@redhat.com X-Mailman-Version: 2.1.5 Precedence: junk Reply-To: device-mapper development List-Id: device-mapper development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dm-devel-bounces@redhat.com Errors-To: dm-devel-bounces@redhat.com 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 --- drivers/md/dm-kcopyd.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel 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);