ACK: [Precise SRU PATCH] eCryptfs: Revert to a writethrough cache model

Stefan Bader stefan.bader at canonical.com
Tue Aug 7 15:54:16 UTC 2012


On 07.08.2012 16:36, Tim Gardner wrote:
> From: Tyler Hicks <tyhicks at canonical.com>
> 
> BugLink: http://bugs.launchpad.net/bugs/1034012
> 
> A change was made about a year ago to get eCryptfs to better utilize its
> page cache during writes. The idea was to do the page encryption
> operations during page writeback, rather than doing them when initially
> writing into the page cache, to reduce the number of page encryption
> operations during sequential writes. This meant that the encrypted page
> would only be written to the lower filesystem during page writeback,
> which was a change from how eCryptfs had previously wrote to the lower
> filesystem in ecryptfs_write_end().
> 
> The change caused a few eCryptfs-internal bugs that were shook out.
> Unfortunately, more grave side effects have been identified that will
> force changes outside of eCryptfs. Because the lower filesystem isn't
> consulted until page writeback, eCryptfs has no way to pass lower write
> errors (ENOSPC, mainly) back to userspace. Additionaly, it was reported
> that quotas could be bypassed because of the way eCryptfs may sometimes
> open the lower filesystem using a privileged kthread.
> 
> It would be nice to resolve the latest issues, but it is best if the
> eCryptfs commits be reverted to the old behavior in the meantime.
> 
> This reverts:
> 32001d6f "eCryptfs: Flush file in vma close"
> 5be79de2 "eCryptfs: Flush dirty pages in setattr"
> 57db4e8d "ecryptfs: modify write path to encrypt page in writepage"
> 
> Signed-off-by: Tyler Hicks <tyhicks at canonical.com>
> Tested-by: Colin King <colin.king at canonical.com>
> Cc: Colin King <colin.king at canonical.com>
> Cc: Thieu Le <thieule at google.com>
> (cherry picked from commit 821f7494a77627fb1ab539591c57b22cdca702d6)
> 
> Signed-off-by: Tim Gardner <tim.gardner at canonical.com>
> ---
>  fs/ecryptfs/file.c  |   33 ++-------------------------------
>  fs/ecryptfs/inode.c |    6 ------
>  fs/ecryptfs/mmap.c  |   39 +++++++++++++--------------------------
>  3 files changed, 15 insertions(+), 63 deletions(-)
> 
> diff --git a/fs/ecryptfs/file.c b/fs/ecryptfs/file.c
> index d3f95f9..6860fd4 100644
> --- a/fs/ecryptfs/file.c
> +++ b/fs/ecryptfs/file.c
> @@ -139,27 +139,6 @@ out:
>  	return rc;
>  }
>  
> -static void ecryptfs_vma_close(struct vm_area_struct *vma)
> -{
> -	filemap_write_and_wait(vma->vm_file->f_mapping);
> -}
> -
> -static const struct vm_operations_struct ecryptfs_file_vm_ops = {
> -	.close		= ecryptfs_vma_close,
> -	.fault		= filemap_fault,
> -};
> -
> -static int ecryptfs_file_mmap(struct file *file, struct vm_area_struct *vma)
> -{
> -	int rc;
> -
> -	rc = generic_file_mmap(file, vma);
> -	if (!rc)
> -		vma->vm_ops = &ecryptfs_file_vm_ops;
> -
> -	return rc;
> -}
> -
>  struct kmem_cache *ecryptfs_file_info_cache;
>  
>  /**
> @@ -293,15 +272,7 @@ static int ecryptfs_release(struct inode *inode, struct file *file)
>  static int
>  ecryptfs_fsync(struct file *file, loff_t start, loff_t end, int datasync)
>  {
> -	int rc = 0;
> -
> -	rc = generic_file_fsync(file, start, end, datasync);
> -	if (rc)
> -		goto out;
> -	rc = vfs_fsync_range(ecryptfs_file_to_lower(file), start, end,
> -			     datasync);
> -out:
> -	return rc;
> +	return vfs_fsync(ecryptfs_file_to_lower(file), datasync);
>  }
>  
>  static int ecryptfs_fasync(int fd, struct file *file, int flag)
> @@ -370,7 +341,7 @@ const struct file_operations ecryptfs_main_fops = {
>  #ifdef CONFIG_COMPAT
>  	.compat_ioctl = ecryptfs_compat_ioctl,
>  #endif
> -	.mmap = ecryptfs_file_mmap,
> +	.mmap = generic_file_mmap,
>  	.open = ecryptfs_open,
>  	.flush = ecryptfs_flush,
>  	.release = ecryptfs_release,
> diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c
> index af11098..445707c 100644
> --- a/fs/ecryptfs/inode.c
> +++ b/fs/ecryptfs/inode.c
> @@ -1021,12 +1021,6 @@ static int ecryptfs_setattr(struct dentry *dentry, struct iattr *ia)
>  			goto out;
>  	}
>  
> -	if (S_ISREG(inode->i_mode)) {
> -		rc = filemap_write_and_wait(inode->i_mapping);
> -		if (rc)
> -			goto out;
> -		fsstack_copy_attr_all(inode, lower_inode);
> -	}
>  	memcpy(&lower_ia, ia, sizeof(lower_ia));
>  	if (ia->ia_valid & ATTR_FILE)
>  		lower_ia.ia_file = ecryptfs_file_to_lower(ia->ia_file);
> diff --git a/fs/ecryptfs/mmap.c b/fs/ecryptfs/mmap.c
> index 6a44148..93a998a 100644
> --- a/fs/ecryptfs/mmap.c
> +++ b/fs/ecryptfs/mmap.c
> @@ -62,18 +62,6 @@ static int ecryptfs_writepage(struct page *page, struct writeback_control *wbc)
>  {
>  	int rc;
>  
> -	/*
> -	 * 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);
> -		rc = 0;
> -		goto out;
> -	}
> -
>  	rc = ecryptfs_encrypt_page(page);
>  	if (rc) {
>  		ecryptfs_printk(KERN_WARNING, "Error encrypting "
> @@ -498,7 +486,6 @@ static int ecryptfs_write_end(struct file *file,
>  	struct ecryptfs_crypt_stat *crypt_stat =
>  		&ecryptfs_inode_to_private(ecryptfs_inode)->crypt_stat;
>  	int rc;
> -	int need_unlock_page = 1;
>  
>  	ecryptfs_printk(KERN_DEBUG, "Calling fill_zeros_to_end_of_page"
>  			"(page w/ index = [0x%.16lx], to = [%d])\n", index, to);
> @@ -519,26 +506,26 @@ static int ecryptfs_write_end(struct file *file,
>  			"zeros in page with index = [0x%.16lx]\n", index);
>  		goto out;
>  	}
> -	set_page_dirty(page);
> -	unlock_page(page);
> -	need_unlock_page = 0;
> +	rc = ecryptfs_encrypt_page(page);
> +	if (rc) {
> +		ecryptfs_printk(KERN_WARNING, "Error encrypting page (upper "
> +				"index [0x%.16lx])\n", index);
> +		goto out;
> +	}
>  	if (pos + copied > i_size_read(ecryptfs_inode)) {
>  		i_size_write(ecryptfs_inode, pos + copied);
>  		ecryptfs_printk(KERN_DEBUG, "Expanded file size to "
>  			"[0x%.16llx]\n",
>  			(unsigned long long)i_size_read(ecryptfs_inode));
> -		balance_dirty_pages_ratelimited(mapping);
> -		rc = ecryptfs_write_inode_size_to_metadata(ecryptfs_inode);
> -		if (rc) {
> -			printk(KERN_ERR "Error writing inode size to metadata; "
> -			       "rc = [%d]\n", rc);
> -			goto out;
> -		}
>  	}
> -	rc = copied;
> +	rc = ecryptfs_write_inode_size_to_metadata(ecryptfs_inode);
> +	if (rc)
> +		printk(KERN_ERR "Error writing inode size to metadata; "
> +		       "rc = [%d]\n", rc);
> +	else
> +		rc = copied;
>  out:
> -	if (need_unlock_page)
> -		unlock_page(page);
> +	unlock_page(page);
>  	page_cache_release(page);
>  	return rc;
>  }
> 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 900 bytes
Desc: OpenPGP digital signature
URL: <https://lists.ubuntu.com/archives/kernel-team/attachments/20120807/8505693c/attachment.sig>


More information about the kernel-team mailing list