Re: [PATCH 5/5] union: hybrid union filesystem prototype

On Thu, Aug 26, 2010 at 08:33:45PM +0200, Miklos Szeredi wrote:
> From: Miklos Szeredi <mszeredi@suse.cz>
> 
> This union filesystem is a hybrid of entirely filesystem based
> (unionfs, aufs) and entierly VFS based (union mounts) solutions.

This is elegant and readable code.  I am still reviewing it but have a
few comments now.

> +static int union_upper_create(struct dentry *dentry, struct iattr *attr,
> +			      dev_t rdev, const char *link, struct path *src)
> +{
> +	int err;
> +	int attr_update = ATTR_UID | ATTR_GID | ATTR_ATIME_SET | ATTR_MTIME_SET;
> +	struct dentry *parent = dget_parent(dentry);
> +	struct union_entry *ue = dentry->d_fsdata;
> +	struct union_entry *pue = parent->d_fsdata;
> +	struct inode *upperdir = pue->upperpath.dentry->d_inode;
> +	struct dentry *newdentry;
> +	struct path newpath;
> +
> +	mutex_lock_nested(&upperdir->i_mutex, I_MUTEX_PARENT);
> +
> +	/*
> +	 * Using upper filesystem locking to protect against copy up
> +	 * racing with rename (rename means the copy up was already
> +	 * successful).
> +	 */
> +	err = -EEXIST;
> +	if (dentry->d_parent != parent)
> +		goto out_unlock;
> +
> +	newdentry = union_lookup_create(ue, pue, &dentry->d_name);
> +	err = PTR_ERR(newdentry);
> +	if (IS_ERR(newdentry))
> +		goto out_unlock;
> +
> +	newpath.dentry = newdentry;
> +	newpath.mnt = pue->upperpath.mnt;
> +
> +	switch (attr->ia_mode & S_IFMT) {
> +	case S_IFREG:
> +		if (src)
> +			WARN_ON(!(attr->ia_valid & ATTR_SIZE));
> +		else
> +			WARN_ON((attr->ia_valid & ATTR_SIZE));
> +
> +		err = vfs_create(upperdir, newdentry, attr->ia_mode, NULL);

Passing a NULL namiedata pointer to vfs_create() is a convenient
temporary hack, but unfortunately NFS, ceph, etc. still use the
nameidata passed to vfs_create() and other ops.

The way union mounts gets a valid nameidata is by doing the create in
the VFS before calling file system ops that may trigger a copyup,
while we still have the original nameidata.  This is one of the major
reasons union mounts lives in the VFS.

A lot of my conversations about union mounts with Al go like this:

Al: "Rewrite it this way."
Val: "But then how do we get the nameidata?"
Al: "Arrrrrrrrrrrrrggggh."

Can you think of a way to construct a good nameidata for these
implicit copyups?  That might be a solution.

-VAL