GIT: unionfs2-2.6.27.y: Unionfs: Change the semantics of sb info's rwsem

Erez Zadok ezk at fsl.cs.sunysb.edu
Thu Aug 12 23:16:42 EDT 2010


commit bdb6fb050b62ec401535d64e053cd30c0e87b2b8
Author: Erez_Zadok <ezk at cs.sunysb.edu>
Date:   Fri Jun 29 02:00:24 2007 -0400

    Unionfs: Change the semantics of sb info's rwsem
    
    This rw semaphore is used to make sure that a branch management
    operation...
    
    1) will not begin before all currently in-flight operations complete
    
    2) any new operations do not execute until the currently running branch
    management operation completes
    
    Reworked the patch a bit, added comments, and fixed some bugs, from the
    version originally committed into the master branch.
    
    TODO: rename the functions unionfs_{read,write}_{,un}lock() to something
    more descriptive.
    
    Signed-off-by: Josef 'Jeff' Sipek <jsipek at cs.sunysb.edu>
    Signed-off-by: Erez Zadok <ezk at cs.sunysb.edu>

diff --git a/fs/unionfs/commonfops.c b/fs/unionfs/commonfops.c
index 4cfe321..f7f49b4 100644
--- a/fs/unionfs/commonfops.c
+++ b/fs/unionfs/commonfops.c
@@ -127,10 +127,8 @@ static void cleanup_file(struct file *file)
 				printk(KERN_ERR "unionfs: no superblock for "
 				       "file %p\n", file);
 			else {
-				unionfs_read_lock(sb);
 				/* decrement count of open files */
 				branchput(sb, i);
-				unionfs_read_unlock(sb);
 				/*
 				 * fput will perform an mntput for us on the
 				 * correct branch.  Although we're using the
@@ -172,9 +170,7 @@ static int open_all_files(struct file *file)
 
 		dget(lower_dentry);
 		unionfs_mntget(dentry, bindex);
-		unionfs_read_lock(sb);
 		branchget(sb, bindex);
-		unionfs_read_unlock(sb);
 
 		lower_file =
 			dentry_open(lower_dentry,
@@ -220,9 +216,7 @@ static int open_highest_file(struct file *file, int willwrite)
 
 	dget(lower_dentry);
 	unionfs_mntget(dentry, bstart);
-	unionfs_read_lock(sb);
 	branchget(sb, bstart);
-	unionfs_read_unlock(sb);
 	lower_file = dentry_open(lower_dentry,
 				 unionfs_lower_mnt_idx(dentry, bstart),
 				 file->f_flags);
@@ -270,9 +264,7 @@ static int do_delayed_copyup(struct file *file)
 	bend = fbend(file);
 	for (bindex = bstart; bindex <= bend; bindex++) {
 		if (unionfs_lower_file_idx(file, bindex)) {
-			unionfs_read_lock(dentry->d_sb);
 			branchput(dentry->d_sb, bindex);
-			unionfs_read_unlock(dentry->d_sb);
 			fput(unionfs_lower_file_idx(file, bindex));
 			unionfs_set_lower_file_idx(file, bindex, NULL);
 		}
@@ -430,9 +422,7 @@ static int __open_dir(struct inode *inode, struct file *file)
 		 * The branchget goes after the open, because otherwise
 		 * we would miss the reference on release.
 		 */
-		unionfs_read_lock(inode->i_sb);
 		branchget(inode->i_sb, bindex);
-		unionfs_read_unlock(inode->i_sb);
 	}
 
 	return 0;
@@ -493,9 +483,7 @@ static int __open_file(struct inode *inode, struct file *file)
 		return PTR_ERR(lower_file);
 
 	unionfs_set_lower_file(file, lower_file);
-	unionfs_read_lock(inode->i_sb);
 	branchget(inode->i_sb, bstart);
-	unionfs_read_unlock(inode->i_sb);
 
 	return 0;
 }
@@ -509,6 +497,7 @@ int unionfs_open(struct inode *inode, struct file *file)
 	int size;
 
 	unionfs_read_lock(inode->i_sb);
+
 	file->private_data =
 		kzalloc(sizeof(struct unionfs_file_info), GFP_KERNEL);
 	if (!UNIONFS_F(file)) {
@@ -559,9 +548,7 @@ int unionfs_open(struct inode *inode, struct file *file)
 			if (!lower_file)
 				continue;
 
-			unionfs_read_lock(file->f_dentry->d_sb);
 			branchput(file->f_dentry->d_sb, bindex);
-			unionfs_read_unlock(file->f_dentry->d_sb);
 			/* fput calls dput for lower_dentry */
 			fput(lower_file);
 		}
@@ -585,7 +572,11 @@ out_nofree:
 	return err;
 }
 
-/* release all lower object references & free the file info structure */
+/*
+ * release all lower object references & free the file info structure
+ *
+ * No need to grab sb info's rwsem.
+ */
 int unionfs_file_release(struct inode *inode, struct file *file)
 {
 	struct file *lower_file = NULL;
@@ -618,9 +609,7 @@ int unionfs_file_release(struct inode *inode, struct file *file)
 
 		if (lower_file) {
 			fput(lower_file);
-			unionfs_read_lock(sb);
 			branchput(sb, bindex);
-			unionfs_read_unlock(sb);
 		}
 	}
 	kfree(fileinfo->lower_files);
@@ -742,7 +731,8 @@ long unionfs_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 {
 	long err;
 
-	unionfs_read_lock(file->f_dentry->d_sb);
+	unionfs_read_lock(file->f_path.dentry->d_sb);
+
 	if ((err = unionfs_file_revalidate(file, 1)))
 		goto out;
 
@@ -767,7 +757,7 @@ long unionfs_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 	}
 
 out:
-	unionfs_read_unlock(file->f_dentry->d_sb);
+	unionfs_read_unlock(file->f_path.dentry->d_sb);
 	unionfs_check_file(file);
 	return err;
 }
@@ -776,7 +766,7 @@ int unionfs_flush(struct file *file, fl_owner_t id)
 {
 	int err = 0;
 	struct file *lower_file = NULL;
-	struct dentry *dentry = file->f_dentry;
+	struct dentry *dentry = file->f_path.dentry;
 	int bindex, bstart, bend;
 
 	unionfs_read_lock(dentry->d_sb);
diff --git a/fs/unionfs/copyup.c b/fs/unionfs/copyup.c
index 359bbcc..9a68813 100644
--- a/fs/unionfs/copyup.c
+++ b/fs/unionfs/copyup.c
@@ -193,9 +193,7 @@ static int __copyup_reg_data(struct dentry *dentry,
 
 	/* open old file */
 	unionfs_mntget(dentry, old_bindex);
-	unionfs_read_lock(sb);
 	branchget(sb, old_bindex);
-	unionfs_read_unlock(sb);
 	/* dentry_open calls dput and mntput if it returns an error */
 	input_file = dentry_open(old_lower_dentry,
 				 unionfs_lower_mnt_idx(dentry, old_bindex),
@@ -213,9 +211,7 @@ static int __copyup_reg_data(struct dentry *dentry,
 	/* open new file */
 	dget(new_lower_dentry);
 	output_mnt = unionfs_mntget(sb->s_root, new_bindex);
-	unionfs_read_lock(sb);
 	branchget(sb, new_bindex);
-	unionfs_read_unlock(sb);
 	output_file = dentry_open(new_lower_dentry, output_mnt,
 				  O_WRONLY | O_LARGEFILE);
 	if (IS_ERR(output_file)) {
@@ -290,17 +286,13 @@ out_close_out:
 	fput(output_file);
 
 out_close_in2:
-	unionfs_read_lock(sb);
 	branchput(sb, new_bindex);
-	unionfs_read_unlock(sb);
 
 out_close_in:
 	fput(input_file);
 
 out:
-	unionfs_read_lock(sb);
 	branchput(sb, old_bindex);
-	unionfs_read_unlock(sb);
 
 	return err;
 }
@@ -453,9 +445,7 @@ out_unlink:
 		/* need to close the file */
 
 		fput(*copyup_file);
-		unionfs_read_lock(sb);
 		branchput(sb, new_bindex);
-		unionfs_read_unlock(sb);
 	}
 
 	/*
diff --git a/fs/unionfs/dentry.c b/fs/unionfs/dentry.c
index 6310c32..fb425ed 100644
--- a/fs/unionfs/dentry.c
+++ b/fs/unionfs/dentry.c
@@ -408,11 +408,15 @@ static int unionfs_d_revalidate(struct dentry *dentry, struct nameidata *nd)
 {
 	int err;
 
+	unionfs_read_lock(dentry->d_sb);
+
 	unionfs_lock_dentry(dentry);
 	err = __unionfs_d_revalidate_chain(dentry, nd, 0);
 	unionfs_unlock_dentry(dentry);
 	unionfs_check_dentry(dentry);
 
+	unionfs_read_unlock(dentry->d_sb);
+
 	return err;
 }
 
@@ -424,6 +428,8 @@ static void unionfs_d_release(struct dentry *dentry)
 {
 	int bindex, bstart, bend;
 
+	unionfs_read_lock(dentry->d_sb);
+
 	unionfs_check_dentry(dentry);
 	/* this could be a negative dentry, so check first */
 	if (!UNIONFS_D(dentry)) {
@@ -459,6 +465,7 @@ out_free:
 	free_dentry_private_data(dentry);
 
 out:
+	unionfs_read_unlock(dentry->d_sb);
 	return;
 }
 
diff --git a/fs/unionfs/dirfops.c b/fs/unionfs/dirfops.c
index a7ba947..8503411 100644
--- a/fs/unionfs/dirfops.c
+++ b/fs/unionfs/dirfops.c
@@ -97,7 +97,8 @@ static int unionfs_readdir(struct file *file, void *dirent, filldir_t filldir)
 	int bend;
 	loff_t offset;
 
-	unionfs_read_lock(file->f_dentry->d_sb);
+	unionfs_read_lock(file->f_path.dentry->d_sb);
+
 	if ((err = unionfs_file_revalidate(file, 0)))
 		goto out;
 
@@ -179,7 +180,7 @@ static int unionfs_readdir(struct file *file, void *dirent, filldir_t filldir)
 		file->f_pos = rdstate2offset(uds);
 
 out:
-	unionfs_read_unlock(file->f_dentry->d_sb);
+	unionfs_read_unlock(file->f_path.dentry->d_sb);
 	return err;
 }
 
@@ -198,7 +199,8 @@ static loff_t unionfs_dir_llseek(struct file *file, loff_t offset, int origin)
 	struct unionfs_dir_state *rdstate;
 	loff_t err;
 
-	unionfs_read_lock(file->f_dentry->d_sb);
+	unionfs_read_lock(file->f_path.dentry->d_sb);
+
 	if ((err = unionfs_file_revalidate(file, 0)))
 		goto out;
 
@@ -255,7 +257,7 @@ static loff_t unionfs_dir_llseek(struct file *file, loff_t offset, int origin)
 	}
 
 out:
-	unionfs_read_unlock(file->f_dentry->d_sb);
+	unionfs_read_unlock(file->f_path.dentry->d_sb);
 	return err;
 }
 
diff --git a/fs/unionfs/dirhelper.c b/fs/unionfs/dirhelper.c
index 80d1427..a72f711 100644
--- a/fs/unionfs/dirhelper.c
+++ b/fs/unionfs/dirhelper.c
@@ -97,7 +97,6 @@ int delete_whiteouts(struct dentry *dentry, int bindex,
 	struct sioq_args args;
 
 	sb = dentry->d_sb;
-	unionfs_read_lock(sb);
 
 	BUG_ON(!S_ISDIR(dentry->d_inode->i_mode));
 	BUG_ON(bindex < dbstart(dentry));
@@ -124,7 +123,6 @@ int delete_whiteouts(struct dentry *dentry, int bindex,
 	mutex_unlock(&lower_dir->i_mutex);
 
 out:
-	unionfs_read_unlock(sb);
 	return err;
 }
 
@@ -193,7 +191,6 @@ int check_empty(struct dentry *dentry, struct unionfs_dir_state **namelist)
 
 	sb = dentry->d_sb;
 
-	unionfs_read_lock(sb);
 
 	BUG_ON(!S_ISDIR(dentry->d_inode->i_mode));
 
@@ -231,9 +228,7 @@ int check_empty(struct dentry *dentry, struct unionfs_dir_state **namelist)
 
 		dget(lower_dentry);
 		unionfs_mntget(dentry, bindex);
-		unionfs_read_lock(sb);
 		branchget(sb, bindex);
-		unionfs_read_unlock(sb);
 		lower_file =
 			dentry_open(lower_dentry,
 				    unionfs_lower_mnt_idx(dentry, bindex),
@@ -241,9 +236,7 @@ int check_empty(struct dentry *dentry, struct unionfs_dir_state **namelist)
 		if (IS_ERR(lower_file)) {
 			err = PTR_ERR(lower_file);
 			dput(lower_dentry);
-			unionfs_read_lock(sb);
 			branchput(sb, bindex);
-			unionfs_read_unlock(sb);
 			goto out;
 		}
 
@@ -258,9 +251,7 @@ int check_empty(struct dentry *dentry, struct unionfs_dir_state **namelist)
 
 		/* fput calls dput for lower_dentry */
 		fput(lower_file);
-		unionfs_read_lock(sb);
 		branchput(sb, bindex);
-		unionfs_read_unlock(sb);
 
 		if (err < 0)
 			goto out;
@@ -275,7 +266,6 @@ out:
 		kfree(buf);
 	}
 
-	unionfs_read_unlock(sb);
 
 	return err;
 }
diff --git a/fs/unionfs/file.c b/fs/unionfs/file.c
index df413eb..8f42da3 100644
--- a/fs/unionfs/file.c
+++ b/fs/unionfs/file.c
@@ -23,7 +23,7 @@ static ssize_t unionfs_read(struct file *file, char __user *buf,
 {
 	int err;
 
-	unionfs_read_lock(file->f_dentry->d_sb);
+	unionfs_read_lock(file->f_path.dentry->d_sb);
 	if ((err = unionfs_file_revalidate(file, 0)))
 		goto out;
 	unionfs_check_file(file);
@@ -35,7 +35,7 @@ static ssize_t unionfs_read(struct file *file, char __user *buf,
 			    unionfs_lower_dentry(file->f_path.dentry));
 
 out:
-	unionfs_read_unlock(file->f_dentry->d_sb);
+	unionfs_read_unlock(file->f_path.dentry->d_sb);
 	unionfs_check_file(file);
 	return err;
 }
@@ -46,7 +46,7 @@ static ssize_t unionfs_aio_read(struct kiocb *iocb, const struct iovec *iov,
 	int err = 0;
 	struct file *file = iocb->ki_filp;
 
-	unionfs_read_lock(file->f_dentry->d_sb);
+	unionfs_read_lock(file->f_path.dentry->d_sb);
 	if ((err = unionfs_file_revalidate(file, 0)))
 		goto out;
 	unionfs_check_file(file);
@@ -61,7 +61,7 @@ static ssize_t unionfs_aio_read(struct kiocb *iocb, const struct iovec *iov,
 			    unionfs_lower_dentry(file->f_path.dentry));
 
 out:
-	unionfs_read_unlock(file->f_dentry->d_sb);
+	unionfs_read_unlock(file->f_path.dentry->d_sb);
 	unionfs_check_file(file);
 	return err;
 }
@@ -71,7 +71,7 @@ static ssize_t unionfs_write(struct file *file, const char __user *buf,
 {
 	int err = 0;
 
-	unionfs_read_lock(file->f_dentry->d_sb);
+	unionfs_read_lock(file->f_path.dentry->d_sb);
 	if ((err = unionfs_file_revalidate(file, 1)))
 		goto out;
 	unionfs_check_file(file);
@@ -84,7 +84,7 @@ static ssize_t unionfs_write(struct file *file, const char __user *buf,
 	}
 
 out:
-	unionfs_read_unlock(file->f_dentry->d_sb);
+	unionfs_read_unlock(file->f_path.dentry->d_sb);
 	return err;
 }
 
@@ -100,7 +100,7 @@ static int unionfs_mmap(struct file *file, struct vm_area_struct *vma)
 	int willwrite;
 	struct file *lower_file;
 
-	unionfs_read_lock(file->f_dentry->d_sb);
+	unionfs_read_lock(file->f_path.dentry->d_sb);
 
 	/* This might be deferred to mmap's writepage */
 	willwrite = ((vma->vm_flags | VM_SHARED | VM_WRITE) == vma->vm_flags);
@@ -130,7 +130,7 @@ static int unionfs_mmap(struct file *file, struct vm_area_struct *vma)
 	}
 
 out:
-	unionfs_read_unlock(file->f_dentry->d_sb);
+	unionfs_read_unlock(file->f_path.dentry->d_sb);
 	if (!err) {
 		/* copyup could cause parent dir times to change */
 		unionfs_copy_attr_times(file->f_dentry->d_parent->d_inode);
diff --git a/fs/unionfs/inode.c b/fs/unionfs/inode.c
index 20b234d..384800c 100644
--- a/fs/unionfs/inode.c
+++ b/fs/unionfs/inode.c
@@ -30,16 +30,6 @@ static int unionfs_create(struct inode *parent, struct dentry *dentry,
 	char *name = NULL;
 	int valid = 0;
 
-	/*
-	 * We have to read-lock the superblock rwsem, and we have to
-	 * revalidate the parent dentry and this one.  A branch-management
-	 * operation could have taken place, mid-way through a VFS operation
-	 * that eventually reaches here.  So we have to ensure consistency,
-	 * just as we do with the file operations.
-	 *
-	 * XXX: we may need to do this for all other inode ops that take a
-	 * dentry.
-	 */
 	unionfs_read_lock(dentry->d_sb);
 	unionfs_lock_dentry(dentry);
 
@@ -244,6 +234,11 @@ out:
 	return err;
 }
 
+/*
+ * unionfs_lookup is the only special function which takes a dentry, yet we
+ * do NOT want to call __unionfs_d_revalidate_chain because by definition,
+ * we don't have a valid dentry here yet.
+ */
 static struct dentry *unionfs_lookup(struct inode *parent,
 				     struct dentry *dentry,
 				     struct nameidata *nd)
@@ -251,11 +246,7 @@ static struct dentry *unionfs_lookup(struct inode *parent,
 	struct path path_save;
 	struct dentry *ret;
 
-	/*
-	 * lookup is the only special function which takes a dentry, yet we
-	 * do NOT want to call __unionfs_d_revalidate_chain because by
-	 * definition, we don't have a valid dentry here yet.
-	 */
+	unionfs_read_lock(dentry->d_sb);
 
 	/* save the dentry & vfsmnt from namei */
 	if (nd) {
@@ -281,6 +272,8 @@ static struct dentry *unionfs_lookup(struct inode *parent,
 	unionfs_check_inode(parent);
 	unionfs_check_dentry(dentry);
 	unionfs_check_dentry(dentry->d_parent);
+	unionfs_read_unlock(dentry->d_sb);
+
 	return ret;
 }
 
@@ -294,6 +287,7 @@ static int unionfs_link(struct dentry *old_dentry, struct inode *dir,
 	struct dentry *whiteout_dentry;
 	char *name = NULL;
 
+	unionfs_read_lock(old_dentry->d_sb);
 	unionfs_double_lock_dentry(new_dentry, old_dentry);
 
 	if (!__unionfs_d_revalidate_chain(old_dentry, NULL, 0)) {
@@ -426,6 +420,8 @@ out:
 	unionfs_check_inode(dir);
 	unionfs_check_dentry(new_dentry);
 	unionfs_check_dentry(old_dentry);
+	unionfs_read_unlock(old_dentry->d_sb);
+
 	return err;
 }
 
@@ -440,6 +436,7 @@ static int unionfs_symlink(struct inode *dir, struct dentry *dentry,
 	int bindex = 0, bstart;
 	char *name = NULL;
 
+	unionfs_read_lock(dentry->d_sb);
 	unionfs_lock_dentry(dentry);
 
 	if (dentry->d_inode &&
@@ -585,6 +582,8 @@ out:
 
 	unionfs_check_inode(dir);
 	unionfs_check_dentry(dentry);
+	unionfs_read_unlock(dentry->d_sb);
+
 	return err;
 }
 
@@ -598,6 +597,7 @@ static int unionfs_mkdir(struct inode *parent, struct dentry *dentry, int mode)
 	int whiteout_unlinked = 0;
 	struct sioq_args args;
 
+	unionfs_read_lock(dentry->d_sb);
 	unionfs_lock_dentry(dentry);
 
 	if (dentry->d_inode &&
@@ -732,6 +732,8 @@ out:
 	unionfs_unlock_dentry(dentry);
 	unionfs_check_inode(parent);
 	unionfs_check_dentry(dentry);
+	unionfs_read_unlock(dentry->d_sb);
+
 	return err;
 }
 
@@ -745,6 +747,7 @@ static int unionfs_mknod(struct inode *dir, struct dentry *dentry, int mode,
 	char *name = NULL;
 	int whiteout_unlinked = 0;
 
+	unionfs_read_lock(dentry->d_sb);
 	unionfs_lock_dentry(dentry);
 
 	if (dentry->d_inode &&
@@ -858,6 +861,8 @@ out:
 
 	unionfs_check_inode(dir);
 	unionfs_check_dentry(dentry);
+	unionfs_read_unlock(dentry->d_sb);
+
 	return err;
 }
 
@@ -867,6 +872,7 @@ static int unionfs_readlink(struct dentry *dentry, char __user *buf,
 	int err;
 	struct dentry *lower_dentry;
 
+	unionfs_read_lock(dentry->d_sb);
 	unionfs_lock_dentry(dentry);
 
 	if (!__unionfs_d_revalidate_chain(dentry, NULL, 0)) {
@@ -891,23 +897,28 @@ static int unionfs_readlink(struct dentry *dentry, char __user *buf,
 out:
 	unionfs_unlock_dentry(dentry);
 	unionfs_check_dentry(dentry);
+	unionfs_read_unlock(dentry->d_sb);
+
 	return err;
 }
 
-/* We don't lock the dentry here, because readlink does the heavy lifting. */
+/*
+ * unionfs_follow_link takes a dentry, but it is simple.  It only needs to
+ * allocate some memory and then call our ->readlink method.  Our
+ * unionfs_readlink *does* lock our dentry and revalidate the dentry.
+ * Therefore, we do not have to lock our dentry here, to prevent a deadlock;
+ * nor do we need to revalidate it either.  It is safe to not lock our
+ * dentry here because unionfs_follow_link does not do anything (prior to
+ * calling ->readlink) which could become inconsistent due to branch
+ * management.
+ */
 static void *unionfs_follow_link(struct dentry *dentry, struct nameidata *nd)
 {
 	char *buf;
 	int len = PAGE_SIZE, err;
 	mm_segment_t old_fs;
 
-	unionfs_lock_dentry(dentry);
-
-	if (dentry->d_inode &&
-	    !__unionfs_d_revalidate_chain(dentry, nd, 0)) {
-		err = -ESTALE;
-		goto out;
-	}
+ 	unionfs_read_lock(dentry->d_sb);
 
 	/* This is freed by the put_link method assuming a successful call. */
 	buf = kmalloc(len, GFP_KERNEL);
@@ -931,14 +942,17 @@ static void *unionfs_follow_link(struct dentry *dentry, struct nameidata *nd)
 	err = 0;
 
 out:
-	unionfs_unlock_dentry(dentry);
 	unionfs_check_dentry(dentry);
+ 	unionfs_read_unlock(dentry->d_sb);
 	return ERR_PTR(err);
 }
 
+/* FIXME: We may not have to lock here */
 static void unionfs_put_link(struct dentry *dentry, struct nameidata *nd,
 			     void *cookie)
 {
+	unionfs_read_lock(dentry->d_sb);
+
 	unionfs_lock_dentry(dentry);
 	if (!__unionfs_d_revalidate_chain(dentry, nd, 0))
 		printk("unionfs: put_link failed to revalidate dentry\n");
@@ -946,6 +960,7 @@ static void unionfs_put_link(struct dentry *dentry, struct nameidata *nd,
 
 	unionfs_check_dentry(dentry);
 	kfree(nd_get_link(nd));
+	unionfs_read_unlock(dentry->d_sb);
 }
 
 /*
@@ -987,9 +1002,7 @@ static int inode_permission(struct inode *inode, int mask,
 		    (!strcmp("nfs", (inode)->i_sb->s_type->name)) &&
 		    (nd) && (nd->mnt) && (nd->mnt->mnt_sb)) {
 			int perms;
-			unionfs_read_lock(nd->mnt->mnt_sb);
 			perms = branchperms(nd->mnt->mnt_sb, bindex);
-			unionfs_read_unlock(nd->mnt->mnt_sb);
 			if (perms & MAY_NFSRO)
 				retval = generic_permission(inode, submask,
 							    NULL);
@@ -1101,6 +1114,7 @@ static int unionfs_setattr(struct dentry *dentry, struct iattr *ia)
 	int i;
 	int copyup = 0;
 
+	unionfs_read_lock(dentry->d_sb);
 	unionfs_lock_dentry(dentry);
 
 	if (!__unionfs_d_revalidate_chain(dentry, NULL, 0)) {
@@ -1175,6 +1189,8 @@ out:
 	unionfs_unlock_dentry(dentry);
 	unionfs_check_dentry(dentry);
 	unionfs_check_dentry(dentry->d_parent);
+	unionfs_read_unlock(dentry->d_sb);
+
 	return err;
 }
 
diff --git a/fs/unionfs/main.c b/fs/unionfs/main.c
index 2f884ab..e437edb 100644
--- a/fs/unionfs/main.c
+++ b/fs/unionfs/main.c
@@ -271,7 +271,13 @@ int parse_branch_mode(const char *name)
 	return perms;
 }
 
-/* parse the dirs= mount argument */
+/* 
+ * parse the dirs= mount argument
+ *
+ * We don't need to lock the superblock private data's rwsem, as we get
+ * called only by unionfs_read_super - it is still a long time before anyone
+ * can even get a reference to us.
+ */
 static int parse_dirs_option(struct super_block *sb, struct unionfs_dentry_info
 			     *lower_root_info, char *options)
 {
@@ -357,11 +363,9 @@ static int parse_dirs_option(struct super_block *sb, struct unionfs_dentry_info
 		lower_root_info->lower_paths[bindex].dentry = nd.dentry;
 		lower_root_info->lower_paths[bindex].mnt = nd.mnt;
 
-		unionfs_write_lock(sb);
 		set_branchperms(sb, bindex, perms);
 		set_branch_count(sb, bindex, 0);
 		new_branch_id(sb, bindex);
-		unionfs_write_unlock(sb);
 
 		if (lower_root_info->bstart < 0)
 			lower_root_info->bstart = bindex;
@@ -561,6 +565,10 @@ static struct dentry *unionfs_d_alloc_root(struct super_block *sb)
 	return ret;
 }
 
+/*
+ * There is no need to lock the unionfs_super_info's rwsem as there is no
+ * way anyone can have a reference to the superblock at this point in time.
+ */
 static int unionfs_read_super(struct super_block *sb, void *raw_data,
 			      int silent)
 {
@@ -607,19 +615,12 @@ static int unionfs_read_super(struct super_block *sb, void *raw_data,
 	BUG_ON(bstart != 0);
 	sbend(sb) = bend = lower_root_info->bend;
 	for (bindex = bstart; bindex <= bend; bindex++) {
-		struct dentry *d;
-
-		d = lower_root_info->lower_paths[bindex].dentry;
-
-		unionfs_write_lock(sb);
+		struct dentry *d = lower_root_info->lower_paths[bindex].dentry;
 		unionfs_set_lower_super_idx(sb, bindex, d->d_sb);
-		unionfs_write_unlock(sb);
 	}
 
 	/* max Bytes is the maximum bytes from highest priority branch */
-	unionfs_read_lock(sb);
 	sb->s_maxbytes = unionfs_lower_super_idx(sb, 0)->s_maxbytes;
-	unionfs_read_unlock(sb);
 
 	sb->s_op = &unionfs_sops;
 
diff --git a/fs/unionfs/rename.c b/fs/unionfs/rename.c
index f9f9b1e..0316258 100644
--- a/fs/unionfs/rename.c
+++ b/fs/unionfs/rename.c
@@ -401,6 +401,7 @@ int unionfs_rename(struct inode *old_dir, struct dentry *old_dentry,
 	int err = 0;
 	struct dentry *wh_dentry;
 
+	unionfs_read_lock(old_dentry->d_sb);
 	unionfs_double_lock_dentry(old_dentry, new_dentry);
 
 	if (!__unionfs_d_revalidate_chain(old_dentry, NULL, 0)) {
@@ -508,5 +509,6 @@ out:
 
 	unionfs_unlock_dentry(new_dentry);
 	unionfs_unlock_dentry(old_dentry);
+	unionfs_read_unlock(old_dentry->d_sb);
 	return err;
 }
diff --git a/fs/unionfs/super.c b/fs/unionfs/super.c
index b150412..6c9bc1d 100644
--- a/fs/unionfs/super.c
+++ b/fs/unionfs/super.c
@@ -65,6 +65,8 @@ static void unionfs_read_inode(struct inode *inode)
  * else that's needed, and the other is fine.  This way we truncate the inode
  * size (and its pages) and then clear our own inode, which will do an iput
  * on our and the lower inode.
+ *
+ * No need to lock sb info's rwsem.
  */
 static void unionfs_delete_inode(struct inode *inode)
 {
@@ -76,7 +78,11 @@ static void unionfs_delete_inode(struct inode *inode)
 	clear_inode(inode);
 }
 
-/* final actions when unmounting a file system */
+/*
+ * final actions when unmounting a file system
+ *
+ * No need to lock rwsem.
+ */
 static void unionfs_put_super(struct super_block *sb)
 {
 	int bindex, bstart, bend;
@@ -115,6 +121,9 @@ static int unionfs_statfs(struct dentry *dentry, struct kstatfs *buf)
 	struct super_block *sb;
 	struct dentry *lower_dentry;
 
+	sb = dentry->d_sb;
+
+	unionfs_read_lock(sb);
 	unionfs_lock_dentry(dentry);
 
 	if (!__unionfs_d_revalidate_chain(dentry, NULL, 0)) {
@@ -123,11 +132,7 @@ static int unionfs_statfs(struct dentry *dentry, struct kstatfs *buf)
 	}
 	unionfs_check_dentry(dentry);
 
-	sb = dentry->d_sb;
-
-	unionfs_read_lock(sb);
 	lower_dentry = unionfs_lower_dentry(sb->s_root);
-	unionfs_read_unlock(sb);
 	err = vfs_statfs(lower_dentry, buf);
 
 	/* set return buf to our f/s to avoid confusing user-level utils */
@@ -150,6 +155,7 @@ static int unionfs_statfs(struct dentry *dentry, struct kstatfs *buf)
 out:
 	unionfs_unlock_dentry(dentry);
 	unionfs_check_dentry(dentry);
+	unionfs_read_unlock(sb);
 	return err;
 }
 
@@ -790,6 +796,8 @@ out_error:
  * and the inode is not hashed anywhere.  Used to clear anything
  * that needs to be, before the inode is completely destroyed and put
  * on the inode free list.
+ *
+ * No need to lock sb info's rwsem.
  */
 static void unionfs_clear_inode(struct inode *inode)
 {
@@ -874,6 +882,8 @@ void unionfs_destroy_inode_cache(void)
 /*
  * Called when we have a dirty inode, right here we only throw out
  * parts of our readdir list that are too old.
+ *
+ * No need to grab sb info's rwsem.
  */
 static int unionfs_write_inode(struct inode *inode, int sync)
 {
@@ -914,18 +924,20 @@ static void unionfs_umount_begin(struct vfsmount *mnt, int flags)
 
 	sb = mnt->mnt_sb;
 
+	unionfs_read_lock(sb);
+
 	bstart = sbstart(sb);
 	bend = sbend(sb);
 	for (bindex = bstart; bindex <= bend; bindex++) {
 		lower_mnt = unionfs_lower_mnt_idx(sb->s_root, bindex);
-		unionfs_read_lock(sb);
 		lower_sb = unionfs_lower_super_idx(sb, bindex);
-		unionfs_read_unlock(sb);
 
 		if (lower_mnt && lower_sb && lower_sb->s_op &&
 		    lower_sb->s_op->umount_begin)
 			lower_sb->s_op->umount_begin(lower_mnt, flags);
 	}
+
+	unionfs_read_unlock(sb);
 }
 
 static int unionfs_show_options(struct seq_file *m, struct vfsmount *mnt)
@@ -937,6 +949,8 @@ static int unionfs_show_options(struct seq_file *m, struct vfsmount *mnt)
 	int bindex, bstart, bend;
 	int perms;
 
+	unionfs_read_lock(sb);
+
 	unionfs_lock_dentry(sb->s_root);
 
 	tmp_page = (char*) __get_free_page(GFP_KERNEL);
@@ -958,9 +972,7 @@ static int unionfs_show_options(struct seq_file *m, struct vfsmount *mnt)
 			goto out;
 		}
 
-		unionfs_read_lock(sb);
 		perms = branchperms(sb, bindex);
-		unionfs_read_unlock(sb);
 
 		seq_printf(m, "%s=%s", path,
 			   perms & MAY_WRITE ? "rw" : "ro");
@@ -973,6 +985,8 @@ out:
 
 	unionfs_unlock_dentry(sb->s_root);
 
+	unionfs_read_unlock(sb);
+
 	return ret;
 }
 
diff --git a/fs/unionfs/union.h b/fs/unionfs/union.h
index b66ff60..2bedaec 100644
--- a/fs/unionfs/union.h
+++ b/fs/unionfs/union.h
@@ -134,7 +134,16 @@ struct unionfs_sb_info {
 	int bend;
 
 	atomic_t generation;
-	struct rw_semaphore rwsem; /* protects access to data+id fields */
+
+	/*
+	 * This rwsem is used to make sure that a branch management
+	 * operation...
+	 *   1) will not begin before all currently in-flight operations
+	 *      complete
+	 *   2) any new operations do not execute until the currently
+	 *      running branch management operation completes
+	 */
+	struct rw_semaphore rwsem;
 	int high_branch_id;	/* last unique branch ID given */
 	struct unionfs_data *data;
 };
@@ -376,9 +385,7 @@ static inline int is_robranch_super(const struct super_block *sb, int index)
 {
 	int ret;
 
-	unionfs_read_lock(sb);
   	ret = (!(branchperms(sb, index) & MAY_WRITE)) ? -EROFS : 0;
-	unionfs_read_unlock(sb);
 	return ret;
 }
 
@@ -389,11 +396,9 @@ static inline int is_robranch_idx(const struct dentry *dentry, int index)
 
 	BUG_ON(index < 0);
 
-	unionfs_read_lock(dentry->d_sb);
 	if ((!(branchperms(dentry->d_sb, index) & MAY_WRITE)) ||
 	    IS_RDONLY(unionfs_lower_dentry_idx(dentry, index)->d_inode))
 		err = -EROFS;
-	unionfs_read_unlock(dentry->d_sb);
 	return err;
 }
 
diff --git a/fs/unionfs/unlink.c b/fs/unionfs/unlink.c
index bffa458..47bebab 100644
--- a/fs/unionfs/unlink.c
+++ b/fs/unionfs/unlink.c
@@ -76,6 +76,7 @@ int unionfs_unlink(struct inode *dir, struct dentry *dentry)
 {
 	int err = 0;
 
+	unionfs_read_lock(dentry->d_sb);
 	unionfs_lock_dentry(dentry);
 
 	if (!__unionfs_d_revalidate_chain(dentry, NULL, 0)) {
@@ -103,6 +104,7 @@ out:
 		unionfs_check_inode(dir);
 	}
 	unionfs_unlock_dentry(dentry);
+	unionfs_read_unlock(dentry->d_sb);
 	return err;
 }
 
@@ -143,6 +145,7 @@ int unionfs_rmdir(struct inode *dir, struct dentry *dentry)
 	int err = 0;
 	struct unionfs_dir_state *namelist = NULL;
 
+	unionfs_read_lock(dentry->d_sb);
 	unionfs_lock_dentry(dentry);
 
 	if (!__unionfs_d_revalidate_chain(dentry, NULL, 0)) {
@@ -184,5 +187,6 @@ out:
 		free_rdstate(namelist);
 
 	unionfs_unlock_dentry(dentry);
+	unionfs_read_unlock(dentry->d_sb);
 	return err;
 }
diff --git a/fs/unionfs/xattr.c b/fs/unionfs/xattr.c
index b5ae59c..d6f4d53 100644
--- a/fs/unionfs/xattr.c
+++ b/fs/unionfs/xattr.c
@@ -57,6 +57,7 @@ ssize_t unionfs_getxattr(struct dentry *dentry, const char *name, void *value,
 	struct dentry *lower_dentry = NULL;
 	int err = -EOPNOTSUPP;
 
+	unionfs_read_lock(dentry->d_sb);
 	unionfs_lock_dentry(dentry);
 
 	if (!__unionfs_d_revalidate_chain(dentry, NULL, 0)) {
@@ -71,6 +72,7 @@ ssize_t unionfs_getxattr(struct dentry *dentry, const char *name, void *value,
 out:
 	unionfs_unlock_dentry(dentry);
 	unionfs_check_dentry(dentry);
+	unionfs_read_unlock(dentry->d_sb);
 	return err;
 }
 
@@ -84,6 +86,7 @@ int unionfs_setxattr(struct dentry *dentry, const char *name,
 	struct dentry *lower_dentry = NULL;
 	int err = -EOPNOTSUPP;
 
+	unionfs_read_lock(dentry->d_sb);
 	unionfs_lock_dentry(dentry);
 
 	if (!__unionfs_d_revalidate_chain(dentry, NULL, 0)) {
@@ -99,6 +102,7 @@ int unionfs_setxattr(struct dentry *dentry, const char *name,
 out:
 	unionfs_unlock_dentry(dentry);
 	unionfs_check_dentry(dentry);
+	unionfs_read_unlock(dentry->d_sb);
 	return err;
 }
 
@@ -111,6 +115,7 @@ int unionfs_removexattr(struct dentry *dentry, const char *name)
 	struct dentry *lower_dentry = NULL;
 	int err = -EOPNOTSUPP;
 
+	unionfs_read_lock(dentry->d_sb);
 	unionfs_lock_dentry(dentry);
 
 	if (!__unionfs_d_revalidate_chain(dentry, NULL, 0)) {
@@ -125,6 +130,7 @@ int unionfs_removexattr(struct dentry *dentry, const char *name)
 out:
 	unionfs_unlock_dentry(dentry);
 	unionfs_check_dentry(dentry);
+	unionfs_read_unlock(dentry->d_sb);
 	return err;
 }
 
@@ -138,6 +144,7 @@ ssize_t unionfs_listxattr(struct dentry *dentry, char *list, size_t size)
 	int err = -EOPNOTSUPP;
 	char *encoded_list = NULL;
 
+	unionfs_read_lock(dentry->d_sb);
 	unionfs_lock_dentry(dentry);
 
 	if (!__unionfs_d_revalidate_chain(dentry, NULL, 0)) {
@@ -153,5 +160,6 @@ ssize_t unionfs_listxattr(struct dentry *dentry, char *list, size_t size)
 out:
 	unionfs_unlock_dentry(dentry);
 	unionfs_check_dentry(dentry);
+	unionfs_read_unlock(dentry->d_sb);
 	return err;
 }


More information about the unionfs-cvs mailing list