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

[Tyler Hicks <tyhicks@canonical.com>]

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

On 2012-06-13 15:03:42, Thieu Le wrote:
> On Wed, Jun 13, 2012 at 2:17 PM, Tyler Hicks <tyhicks@canonical.com> wrote:
> > 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;
> > }
> 
> 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.

Ok, now I see the source of my confusion. The wait_for_completion()
added in ecryptfs_encrypt_page() was throwing me off. I initially
noticed that and didn't realize that wait_for_completion() was *not*
being called in ecryptfs_writepage().

I hope to give the rest of the patch a thorough review by the end of the
week. Thanks for your help!

Tyler

> 
> 
> >
> >
> >> 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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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