Date: Tue, 13 Jul 2010 12:56:20 +0800
From: Ian Kent <raven@themaw.net>
To: Valerie Aurora <vaurora@redhat.com>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>,
        Miklos Szeredi <miklos@szeredi.hu>, Jan Blunck <jblunck@suse.de>,
        Christoph Hellwig <hch@infradead.org>, linux-kernel@vger.kernel.org,
        linux-fsdevel@vger.kernel.org, Valdis.Kletnieks@vt.edu
Subject: Re: [PATCH 27/38] union-mount: In-kernel file copyup routines
Message-ID: <20100713045619.GI3949@zeus.themaw.net>
References: <1276627208-17242-1-git-send-email-vaurora@redhat.com>
 <1276627208-17242-28-git-send-email-vaurora@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1276627208-17242-28-git-send-email-vaurora@redhat.com>
User-Agent: Mutt/1.5.20 (2009-08-17)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 11862
Lines: 406

On Tue, Jun 15, 2010 at 11:39:57AM -0700, Valerie Aurora wrote:
> When a file on the read-only layer of a union mount is altered, it
> must be copied up to the topmost read-write layer.  This patch creates
> union_copyup() and its supporting routines.
> 
> Thanks to Valdis Kletnieks for a bug fix.
> 
> Cc: Valdis.Kletnieks@vt.edu
> ---
>  fs/union.c |  323 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  fs/union.h |    7 +-
>  2 files changed, 329 insertions(+), 1 deletions(-)
> 
> diff --git a/fs/union.c b/fs/union.c
> index 76a6c34..0982446 100644
> --- a/fs/union.c
> +++ b/fs/union.c
> @@ -24,6 +24,8 @@
>  #include <linux/namei.h>
>  #include <linux/file.h>
>  #include <linux/security.h>
> +#include <linux/splice.h>
> +#include <linux/xattr.h>
>  
>  #include "union.h"
>  
> @@ -191,6 +193,72 @@ int needs_lookup_union(struct path *parent_path, struct path *path)
>  	return 1;
>  }
>  
> +/**
> + * union_copyup_xattr
> + *
> + * @old: dentry of original file
> + * @new: dentry of new copy
> + *
> + * Copy up extended attributes from the original file to the new one.
> + *
> + * XXX - Permissions?  For now, copying up every xattr.
> + */
> +
> +static int union_copyup_xattr(struct dentry *old, struct dentry *new)
> +{
> +	ssize_t list_size, size;
> +	char *buf, *name, *value;
> +	int error;
> +
> +	/* Check for xattr support */
> +	if (!old->d_inode->i_op->getxattr ||
> +	    !new->d_inode->i_op->getxattr)
> +		return 0;
> +
> +	/* Find out how big the list of xattrs is */
> +	list_size = vfs_listxattr(old, NULL, 0);
> +	if (list_size <= 0)
> +		return list_size;
> +
> +	/* Allocate memory for the list */
> +	buf = kzalloc(list_size, GFP_KERNEL);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	/* Allocate memory for the xattr's value */
> +	error = -ENOMEM;
> +	value = kmalloc(XATTR_SIZE_MAX, GFP_KERNEL);
> +	if (!value)
> +		goto out;
> +
> +	/* Actually get the list of xattrs */
> +	list_size = vfs_listxattr(old, buf, list_size);
> +	if (list_size <= 0) {
> +		error = list_size;
> +		goto out_free_value;
> +	}
> +
> +	for (name = buf; name < (buf + list_size); name += strlen(name) + 1) {
> +		/* XXX Locking? old is on read-only fs */
> +		size = vfs_getxattr(old, name, value, XATTR_SIZE_MAX);
> +		if (size <= 0) {
> +			error = size;
> +			goto out_free_value;
> +		}
> +		/* XXX do we really need to check for size overflow? */
> +		/* XXX locks new dentry, lock ordering problems? */
> +		error = vfs_setxattr(new, name, value, size, 0);
> +		if (error)
> +			goto out_free_value;
> +	}
> +
> +out_free_value:
> +	kfree(value);
> +out:
> +	kfree(buf);
> +	return error;
> +}
> +
>  /*
>   * union_create_topmost_dir - Create a matching dir in the topmost file system
>   */
> @@ -209,6 +277,13 @@ int union_create_topmost_dir(struct path *parent, struct qstr *name,
>  
>  	res = vfs_mkdir(parent->dentry->d_inode, topmost->dentry, mode);
>  
> +	if (res)
> +		goto out;
> +
> +	res = union_copyup_xattr(lower->dentry, topmost->dentry);
> +	if (res)
> +		dput(topmost->dentry);
> +out:
>  	mnt_drop_write(parent->mnt);
>  
>  	return res;
> @@ -368,3 +443,251 @@ out_fput:
>  	mnt_drop_write(topmost_path->mnt);
>  	return res;
>  }
> +
> +/**
> + * union_create_file
> + *
> + * @nd: namediata for source file
> + * @old: path of the source file
> + * @new: path of the new file, negative dentry
> + *
> + * Must already have mnt_want_write() on the mnt and the parent's
> + * i_mutex.
> + */
> +
> +static int union_create_file(struct nameidata *nd, struct path *old,
> +			     struct dentry *new)
> +{
> +	struct path *parent = &nd->path;
> +	BUG_ON(!mutex_is_locked(&parent->dentry->d_inode->i_mutex));
> +
> +	return vfs_create(parent->dentry->d_inode, new,
> +			  old->dentry->d_inode->i_mode, nd);
> +}
> +
> +/**
> + * union_create_symlink
> + *
> + * @nd: namediata for source symlink
> + * @old: path of the source symlink
> + * @new: path of the new symlink, negative dentry
> + *
> + * Must already have mnt_want_write() on the mnt and the parent's
> + * i_mutex.
> + */
> +
> +static int union_create_symlink(struct nameidata *nd, struct path *old,
> +				struct dentry *new)
> +{
> +	void *cookie;
> +	int error;
> +
> +	BUG_ON(!mutex_is_locked(&nd->path.dentry->d_inode->i_mutex));
> +	/*
> +	 * We want the contents of this symlink, not to follow it, so
> +	 * this is modeled on generic_readlink() rather than
> +	 * do_follow_link().
> +	 */
> +	nd->depth = 0;
> +	cookie = old->dentry->d_inode->i_op->follow_link(old->dentry, nd);
> +	if (IS_ERR(cookie))
> +		return PTR_ERR(cookie);
> +	/* Create a copy of the link on the top layer */
> +	error = vfs_symlink(nd->path.dentry->d_inode, new,
> +			    nd_get_link(nd));
> +	if (old->dentry->d_inode->i_op->put_link)
> +		old->dentry->d_inode->i_op->put_link(old->dentry, nd, cookie);
> +	return error;
> +}
> +
> +/**
> + * union_copyup_data - Copy up len bytes of old's data to new
> + *
> + * @old: path of source file
> + * @new_mnt: vfsmount of target file
> + * @new_dentry: dentry of target file
> + * @len: number of bytes to copy
> + */
> +
> +static int union_copyup_data(struct path *old, struct vfsmount *new_mnt,
> +			     struct dentry *new_dentry, size_t len)
> +{
> +	struct file *old_file;
> +	struct file *new_file;
> +	const struct cred *cred = current_cred();
> +	loff_t offset = 0;
> +	long bytes;
> +	int error = 0;
> +
> +	if (len == 0)
> +		return 0;
> +
> +	/* Get reference to balance later fput() */
> +	path_get(old);
> +	old_file = dentry_open(old->dentry, old->mnt, O_RDONLY, cred);
> +	if (IS_ERR(old_file))
> +		return PTR_ERR(old_file);
> +
> +	mntget(new_mnt);
> +	dget(new_dentry);
> +	new_file = dentry_open(new_dentry, new_mnt, O_WRONLY, cred);
> +	if (IS_ERR(new_file)) {
> +		error = PTR_ERR(new_file);
> +		goto out_fput;
> +	}
> +
> +	bytes = do_splice_direct(old_file, &offset, new_file, len,
> +				 SPLICE_F_MOVE);
> +	if (bytes < 0)
> +		error = bytes;
> +
> +	fput(new_file);
> +out_fput:
> +	fput(old_file);
> +	return error;
> +}
> +
> +/**
> + * __union_copyup_len - Copy up a file and len bytes of data
> + *
> + * @nd: nameidata for topmost parent dir
> + * @path: path of file to be copied up
> + * @len: number of bytes of file data to copy up
> + *
> + * Parent's i_mutex must be held by caller.  Newly copied up path is
> + * returned in @path and original is path_put().
> + */
> +
> +static int __union_copyup_len(struct nameidata *nd, struct path *path,
> +			      size_t len)
> +{
> +	struct path *parent = &nd->path;
> +	struct dentry *dentry;
> +	int error;
> +
> +	BUG_ON(!mutex_is_locked(&parent->dentry->d_inode->i_mutex));
> +
> +	dentry = lookup_one_len(path->dentry->d_name.name, parent->dentry,
> +				path->dentry->d_name.len);
> +	if (IS_ERR(dentry))
> +		return PTR_ERR(dentry);
> +
> +	if (dentry->d_inode) {
> +		/*
> +		 * We raced with someone else and "lost."  That's
> +		 * okay, they did all the work of copying up the file.
> +		 * Note that currently data copyup happens under the
> +		 * parent dir's i_mutex.  If we move it outside that,
> +		 * we'll need some way of waiting for the data copyup
> +		 * to complete here.
> +		 */
> +		error = 0;
> +		goto out_newpath;
> +	}
> +	if (S_ISREG(path->dentry->d_inode->i_mode)) {
> +		/* Create file */
> +		error = union_create_file(nd, path, dentry);
> +		if (error)
> +			goto out_dput;
> +		/* Copyup data */
> +		error = union_copyup_data(path, parent->mnt, dentry, len);
> +	} else {
> +		BUG_ON(!S_ISLNK(path->dentry->d_inode->i_mode));
> +		error = union_create_symlink(nd, path, dentry);
> +	}
> +	if (error) {
> +		/* Most likely error: ENOSPC */
> +		vfs_unlink(parent->dentry->d_inode, dentry);
> +		goto out_dput;
> +	}
> +	/* XXX Copyup xattrs and any other dangly bits */
> +	error = union_copyup_xattr(path->dentry, dentry);
> +	if (error)
> +		goto out_dput;
> +out_newpath:
> +	/* path_put() of original must happen before we copy in new */
> +	path_put(path);
> +	path->dentry = dentry;
> +	path->mnt = mntget(parent->mnt);
> +	return error;
> +out_dput:
> +	/* Don't path_put(path), let caller unwind */
> +	dput(dentry);
> +	return error;
> +}
> +
> +/**
> + * do_union_copyup_len - Copy up a file given its path (and its parent's)
> + *
> + * @nd: nameidata for topmost parent dir
> + * @path: path of file to be copied up
> + * @copy_all: if set, copy all of the file's data and ignore @len
> + * @len: if @copy_all is not set, number of bytes of file data to copy up
> + *
> + * Newly copied up path is returned in @path.
> + */
> +
> +static int do_union_copyup_len(struct nameidata *nd, struct path *path,
> +			       int copy_all, size_t len)
> +{
> +	struct path *parent = &nd->path;
> +	int error;
> +
> +	if (!IS_DIR_UNIONED(parent->dentry))
> +		return 0;
> +	if (parent->mnt == path->mnt)
> +		return 0;
> +	if (!S_ISREG(path->dentry->d_inode->i_mode) &&
> +	    !S_ISLNK(path->dentry->d_inode->i_mode))
> +		return 0;
> +
> +	BUG_ON(!S_ISDIR(parent->dentry->d_inode->i_mode));
> +
> +	mutex_lock(&parent->dentry->d_inode->i_mutex);
> +	error = -ENOENT;
> +	if (IS_DEADDIR(parent->dentry->d_inode))
> +		goto out_unlock;
> +
> +	if (copy_all && S_ISREG(path->dentry->d_inode->i_mode)) {
> +		error = -EFBIG;
> +		len = i_size_read(path->dentry->d_inode);
> +		if (((size_t)len != len) || ((ssize_t)len != len))
> +			goto out_unlock;

OK, call me dumb, but what does this comparison of len to len do?

> +	}
> +
> +	error = __union_copyup_len(nd, path, len);
> +
> +out_unlock:
> +	mutex_unlock(&parent->dentry->d_inode->i_mutex);
> +	return error;
> +}
> +
> +/*
> + * Helper function to copy up all of a file
> + */
> +int union_copyup(struct nameidata *nd, struct path *path)
> +{
> +	return do_union_copyup_len(nd, path, 1, 0);
> +}
> +
> +/*
> + * Unlocked helper function to copy up all of a file
> + */
> +int __union_copyup(struct nameidata *nd, struct path *path)
> +{
> +	size_t len;
> +	len = i_size_read(path->dentry->d_inode);
> +	if (((size_t)len != len) || ((ssize_t)len != len))
> +		return -EFBIG;
> +
> +	return __union_copyup_len(nd, path, len);
> +}
> +
> +/*
> + * Helper function to copy up part of a file
> + */
> +int union_copyup_len(struct nameidata *nd, struct path *path, size_t len)
> +{
> +	return do_union_copyup_len(nd, path, 0, len);
> +}
> +
> diff --git a/fs/union.h b/fs/union.h
> index 80c2421..01fa183 100644
> --- a/fs/union.h
> +++ b/fs/union.h
> @@ -59,7 +59,9 @@ int needs_lookup_union(struct path *, struct path *);
>  int union_create_topmost_dir(struct path *, struct qstr *, struct path *,
>  			     struct path *);
>  extern int union_copyup_dir(struct path *);
> -
> +extern int union_copyup(struct nameidata *, struct path *);
> +extern int __union_copyup(struct nameidata *, struct path *);
> +extern int union_copyup_len(struct nameidata *, struct path *, size_t len);
>  #else /* CONFIG_UNION_MOUNT */
>  
>  #define IS_MNT_UNION(x)			(0)
> @@ -69,6 +71,9 @@ extern int union_copyup_dir(struct path *);
>  #define needs_lookup_union(x, y)	({ (0); })
>  #define union_create_topmost_dir(w, x, y, z)	({ BUG(); (NULL); })
>  #define union_copyup_dir(x)		({ BUG(); (0); })
> +#define union_copyup(x, y)		({ BUG(); (NULL); })
> +#define __union_copyup(x, y)		({ BUG(); (NULL); })
> +#define union_copyup_len(x, y, z)	({ BUG(); (NULL); })
>  
>  #endif	/* CONFIG_UNION_MOUNT */
>  #endif	/* __KERNEL__ */
> -- 
> 1.6.3.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/