[PATCH] hpfs: fix locking scheme

	Fixed the locking scheme.  The need of extra locking was caused by
the fact that hpfs_write_inode() must update directory entry; since HPFS
directories are implemented as b-trees, we must provide protection both
against rename() (to make sure that we update the entry in right directory)
and against rebalancing of the parent.

	Old scheme had both deadlocks and races - to start with, we had no
protection against rename()/unlink()/rmdir(), since (a) locking parent
was done without any warranties that it will remain our parent and (b)
check that we still have a directory entry (== have positive nlink) was
done before we tried to lock the parent.  Moreover, iget serialization
killed two steps ago gave immediate deadlocks if iget() of parent had
triggered another hpfs_write_inode().

	New scheme introduces another per-inode semaphore (hpfs-only,
obviously) protecting the reference to parent.  It's taken on
rename/rmdir/unlink victims and inode being moved by rename.  Old semaphores
are taken only on parent(s) and only after we grab one(s) of the new kind.
hpfs_write_inode() gets the new semaphore on our inode, checks nlink and
if it's non-zero grabs parent and takes the old semaphore on it.

	Order among the semaphores of the same kind is arbitrary - the only
function that might take more than one of the same kind is hpfs_rename()
and it's serialized by VFS.

	We might get away with only one semaphore, but then the ordering
issues would bite us big way - we would have to make sure that child is
always locked before parent (hpfs_write_inode() leaves no other choice)
and while that's easy to do for almost all operations, rename() is a bitch -
as always.  And per-superblock rwsem giving rename() vs. write_inode()
exclusion on hpfs would make the entire thing too baroque for my taste.
	->readdir() takes no locks at all (protection against directory
modifications is provided by VFS exclusion), ditto for ->lookup().
	->llseek() on directories switched to use of (VFS) ->i_sem, so
it's safe from directory modifications and ->readdir() is safe from it -
no hpfs locks are needed here.

fs/hpfs/buffer.c

@@ -26,22 +26,6 @@ void hpfs_unlock_creation(struct super_block *s)
 	up(&hpfs_sb(s)->hpfs_creation_de);
 }
 
-void hpfs_lock_inode(struct inode *i)
-{
-	if (i) {
-		struct hpfs_inode_info *hpfs_inode = hpfs_i(i);
-		down(&hpfs_inode->i_sem);
-	}
-}
-
-void hpfs_unlock_inode(struct inode *i)
-{
-	if (i) {
-		struct hpfs_inode_info *hpfs_inode = hpfs_i(i);
-		up(&hpfs_inode->i_sem);
-	}
-}
-
 /* Map a sector into a buffer and return pointers to it and to the buffer. */
 
 void *hpfs_map_sector(struct super_block *s, unsigned secno, struct buffer_head **bhp,

fs/hpfs/dir.c

@@ -35,19 +35,19 @@ loff_t hpfs_dir_lseek(struct file *filp, loff_t off, int whence)
 
 	/*printk("dir lseek\n");*/
 	if (new_off == 0 || new_off == 1 || new_off == 11 || new_off == 12 || new_off == 13) goto ok;
-	hpfs_lock_inode(i);
+	down(&i->i_sem);
 	pos = ((loff_t) hpfs_de_as_down_as_possible(s, hpfs_inode->i_dno) << 4) + 1;
 	while (pos != new_off) {
 		if (map_pos_dirent(i, &pos, &qbh)) hpfs_brelse4(&qbh);
 		else goto fail;
 		if (pos == 12) goto fail;
 	}
-	hpfs_unlock_inode(i);
-	ok:
+	up(&i->i_sem);
+ok:
 	unlock_kernel();
 	return filp->f_pos = new_off;
-	fail:
-	hpfs_unlock_inode(i);
+fail:
+	up(&i->i_sem);
 	/*printk("illegal lseek: %016llx\n", new_off);*/
 	unlock_kernel();
 	return -ESPIPE;
@@ -109,8 +109,6 @@ int hpfs_readdir(struct file *filp, void *dirent, filldir_t filldir)
 		goto out;
 	}
 	
-	hpfs_lock_inode(inode);
-	
 	while (1) {
 		again:
 		/* This won't work when cycle is longer than number of dirents
@@ -118,31 +116,23 @@ int hpfs_readdir(struct file *filp, void *dirent, filldir_t filldir)
 		   maybe killall -9 ls helps */
 		if (hpfs_sb(inode->i_sb)->sb_chk)
 			if (hpfs_stop_cycles(inode->i_sb, filp->f_pos, &c1, &c2, "hpfs_readdir")) {
-				hpfs_unlock_inode(inode);
 				ret = -EFSERROR;
 				goto out;
 			}
-		if (filp->f_pos == 12) {
-			hpfs_unlock_inode(inode);
+		if (filp->f_pos == 12)
 			goto out;
-		}
 		if (filp->f_pos == 3 || filp->f_pos == 4 || filp->f_pos == 5) {
 			printk("HPFS: warning: pos==%d\n",(int)filp->f_pos);
-			hpfs_unlock_inode(inode);
 			goto out;
 		}
 		if (filp->f_pos == 0) {
-			if (filldir(dirent, ".", 1, filp->f_pos, inode->i_ino, DT_DIR) < 0) {
-				hpfs_unlock_inode(inode);
+			if (filldir(dirent, ".", 1, filp->f_pos, inode->i_ino, DT_DIR) < 0)
 				goto out;
-			}
 			filp->f_pos = 11;
 		}
 		if (filp->f_pos == 11) {
-			if (filldir(dirent, "..", 2, filp->f_pos, hpfs_inode->i_parent_dir, DT_DIR) < 0) {
-				hpfs_unlock_inode(inode);
+			if (filldir(dirent, "..", 2, filp->f_pos, hpfs_inode->i_parent_dir, DT_DIR) < 0)
 				goto out;
-			}
 			filp->f_pos = 1;
 		}
 		if (filp->f_pos == 1) {
@@ -151,13 +141,11 @@ int hpfs_readdir(struct file *filp, void *dirent, filldir_t filldir)
 			filp->f_version = inode->i_version;
 		}
 			/*if (filp->f_version != inode->i_version) {
-				hpfs_unlock_inode(inode);
 				ret = -ENOENT;
 				goto out;
 			}*/	
 			old_pos = filp->f_pos;
 			if (!(de = map_pos_dirent(inode, &filp->f_pos, &qbh))) {
-				hpfs_unlock_inode(inode);
 				ret = -EIOERROR;
 				goto out;
 			}
@@ -174,7 +162,6 @@ int hpfs_readdir(struct file *filp, void *dirent, filldir_t filldir)
 				filp->f_pos = old_pos;
 				if (tempname != (char *)de->name) kfree(tempname);
 				hpfs_brelse4(&qbh);
-				hpfs_unlock_inode(inode);
 				goto out;
 			}
 			if (tempname != (char *)de->name) kfree(tempname);
@@ -220,7 +207,6 @@ struct dentry *hpfs_lookup(struct inode *dir, struct dentry *dentry, struct name
 		goto end_add;
 	}
 
-	hpfs_lock_inode(dir);
 	/*
 	 * '.' and '..' will never be passed here.
 	 */
@@ -312,7 +298,6 @@ struct dentry *hpfs_lookup(struct inode *dir, struct dentry *dentry, struct name
 	 */
 
 	end:
-	hpfs_unlock_inode(dir);
 	end_add:
 	hpfs_set_dentry_operations(dentry);
 	unlock_kernel();
@@ -328,7 +313,6 @@ struct dentry *hpfs_lookup(struct inode *dir, struct dentry *dentry, struct name
 	
 	/*bail:*/
 
-	hpfs_unlock_inode(dir);
 	unlock_kernel();
 	return ERR_PTR(-ENOENT);
 }

fs/hpfs/file.c

@@ -15,17 +15,6 @@
 
 #define BLOCKS(size) (((size) + 511) >> 9)
 
-/* HUH? */
-int hpfs_open(struct inode *i, struct file *f)
-{
-	lock_kernel();
-	hpfs_lock_inode(i);
-	hpfs_unlock_inode(i); /* make sure nobody is deleting the file */
-	unlock_kernel();
-	if (!i->i_nlink) return -ENOENT;
-	return 0;
-}
-
 int hpfs_file_release(struct inode *inode, struct file *file)
 {
 	lock_kernel();

fs/hpfs/hpfs_fn.h

@@ -192,8 +192,6 @@ void hpfs_remove_fnode(struct super_block *, fnode_secno fno);
 
 void hpfs_lock_creation(struct super_block *);
 void hpfs_unlock_creation(struct super_block *);
-void hpfs_lock_inode(struct inode *);
-void hpfs_unlock_inode(struct inode *);
 void *hpfs_map_sector(struct super_block *, unsigned, struct buffer_head **, int);
 void *hpfs_get_sector(struct super_block *, unsigned, struct buffer_head **);
 void *hpfs_map_4sectors(struct super_block *, unsigned, struct quad_buffer_head *, int);

fs/hpfs/inode.c

@@ -229,13 +229,17 @@ void hpfs_write_inode(struct inode *i)
 {
 	struct hpfs_inode_info *hpfs_inode = hpfs_i(i);
 	struct inode *parent;
-	if (!i->i_nlink) return;
 	if (i->i_ino == hpfs_sb(i->i_sb)->sb_root) return;
 	if (hpfs_inode->i_rddir_off && !atomic_read(&i->i_count)) {
 		if (*hpfs_inode->i_rddir_off) printk("HPFS: write_inode: some position still there\n");
 		kfree(hpfs_inode->i_rddir_off);
 		hpfs_inode->i_rddir_off = NULL;
 	}
+	down(&hpfs_inode->i_parent);
+	if (!i->i_nlink) {
+		up(&hpfs_inode->i_parent);
+		return;
+	}
 	parent = iget_locked(i->i_sb, hpfs_inode->i_parent_dir);
 	if (parent) {
 		hpfs_inode->i_dirty = 0;
@@ -244,13 +248,14 @@ void hpfs_write_inode(struct inode *i)
 			hpfs_read_inode(parent);
 			unlock_new_inode(parent);
 		}
-		hpfs_lock_inode(parent);
+		down(&hpfs_inode->i_sem);
 		hpfs_write_inode_nolock(i);
-		hpfs_unlock_inode(parent);
+		up(&hpfs_inode->i_sem);
 		iput(parent);
 	} else {
 		mark_inode_dirty(i);
 	}
+	up(&hpfs_inode->i_parent);
 }
 
 void hpfs_write_inode_nolock(struct inode *i)

fs/hpfs/namei.c

@@ -63,7 +63,7 @@ int hpfs_mkdir(struct inode *dir, struct dentry *dentry, int mode)
 	if (dee.read_only)
 		result->i_mode &= ~0222;
 
-	hpfs_lock_inode(dir);
+	down(&hpfs_i(dir)->i_sem);
 	r = hpfs_add_dirent(dir, (char *)name, len, &dee, 0);
 	if (r == 1)
 		goto bail3;
@@ -104,11 +104,11 @@ int hpfs_mkdir(struct inode *dir, struct dentry *dentry, int mode)
 		hpfs_write_inode_nolock(result);
 	}
 	d_instantiate(dentry, result);
-	hpfs_unlock_inode(dir);
+	up(&hpfs_i(dir)->i_sem);
 	unlock_kernel();
 	return 0;
 bail3:
-	hpfs_unlock_inode(dir);
+	up(&hpfs_i(dir)->i_sem);
 	iput(result);
 bail2:
 	hpfs_brelse4(&qbh0);
@@ -171,7 +171,7 @@ int hpfs_create(struct inode *dir, struct dentry *dentry, int mode, struct namei
 	result->i_data.a_ops = &hpfs_aops;
 	hpfs_i(result)->mmu_private = 0;
 
-	hpfs_lock_inode(dir);
+	down(&hpfs_i(dir)->i_sem);
 	r = hpfs_add_dirent(dir, (char *)name, len, &dee, 0);
 	if (r == 1)
 		goto bail2;
@@ -196,12 +196,12 @@ int hpfs_create(struct inode *dir, struct dentry *dentry, int mode, struct namei
 		hpfs_write_inode_nolock(result);
 	}
 	d_instantiate(dentry, result);
-	hpfs_unlock_inode(dir);
+	up(&hpfs_i(dir)->i_sem);
 	unlock_kernel();
 	return 0;
 
 bail2:
-	hpfs_unlock_inode(dir);
+	up(&hpfs_i(dir)->i_sem);
 	iput(result);
 bail1:
 	brelse(bh);
@@ -257,7 +257,7 @@ int hpfs_mknod(struct inode *dir, struct dentry *dentry, int mode, dev_t rdev)
 	result->i_blocks = 1;
 	init_special_inode(result, mode, rdev);
 
-	hpfs_lock_inode(dir);
+	down(&hpfs_i(dir)->i_sem);
 	r = hpfs_add_dirent(dir, (char *)name, len, &dee, 0);
 	if (r == 1)
 		goto bail2;
@@ -274,12 +274,12 @@ int hpfs_mknod(struct inode *dir, struct dentry *dentry, int mode, dev_t rdev)
 
 	hpfs_write_inode_nolock(result);
 	d_instantiate(dentry, result);
-	hpfs_unlock_inode(dir);
+	up(&hpfs_i(dir)->i_sem);
 	brelse(bh);
 	unlock_kernel();
 	return 0;
 bail2:
-	hpfs_unlock_inode(dir);
+	up(&hpfs_i(dir)->i_sem);
 	iput(result);
 bail1:
 	brelse(bh);
@@ -338,7 +338,7 @@ int hpfs_symlink(struct inode *dir, struct dentry *dentry, const char *symlink)
 	result->i_op = &page_symlink_inode_operations;
 	result->i_data.a_ops = &hpfs_symlink_aops;
 
-	hpfs_lock_inode(dir);
+	down(&hpfs_i(dir)->i_sem);
 	r = hpfs_add_dirent(dir, (char *)name, len, &dee, 0);
 	if (r == 1)
 		goto bail2;
@@ -357,11 +357,11 @@ int hpfs_symlink(struct inode *dir, struct dentry *dentry, const char *symlink)
 
 	hpfs_write_inode_nolock(result);
 	d_instantiate(dentry, result);
-	hpfs_unlock_inode(dir);
+	up(&hpfs_i(dir)->i_sem);
 	unlock_kernel();
 	return 0;
 bail2:
-	hpfs_unlock_inode(dir);
+	up(&hpfs_i(dir)->i_sem);
 	iput(result);
 bail1:
 	brelse(bh);
@@ -387,8 +387,8 @@ int hpfs_unlink(struct inode *dir, struct dentry *dentry)
 	lock_kernel();
 	hpfs_adjust_length((char *)name, &len);
 again:
-	hpfs_lock_inode(dir);
-	hpfs_lock_inode(inode);
+	down(&hpfs_i(inode)->i_parent);
+	down(&hpfs_i(dir)->i_sem);
 	err = -ENOENT;
 	de = map_dirent(dir, hpfs_i(dir)->i_dno, (char *)name, len, &dno, &qbh);
 	if (!de)
@@ -415,8 +415,8 @@ int hpfs_unlink(struct inode *dir, struct dentry *dentry)
 		if (rep++)
 			break;
 
-		hpfs_unlock_inode(inode);
-		hpfs_unlock_inode(dir);
+		up(&hpfs_i(dir)->i_sem);
+		up(&hpfs_i(inode)->i_parent);
 		d_drop(dentry);
 		spin_lock(&dentry->d_lock);
 		if (atomic_read(&dentry->d_count) > 1 ||
@@ -447,8 +447,8 @@ int hpfs_unlink(struct inode *dir, struct dentry *dentry)
 out1:
 	hpfs_brelse4(&qbh);
 out:
-	hpfs_unlock_inode(inode);
-	hpfs_unlock_inode(dir);
+	up(&hpfs_i(dir)->i_sem);
+	up(&hpfs_i(inode)->i_parent);
 	unlock_kernel();
 	return err;
 }
@@ -468,8 +468,8 @@ int hpfs_rmdir(struct inode *dir, struct dentry *dentry)
 
 	hpfs_adjust_length((char *)name, &len);
 	lock_kernel();
-	hpfs_lock_inode(dir);
-	hpfs_lock_inode(inode);
+	down(&hpfs_i(inode)->i_parent);
+	down(&hpfs_i(dir)->i_sem);
 	err = -ENOENT;
 	de = map_dirent(dir, hpfs_i(dir)->i_dno, (char *)name, len, &dno, &qbh);
 	if (!de)
@@ -507,8 +507,8 @@ int hpfs_rmdir(struct inode *dir, struct dentry *dentry)
 out1:
 	hpfs_brelse4(&qbh);
 out:
-	hpfs_unlock_inode(inode);
-	hpfs_unlock_inode(dir);
+	up(&hpfs_i(dir)->i_sem);
+	up(&hpfs_i(inode)->i_parent);
 	unlock_kernel();
 	return err;
 }
@@ -565,10 +565,13 @@ int hpfs_rename(struct inode *old_dir, struct dentry *old_dentry,
 	hpfs_adjust_length((char *)old_name, &old_len);
 
 	lock_kernel();
-	hpfs_lock_inode(old_dir);
+	/* order doesn't matter, due to VFS exclusion */
+	down(&hpfs_i(i)->i_parent);
+	if (new_inode)
+		down(&hpfs_i(new_inode)->i_parent);
+	down(&hpfs_i(old_dir)->i_sem);
 	if (new_dir != old_dir)
-		hpfs_lock_inode(new_dir);
-	hpfs_lock_inode(i);
+		down(&hpfs_i(new_dir)->i_sem);
 	
 	/* Erm? Moving over the empty non-busy directory is perfectly legal */
 	if (new_inode && S_ISDIR(new_inode->i_mode)) {
@@ -646,11 +649,13 @@ int hpfs_rename(struct inode *old_dir, struct dentry *old_dentry,
 	}
 	hpfs_i(i)->i_conv = hpfs_sb(i->i_sb)->sb_conv;
 	hpfs_decide_conv(i, (char *)new_name, new_len);
-	end1:
-	hpfs_unlock_inode(i);
+end1:
 	if (old_dir != new_dir)
-		hpfs_unlock_inode(new_dir);
-	hpfs_unlock_inode(old_dir);
+		up(&hpfs_i(new_dir)->i_sem);
+	up(&hpfs_i(old_dir)->i_sem);
+	up(&hpfs_i(i)->i_parent);
+	if (new_inode)
+		up(&hpfs_i(new_inode)->i_parent);
 	unlock_kernel();
 	return err;
 }

fs/hpfs/super.c

@@ -184,6 +184,7 @@ static void init_once(void * foo, kmem_cache_t * cachep, unsigned long flags)
 	if ((flags & (SLAB_CTOR_VERIFY|SLAB_CTOR_CONSTRUCTOR)) ==
 	    SLAB_CTOR_CONSTRUCTOR) {
 		init_MUTEX(&ei->i_sem);
+		init_MUTEX(&ei->i_parent);
 		inode_init_once(&ei->vfs_inode);
 	}
 }

include/linux/hpfs_fs_i.h

@@ -16,7 +16,8 @@ struct hpfs_inode_info {
 	unsigned i_ea_uid : 1;	/* file's uid is stored in ea */
 	unsigned i_ea_gid : 1;	/* file's gid is stored in ea */
 	unsigned i_dirty : 1;
-	struct semaphore i_sem;	/* semaphore */
+	struct semaphore i_sem;
+	struct semaphore i_parent;
 	loff_t **i_rddir_off;
 	struct inode vfs_inode;
 };