On 2012-06-18 10:17:29, Thieu Le wrote:
> 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.
I'm repeating myself from earlier in the thread, but I think that
implementing ->readpages would be easy and could really benefit from an
async patch.
Tyler
>
>
> >
> > 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
Attachment:
signature.asc
Description: Digital signature
