Re: [PATCH 2/2][RFC] Add dm-crypt target

[Christophe Saout <christophe@saout.de>]

On Tue, Dec 23, 2003 at 03:43:25PM +0000, Joe Thornber wrote:

> On Tue, Dec 23, 2003 at 10:31:43AM -0500, Jeff Garzik wrote:
> > I agree w/ Christoph...  overly defensive programming like this just
> > creates a new class of programmer errors, doesn't really solve anything.
> > It's standard Linux kernel style, and making code look like all other
> > code has benefits in review and debugging.  Finally, the programmer
> > should be paying attention to what kernel APIs he/she uses, and add
> > headers accordingly.
> 
> ok, I don't feel strongly enough about it to argue, so we'll change it.

Ok, me too. :-)

So how does this look?

--- linux.orig/drivers/md/dm-crypt.c	2003-12-23 16:43:48.409077048 +0100
+++ linux/drivers/md/dm-crypt.c	2003-12-23 16:52:11.946527720 +0100
@@ -4,9 +4,6 @@
  * This file is released under the GPL.
  */
 
-#include "dm.h"
-#include "dm-daemon.h"
-
 #include <linux/module.h>
 #include <linux/init.h>
 #include <linux/bio.h>
@@ -16,6 +13,9 @@
 #include <linux/spinlock.h>
 #include <asm/scatterlist.h>
 
+#include "dm.h"
+#include "dm-daemon.h"
+
 /*
  * per bio private data
  */
@@ -70,10 +70,10 @@
 #define MIN_POOL_PAGES 16
 #define MIN_BIO_PAGES  8
 
-static kmem_cache_t *_io_cache;
+static kmem_cache_t *crypt_io_pool;
 
 /*
- * Mempool alloc and free functions for the page and io pool
+ * Mempool alloc and free functions for the page
  */
 static void *mempool_alloc_page(int gfp_mask, void *data)
 {
@@ -85,26 +85,6 @@
 	__free_page(page);
 }
 
-static inline struct page *crypt_alloc_page(struct crypt_c *cc, int gfp_mask)
-{
-	return mempool_alloc(cc->page_pool, gfp_mask);
-}
-
-static inline void crypt_free_page(struct crypt_c *cc, struct page *page)
-{
-	 mempool_free(page, cc->page_pool);
-}
-
-static inline struct crypt_io *crypt_alloc_io(struct crypt_c *cc)
-{
-	return mempool_alloc(cc->io_pool, GFP_NOIO);
-}
-
-static inline void crypt_free_io(struct crypt_c *cc, struct crypt_io *io)
-{
-	return mempool_free(io, cc->io_pool);
-}
-
 /*
  * Encrypt / decrypt a single sector, source and destination buffers
  * are stored in scatterlists. In CBC mode initialise the "previous
@@ -232,7 +212,7 @@
 	for(i = bio->bi_idx; i < nr_iovecs; i++) {
 		struct bio_vec *bv = bio_iovec_idx(bio, i);
 
-		bv->bv_page = crypt_alloc_page(cc, gfp_mask);
+		bv->bv_page = mempool_alloc(cc->page_pool, gfp_mask);
 		if (!bv->bv_page)
 			break;
 
@@ -276,7 +256,7 @@
 
 	while(bytes) {
 		struct bio_vec *bv = bio_iovec_idx(bio, i++);
-		crypt_free_page(cc, bv->bv_page);
+		mempool_free(bv->bv_page, cc->page_pool);
 		bytes -= bv->bv_len;
 	}
 }
@@ -301,7 +281,7 @@
 	if (io->bio)
 		bio_endio(io->bio, io->bio->bi_size, io->error);
 
-	crypt_free_io(cc, io);
+	mempool_free(io, cc->io_pool);
 }
 
 /*
@@ -311,11 +291,11 @@
  * interrupt context, so returning bios from read requests get
  * queued here.
  */
-static spinlock_t _kcryptd_lock = SPIN_LOCK_UNLOCKED;
-static struct bio *_bio_head;
-static struct bio *_bio_tail;
+static spinlock_t kcryptd_lock = SPIN_LOCK_UNLOCKED;
+static struct bio *kcryptd_bio_head;
+static struct bio *kcryptd_bio_tail;
 
-static struct dm_daemon _kcryptd;
+static struct dm_daemon kcryptd;
 
 /*
  * Fetch a list of the complete bios.
@@ -324,11 +304,11 @@
 {
 	struct bio *bio;
 
-	spin_lock_irq(&_kcryptd_lock);
-	bio = _bio_head;
+	spin_lock_irq(&kcryptd_lock);
+	bio = kcryptd_bio_head;
 	if (bio)
-		_bio_head = _bio_tail = NULL;
-	spin_unlock_irq(&_kcryptd_lock);
+		kcryptd_bio_head = kcryptd_bio_tail = NULL;
+	spin_unlock_irq(&kcryptd_lock);
 
 	return bio;
 }
@@ -340,16 +320,16 @@
 {
 	unsigned long flags;
 
-	spin_lock_irqsave(&_kcryptd_lock, flags);
-	if (_bio_tail)
-		_bio_tail->bi_next = bio;
+	spin_lock_irqsave(&kcryptd_lock, flags);
+	if (kcryptd_bio_tail)
+		kcryptd_bio_tail->bi_next = bio;
 	else
-		_bio_head = bio;
-	_bio_tail = bio;
-	spin_unlock_irqrestore(&_kcryptd_lock, flags);
+		kcryptd_bio_head = bio;
+	kcryptd_bio_tail = bio;
+	spin_unlock_irqrestore(&kcryptd_lock, flags);
 }
 
-static jiffy_t kcryptd(void)
+static jiffy_t kcryptd_do_work(void)
 {
 	int r;
 	struct bio *bio;
@@ -493,7 +473,7 @@
 	}
 
 	cc->io_pool = mempool_create(MIN_IOS, mempool_alloc_slab,
-				     mempool_free_slab, _io_cache);
+				     mempool_free_slab, crypt_io_pool);
 	if (!cc->io_pool) {
 		ti->error = "dm-crypt: Cannot allocate crypt io mempool";
 		goto bad1;
@@ -584,7 +564,7 @@
 	if ((bio_rw(bio) == READ || bio_rw(bio) == READA)
 	    && bio_flagged(bio, BIO_UPTODATE)) {
 		kcryptd_queue_bio(bio);
-		dm_daemon_wake(&_kcryptd);
+		dm_daemon_wake(&kcryptd);
 		return 0;
 	}
 
@@ -597,7 +577,7 @@
 static int crypt_map(struct dm_target *ti, struct bio *bio)
 {
 	struct crypt_c *cc = (struct crypt_c *) ti->private;
-	struct crypt_io *io = crypt_alloc_io(cc);
+	struct crypt_io *io = mempool_alloc(cc->io_pool, GFP_NOIO);
 	struct bio *clone = NULL;
 	struct convert_context ctx;
 	unsigned int remaining = bio->bi_size;
@@ -675,7 +655,7 @@
 	}
 
 	/* if no bio has been dispatched yet, we can directly return the error */
-	crypt_free_io(cc, io);
+	mempool_free(io, cc->io_pool);
 	return r;
 }
 
@@ -740,48 +720,45 @@
 	.status = crypt_status,
 };
 
-int __init dm_crypt_init(void)
+static int __init dm_crypt_init(void)
 {
 	int r;
 
-	_io_cache = kmem_cache_create("dm-crypt_io", sizeof(struct crypt_io),
-	                              0, 0, NULL, NULL);
-	if (!_io_cache)
+	crypt_io_pool = kmem_cache_create("dm-crypt_io", sizeof(struct crypt_io),
+	                                  0, 0, NULL, NULL);
+	if (!crypt_io_pool)
 		return -ENOMEM;
 
-	r = dm_daemon_start(&_kcryptd, "kcryptd", kcryptd);
+	r = dm_daemon_start(&kcryptd, "kcryptd", kcryptd_do_work);
 	if (r) {
 		DMERR("couldn't create kcryptd: %d", r);
-		kmem_cache_destroy(_io_cache);
+		kmem_cache_destroy(crypt_io_pool);
 		return r;
 	}
 
 	r = dm_register_target(&crypt_target);
 	if (r < 0) {
 		DMERR("crypt: register failed %d", r);
-		dm_daemon_stop(&_kcryptd);
-		kmem_cache_destroy(_io_cache);
+		dm_daemon_stop(&kcryptd);
+		kmem_cache_destroy(crypt_io_pool);
 	}
 
 	return r;
 }
 
-void __exit dm_crypt_exit(void)
+static void __exit dm_crypt_exit(void)
 {
 	int r = dm_unregister_target(&crypt_target);
 
 	if (r < 0)
 		DMERR("crypt: unregister failed %d", r);
 
-	dm_daemon_stop(&_kcryptd);
-	kmem_cache_destroy(_io_cache);
+	dm_daemon_stop(&kcryptd);
+	kmem_cache_destroy(crypt_io_pool);
 }
 
-/*
- * module hooks
- */
-module_init(dm_crypt_init)
-module_exit(dm_crypt_exit)
+module_init(dm_crypt_init);
+module_exit(dm_crypt_exit);
 
 MODULE_AUTHOR("Christophe Saout <christophe@saout.de>");
 MODULE_DESCRIPTION(DM_NAME " target for transparent encryption / decryption");