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

[Thieu Le <thieule@xxxxxxxxxx>]

On Wed, Jun 13, 2012 at 2:17 PM, Tyler Hicks <tyhicks@xxxxxxxxxxxxx> wrote:
> On Wed, Jun 13, 2012 at 11:53 AM, Thieu Le <thieule@xxxxxxxxxx> 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;
> }

Will this make ecryptfs_encrypt_page_async() block until all of the
extents are encrypted and written to the lower file before returning?

In the current patch, ecryptfs_encrypt_page_async() returns
immediately after the extents are submitted to the crypto layer.
ecryptfs_writepage() will also return before the encryption and write
to the lower file completes.  This allows the OS to start writing
other pending pages without being blocked.


>
>
>> 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
--
To unsubscribe from this list: send the line "unsubscribe ecryptfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html