Inline.
On Sat, Jun 16, 2012 at 4:12 AM, dragonylffly <dragonylffly@xxxxxxx> wrote:
> 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.
If I understand you correctly, I think there could be some slowdown in
your scenario if we assume the sync and async crypto code paths are
similar or the async path is longer. However, if there are are
multiple extents per page, the async approach will allow us to run the
crypto requests in parallel thereby lowering the amount of time under
page lock.
>
> 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.
There are two ways that this patch can speed up performance in the read path:
1. If the page contains multiple extents, this patch will submit the
extent decryption requests to the crypto API in parallel.
2. The readahead does not wait for the page to be read thereby
allowing us to submit multiple extents decryption requests in
parallel.
>
> 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
