Messages in this thread |  | | | Subject | Re: dm-crypt using kthread | | From | Christophe Saout <> | | Date | Mon, 16 Feb 2004 03:40:44 +0100 |
| |
Am Mo, den 16.02.2004 schrieb Jeff Garzik um 03:10:
> > + /* > > + * if additional pages cannot be allocated without waiting, > > + * return a partially allocated bio, the caller will then try > > + * to allocate additional bios while submitting this partial bio > > + */ > > + if ((i - bio->bi_idx) == (MIN_BIO_PAGES - 1)) > > + gfp_mask = (gfp_mask | __GFP_NOWARN) & ~__GFP_WAIT; > > If the caller said they can wait, why not wait?
How can the caller say this?
This is basically to avoid deadlocks. The kernel might decide to flush data in order to free memory (or even swap out). dm-crypt needs to allocate buffers for encryption. If we run out of memory we need to be able to write something in order to get new memory. Successful writes will return some pages to the pool and potentially wake up some threads needing these.
> > +static void dec_pending(struct crypt_io *io, int error) > > [...] > > + if (io->bio) > > + bio_endio(io->bio, io->bio->bi_size, io->error); > > when does io->bio==NULL ?
Umm, right. Can't ever happen. Thanks.
> > + set_task_state(current, TASK_INTERRUPTIBLE); > > + while (!(bio = kcryptd_get_bios())) { > > + schedule(); > > + if (signal_pending(current)) > > + return 0; > > + } > > + set_task_state(current, TASK_RUNNING); > > You just keep calling schedule() rapid-fire until you get a bio? That's > a bit sub-optimal.
That's wrong anyway. I was just making sure I was calling kcryptd_get_bios after schedule. schedule() will sleep and woken after someone added a bio to the list.
I've changed it to an if now and call kcryptd_get_bios after schedule.
I'm calling it twice because it is likely that someone started a new list while the old list is being processed and I don't want to sleep in this case, just fall through.
The kcryptd_get_bios needs to be after state = TASK_INTERRUPTIBLE to avoid a race. If someone wakes the process after kcryptd_get_bios but before schedule it resets the state to TASK_RUNNING so that the schedule won't sleep.
> > +/* > > + * Encode key into its hex representation > > + */ > > +static void crypt_encode_key(char *hex, u8 *key, int size) > > +{ > > + static char hex_digits[] = "0123456789abcdef"; > > static const
Or sprintf... I doubt this would result in shorter code but it would be more consistent.
> > + if (!mode || strcmp(mode, "plain") == 0) > > + cc->iv_generator = crypt_iv_plain; > > + else if (strcmp(mode, "ecb") == 0) > > + cc->iv_generator = NULL; > > + else { > > + ti->error = "dm-crypt: Invalid chaining mode"; > > + return -EINVAL; > > + } > > memory leak on error
Ouch. Thanks.
> > +static int crypt_endio(struct bio *bio, unsigned int done, int error) > > +{ > > + struct crypt_io *io = (struct crypt_io *) bio->bi_private; > > + struct crypt_config *cc = (struct crypt_config *) io->target->private; > > + > > + if (bio_rw(bio) == WRITE) { > > + /* > > + * free the processed pages, even if > > + * it's only a partially completed write > > + */ > > + crypt_free_buffer_pages(cc, bio, done); > > + } > > + > > + if (bio->bi_size) > > + return 1; > > dumb question, for my own knowledge: what does this 'if' test do?
It checks whether has bio has been completed. The block layer may notify us if parts of the bio has been finished.
> > > +static int crypt_map(struct dm_target *ti, struct bio *bio, > > + union map_info *map_context) > > [...] > > would be nice to move this code into a separate "create and init my > clone" function, simply to be ease review and make things a bit more > clear...
create and init would be the whole content of the loop. But if you think so, I can do that.
> might want to use the standard 'goto' style exception handling here too, > instead of duplicating the error unwind code.
Yes, the function got larger over time.
> Overall, needs a tiny bit of work, but I like it.
Thanks.
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
|  |