Re: [PATCH 2/2] ecryptfs: Preallocate space in lower file in write path

[Tyler Hicks <tyhicks@xxxxxxxxxxxxxxxxxx>]

I apologize for the slow review, Thieu. When I originally reviewed this
patch, our email infrastructure was down for maintenance, so I saved it
off in a local drafts folder. I forgot about it until you pinged me. :/

Original review below...

On Thu Jul 28, 2011 at 05:57:47PM -0700, Thieu Le <thieule@xxxxxxxxxxxx> wrote:
> eCryptfs does not allocate space in the lower file until writepage.  In
> low free space situation, this leads to the application thinking the
> write succeeds but it actually fails later when the page is written out.
> This patch preallocates the space in the write path using fallocate()
> first.  For lower file systems that do not support fallocate(), it falls back
> to writing the encrypted page directly to the lower file.  The
> preallocation is only done for writes that extend the file.
> 
> Signed-off-by: Thieu Le <thieule@xxxxxxxxxxxx>
> ---
>  fs/ecryptfs/mmap.c |   26 ++++++++++++++++++++++++--
>  1 files changed, 24 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ecryptfs/mmap.c b/fs/ecryptfs/mmap.c
> index 6a44148..ed0eace 100644
> --- a/fs/ecryptfs/mmap.c
> +++ b/fs/ecryptfs/mmap.c
> @@ -520,9 +520,29 @@ static int ecryptfs_write_end(struct file *file,
>  		goto out;
>  	}
>  	set_page_dirty(page);
> -	unlock_page(page);
> -	need_unlock_page = 0;
>  	if (pos + copied > i_size_read(ecryptfs_inode)) {
> +		struct ecryptfs_inode_info *inode_info =
> +			ecryptfs_inode_to_private(ecryptfs_inode);
> +		loff_t offset = ecryptfs_lower_header_size(crypt_stat) + pos;
> +		BUG_ON(!inode_info->lower_file);
> +		rc = do_fallocate(inode_info->lower_file, 0, offset,
> +				  PAGE_CACHE_SIZE);

do_fallocate() isn't exported to modules:

WARNING: "do_fallocate" [fs/ecryptfs/ecryptfs.ko] undefined!

> +		if (rc == -EOPNOTSUPP)
> +			rc = ecryptfs_encrypt_page(page);
> +		if (rc) {
> +			if (rc != -ENOSPC) {
> +				ecryptfs_printk(KERN_ERR,
> +						"Error preallocating page "
> +						"(upper index "
> +						"[0x%.16lx], rc = [%d])\n",
> +						index, rc);
> +			}
> +			goto out;
> +		}
> +
> +		unlock_page(page);
> +		need_unlock_page = 0;
> +
>  		i_size_write(ecryptfs_inode, pos + copied);
>  		ecryptfs_printk(KERN_DEBUG, "Expanded file size to "
>  			"[0x%.16llx]\n",
> @@ -540,6 +560,8 @@ out:
>  	if (need_unlock_page)
>  		unlock_page(page);
>  	page_cache_release(page);
> +	if (rc)
> +		truncate_inode_pages(mapping, i_size_read(ecryptfs_inode));

This change seems to be silently fixing a separate issue (truncating
after a failed write).

I wonder if we should be using ecryptfs_truncate() here, instead? It
truncates the lower file, as well.

Tyler

>  	return rc;
>  }
> 
> -- 
> 1.7.3.1
> 
--
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