* Re: eCryptfs ablkcipher patch
2013-01-10 18:36 ` Tyler Hicks
@ 2013-01-20 12:00 ` Zeev Zilberman
2013-01-20 13:26 ` Zeev Zilberman
1 sibling, 0 replies; 4+ messages in thread
From: Zeev Zilberman @ 2013-01-20 12:00 UTC (permalink / raw)
To: Tyler Hicks; +Cc: ecryptfs@vger.kernel.org
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: eCryptfs ablkcipher patch
2013-01-10 18:36 ` Tyler Hicks
2013-01-20 12:00 ` Zeev Zilberman
@ 2013-01-20 13:26 ` Zeev Zilberman
1 sibling, 0 replies; 4+ messages in thread
From: Zeev Zilberman @ 2013-01-20 13:26 UTC (permalink / raw)
To: Tyler Hicks; +Cc: ecryptfs@vger.kernel.org
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
^ permalink raw reply related [flat|nested] 4+ messages in thread