dm crypt: fix ctx pending
Alasdair G Kergon
maintainerFrom: Milan Broz <[email protected]>
Fix regression in dm-crypt introduced in commit3a7f6c990ad04e6f576a159876c602d14d6f7fef
(dm crypt: use async crypto).
If write requests need to be split into pieces, the code must not process them in parallel because the crypto context cannot be shared. So there can be parallel crypto operations on one part of the write, but only one write bio can be processed at a time.
This is not optimal and the workqueue code needs to be optimized for parallel processing, but for now it solves the problem without affecting the performance of synchronous crypto operation (most of current dm-crypt users).
http://bugzilla.kernel.org/show_bug.cgi?id=10242
http://bugzilla.kernel.org/show_bug.cgi?id=10207
--- drivers/md/dm-crypt.c | 58 +++++++++++++++++++++++++------------------------- 1 files changed, 30 insertions(+), 28 deletions(-)
drivers/md/dm-crypt.c +30 -28
@@ -1,7 +1,7 @@/* * Copyright (C) 2003 Christophe Saout <[email protected]> * Copyright (C) 2004 Clemens Fruhwirth <clemens-Yy/[email protected]> -* Copyright (C) 2006-2007 Red Hat, Inc. All rights reserved. +* Copyright (C) 2006-2008 Red Hat, Inc. All rights reserved. * * This file is released under the GPL. */
@@ -93,6 +93,8 @@ struct crypt_config {struct workqueue_struct *io_queue;struct workqueue_struct *crypt_queue; +wait_queue_head_t writeq; +/* * crypto related data */
@@ -331,14 +333,7 @@ static void crypt_convert_init(struct cr ctx->idx_out = bio_out ? bio_out->bi_idx :0 ; ctx->sector = sector + cc->iv_offset;init_completion (&ctx->restart); -/* -* Crypto operation can be asynchronous, -* ctx->pending is increased after request submission. -* We need to ensure that we don't call the crypt finish -* operation before pending got incremented -* (dependent on crypt submission return code). -*/ -atomic_set (&ctx->pending,2 ); +atomic_set (&ctx->pending,1 ); } static int crypt_convert_block(struct crypt_config *cc,
@@ -411,43 +406,42 @@ static void crypt_alloc_req(struct cryptstatic int crypt_convert (struct crypt_config *cc,struct convert_context *ctx) { -int r =0 ; +int r;while (ctx->idx_in < ctx->bio_in->bi_vcnt && ctx->idx_out < ctx->bio_out->bi_vcnt) {crypt_alloc_req (cc, ctx); +atomic_inc (&ctx->pending); + r =crypt_convert_block (cc, ctx, cc->req);switch (r) { +/* async */ case -EBUSY :wait_for_completion (&ctx->restart);INIT_COMPLETION (ctx->restart);/* fall through*/ case -EINPROGRESS : -atomic_inc (&ctx->pending); cc->req =NULL ; - r =0 ; -/* fall through*/ + ctx->sector++; +continue ; + +/* sync */ case 0 : +atomic_dec (&ctx->pending); ctx->sector++;continue ; - } -break ; +/* error */ +default : +atomic_dec (&ctx->pending); +return r; + } } -/* -* If there are pending crypto operation run async -* code. Otherwise process return code synchronously. -* The step of 2 ensures that async finish doesn't -* call crypto finish too early. -*/ -if (atomic_sub_return (2 , &ctx->pending)) -return -EINPROGRESS ; - -return r; +return 0 ; }static void dm_crypt_bio_destructor (struct bio *bio)
@@ -624,8 +618,10 @@ static void kcryptd_io_read(struct dm_crstatic void kcryptd_io_write (struct dm_crypt_io *io) {struct bio *clone = io->ctx.bio_out; +struct crypt_config *cc = io->target->private;generic_make_request (clone); +wake_up (&cc->writeq); }static void kcryptd_io (struct work_struct *work)
@@ -698,7 +694,8 @@ static void kcryptd_crypt_write_convert_ r =crypt_convert (cc, &io->ctx); -if (r != -EINPROGRESS ) { +if (atomic_dec_and_test (&io->ctx.pending)) { +/* processed, no running async crypto */ kcryptd_crypt_write_io_submit (io, r,0 );if (unlikely (r <0 ))return ;
@@ -706,8 +703,12 @@ static void kcryptd_crypt_write_convert_atomic_inc (&io->pending);/* out of memory -> run queues */ -if (unlikely (remaining)) +if (unlikely (remaining)) { +/* wait for async crypto then reinitialize pending */ +wait_event (cc->writeq, !atomic_read (&io->ctx.pending)); +atomic_set (&io->ctx.pending,1 );congestion_wait (WRITE ,HZ /100 ); + } } }
@@ -746,7 +747,7 @@ static void kcryptd_crypt_read_convert(s r =crypt_convert (cc, &io->ctx); -if (r != -EINPROGRESS ) +if (atomic_dec_and_test (&io->ctx.pending))kcryptd_crypt_read_done (io, r);crypt_dec_pending (io);
@@ -1047,6 +1048,7 @@ static int crypt_ctr(struct dm_target *t goto bad_crypt_queue; } +init_waitqueue_head (&cc->writeq); ti->private = cc;return 0 ;
Alasdair G Kergon
maintainer[2.6.25-rc8] dm io: write error bits form long not int
From: Alasdair G Kergon <[email protected]>
write_err is an unsigned long used with set_bit() so should not be passed around as unsigned int.
http://bugzilla.kernel.org/show_bug.cgi?id=10271
--- drivers/md/dm-io.c | 2 +- drivers/md/dm-raid1.c | 4 ++-- drivers/md/dm-snap.c | 2 +- drivers/md/kcopyd.c | 10 +++++----- drivers/md/kcopyd.h | 4 ++-- 5 files changed, 11 insertions(+), 11 deletions(-)
drivers/md/dm-io.c +1 -1
@@ -114,7 +114,7 @@ static void dec_count(struct io *io, unswake_up_process (io->sleeper); else { -int r = io->error; +unsigned long r = io->error;io_notify_fn fn = io->callback;void *context = io->context;
drivers/md/dm-raid1.c +2 -2
@@ -753,7 +753,7 @@ out:* are in the no-sync state. We have to recover these by * recopying from the default mirror to all the others. *---------------------------------------------------------------*/ -static void recovery_complete (int read_err,unsigned int write_err, +static void recovery_complete (int read_err,unsigned long write_err,void *context) {struct region *reg = (struct region *)context;
@@ -767,7 +767,7 @@ static void recovery_complete(int read_e }if (write_err) { -DMERR_LIMIT ("Write error during recovery (error = 0x%x)" , +DMERR_LIMIT ("Write error during recovery (error = 0x%lx)" , write_err);/* * Bits correspond to devices (excluding default mirror).
drivers/md/dm-snap.c +1 -1
@@ -804,7 +804,7 @@ static void commit_callback(void *contex* Called when the copy I/O has finished. kcopyd actually runs * this code so don't block. */ -static void copy_callback (int read_err,unsigned int write_err,void *context) +static void copy_callback (int read_err,unsigned long write_err,void *context) {struct dm_snap_pending_exception *pe = context;struct dm_snapshot *s = pe->snap;
drivers/md/kcopyd.c +5 -5
@@ -169,7 +169,7 @@ struct kcopyd_job {* Error state of the job. */ int read_err; -unsigned int write_err; +unsigned long write_err;/* * Either READ or WRITE
@@ -293,7 +293,7 @@ static int run_complete_job(struct kcopy {void *context = job->context;int read_err = job->read_err; -unsigned int write_err = job->write_err; +unsigned long write_err = job->write_err;kcopyd_notify_fn fn = job->fn;struct kcopyd_client *kc = job->kc;
@@ -396,7 +396,7 @@ static int process_jobs(struct list_headif (r <0 ) {/* error this rogue job */ if (job->rw ==WRITE ) - job->write_err = (unsigned int )-1 ; + job->write_err = (unsigned long )-1L ;else job->read_err =1 ;push (&_complete_jobs, job);
@@ -448,8 +448,8 @@ static void dispatch_job(struct kcopyd_j }#define SUB_JOB_SIZE 128 -static void segment_complete (int read_err, -unsigned int write_err,void *context) +static void segment_complete (int read_err,unsigned long write_err, +void *context) {/* FIXME: tidy this function */ sector_t progress =0 ;
drivers/md/kcopyd.h +2 -2
@@ -32,8 +32,8 @@ void kcopyd_client_destroy(struct kcopyd* read_err is a boolean, * write_err is a bitset, with 1 bit for each destination region */ -typedef void (*kcopyd_notify_fn )(int read_err, -unsigned int write_err,void *context); +typedef void (*kcopyd_notify_fn )(int read_err,unsigned long write_err, +void *context); int kcopyd_copy(struct kcopyd_client *kc, struct io_region *from, unsigned int num_dests, struct io_region *dests,