[Cluster-devel] cluster/gfs-kernel/src/gfs ops_address.c ops_i ...

[wcheng@sourceware.org <wcheng@sourceware.org>]

CVSROOT:	/cvs/cluster
Module name:	cluster
Changes by:	wcheng at sourceware.org	2006-10-15 06:32:06

Modified files:
	gfs-kernel/src/gfs: ops_address.c ops_inode.c 

Log message:
	Bugzilla 203170 - direct IO deadlock: We'll have the same deadlock as
	described in bugzilla 173912 without RHEL4 kernel DIO_CLUSTER_LOCKING
	flag. To work around this issue, the i_alloc_sem is dropped from GFS.
	We expect glock will be able to handle the local synchronization.

Patches:
http://sourceware.org/cgi-bin/cvsweb.cgi/cluster/gfs-kernel/src/gfs/ops_address.c.diff?cvsroot=cluster&r1=1.13&r2=1.14
http://sourceware.org/cgi-bin/cvsweb.cgi/cluster/gfs-kernel/src/gfs/ops_inode.c.diff?cvsroot=cluster&r1=1.12&r2=1.13

--- cluster/gfs-kernel/src/gfs/ops_address.c	2006/08/02 17:21:19	1.13
+++ cluster/gfs-kernel/src/gfs/ops_address.c	2006/10/15 06:32:06	1.14
@@ -466,9 +466,15 @@
 	if (rw == WRITE && !get_transaction)
 		gb = get_blocks_noalloc;
 
-	return blockdev_direct_IO(rw, iocb, inode,
+	if (rw == WRITE)
+		return blockdev_direct_IO(rw, iocb, inode,
 				  inode->i_sb->s_bdev, iov,
 				  offset, nr_segs, gb, NULL);
+	else
+		return blockdev_direct_IO_no_locking(rw, iocb, inode,
+				  inode->i_sb->s_bdev, iov,
+				  offset, nr_segs, gb, NULL);
+
 }
 
 struct address_space_operations gfs_file_aops = {
--- cluster/gfs-kernel/src/gfs/ops_inode.c	2006/10/03 17:27:34	1.12
+++ cluster/gfs-kernel/src/gfs/ops_inode.c	2006/10/15 06:32:06	1.13
@@ -1340,7 +1340,36 @@
 
 	atomic_inc(&sdp->sd_ops_inode);
 
+	/* Bugzilla 203170: we'll have the same deadlock as described 
+	 * in bugzilla 173912 if
+	 * 1. without RHEL4's DIO_CLUSTER_LOCKING, and
+	 * 2. we come down to this line of code from do_truncate()
+	 *    where i_sem(i_mutex) and i_alloc_sem have been taken, and
+	 * 3. grab the exclusive glock here.
+	 * To avoid this to happen, i_alloc_sem must be dropped and trust
+	 * be put into glock that it can carry the same protection. 
+	 *
+	 * One issue with dropping i_alloc_sem is gfs_setattr() can be 
+	 * called from other code path without this sempaphore. Since linux
+	 * semaphore implementation doesn't include owner id, we have no way 
+	 * to reliably decide whether the following "up" is a correct reset. 
+	 * This implies if i_alloc_sem is ever used by non-direct_IO code 
+	 * path in the future, this hack will fall apart. In short, with this 
+	 * change, i_alloc_sem has become a meaningless lock within GFS and 
+	 * don't expect its counter representing any correct state. 
+	 *
+	 * wcheng at redhat.com 10/14/06  
+	 */ 
+	if (attr->ia_valid & ATTR_SIZE) {
+		up_write(&dentry->d_inode->i_alloc_sem); 
+	}
+	
 	error = gfs_glock_nq_init(ip->i_gl, LM_ST_EXCLUSIVE, 0, &i_gh);
+
+	if (attr->ia_valid & ATTR_SIZE) {
+		down_write(&dentry->d_inode->i_alloc_sem); 
+	}
+	
 	if (error)
 		return error;