Linux Archive - ACK: eCryptfs: Revert to a writethrough cache model

On Mon, Aug 20, 2012 at 12:59:58PM +0100, Colin King wrote:
> From: Colin Ian King <colin.king@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@canonical.com>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> Tested-by: Colin King <colin.king@canonical.com>
> Cc: Colin King <colin.king@canonical.com>
> Cc: Thieu Le <thieule@google.com>A
> (backported fromn commit 821f7494a77627fb1ab539591c57b22cdca702d6)
> ---
> fs/ecryptfs/file.c | 32 ++------------------------------
> fs/ecryptfs/inode.c | 6 ------
> fs/ecryptfs/mmap.c | 39 +++++++++++++--------------------------
> 3 files changed, 15 insertions(+), 62 deletions(-)
>
> diff --git a/fs/ecryptfs/file.c b/fs/ecryptfs/file.c
> index 9b3ad32..3a5937b 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;
>
> static int read_or_initialize_metadata(struct dentry *dentry)
> @@ -312,14 +291,7 @@ static int ecryptfs_release(struct inode *inode, struct file *file)
> static int
> ecryptfs_fsync(struct file *file, int datasync)
> {
> - int rc = 0;
> -
> - rc = generic_file_fsync(file, datasync);
> - if (rc)
> - goto out;
> - rc = vfs_fsync(ecryptfs_file_to_lower(file), datasync);
> -out:
> - return rc;
> + return vfs_fsync(ecryptfs_file_to_lower(file), datasync);
> }
>
> static int ecryptfs_fasync(int fd, struct file *file, int flag)
> @@ -388,7 +360,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 6e57feb..654a71e 100644
> --- a/fs/ecryptfs/inode.c
> +++ b/fs/ecryptfs/inode.c
> @@ -1046,12 +1046,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])
", index, to);
> @@ -519,26 +506,26 @@ static int ecryptfs_write_end(struct file *file,
> "zeros in page with index = [0x%.16lx]
", 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])
", 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]
",
> (unsigned long long)i_size_read(ecryptfs_inode));
> - balance_dirty_pages_ratelimited(mapping);
> - rc = ecryptfs_write_inode_size_to_metadata(ecryptfs_ino de);
> - if (rc) {
> - printk(KERN_ERR "Error writing inode size to metadata; "
> - "rc = [%d]
", rc);
> - goto out;
> - }
> }
> - rc = copied;
> + rc = ecryptfs_write_inode_size_to_metadata(ecryptfs_ino de);
> + if (rc)
> + printk(KERN_ERR "Error writing inode size to metadata; "
> + "rc = [%d]
", rc);
> + else
> + rc = copied;
> out:
> - if (need_unlock_page)
> - unlock_page(page);
> + unlock_page(page);
> page_cache_release(page);
> return rc;
> }
> --
> 1.7.5.4
>
>
> --
> kernel-team mailing list
> kernel-team@lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team

--
kernel-team mailing list
kernel-team@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/kernel-team