HI,
I did not think it thoroughly, I have two questions,
1 For asynchronous encryption, although it may enjoy a throughput
improvement for a bunch of pages, however, it seems that each dirty
page will now more likely have a longer time to be written back after
marked PG_WRITEBACK,
in other words, it is being locked for a longer time, what if a write
happens on that locked page? so it seems it may slow down the
performance on some REWRITE cases.
2 It is not very clear that why it could speed up read performance,
from the Linux source code, it seems the kernel will wait for the
non uptodate page being uptodate (do_generic_file_read) before trying next page.
Cheers,
Li Wang
At 2012-06-14 06:25:28,"Thieu Le" <thieule@xxxxxxxxxx> wrote:
>Kewl :)
>
>Let me know if you have more questions.
>
>
>On Wed, Jun 13, 2012 at 3:20 PM, Tyler Hicks <tyhicks@xxxxxxxxxxxxx> wrote:
>> On 2012-06-13 15:03:42, Thieu Le wrote:
>>> 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.
>>
>> 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@xxxxxxxxxxxxxxx
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>--
>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
--
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
