Re: [PATCH] eCryptfs: write optimization

[Tyler Hicks]

On 2012-01-25 23:08:58, Li Wang wrote:
> Hi,
>   ecryptfs_write_begin() grabs a page from page cache for writing. 
> If the page does not contain valid data, or the data are older than 
> the counterpart on the disk, eCryptfs will read out the corresponding data 
> from the disk into the eCryptfs page cache, decrypt them, 
> then perform writing. However, for current page, if the length of 
> the data to be written into is equal to page size, that means the whole 
> page of data will be overwritten, in which case, it does not matter whatever 
> the data were before, it is beneficial to perform writing directly.
> 
> This is useful while using eCryptfs in backup situation, user copies file
> out from eCryptfs folder, modifies, and copies the revised file back to
> replace the original one. 
> 
> With this optimization, according to our test, iozone 'write' operation on an existing 
> file with write size being multiple of page size will enjoy a steady 3x speedup.

I've coded this same optimization up once before and did see the nice
performance boost it provides.

What stopped me from committing it is that I noticed a short copy is
possible during the iov_iter_copy_from_user_atomic() call in
generic_perform_write().

I didn't ever follow up to figure out the best way to handle the short
copy in ecryptfs_write_end() and then forgot about it all. :/

How do you propose that case be handled? I suppose we could *detect* it
by not marking the page uptodate in ecryptfs_write_begin() and then if
ecryptfs_write_end() sees a non-uptodate page and len is less than
PAGE_CACHE_SIZE, we would know a short copy happened. But I'm not sure
what the acceptable options are to *handle* that case. Just return an
error? Try to fill the page in ecryptfs_write_end() (ugly, IMO)?

Adding linux-fsdevel for suggestions, since I assume other filesystems
may be doing this.

Tyler

> 
> Signed-off-by: Li Wang <liwang@xxxxxxxxxxx>
> 		Yunchuan Wen <wenyunchuan@xxxxxxxxxxxxxx>
> 
> ---
> 
>  fs/ecryptfs/mmap.c |    5 +++--
>  1 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ecryptfs/mmap.c b/fs/ecryptfs/mmap.c
> index 6a44148..9724ef2 100644
> --- a/fs/ecryptfs/mmap.c
> +++ b/fs/ecryptfs/mmap.c
> @@ -346,7 +346,8 @@ static int ecryptfs_write_begin(struct file *file,
>  			if (prev_page_end_size
>  			    >= i_size_read(page->mapping->host)) {
>  				zero_user(page, 0, PAGE_CACHE_SIZE);
> -			} else {
> +				SetPageUptodate(page);
> +			} else if (len < PAGE_CACHE_SIZE) {
>  				rc = ecryptfs_decrypt_page(page);
>  				if (rc) {
>  					printk(KERN_ERR "%s: Error decrypting "
> @@ -356,8 +357,8 @@ static int ecryptfs_write_begin(struct file *file,
>  					ClearPageUptodate(page);
>  					goto out;
>  				}
> +				SetPageUptodate(page);
>  			}
> -			SetPageUptodate(page);
>  		}
>  	}
>  	/* If creating a page or more of holes, zero them out via truncate.  
> 
> --
> 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