[PATCH 1/4] udf: Add missed protection for s_lvid_dirty

[PATCH 1/4] udf: Add missed protection for s_lvid_dirty

[Alessio Igor Bogani]

As reported in udf_sb.h the udf_sb_infoi's structure member s_lvid_dirty should
be protected by s_alloc_mutex. Added that mutex on a couple of places where it
miss.

This work was supported by a hardware donation from the CE Linux Forum.

Signed-off-by: Alessio Igor Bogani <abogani@texware.it>
---
 fs/udf/super.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/fs/udf/super.c b/fs/udf/super.c
index 4a5c7c6..c05834e 100644
--- a/fs/udf/super.c
+++ b/fs/udf/super.c
@@ -1789,7 +1789,9 @@ static void udf_open_lvid(struct super_block *sb)
 
 	lvid->descTag.tagChecksum = udf_tag_checksum(&lvid->descTag);
 	mark_buffer_dirty(bh);
+	mutex_lock(&sbi->s_alloc_mutex);
 	sbi->s_lvid_dirty = 0;
+	mutex_unlock(&sbi->s_alloc_mutex);
 }
 
 static void udf_close_lvid(struct super_block *sb)
@@ -1821,7 +1823,9 @@ static void udf_close_lvid(struct super_block *sb)
 
 	lvid->descTag.tagChecksum = udf_tag_checksum(&lvid->descTag);
 	mark_buffer_dirty(bh);
+	mutex_lock(&sbi->s_alloc_mutex);
 	sbi->s_lvid_dirty = 0;
+	mutex_unlock(&sbi->s_alloc_mutex);
 }
 
 static void udf_sb_free_bitmap(struct udf_bitmap *bitmap)
-- 
1.7.0.4

[PATCH 2/4] udf: Remove unnecessary bkl usages

[Alessio Igor Bogani]

The udf_lookup, udf_create, udf_mknod, udf_mkdir, udf_rmdir, udf_link,
udf_unlink and udf_readdir functions seems already adequately protected
by i_mutex held by VFS invoking calls. The udf_rename function instead
should be already protected by lock_rename again by VFS.

The udf_get_parent, udf_count_free_bitmap, udf_count_free_table and
udf_put_super functions don't require any protection.

This work was supported by a hardware donation from the CE Linux Forum.

Signed-off-by: Alessio Igor Bogani <abogani@texware.it>
---
 fs/udf/dir.c   |    5 -----
 fs/udf/namei.c |   26 --------------------------
 fs/udf/super.c |   12 ------------
 3 files changed, 0 insertions(+), 43 deletions(-)

diff --git a/fs/udf/dir.c b/fs/udf/dir.c
index 51552bf..eb8bfe2 100644
--- a/fs/udf/dir.c
+++ b/fs/udf/dir.c
@@ -30,7 +30,6 @@
 #include <linux/errno.h>
 #include <linux/mm.h>
 #include <linux/slab.h>
-#include <linux/smp_lock.h>
 #include <linux/buffer_head.h>
 
 #include "udf_i.h"
@@ -190,18 +189,14 @@ static int udf_readdir(struct file *filp, void *dirent, filldir_t filldir)
 	struct inode *dir = filp->f_path.dentry->d_inode;
 	int result;
 
-	lock_kernel();
-
 	if (filp->f_pos == 0) {
 		if (filldir(dirent, ".", 1, filp->f_pos, dir->i_ino, DT_DIR) < 0) {
-			unlock_kernel();
 			return 0;
 		}
 		filp->f_pos++;
 	}
 
 	result = do_udf_readdir(dir, filp, filldir, dirent);
-	unlock_kernel();
  	return result;
 }
 
diff --git a/fs/udf/namei.c b/fs/udf/namei.c
index 6d8dc02..6e7c1b9 100644
--- a/fs/udf/namei.c
+++ b/fs/udf/namei.c
@@ -263,7 +263,6 @@ static struct dentry *udf_lookup(struct inode *dir, struct dentry *dentry,
 	if (dentry->d_name.len > UDF_NAME_LEN - 2)
 		return ERR_PTR(-ENAMETOOLONG);
 
-	lock_kernel();
 #ifdef UDF_RECOVERY
 	/* temporary shorthand for specifying files by inode number */
 	if (!strncmp(dentry->d_name.name, ".B=", 3)) {
@@ -275,7 +274,6 @@ static struct dentry *udf_lookup(struct inode *dir, struct dentry *dentry,
 		};
 		inode = udf_iget(dir->i_sb, lb);
 		if (!inode) {
-			unlock_kernel();
 			return ERR_PTR(-EACCES);
 		}
 	} else
@@ -291,11 +289,9 @@ static struct dentry *udf_lookup(struct inode *dir, struct dentry *dentry,
 		loc = lelb_to_cpu(cfi.icb.extLocation);
 		inode = udf_iget(dir->i_sb, &loc);
 		if (!inode) {
-			unlock_kernel();
 			return ERR_PTR(-EACCES);
 		}
 	}
-	unlock_kernel();
 
 	return d_splice_alias(inode, dentry);
 }
@@ -562,10 +558,8 @@ static int udf_create(struct inode *dir, struct dentry *dentry, int mode,
 	int err;
 	struct udf_inode_info *iinfo;
 
-	lock_kernel();
 	inode = udf_new_inode(dir, mode, &err);
 	if (!inode) {
-		unlock_kernel();
 		return err;
 	}
 
@@ -583,7 +577,6 @@ static int udf_create(struct inode *dir, struct dentry *dentry, int mode,
 		inode->i_nlink--;
 		mark_inode_dirty(inode);
 		iput(inode);
-		unlock_kernel();
 		return err;
 	}
 	cfi.icb.extLength = cpu_to_le32(inode->i_sb->s_blocksize);
@@ -596,7 +589,6 @@ static int udf_create(struct inode *dir, struct dentry *dentry, int mode,
 	if (fibh.sbh != fibh.ebh)
 		brelse(fibh.ebh);
 	brelse(fibh.sbh);
-	unlock_kernel();
 	d_instantiate(dentry, inode);
 
 	return 0;
@@ -614,7 +606,6 @@ static int udf_mknod(struct inode *dir, struct dentry *dentry, int mode,
 	if (!old_valid_dev(rdev))
 		return -EINVAL;
 
-	lock_kernel();
 	err = -EIO;
 	inode = udf_new_inode(dir, mode, &err);
 	if (!inode)
@@ -627,7 +618,6 @@ static int udf_mknod(struct inode *dir, struct dentry *dentry, int mode,
 		inode->i_nlink--;
 		mark_inode_dirty(inode);
 		iput(inode);
-		unlock_kernel();
 		return err;
 	}
 	cfi.icb.extLength = cpu_to_le32(inode->i_sb->s_blocksize);
@@ -646,7 +636,6 @@ static int udf_mknod(struct inode *dir, struct dentry *dentry, int mode,
 	err = 0;
 
 out:
-	unlock_kernel();
 	return err;
 }
 
@@ -659,7 +648,6 @@ static int udf_mkdir(struct inode *dir, struct dentry *dentry, int mode)
 	struct udf_inode_info *dinfo = UDF_I(dir);
 	struct udf_inode_info *iinfo;
 
-	lock_kernel();
 	err = -EMLINK;
 	if (dir->i_nlink >= (256 << sizeof(dir->i_nlink)) - 1)
 		goto out;
@@ -712,7 +700,6 @@ static int udf_mkdir(struct inode *dir, struct dentry *dentry, int mode)
 	err = 0;
 
 out:
-	unlock_kernel();
 	return err;
 }
 
@@ -794,7 +781,6 @@ static int udf_rmdir(struct inode *dir, struct dentry *dentry)
 	struct kernel_lb_addr tloc;
 
 	retval = -ENOENT;
-	lock_kernel();
 	fi = udf_find_entry(dir, &dentry->d_name, &fibh, &cfi);
 	if (!fi)
 		goto out;
@@ -826,7 +812,6 @@ end_rmdir:
 	brelse(fibh.sbh);
 
 out:
-	unlock_kernel();
 	return retval;
 }
 
@@ -840,7 +825,6 @@ static int udf_unlink(struct inode *dir, struct dentry *dentry)
 	struct kernel_lb_addr tloc;
 
 	retval = -ENOENT;
-	lock_kernel();
 	fi = udf_find_entry(dir, &dentry->d_name, &fibh, &cfi);
 	if (!fi)
 		goto out;
@@ -870,7 +854,6 @@ end_unlink:
 	brelse(fibh.sbh);
 
 out:
-	unlock_kernel();
 	return retval;
 }
 
@@ -1062,15 +1045,12 @@ static int udf_link(struct dentry *old_dentry, struct inode *dir,
 	int err;
 	struct buffer_head *bh;
 
-	lock_kernel();
 	if (inode->i_nlink >= (256 << sizeof(inode->i_nlink)) - 1) {
-		unlock_kernel();
 		return -EMLINK;
 	}
 
 	fi = udf_add_entry(dir, dentry, &fibh, &cfi, &err);
 	if (!fi) {
-		unlock_kernel();
 		return err;
 	}
 	cfi.icb.extLength = cpu_to_le32(inode->i_sb->s_blocksize);
@@ -1103,7 +1083,6 @@ static int udf_link(struct dentry *old_dentry, struct inode *dir,
 	mark_inode_dirty(inode);
 	ihold(inode);
 	d_instantiate(dentry, inode);
-	unlock_kernel();
 
 	return 0;
 }
@@ -1124,7 +1103,6 @@ static int udf_rename(struct inode *old_dir, struct dentry *old_dentry,
 	struct kernel_lb_addr tloc;
 	struct udf_inode_info *old_iinfo = UDF_I(old_inode);
 
-	lock_kernel();
 	ofi = udf_find_entry(old_dir, &old_dentry->d_name, &ofibh, &ocfi);
 	if (ofi) {
 		if (ofibh.sbh != ofibh.ebh)
@@ -1248,7 +1226,6 @@ end_rename:
 			brelse(nfibh.ebh);
 		brelse(nfibh.sbh);
 	}
-	unlock_kernel();
 
 	return retval;
 }
@@ -1261,7 +1238,6 @@ static struct dentry *udf_get_parent(struct dentry *child)
 	struct fileIdentDesc cfi;
 	struct udf_fileident_bh fibh;
 
-	lock_kernel();
 	if (!udf_find_entry(child->d_inode, &dotdot, &fibh, &cfi))
 		goto out_unlock;
 
@@ -1273,11 +1249,9 @@ static struct dentry *udf_get_parent(struct dentry *child)
 	inode = udf_iget(child->d_inode->i_sb, &tloc);
 	if (!inode)
 		goto out_unlock;
-	unlock_kernel();
 
 	return d_obtain_alias(inode);
 out_unlock:
-	unlock_kernel();
 	return ERR_PTR(-EACCES);
 }
 
diff --git a/fs/udf/super.c b/fs/udf/super.c
index c05834e..0027c1f 100644
--- a/fs/udf/super.c
+++ b/fs/udf/super.c
@@ -2102,8 +2102,6 @@ static void udf_put_super(struct super_block *sb)
 
 	sbi = UDF_SB(sb);
 
-	lock_kernel();
-
 	if (sbi->s_vat_inode)
 		iput(sbi->s_vat_inode);
 	if (sbi->s_partitions)
@@ -2119,8 +2117,6 @@ static void udf_put_super(struct super_block *sb)
 	kfree(sbi->s_partmaps);
 	kfree(sb->s_fs_info);
 	sb->s_fs_info = NULL;
-
-	unlock_kernel();
 }
 
 static int udf_sync_fs(struct super_block *sb, int wait)
@@ -2183,8 +2179,6 @@ static unsigned int udf_count_free_bitmap(struct super_block *sb,
 	uint16_t ident;
 	struct spaceBitmapDesc *bm;
 
-	lock_kernel();
-
 	loc.logicalBlockNum = bitmap->s_extPosition;
 	loc.partitionReferenceNum = UDF_SB(sb)->s_partition;
 	bh = udf_read_ptagged(sb, &loc, 0, &ident);
@@ -2223,8 +2217,6 @@ static unsigned int udf_count_free_bitmap(struct super_block *sb,
 	brelse(bh);
 
 out:
-	unlock_kernel();
-
 	return accum;
 }
 
@@ -2237,8 +2229,6 @@ static unsigned int udf_count_free_table(struct super_block *sb,
 	int8_t etype;
 	struct extent_position epos;
 
-	lock_kernel();
-
 	epos.block = UDF_I(table)->i_location;
 	epos.offset = sizeof(struct unallocSpaceEntry);
 	epos.bh = NULL;
@@ -2248,8 +2238,6 @@ static unsigned int udf_count_free_table(struct super_block *sb,
 
 	brelse(epos.bh);
 
-	unlock_kernel();
-
 	return accum;
 }
 
-- 
1.7.0.4

Re: [PATCH 2/4] udf: Remove unnecessary bkl usages

[Christoph Hellwig]

On Sat, Nov 06, 2010 at 06:47:09PM +0100, Alessio Igor Bogani wrote:
> The udf_lookup, udf_create, udf_mknod, udf_mkdir, udf_rmdir, udf_link,
> udf_unlink and udf_readdir functions seems already adequately protected
> by i_mutex held by VFS invoking calls. The udf_rename function instead
> should be already protected by lock_rename again by VFS.
> 
> The udf_get_parent, udf_count_free_bitmap, udf_count_free_table and
> udf_put_super functions don't require any protection.

This looks correct to me.  But I'd suggest to reorder it to be the last
patch in the series, after the fine grained locking for the low level
code has been in place.

[PATCH 3/4] udf: Replace bkl with a mutex for protect udf_inode_info struct

[Alessio Igor Bogani]

Replace bkl with a mutex in udf_release_file, udf_symlink, udf_symlink_filler,
udf_truncate, udf_get_block, udf_block_map, udf_evict_inode and udf_write_inode
functions.

This work was supported by a hardware donation from the CE Linux Forum.

Signed-off-by: Alessio Igor Bogani <abogani@texware.it>
---
 fs/udf/file.c    |    9 +++++----
 fs/udf/inode.c   |   29 ++++++++++++++---------------
 fs/udf/namei.c   |    6 +++---
 fs/udf/super.c   |    3 +++
 fs/udf/symlink.c |   13 ++++++++-----
 fs/udf/udf_i.h   |    3 +++
 6 files changed, 36 insertions(+), 27 deletions(-)

diff --git a/fs/udf/file.c b/fs/udf/file.c
index 66b9e7e..688e6ea 100644
--- a/fs/udf/file.c
+++ b/fs/udf/file.c
@@ -33,6 +33,7 @@
 #include <linux/capability.h>
 #include <linux/errno.h>
 #include <linux/smp_lock.h>
+#include <linux/mutex.h>
 #include <linux/pagemap.h>
 #include <linux/buffer_head.h>
 #include <linux/aio.h>
@@ -202,13 +203,13 @@ out:
 
 static int udf_release_file(struct inode *inode, struct file *filp)
 {
+	struct udf_inode_info *iinfo = UDF_I(inode);
+
 	if (filp->f_mode & FMODE_WRITE) {
-		mutex_lock(&inode->i_mutex);
-		lock_kernel();
+		mutex_lock(&iinfo->lock);
 		udf_discard_prealloc(inode);
 		udf_truncate_tail_extent(inode);
-		unlock_kernel();
-		mutex_unlock(&inode->i_mutex);
+		mutex_unlock(&iinfo->lock);
 	}
 	return 0;
 }
diff --git a/fs/udf/inode.c b/fs/udf/inode.c
index fc48f37..6111af5 100644
--- a/fs/udf/inode.c
+++ b/fs/udf/inode.c
@@ -31,7 +31,7 @@
 
 #include "udfdecl.h"
 #include <linux/mm.h>
-#include <linux/smp_lock.h>
+#include <linux/mutex.h>
 #include <linux/module.h>
 #include <linux/pagemap.h>
 #include <linux/buffer_head.h>
@@ -79,9 +79,9 @@ void udf_evict_inode(struct inode *inode)
 		want_delete = 1;
 		inode->i_size = 0;
 		udf_truncate(inode);
-		lock_kernel();
+		mutex_lock(&iinfo->lock);
 		udf_update_inode(inode, IS_SYNC(inode));
-		unlock_kernel();
+		mutex_unlock(&iinfo->lock);
 	}
 	invalidate_inode_buffers(inode);
 	end_writeback(inode);
@@ -97,9 +97,7 @@ void udf_evict_inode(struct inode *inode)
 	kfree(iinfo->i_ext.i_data);
 	iinfo->i_ext.i_data = NULL;
 	if (want_delete) {
-		lock_kernel();
 		udf_free_inode(inode);
-		unlock_kernel();
 	}
 }
 
@@ -303,9 +301,8 @@ static int udf_get_block(struct inode *inode, sector_t block,
 	new = 0;
 	bh = NULL;
 
-	lock_kernel();
-
 	iinfo = UDF_I(inode);
+	mutex_lock(&iinfo->lock);
 	if (block == iinfo->i_next_alloc_block + 1) {
 		iinfo->i_next_alloc_block++;
 		iinfo->i_next_alloc_goal++;
@@ -324,7 +321,7 @@ static int udf_get_block(struct inode *inode, sector_t block,
 	map_bh(bh_result, inode->i_sb, phys);
 
 abort:
-	unlock_kernel();
+	mutex_unlock(&iinfo->lock);
 	return err;
 }
 
@@ -1022,8 +1019,8 @@ void udf_truncate(struct inode *inode)
 	if (IS_APPEND(inode) || IS_IMMUTABLE(inode))
 		return;
 
-	lock_kernel();
 	iinfo = UDF_I(inode);
+	mutex_lock(&iinfo->lock);
 	if (iinfo->i_alloc_type == ICBTAG_FLAG_AD_IN_ICB) {
 		if (inode->i_sb->s_blocksize <
 				(udf_file_entry_alloc_offset(inode) +
@@ -1031,7 +1028,7 @@ void udf_truncate(struct inode *inode)
 			udf_expand_file_adinicb(inode, inode->i_size, &err);
 			if (iinfo->i_alloc_type == ICBTAG_FLAG_AD_IN_ICB) {
 				inode->i_size = iinfo->i_lenAlloc;
-				unlock_kernel();
+				mutex_unlock(&iinfo->lock);
 				return;
 			} else
 				udf_truncate_extents(inode);
@@ -1053,7 +1050,7 @@ void udf_truncate(struct inode *inode)
 		udf_sync_inode(inode);
 	else
 		mark_inode_dirty(inode);
-	unlock_kernel();
+	mutex_unlock(&iinfo->lock);
 }
 
 static void __udf_read_inode(struct inode *inode)
@@ -1374,10 +1371,11 @@ static mode_t udf_convert_permissions(struct fileEntry *fe)
 int udf_write_inode(struct inode *inode, struct writeback_control *wbc)
 {
 	int ret;
+	struct udf_inode_info *iinfo = UDF_I(inode);
 
-	lock_kernel();
+	mutex_lock(&iinfo->lock);
 	ret = udf_update_inode(inode, wbc->sync_mode == WB_SYNC_ALL);
-	unlock_kernel();
+	mutex_unlock(&iinfo->lock);
 
 	return ret;
 }
@@ -2047,8 +2045,9 @@ long udf_block_map(struct inode *inode, sector_t block)
 	sector_t offset;
 	struct extent_position epos = {};
 	int ret;
+	struct udf_inode_info *iinfo = UDF_I(inode);
 
-	lock_kernel();
+	mutex_lock(&iinfo->lock);
 
 	if (inode_bmap(inode, block, &epos, &eloc, &elen, &offset) ==
 						(EXT_RECORDED_ALLOCATED >> 30))
@@ -2056,7 +2055,7 @@ long udf_block_map(struct inode *inode, sector_t block)
 	else
 		ret = 0;
 
-	unlock_kernel();
+	mutex_unlock(&iinfo->lock);
 	brelse(epos.bh);
 
 	if (UDF_QUERY_FLAG(inode->i_sb, UDF_FLAG_VARCONV))
diff --git a/fs/udf/namei.c b/fs/udf/namei.c
index 6e7c1b9..6022298 100644
--- a/fs/udf/namei.c
+++ b/fs/udf/namei.c
@@ -27,7 +27,7 @@
 #include <linux/errno.h>
 #include <linux/mm.h>
 #include <linux/slab.h>
-#include <linux/smp_lock.h>
+#include <linux/mutex.h>
 #include <linux/buffer_head.h>
 #include <linux/sched.h>
 #include <linux/crc-itu-t.h>
@@ -876,7 +876,6 @@ static int udf_symlink(struct inode *dir, struct dentry *dentry,
 	struct buffer_head *bh;
 	struct udf_inode_info *iinfo;
 
-	lock_kernel();
 	inode = udf_new_inode(dir, S_IFLNK | S_IRWXUGO, &err);
 	if (!inode)
 		goto out;
@@ -888,6 +887,7 @@ static int udf_symlink(struct inode *dir, struct dentry *dentry,
 	}
 
 	iinfo = UDF_I(inode);
+	mutex_lock(&iinfo->lock);
 	inode->i_data.a_ops = &udf_symlink_aops;
 	inode->i_op = &udf_symlink_inode_operations;
 
@@ -1026,8 +1026,8 @@ static int udf_symlink(struct inode *dir, struct dentry *dentry,
 	err = 0;
 
 out:
+	mutex_unlock(&iinfo->lock);
 	kfree(name);
-	unlock_kernel();
 	return err;
 
 out_no_entry:
diff --git a/fs/udf/super.c b/fs/udf/super.c
index 0027c1f..615859b 100644
--- a/fs/udf/super.c
+++ b/fs/udf/super.c
@@ -49,6 +49,7 @@
 #include <linux/cdrom.h>
 #include <linux/nls.h>
 #include <linux/smp_lock.h>
+#include <linux/mutex.h>
 #include <linux/buffer_head.h>
 #include <linux/vfs.h>
 #include <linux/vmalloc.h>
@@ -130,6 +131,8 @@ static struct inode *udf_alloc_inode(struct super_block *sb)
 	if (!ei)
 		return NULL;
 
+	mutex_init(&ei->lock);
+
 	ei->i_unique = 0;
 	ei->i_lenExtents = 0;
 	ei->i_next_alloc_block = 0;
diff --git a/fs/udf/symlink.c b/fs/udf/symlink.c
index 1606478..5ef30b9 100644
--- a/fs/udf/symlink.c
+++ b/fs/udf/symlink.c
@@ -27,7 +27,7 @@
 #include <linux/mm.h>
 #include <linux/stat.h>
 #include <linux/pagemap.h>
-#include <linux/smp_lock.h>
+#include <linux/mutex.h>
 #include <linux/buffer_head.h>
 #include "udf_i.h"
 
@@ -78,13 +78,16 @@ static int udf_symlink_filler(struct file *file, struct page *page)
 	int err = -EIO;
 	unsigned char *p = kmap(page);
 	struct udf_inode_info *iinfo;
+	uint32_t pos;
 
-	lock_kernel();
 	iinfo = UDF_I(inode);
+	pos = udf_block_map(inode, 0);
+
+	mutex_lock(&iinfo->lock);
 	if (iinfo->i_alloc_type == ICBTAG_FLAG_AD_IN_ICB) {
 		symlink = iinfo->i_ext.i_data + iinfo->i_lenEAttr;
 	} else {
-		bh = sb_bread(inode->i_sb, udf_block_map(inode, 0));
+		bh = sb_bread(inode->i_sb, pos);
 
 		if (!bh)
 			goto out;
@@ -95,14 +98,14 @@ static int udf_symlink_filler(struct file *file, struct page *page)
 	udf_pc_to_char(inode->i_sb, symlink, inode->i_size, p);
 	brelse(bh);
 
-	unlock_kernel();
+	mutex_unlock(&iinfo->lock);
 	SetPageUptodate(page);
 	kunmap(page);
 	unlock_page(page);
 	return 0;
 
 out:
-	unlock_kernel();
+	mutex_unlock(&iinfo->lock);
 	SetPageError(page);
 	kunmap(page);
 	unlock_page(page);
diff --git a/fs/udf/udf_i.h b/fs/udf/udf_i.h
index e58d1de..8f140ad 100644
--- a/fs/udf/udf_i.h
+++ b/fs/udf/udf_i.h
@@ -22,6 +22,9 @@ struct udf_inode_info {
 		__u8		*i_data;
 	} i_ext;
 	struct inode vfs_inode;
+
+	/* Serialize writer access, replace the old bkl */
+	struct mutex lock;
 };
 
 static inline struct udf_inode_info *UDF_I(struct inode *inode)
-- 
1.7.0.4

Re: [PATCH 3/4] udf: Replace bkl with a mutex for protect udf_inode_info struct

[Christoph Hellwig]

>  	struct inode vfs_inode;
> +
> +	/* Serialize writer access, replace the old bkl */
> +	struct mutex lock;

mentioning that it replaces bkl is not very useful for someone looking
at the code.  Neither is "Seriazlize writer access".  What would be
more useful is to document what exact fields are supposed to
be protected by this field.  Take a look at hfsplus_inode_info, where
I documented the locking as part of the BKL removal during the 2.6.37
cycle.  

[PATCH 4/4] udf: Replace bkl with a mutex for protect udf_sb_info struct

[Alessio Igor Bogani]

Replace bkl with a mutex in udf_ioctl, udf_remount_fs and  udf_fill_super
functions.

This work was supported by a hardware donation from the CE Linux Forum.

Signed-off-by: Alessio Igor Bogani <abogani@texware.it>
---
 fs/udf/file.c   |    7 +++----
 fs/udf/super.c  |   15 +++++++--------
 fs/udf/udf_sb.h |    3 +++
 3 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/fs/udf/file.c b/fs/udf/file.c
index 688e6ea..aa36fce 100644
--- a/fs/udf/file.c
+++ b/fs/udf/file.c
@@ -32,7 +32,6 @@
 #include <linux/string.h> /* memset */
 #include <linux/capability.h>
 #include <linux/errno.h>
-#include <linux/smp_lock.h>
 #include <linux/mutex.h>
 #include <linux/pagemap.h>
 #include <linux/buffer_head.h>
@@ -149,8 +148,7 @@ long udf_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 	struct inode *inode = filp->f_dentry->d_inode;
 	long old_block, new_block;
 	int result = -EINVAL;
-
-	lock_kernel();
+	struct udf_sb_info *sbi = UDF_SB(inode->i_sb);
 
 	if (file_permission(filp, MAY_READ) != 0) {
 		udf_debug("no permission to access inode %lu\n", inode->i_ino);
@@ -181,8 +179,10 @@ long udf_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 			result = -EFAULT;
 			goto out;
 		}
+		mutex_lock(&sbi->lock);
 		result = udf_relocate_blocks(inode->i_sb,
 						old_block, &new_block);
+		mutex_unlock(&sbi->lock);
 		if (result == 0)
 			result = put_user(new_block, (long __user *)arg);
 		goto out;
@@ -197,7 +197,6 @@ long udf_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 	}
 
 out:
-	unlock_kernel();
 	return result;
 }
 
diff --git a/fs/udf/super.c b/fs/udf/super.c
index 615859b..49e7b88 100644
--- a/fs/udf/super.c
+++ b/fs/udf/super.c
@@ -48,7 +48,6 @@
 #include <linux/stat.h>
 #include <linux/cdrom.h>
 #include <linux/nls.h>
-#include <linux/smp_lock.h>
 #include <linux/mutex.h>
 #include <linux/buffer_head.h>
 #include <linux/vfs.h>
@@ -570,7 +569,7 @@ static int udf_remount_fs(struct super_block *sb, int *flags, char *options)
 	if (!udf_parse_options(options, &uopt, true))
 		return -EINVAL;
 
-	lock_kernel();
+	mutex_lock(&sbi->lock);
 	sbi->s_flags = uopt.flags;
 	sbi->s_uid   = uopt.uid;
 	sbi->s_gid   = uopt.gid;
@@ -593,7 +592,7 @@ static int udf_remount_fs(struct super_block *sb, int *flags, char *options)
 		udf_open_lvid(sb);
 
 out_unlock:
-	unlock_kernel();
+	mutex_unlock(&sbi->lock);
 	return error;
 }
 
@@ -1886,8 +1885,6 @@ static int udf_fill_super(struct super_block *sb, void *options, int silent)
 	struct kernel_lb_addr rootdir, fileset;
 	struct udf_sb_info *sbi;
 
-	lock_kernel();
-
 	uopt.flags = (1 << UDF_FLAG_USE_AD_IN_ICB) | (1 << UDF_FLAG_STRICT);
 	uopt.uid = -1;
 	uopt.gid = -1;
@@ -1897,10 +1894,12 @@ static int udf_fill_super(struct super_block *sb, void *options, int silent)
 
 	sbi = kzalloc(sizeof(struct udf_sb_info), GFP_KERNEL);
 	if (!sbi) {
-		unlock_kernel();
 		return -ENOMEM;
 	}
 
+	mutex_init(&sbi->lock);
+	mutex_lock(&sbi->lock);
+
 	sb->s_fs_info = sbi;
 
 	mutex_init(&sbi->s_alloc_mutex);
@@ -2045,7 +2044,7 @@ static int udf_fill_super(struct super_block *sb, void *options, int silent)
 		goto error_out;
 	}
 	sb->s_maxbytes = MAX_LFS_FILESIZE;
-	unlock_kernel();
+	mutex_unlock(&sbi->lock);
 	return 0;
 
 error_out:
@@ -2066,7 +2065,7 @@ error_out:
 	kfree(sbi);
 	sb->s_fs_info = NULL;
 
-	unlock_kernel();
+	mutex_unlock(&sbi->lock);
 	return -EINVAL;
 }
 
diff --git a/fs/udf/udf_sb.h b/fs/udf/udf_sb.h
index d113b72..fd0dc81 100644
--- a/fs/udf/udf_sb.h
+++ b/fs/udf/udf_sb.h
@@ -150,6 +150,9 @@ struct udf_sb_info {
 	struct mutex		s_alloc_mutex;
 	/* Protected by s_alloc_mutex */
 	unsigned int		s_lvid_dirty;
+
+	/* Serialize writer access, replace the old bkl */
+	struct mutex lock;
 };
 
 static inline struct udf_sb_info *UDF_SB(struct super_block *sb)
-- 
1.7.0.4

Re: [PATCH 4/4] udf: Replace bkl with a mutex for protect udf_sb_info struct

[Christoph Hellwig]

> +		mutex_lock(&sbi->lock);
>  		result = udf_relocate_blocks(inode->i_sb,
>  						old_block, &new_block);
> +		mutex_unlock(&sbi->lock);

Moving the locking inside udf_relocate_blocks would be cleaner.

> @@ -570,7 +569,7 @@ static int udf_remount_fs(struct super_block *sb, int *flags, char *options)
>  	if (!udf_parse_options(options, &uopt, true))
>  		return -EINVAL;
>  
> -	lock_kernel();
> +	mutex_lock(&sbi->lock);

What are you protecting against here?  Concurrent remount calls are
protects against by the VFS.  Is there any reader that takes sbi->lock
to get a consistent view of the various options?  It doesn't seem you
introduce one in this series, so it seems we could do fine without any
locking.

> -		unlock_kernel();
>  		return -ENOMEM;
>  	}
>  
> +	mutex_init(&sbi->lock);
> +	mutex_lock(&sbi->lock);

What are you locking against here?  I can't fine anything that puts the
superblock on a global list in fill_super, and I can't find any code
that would look a superblock up that's not fully set up.  I don't think
synchronization here in fill_super is needed at all.

> +	/* Serialize writer access, replace the old bkl */
> +	struct mutex lock;

Same comment as for the per-inode mutex applies here, too.

Re: [PATCH 1/4] udf: Add missed protection for s_lvid_dirty

[Christoph Hellwig]

On Sat, Nov 06, 2010 at 06:47:08PM +0100, Alessio Igor Bogani wrote:
> As reported in udf_sb.h the udf_sb_infoi's structure member s_lvid_dirty should
> be protected by s_alloc_mutex. Added that mutex on a couple of places where it
> miss.

The whole s_lvid_dirty flag doesn't make any sense to me.  As a start it
simply duplicates s_dirty in the VFS superblock, but even more it just
controls the dirty state of s_lvid_bh.  I think you could simply kill
s_lvid_dirty, plus s_dirty inside udf and replace all calls to
udf_updated_lvid with a simple mark_buffer_dirty(sbi->s_lvid_bh) and
also get rid of all the locking around it.

While looking at this I also noticed that large parts of udf_open_lvid
and udf_close_lvid are basically duplicate.  The only difference seems
to be setting an integrityType of LVID_INTEGRITY_TYPE_OPEN vs
LVID_INTEGRITY_TYPE_CLOSE and updating a few revision counters on close.
If you're interested in working on udf that seems like a nice little
cleanup.

Re: [PATCH 1/4] udf: Add missed protection for s_lvid_dirty

[Jan Kara]

On Sat 06-11-10 14:05:30, Christoph Hellwig wrote:
> On Sat, Nov 06, 2010 at 06:47:08PM +0100, Alessio Igor Bogani wrote:
> > As reported in udf_sb.h the udf_sb_infoi's structure member s_lvid_dirty should
> > be protected by s_alloc_mutex. Added that mutex on a couple of places where it
> > miss.
> 
> The whole s_lvid_dirty flag doesn't make any sense to me.  As a start it
> simply duplicates s_dirty in the VFS superblock, but even more it just
> controls the dirty state of s_lvid_bh.  I think you could simply kill
> s_lvid_dirty, plus s_dirty inside udf and replace all calls to
> udf_updated_lvid with a simple mark_buffer_dirty(sbi->s_lvid_bh) and
> also get rid of all the locking around it.
  Well, I should have probably commented more on it :). The problem is that
people sometimes tend to mount CD-RW with UDF filesystem directly under
Linux.  Now LVID is updated on each allocation so if you write to the media,
the block carrying LVID is written rather often (like once per 5 seconds or
so) which quickly deteriorates that block on the media.
  So we basically want to hide the dirty bit on that buffer from VFS so that
it does not write the block so often.
  Looking at it now, cleaner fix might be to move the counters to sbi and
write them to the LVID only during umount. So that might be a nice cleanup
as well.

> While looking at this I also noticed that large parts of udf_open_lvid
> and udf_close_lvid are basically duplicate.  The only difference seems
> to be setting an integrityType of LVID_INTEGRITY_TYPE_OPEN vs
> LVID_INTEGRITY_TYPE_CLOSE and updating a few revision counters on close.
> If you're interested in working on udf that seems like a nice little
> cleanup.
  Yes, that could be cleaned up.

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: [PATCH 1/4] udf: Add missed protection for s_lvid_dirty

[Christoph Hellwig]

On Sun, Nov 07, 2010 at 03:14:03PM +0100, Jan Kara wrote:
>   Looking at it now, cleaner fix might be to move the counters to sbi and
> write them to the LVID only during umount. So that might be a nice cleanup
> as well.

Yes, that's what we're doing in most filesystems for these counters.

Re: [PATCH 1/4] udf: Add missed protection for s_lvid_dirty

[Jan Kara]

On Sat 06-11-10 18:47:08, Alessio Igor Bogani wrote:
> As reported in udf_sb.h the udf_sb_infoi's structure member s_lvid_dirty should
> be protected by s_alloc_mutex. Added that mutex on a couple of places where it
> miss.
> 
> This work was supported by a hardware donation from the CE Linux Forum.
  Thanks for doing the work! I actually did some work on BKL removal in UDF
before I learned that you also started working on it. My series is not
complete and needs testing but the sb-locking is better explained there I
think so it should address Christoph's comments. So could you maybe base your
changes on patches I already have?
  I've pushed the branch with my BKL patches to:
git://git.kernel.org/pub/scm/linux/kernel/git/jack/linux-udf-2.6
  to branch
bkl_removal
  Basically it is missing removal of BKL usage for protection of extents
in a buffer. I wanted to use inode->i_alloc_sem for that if it won't be too
ugly, otherwise some fs-private rw semaphore. I specifically didn't want to
use mutex because that would unnecessarily serialize parallel readers.

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: [PATCH 1/4] udf: Add missed protection for s_lvid_dirty

[Arnd Bergmann]

On Saturday 06 November 2010 18:47:08 Alessio Igor Bogani wrote:
> As reported in udf_sb.h the udf_sb_infoi's structure member s_lvid_dirty should
> be protected by s_alloc_mutex. Added that mutex on a couple of places where it
> miss.
> 
> This work was supported by a hardware donation from the CE Linux Forum.

Any update on this?

I'd really like to see the BKL disabled by default in 2.6.37, and UDF
is the final blocker for this now.

	Arnd

Re: [PATCH 1/4] udf: Add missed protection for s_lvid_dirty

[Jan Kara]

On Mon 15-11-10 23:46:41, Arnd Bergmann wrote:
> On Saturday 06 November 2010 18:47:08 Alessio Igor Bogani wrote:
> > As reported in udf_sb.h the udf_sb_infoi's structure member s_lvid_dirty should
> > be protected by s_alloc_mutex. Added that mutex on a couple of places where it
> > miss.
> > 
> > This work was supported by a hardware donation from the CE Linux Forum.
> 
> Any update on this?
> 
> I'd really like to see the BKL disabled by default in 2.6.37, and UDF
> is the final blocker for this now.
  Allesio sent me another version of patches last week and I sent him today
some comments (also told him to CC the list next time). I think the next
iteration should be fine as far as code review is concerned so after some
testing we can merge it...

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: [PATCH 1/4] udf: Add missed protection for s_lvid_dirty

[Alessio Igor Bogani]

Dear Gentlemen,

2010/11/16 Jan Kara <jack@suse.cz>:
[...]
>> Any update on this?
>>
>> I'd really like to see the BKL disabled by default in 2.6.37, and UDF
>> is the final blocker for this now.

>  Allesio sent me another version of patches last week and I sent him today
> some comments (also told him to CC the list next time). I think the next
> iteration should be fine as far as code review is concerned so after some
> testing we can merge it...

I'll provide the updated patches in few hours.

Ciao,
Alessio