eCryptfs ablkcipher patch

eCryptfs ablkcipher patch

[Zeev Zilberman]

Hello,

I've seen earlier discussions about ecryptfs ablkcipher patch, but I see
it was not merged.
Are you planning to apply this patch in the future, or did you decide to
drop it?

I tried to apply the patch locally and saw the following issues:

1. I've encountered a problem with ecryptfs_encrypt_extent_done that is
calling functions
that can sleep (kmap/kunmap). It fails with ablkcipher crypto drivers that
invoke the
callback from interrupt handler bh (tasklet).
Moving the write part to a work queue (using queue_work) seems to solve it.


2. I saw that ecryptfs was reverted from writeback to writethrough cache
mode.
This seems to be problematic in regard to performance while using async
interfaces.
The original change to writepage (that uses ecryptfs_encrypt_page_async)
allowed
submitting async crypto operations and continuing without waiting for the
result.
write_end uses ecryptfs_encrypt_page (and needs its return value), so
we'll have to wait
for encryption (and write) to complete before continuing to the next
operation.
Are you planning to return ecryptfs cache to writeback mode?


Thank you,
Zeev.

Re: eCryptfs ablkcipher patch

[Tyler Hicks]

[-- Attachment #1: Type: text/plain, Size: 2727 bytes --]

On 2013-01-06 15:36:38, Zeev Zilberman wrote:
> Hello,

Hi Zeev!

> 
> I've seen earlier discussions about ecryptfs ablkcipher patch, but I see
> it was not merged.
> Are you planning to apply this patch in the future, or did you decide to
> drop it?

Its status is somewhere between those two extremes. Not applied, but
also not dropped. It is useful, but I'm not sure that its benefits
outweigh the risks of merging it.

The problem with the patch is that it didn't provide a clear performance
increase across the board. I don't have the performance testing results
handy, but it gave a nice increase on some systems and hurt performance
on others. That, coupled with how risky the patch is, makes me hesitant
to merge it.

I don't currently have the bandwidth to provide the amount of testing
that would be needed (nor the time to spend on fixing regressions) and
the patch author has not been active in eCryptfs development for quite
some time. In other words, no one has stepped up to usher the patch
through.

Very few eCryptfs users test the mainline -rc kernels and I don't want
the users of whatever distro first ships this patch to find all of the
regressions caused by it.

> 
> I tried to apply the patch locally and saw the following issues:
> 
> 1. I've encountered a problem with ecryptfs_encrypt_extent_done that is
> calling functions
> that can sleep (kmap/kunmap). It fails with ablkcipher crypto drivers that
> invoke the
> callback from interrupt handler bh (tasklet).
> Moving the write part to a work queue (using queue_work) seems to solve it.

Please send me patches, and cc this list, for these fixes. I will push
everything to a branch in the eCryptfs git tree on kernel.org so that we
don't lose any useful code.

> 
> 
> 2. I saw that ecryptfs was reverted from writeback to writethrough cache
> mode.

That's correct

> This seems to be problematic in regard to performance while using async
> interfaces.
> The original change to writepage (that uses ecryptfs_encrypt_page_async)
> allowed
> submitting async crypto operations and continuing without waiting for the
> result.

Nice catch, that would have to be changed.

> write_end uses ecryptfs_encrypt_page (and needs its return value), so
> we'll have to wait
> for encryption (and write) to complete before continuing to the next
> operation.
> Are you planning to return ecryptfs cache to writeback mode?

No, it caused quite a few bugs. I suspect that eCryptfs will stay with
the writethrough cache.


Have you done any performance testing yourself? I'm curious if you're
interested in this patch because it made a significant difference in
your use case.

Tyler

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: eCryptfs ablkcipher patch

[Zeev Zilberman]

Hi Tyler,

>Its status is somewhere between those two extremes. Not applied, but
>also not dropped. It is useful, but I'm not sure that its benefits
>outweigh the risks of merging it.
>
>The problem with the patch is that it didn't provide a clear performance
>increase across the board. I don't have the performance testing results
>handy, but it gave a nice increase on some systems and hurt performance
>on others. That, coupled with how risky the patch is, makes me hesitant
>to merge it.
>
>I don't currently have the bandwidth to provide the amount of testing
>that would be needed (nor the time to spend on fixing regressions) and
>the patch author has not been active in eCryptfs development for quite
>some time. In other words, no one has stepped up to usher the patch
>through.
>
>Very few eCryptfs users test the mainline -rc kernels and I don't want
>the users of whatever distro first ships this patch to find all of the
>regressions caused by it.

Maybe the async mode can be added as an experimental config option at the
first stage?
The async mode can be really useful for systems that have HW crypto
accelerators.

Regarding the testing, I still don't have the HW available, I'd be glad to
assist with testing once I do.
Which systems were tested till now?


>Please send me patches, and cc this list, for these fixes. I will push
>everything to a branch in the eCryptfs git tree on kernel.org so that we
>don't lose any useful code.

Sending in a separate mail.


>
>>2. I saw that ecryptfs was reverted from writeback to writethrough cache
>>mode.
>
>That's correct
>
>>This seems to be problematic in regard to performance while using async
>>interfaces.
>>The original change to writepage (that uses ecryptfs_encrypt_page_async)
>>allowed
>>submitting async crypto operations and continuing without waiting for
>>the
>>result.
>
>Nice catch, that would have to be changed.

The problem is that in writethrough mode async operation doesn't seem to
be possible (otherwise the same problems that were encountered in
writeback mode will be encountered, as the io itself will be handled later
by the crypto callback).
It seems that the proper solution is switch to writeback and solve the
issues there.
Is there another solution you think can work?


>
>>write_end uses ecryptfs_encrypt_page (and needs its return value), so
>>we'll have to wait
>>for encryption (and write) to complete before continuing to the next
>>operation.
>>Are you planning to return ecryptfs cache to writeback mode?
>
>No, it caused quite a few bugs. I suspect that eCryptfs will stay with
>the writethrough cache.
>

Could you please elaborate on the bugs that were encountered with
writeback cache?
I saw 2 issues mentioned - the issue of return codes from lower fs and the
issue of quota.
I saw that Thieu Le proposed to use fallocate() in write_end, which seems
to address both of the problems.
Are there more issues that are not covered by this solution?


>
>Have you done any performance testing yourself? I'm curious if you're
>interested in this patch because it made a significant difference in
>your use case.

I will test performance as soon as I'll have the HW available (in a few
months).


Thank you,
Zeev

Re: eCryptfs ablkcipher patch

[Zeev Zilberman]

Hi Tyler,

The patch below adds the workqueue for write operations.
This patch depends on the ecryptfs ablkcipher support patch rebased to 3.7.

Regards,
Ze'ev

---

From 606ea815140caa30ff93bdcd998f43d7f95edf61 Mon Sep 17 00:00:00 2001
From: Zeev Zilberman <zeev@annapurnalabs.com>
Date: Mon, 17 Dec 2012 17:59:22 +0200
Subject: [PATCH] ecryptfs ablkcipher support - add workqueue

ablkcipher callback may called from tasklet, so it can't sleep.
Using work queue for write operations allows using sleeping functions.

Signed-off-by: Zeev Zilberman <zeev@annapurnalabs.com>
---
fs/ecryptfs/crypto.c          |   93
+++++++++++++++++++++++++++++-----------
fs/ecryptfs/ecryptfs_kernel.h |    1 +
2 files changed, 68 insertions(+), 26 deletions(-)

diff --git a/fs/ecryptfs/crypto.c b/fs/ecryptfs/crypto.c
index 7f5ff05..27b1702 100644
--- a/fs/ecryptfs/crypto.c
+++ b/fs/ecryptfs/crypto.c
@@ -34,6 +34,7 @@
#include <linux/file.h>
#include <linux/scatterlist.h>
#include <linux/slab.h>
+#include <linux/workqueue.h>
#include <asm/unaligned.h>
#include "ecryptfs_kernel.h"
@@ -562,6 +563,53 @@ out:
	}
}
+static struct workqueue_struct *crypto_cmp_write_queue;
+
+/**
+ * ecryptfs_encrypt_extent_write
+ * @work: The extent work structure
+ *
+ * This function is called performs the io operations after
+ * the encryption is completed. It is queued in a workqueue by
+ * ecryptfs_encrypt_extent_done, so it can sleep.
+ */
+static void ecryptfs_encrypt_extent_write(struct work_struct *work)
+{
+	struct ecryptfs_extent_crypt_req *extent_crypt_req = container_of(
+			work, struct ecryptfs_extent_crypt_req, work);
+	struct ecryptfs_page_crypt_req *page_crypt_req =
+				extent_crypt_req->page_crypt_req;
+	char *enc_extent_virt = NULL;
+	struct page *page = page_crypt_req->page;
+	struct page *enc_extent_page = extent_crypt_req->enc_extent_page;
+	loff_t offset;
+	int rc = 0;
+
+	enc_extent_virt = kmap(enc_extent_page);
+	ecryptfs_lower_offset_for_extent(
+		&offset,
+		((((loff_t)page->index)
+		  * (PAGE_CACHE_SIZE
+		     / extent_crypt_req->crypt_stat->extent_size))
+		    + extent_crypt_req->extent_offset),
+		extent_crypt_req->crypt_stat);
+	rc = ecryptfs_write_lower(extent_crypt_req->inode, enc_extent_virt,
+				  offset,
+				  extent_crypt_req->crypt_stat->extent_size);
+	if (rc < 0) {
+		atomic_set(&page_crypt_req->rc, rc);
+		ecryptfs_printk(KERN_ERR, "Error attempting "
+				"to write lower page; rc = [%d]"
+				"\n", rc);
+		goto out;
+	}
+out:
+	if (enc_extent_virt)
+		kunmap(enc_extent_page);
+	__free_page(enc_extent_page);
+	ecryptfs_free_extent_crypt_req(extent_crypt_req);
+}
+
/**
  * ecryptfs_encrypt_extent_done
  * @req: The original extent encrypt request
@@ -576,14 +624,11 @@ static void ecryptfs_encrypt_extent_done(
	struct ecryptfs_extent_crypt_req *extent_crypt_req = req->data;
	struct ecryptfs_page_crypt_req *page_crypt_req =
				extent_crypt_req->page_crypt_req;
-	char *enc_extent_virt = NULL;
	struct page *page = page_crypt_req->page;
	struct page *enc_extent_page = extent_crypt_req->enc_extent_page;
	struct ecryptfs_crypt_stat *crypt_stat = extent_crypt_req->crypt_stat;
	loff_t extent_base;
	unsigned long extent_offset = extent_crypt_req->extent_offset;
-	loff_t offset;
-	int rc = 0;
	if (!err && unlikely(ecryptfs_verbosity > 0)) {
		extent_base = (((loff_t)page->index)
@@ -600,32 +645,14 @@ static void ecryptfs_encrypt_extent_done(
		atomic_set(&page_crypt_req->rc, err);
		printk(KERN_ERR "%s: Error encrypting extent; "
		       "rc = [%d]\n", __func__, err);
+		__free_page(enc_extent_page);
+		ecryptfs_free_extent_crypt_req(extent_crypt_req);
		goto out;
	}
-
-	enc_extent_virt = kmap(enc_extent_page);
-	ecryptfs_lower_offset_for_extent(
-		&offset,
-		((((loff_t)page->index)
-		  * (PAGE_CACHE_SIZE
-		     / extent_crypt_req->crypt_stat->extent_size))
-		    + extent_crypt_req->extent_offset),
-		extent_crypt_req->crypt_stat);
-	rc = ecryptfs_write_lower(extent_crypt_req->inode, enc_extent_virt,
-				  offset,
-				  extent_crypt_req->crypt_stat->extent_size);
-	if (rc < 0) {
-		atomic_set(&page_crypt_req->rc, rc);
-		ecryptfs_printk(KERN_ERR, "Error attempting "
-				"to write lower page; rc = [%d]"
-				"\n", rc);
-		goto out;
-	}
+	INIT_WORK(&extent_crypt_req->work, ecryptfs_encrypt_extent_write);
+	queue_work(crypto_cmp_write_queue, &extent_crypt_req->work);
out:
-	if (enc_extent_virt)
-		kunmap(enc_extent_page);
-	__free_page(enc_extent_page);
-	ecryptfs_free_extent_crypt_req(extent_crypt_req);
+	return;
}
/**
@@ -2124,6 +2151,17 @@ int __init ecryptfs_init_crypto(void)
{
	mutex_init(&key_tfm_list_mutex);
	INIT_LIST_HEAD(&key_tfm_list);
+
+	crypto_cmp_write_queue = alloc_workqueue("ecryptfs_cmp_write",
+				       WQ_NON_REENTRANT|
+				       WQ_MEM_RECLAIM,
+				       1);
+	if (!crypto_cmp_write_queue) {
+		printk(KERN_ERR "%s: failed to alloc crypto write workqueue\n",
+				__func__);
+		return -ENOMEM;
+	}
+
	return 0;
}
@@ -2136,6 +2174,9 @@ int ecryptfs_destroy_crypto(void)
{
	struct ecryptfs_key_tfm *key_tfm, *key_tfm_tmp;
+	if (crypto_cmp_write_queue)
+		destroy_workqueue(crypto_cmp_write_queue);
+
	mutex_lock(&key_tfm_list_mutex);
	list_for_each_entry_safe(key_tfm, key_tfm_tmp, &key_tfm_list,
				 key_tfm_list) {
diff --git a/fs/ecryptfs/ecryptfs_kernel.h b/fs/ecryptfs/ecryptfs_kernel.h
index 5af953f..7d4dafd 100644
--- a/fs/ecryptfs/ecryptfs_kernel.h
+++ b/fs/ecryptfs/ecryptfs_kernel.h
@@ -571,6 +571,7 @@ struct ecryptfs_extent_crypt_req {
	unsigned long extent_offset;
	struct scatterlist src_sg;
	struct scatterlist dst_sg;
+	struct work_struct work;
};
struct inode *ecryptfs_get_inode(struct inode *lower_inode,
--
1.7.0.4