Re: [PATCH 1/1] ecryptfs: Migrate to ablkcipher API

[Tyler Hicks <tyhicks@canonical.com>]

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

On Wed, Jun 13, 2012 at 11:53 AM, Thieu Le <thieule@google.com> wrote:
>
> Hi Tyler, I believe the performance improvement from the async
> interface comes from the ability to fully utilize the crypto
> hardware.
>
> Firstly, being able to submit multiple outstanding requests fills
> the crypto engine pipeline which allows it to run more efficiently
> (ie. minimal cycles are wasted waiting for the next crypto request).
>  This perf improvement is similar to network transfer efficiency.
>  Sending a 1GB file via 4K packets synchronously is not going to
> fully saturate a gigabit link but queuing a bunch of 4K packets to
> send will.

Ok, it is clicking for me now. Additionally, I imagine that the async
interface helps in the multicore/multiprocessor case.

> Secondly, if you have crypto hardware that has multiple crypto
> engines, then the multiple outstanding requests allow the crypto
> hardware to put all of those engines to work.
>
> To answer your question about page_crypt_req, it is used to track
> all of the extent_crypt_reqs for a particular page.  When we write a
> page, we break the page up into extents and encrypt each extent.
>  For each extent, we submit the encrypt request using
> extent_crypt_req.  To determine when the entire page has been
> encrypted, we create one page_crypt_req and associates the
> extent_crypt_req to the page by incrementing
> page_crypt_req::num_refs.  As the extent encrypt request completes,
> we decrement num_refs.  The entire page is encrypted when num_refs
> goes to zero, at which point, we end the page writeback.

Alright, that is what I had understood from reviewing the code. No
surprises there.

What I'm suggesting is to do away with the page_crypt_req and simply have
ecryptfs_encrypt_page_async() keep track of the extent_crypt_reqs for
the page it is encrypting. Its prototype would look like this:

int ecryptfs_encrypt_page_async(struct page *page);

An example of how it would be called would be something like this:

static int ecryptfs_writepage(struct page *page, struct writeback_control *wbc)
{
	int rc = 0;

	/*
	 * Refuse to write the page out if we are called from reclaim context
	 * since our writepage() path may potentially allocate memory when
	 * calling into the lower fs vfs_write() which may in turn invoke
	 * us again.
	 */
	if (current->flags & PF_MEMALLOC) {
		redirty_page_for_writepage(wbc, page);
		goto out;
	}

	set_page_writeback(page);
	rc = ecryptfs_encrypt_page_async(page);
	if (unlikely(rc)) {
		ecryptfs_printk(KERN_WARNING, "Error encrypting "
				"page (upper index [0x%.16lx])\n", page->index);
		ClearPageUptodate(page);
		SetPageError(page);
	} else {
		SetPageUptodate(page);
	}
	end_page_writeback(page);
out:    
	unlock_page(page);
	return rc;
}


> We can get rid of page_crypt_req if we can guarantee that the extent
> size and page size are the same.

We can't guarantee that but that doesn't matter because
ecryptfs_encrypt_page_async() already handles that problem. Its caller doesn't
care if the extent size is less than the page size.

Tyler

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