[PATCH 00/11] extent list locking fixes V3

[PATCH 00/11] extent list locking fixes V3

[Christoph Hellwig]

Changes from V2:
	- take care to lock for data vs attr fork differently

Changes from V1:
	- fixed the review feedback
	- includes Bens original patch for completeness

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 01/11] xfs: no need to lock the inode in xfs_find_handle

[Christoph Hellwig]

[-- Attachment #1: xfs-fix-xfs_find_handle-locking --]
[-- Type: text/plain, Size: 940 bytes --]

Both the inode number and the generation do not change on a live inode.

Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: xfs/fs/xfs/xfs_ioctl.c
===================================================================
--- xfs.orig/fs/xfs/xfs_ioctl.c	2013-12-06 19:16:33.819188640 +0100
+++ xfs/fs/xfs/xfs_ioctl.c	2013-12-06 19:17:04.087188019 +0100
@@ -112,15 +112,11 @@ xfs_find_handle(
 		memset(&handle.ha_fid, 0, sizeof(handle.ha_fid));
 		hsize = sizeof(xfs_fsid_t);
 	} else {
-		int		lock_mode;
-
-		lock_mode = xfs_ilock_map_shared(ip);
 		handle.ha_fid.fid_len = sizeof(xfs_fid_t) -
 					sizeof(handle.ha_fid.fid_len);
 		handle.ha_fid.fid_pad = 0;
 		handle.ha_fid.fid_gen = ip->i_d.di_gen;
 		handle.ha_fid.fid_ino = ip->i_ino;
-		xfs_iunlock_map_shared(ip, lock_mode);
 
 		hsize = XFS_HSIZE(handle);
 	}

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 02/11] xfs: remove xfs_iunlock_map_shared

[Christoph Hellwig]

[-- Attachment #1: xfs-remove-xfs_iunlock_map_shared --]
[-- Type: text/plain, Size: 3237 bytes --]

We can just use xfs_iunlock without any loss of clarity.

Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: xfs/fs/xfs/xfs_bmap_util.c
===================================================================
--- xfs.orig/fs/xfs/xfs_bmap_util.c	2013-12-06 19:53:52.175142705 +0100
+++ xfs/fs/xfs/xfs_bmap_util.c	2013-12-06 19:57:22.371138392 +0100
@@ -737,7 +737,7 @@ xfs_getbmap(
  out_free_map:
 	kmem_free(map);
  out_unlock_ilock:
-	xfs_iunlock_map_shared(ip, lock);
+	xfs_iunlock(ip, lock);
  out_unlock_iolock:
 	xfs_iunlock(ip, XFS_IOLOCK_SHARED);
 
Index: xfs/fs/xfs/xfs_file.c
===================================================================
--- xfs.orig/fs/xfs/xfs_file.c	2013-12-06 19:53:52.175142705 +0100
+++ xfs/fs/xfs/xfs_file.c	2013-12-06 19:57:22.371138392 +0100
@@ -1294,7 +1294,7 @@ out:
 	offset = vfs_setpos(file, offset, inode->i_sb->s_maxbytes);
 
 out_unlock:
-	xfs_iunlock_map_shared(ip, lock);
+	xfs_iunlock(ip, lock);
 
 	if (error)
 		return -error;
@@ -1402,7 +1402,7 @@ out:
 	offset = vfs_setpos(file, offset, inode->i_sb->s_maxbytes);
 
 out_unlock:
-	xfs_iunlock_map_shared(ip, lock);
+	xfs_iunlock(ip, lock);
 
 	if (error)
 		return -error;
Index: xfs/fs/xfs/xfs_inode.c
===================================================================
--- xfs.orig/fs/xfs/xfs_inode.c	2013-12-06 19:53:52.175142705 +0100
+++ xfs/fs/xfs/xfs_inode.c	2013-12-06 19:57:33.199138169 +0100
@@ -88,8 +88,7 @@ xfs_get_extsz_hint(
  * have been read in yet, and only lock the inode exclusively if they have not.
  *
  * The function returns a value which should be given to the corresponding
- * xfs_iunlock_map_shared().  This value is the mode in which the lock was
- * actually taken.
+ * xfs_iunlock() call.
  */
 uint
 xfs_ilock_map_shared(
@@ -110,18 +109,6 @@ xfs_ilock_map_shared(
 }
 
 /*
- * This is simply the unlock routine to go with xfs_ilock_map_shared().
- * All it does is call xfs_iunlock() with the given lock_mode.
- */
-void
-xfs_iunlock_map_shared(
-	xfs_inode_t	*ip,
-	unsigned int	lock_mode)
-{
-	xfs_iunlock(ip, lock_mode);
-}
-
-/*
  * The xfs inode contains 2 locks: a multi-reader lock called the
  * i_iolock and a multi-reader lock called the i_lock.  This routine
  * allows either or both of the locks to be obtained.
@@ -590,7 +577,7 @@ xfs_lookup(
 
 	lock_mode = xfs_ilock_map_shared(dp);
 	error = xfs_dir_lookup(NULL, dp, name, &inum, ci_name);
-	xfs_iunlock_map_shared(dp, lock_mode);
+	xfs_iunlock(dp, lock_mode);
 
 	if (error)
 		goto out;
Index: xfs/fs/xfs/xfs_inode.h
===================================================================
--- xfs.orig/fs/xfs/xfs_inode.h	2013-12-06 19:53:52.175142705 +0100
+++ xfs/fs/xfs/xfs_inode.h	2013-12-06 19:57:22.371138392 +0100
@@ -338,7 +338,6 @@ void		xfs_iunlock(xfs_inode_t *, uint);
 void		xfs_ilock_demote(xfs_inode_t *, uint);
 int		xfs_isilocked(xfs_inode_t *, uint);
 uint		xfs_ilock_map_shared(xfs_inode_t *);
-void		xfs_iunlock_map_shared(xfs_inode_t *, uint);
 int		xfs_ialloc(struct xfs_trans *, xfs_inode_t *, umode_t,
 			   xfs_nlink_t, xfs_dev_t, prid_t, int,
 			   struct xfs_buf **, xfs_inode_t **);

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 03/11] xfs: rename xfs_ilock_map_shared

[Christoph Hellwig]

[-- Attachment #1: xfs-rename-xfs_ilock_map_shared --]
[-- Type: text/plain, Size: 3919 bytes --]

Make it clear that we're only locking against the extent map on the data
fork.  Also clean the function up a little bit.

Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: xfs/fs/xfs/xfs_aops.c
===================================================================
--- xfs.orig/fs/xfs/xfs_aops.c	2013-12-06 19:57:22.371138392 +0100
+++ xfs/fs/xfs/xfs_aops.c	2013-12-06 19:58:15.755137296 +0100
@@ -1217,7 +1217,7 @@ __xfs_get_blocks(
 		lockmode = XFS_ILOCK_EXCL;
 		xfs_ilock(ip, lockmode);
 	} else {
-		lockmode = xfs_ilock_map_shared(ip);
+		lockmode = xfs_ilock_data_map_shared(ip);
 	}
 
 	ASSERT(offset <= mp->m_super->s_maxbytes);
Index: xfs/fs/xfs/xfs_file.c
===================================================================
--- xfs.orig/fs/xfs/xfs_file.c	2013-12-06 19:57:22.371138392 +0100
+++ xfs/fs/xfs/xfs_file.c	2013-12-06 19:58:15.759137296 +0100
@@ -912,7 +912,7 @@ xfs_dir_open(
 	 * If there are any blocks, read-ahead block 0 as we're almost
 	 * certain to have the next operation be a read there.
 	 */
-	mode = xfs_ilock_map_shared(ip);
+	mode = xfs_ilock_data_map_shared(ip);
 	if (ip->i_d.di_nextents > 0)
 		xfs_dir3_data_readahead(NULL, ip, 0, -1);
 	xfs_iunlock(ip, mode);
@@ -1215,7 +1215,7 @@ xfs_seek_data(
 	uint			lock;
 	int			error;
 
-	lock = xfs_ilock_map_shared(ip);
+	lock = xfs_ilock_data_map_shared(ip);
 
 	isize = i_size_read(inode);
 	if (start >= isize) {
@@ -1319,7 +1319,7 @@ xfs_seek_hole(
 	if (XFS_FORCED_SHUTDOWN(mp))
 		return -XFS_ERROR(EIO);
 
-	lock = xfs_ilock_map_shared(ip);
+	lock = xfs_ilock_data_map_shared(ip);
 
 	isize = i_size_read(inode);
 	if (start >= isize) {
Index: xfs/fs/xfs/xfs_inode.c
===================================================================
--- xfs.orig/fs/xfs/xfs_inode.c	2013-12-06 19:57:33.199138169 +0100
+++ xfs/fs/xfs/xfs_inode.c	2013-12-06 19:58:41.667136764 +0100
@@ -91,20 +91,15 @@ xfs_get_extsz_hint(
  * xfs_iunlock() call.
  */
 uint
-xfs_ilock_map_shared(
-	xfs_inode_t	*ip)
+xfs_ilock_data_map_shared(
+	struct xfs_inode	*ip)
 {
-	uint	lock_mode;
+	uint			lock_mode = XFS_ILOCK_SHARED;
 
-	if ((ip->i_d.di_format == XFS_DINODE_FMT_BTREE) &&
-	    ((ip->i_df.if_flags & XFS_IFEXTENTS) == 0)) {
+	if (ip->i_d.di_format == XFS_DINODE_FMT_BTREE &&
+	    (ip->i_df.if_flags & XFS_IFEXTENTS) == 0)
 		lock_mode = XFS_ILOCK_EXCL;
-	} else {
-		lock_mode = XFS_ILOCK_SHARED;
-	}
-
 	xfs_ilock(ip, lock_mode);
-
 	return lock_mode;
 }
 
@@ -575,7 +570,7 @@ xfs_lookup(
 	if (XFS_FORCED_SHUTDOWN(dp->i_mount))
 		return XFS_ERROR(EIO);
 
-	lock_mode = xfs_ilock_map_shared(dp);
+	lock_mode = xfs_ilock_data_map_shared(dp);
 	error = xfs_dir_lookup(NULL, dp, name, &inum, ci_name);
 	xfs_iunlock(dp, lock_mode);
 
Index: xfs/fs/xfs/xfs_inode.h
===================================================================
--- xfs.orig/fs/xfs/xfs_inode.h	2013-12-06 19:57:22.371138392 +0100
+++ xfs/fs/xfs/xfs_inode.h	2013-12-06 19:58:15.759137296 +0100
@@ -337,7 +337,7 @@ int		xfs_ilock_nowait(xfs_inode_t *, uin
 void		xfs_iunlock(xfs_inode_t *, uint);
 void		xfs_ilock_demote(xfs_inode_t *, uint);
 int		xfs_isilocked(xfs_inode_t *, uint);
-uint		xfs_ilock_map_shared(xfs_inode_t *);
+uint		xfs_ilock_data_map_shared(struct xfs_inode *);
 int		xfs_ialloc(struct xfs_trans *, xfs_inode_t *, umode_t,
 			   xfs_nlink_t, xfs_dev_t, prid_t, int,
 			   struct xfs_buf **, xfs_inode_t **);
Index: xfs/fs/xfs/xfs_bmap_util.c
===================================================================
--- xfs.orig/fs/xfs/xfs_bmap_util.c	2013-12-06 19:57:22.371138392 +0100
+++ xfs/fs/xfs/xfs_bmap_util.c	2013-12-06 19:58:15.759137296 +0100
@@ -632,7 +632,7 @@ xfs_getbmap(
 		 */
 	}
 
-	lock = xfs_ilock_map_shared(ip);
+	lock = xfs_ilock_data_map_shared(ip);
 
 	/*
 	 * Don't let nex be bigger than the number of extents

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 04/11] xfs: add xfs_ilock_attr_map_shared

[Christoph Hellwig]

[-- Attachment #1: xfs-add-xfs_ilock_attr_map_shared --]
[-- Type: text/plain, Size: 4861 bytes --]

Equivalent to xfs_ilock_data_map_shared, except for the attribute fork.

Make xfs_getbmap use it if called for the attribute fork instead of
xfs_ilock_data_map_shared.

Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: xfs/fs/xfs/xfs_bmap_util.c
===================================================================
--- xfs.orig/fs/xfs/xfs_bmap_util.c	2013-12-06 19:58:15.759137296 +0100
+++ xfs/fs/xfs/xfs_bmap_util.c	2013-12-06 20:02:23.291132216 +0100
@@ -617,22 +617,27 @@ xfs_getbmap(
 		return XFS_ERROR(ENOMEM);
 
 	xfs_ilock(ip, XFS_IOLOCK_SHARED);
-	if (whichfork == XFS_DATA_FORK && !(iflags & BMV_IF_DELALLOC)) {
-		if (ip->i_delayed_blks || XFS_ISIZE(ip) > ip->i_d.di_size) {
+	if (whichfork == XFS_DATA_FORK) {
+		if (!(iflags & BMV_IF_DELALLOC) &&
+		    (ip->i_delayed_blks || XFS_ISIZE(ip) > ip->i_d.di_size)) {
 			error = -filemap_write_and_wait(VFS_I(ip)->i_mapping);
 			if (error)
 				goto out_unlock_iolock;
+
+			/*
+			 * Even after flushing the inode, there can still be
+			 * delalloc blocks on the inode beyond EOF due to
+			 * speculative reallocation.  These are not removed
+			 * until the release function is called or the inode
+			 * is inactivated.  Hence we cannot assert here that
+			 * ip->i_delayed_blks == 0.
+			 */
 		}
-		/*
-		 * even after flushing the inode, there can still be delalloc
-		 * blocks on the inode beyond EOF due to speculative
-		 * preallocation. These are not removed until the release
-		 * function is called or the inode is inactivated. Hence we
-		 * cannot assert here that ip->i_delayed_blks == 0.
-		 */
-	}
 
-	lock = xfs_ilock_data_map_shared(ip);
+		lock = xfs_ilock_data_map_shared(ip);
+	} else {
+		lock = xfs_ilock_attr_map_shared(ip);
+	}
 
 	/*
 	 * Don't let nex be bigger than the number of extents
Index: xfs/fs/xfs/xfs_inode.c
===================================================================
--- xfs.orig/fs/xfs/xfs_inode.c	2013-12-06 19:58:41.667136764 +0100
+++ xfs/fs/xfs/xfs_inode.c	2013-12-06 20:00:44.535134243 +0100
@@ -77,17 +77,18 @@ xfs_get_extsz_hint(
 }
 
 /*
- * This is a wrapper routine around the xfs_ilock() routine used to centralize
- * some grungy code.  It is used in places that wish to lock the inode solely
- * for reading the extents.  The reason these places can't just call
- * xfs_ilock(SHARED) is that the inode lock also guards to bringing in of the
- * extents from disk for a file in b-tree format.  If the inode is in b-tree
- * format, then we need to lock the inode exclusively until the extents are read
- * in.  Locking it exclusively all the time would limit our parallelism
- * unnecessarily, though.  What we do instead is check to see if the extents
- * have been read in yet, and only lock the inode exclusively if they have not.
+ * These two are wrapper routines around the xfs_ilock() routine used to
+ * centralize some grungy code.  They are used in places that wish to lock the
+ * inode solely for reading the extents.  The reason these places can't just
+ * call xfs_ilock(ip, XFS_ILOCK_SHARED) is that the inode lock also guards to
+ * bringing in of the extents from disk for a file in b-tree format.  If the
+ * inode is in b-tree format, then we need to lock the inode exclusively until
+ * the extents are read in.  Locking it exclusively all the time would limit
+ * our parallelism unnecessarily, though.  What we do instead is check to see
+ * if the extents have been read in yet, and only lock the inode exclusively
+ * if they have not.
  *
- * The function returns a value which should be given to the corresponding
+ * The functions return a value which should be given to the corresponding
  * xfs_iunlock() call.
  */
 uint
@@ -101,6 +102,19 @@ xfs_ilock_data_map_shared(
 		lock_mode = XFS_ILOCK_EXCL;
 	xfs_ilock(ip, lock_mode);
 	return lock_mode;
+}
+
+uint
+xfs_ilock_attr_map_shared(
+	struct xfs_inode	*ip)
+{
+	uint			lock_mode = XFS_ILOCK_SHARED;
+
+	if (ip->i_d.di_aformat == XFS_DINODE_FMT_BTREE &&
+	    (ip->i_afp->if_flags & XFS_IFEXTENTS) == 0)
+		lock_mode = XFS_ILOCK_EXCL;
+	xfs_ilock(ip, lock_mode);
+	return lock_mode;
 }
 
 /*
Index: xfs/fs/xfs/xfs_inode.h
===================================================================
--- xfs.orig/fs/xfs/xfs_inode.h	2013-12-06 19:58:15.759137296 +0100
+++ xfs/fs/xfs/xfs_inode.h	2013-12-06 20:00:50.955134111 +0100
@@ -338,6 +338,7 @@ void		xfs_iunlock(xfs_inode_t *, uint);
 void		xfs_ilock_demote(xfs_inode_t *, uint);
 int		xfs_isilocked(xfs_inode_t *, uint);
 uint		xfs_ilock_data_map_shared(struct xfs_inode *);
+uint		xfs_ilock_attr_map_shared(struct xfs_inode *);
 int		xfs_ialloc(struct xfs_trans *, xfs_inode_t *, umode_t,
 			   xfs_nlink_t, xfs_dev_t, prid_t, int,
 			   struct xfs_buf **, xfs_inode_t **);

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 05/11] xfs: reinstate the ilock in xfs_readdir

[Christoph Hellwig]

[-- Attachment #1: iolock.diff --]
[-- Type: text/plain, Size: 1674 bytes --]

From: Ben Myers <bpm@sgi.com>

Although it was removed in commit 051e7cd44ab8, ilock needs to be taken in
xfs_readdir because we might have to read the extent list in from disk.  This
keeps other threads from reading from or writing to the extent list while it is
being read in and is still in a transitional state.

This has been associated with "Access to block zero" messages on directories
with large numbers of extents resulting from excessive filesytem fragmentation,
as well as extent list corruption.  Unfortunately no test case at this point.

Signed-off-by: Ben Myers <bpm@sgi.com>

---
 fs/xfs/xfs_dir2_readdir.c |    4 ++++
 1 file changed, 4 insertions(+)

Index: xfs/fs/xfs/xfs_dir2_readdir.c
===================================================================
--- xfs.orig/fs/xfs/xfs_dir2_readdir.c	2013-12-06 17:20:27.695331596 +0100
+++ xfs/fs/xfs/xfs_dir2_readdir.c	2013-12-06 19:38:08.375162074 +0100
@@ -674,6 +674,7 @@ xfs_readdir(
 {
 	int		rval;		/* return value */
 	int		v;		/* type-checking value */
+	uint		lock_mode;
 
 	trace_xfs_readdir(dp);
 
@@ -683,6 +684,7 @@ xfs_readdir(
 	ASSERT(S_ISDIR(dp->i_d.di_mode));
 	XFS_STATS_INC(xs_dir_getdents);
 
+	lock_mode = xfs_ilock_data_map_shared(dp);
 	if (dp->i_d.di_format == XFS_DINODE_FMT_LOCAL)
 		rval = xfs_dir2_sf_getdents(dp, ctx);
 	else if ((rval = xfs_dir2_isblock(NULL, dp, &v)))
@@ -691,5 +693,7 @@ xfs_readdir(
 		rval = xfs_dir2_block_getdents(dp, ctx);
 	else
 		rval = xfs_dir2_leaf_getdents(dp, ctx, bufsize);
+	xfs_iunlock(dp, lock_mode);
+
 	return rval;
 }

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 06/11] xfs: take the ilock around xfs_bmapi_read in xfs_zero_remaining_bytes

[Christoph Hellwig]

[-- Attachment #1: xfs_zero_remaining_bytes-locking --]
[-- Type: text/plain, Size: 851 bytes --]

Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: xfs/fs/xfs/xfs_bmap_util.c
===================================================================
--- xfs.orig/fs/xfs/xfs_bmap_util.c	2013-12-06 19:37:36.715162723 +0100
+++ xfs/fs/xfs/xfs_bmap_util.c	2013-12-06 19:38:45.643161309 +0100
@@ -1175,9 +1175,15 @@ xfs_zero_remaining_bytes(
 	xfs_buf_unlock(bp);
 
 	for (offset = startoff; offset <= endoff; offset = lastoffset + 1) {
+		uint lock_mode;
+
 		offset_fsb = XFS_B_TO_FSBT(mp, offset);
 		nimap = 1;
+
+		lock_mode = xfs_ilock_data_map_shared(ip);
 		error = xfs_bmapi_read(ip, offset_fsb, 1, &imap, &nimap, 0);
+		xfs_iunlock(ip, lock_mode);
+
 		if (error || nimap < 1)
 			break;
 		ASSERT(imap.br_blockcount >= 1);

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 07/11] xfs: use xfs_ilock_data_map_shared in xfs_qm_dqtobp

[Christoph Hellwig]

[-- Attachment #1: xfs-quota-locking --]
[-- Type: text/plain, Size: 1400 bytes --]

We might not have read in the extent list at this point, so make sure we
take the ilock exclusively if we have to do so.

Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: xfs/fs/xfs/xfs_dquot.c
===================================================================
--- xfs.orig/fs/xfs/xfs_dquot.c	2013-12-06 17:20:27.571331599 +0100
+++ xfs/fs/xfs/xfs_dquot.c	2013-12-06 19:39:42.307160146 +0100
@@ -469,16 +469,17 @@ xfs_qm_dqtobp(
 	struct xfs_mount	*mp = dqp->q_mount;
 	xfs_dqid_t		id = be32_to_cpu(dqp->q_core.d_id);
 	struct xfs_trans	*tp = (tpp ? *tpp : NULL);
+	uint			lock_mode;
 
 	dqp->q_fileoffset = (xfs_fileoff_t)id / mp->m_quotainfo->qi_dqperchunk;
 
-	xfs_ilock(quotip, XFS_ILOCK_SHARED);
+	lock_mode = xfs_ilock_data_map_shared(quotip);
 	if (!xfs_this_quota_on(dqp->q_mount, dqp->dq_flags)) {
 		/*
 		 * Return if this type of quotas is turned off while we
 		 * didn't have the quota inode lock.
 		 */
-		xfs_iunlock(quotip, XFS_ILOCK_SHARED);
+		xfs_iunlock(quotip, lock_mode);
 		return ESRCH;
 	}
 
@@ -488,7 +489,7 @@ xfs_qm_dqtobp(
 	error = xfs_bmapi_read(quotip, dqp->q_fileoffset,
 			       XFS_DQUOT_CLUSTER_SIZE_FSB, &map, &nmaps, 0);
 
-	xfs_iunlock(quotip, XFS_ILOCK_SHARED);
+	xfs_iunlock(quotip, lock_mode);
 	if (error)
 		return error;
 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 08/11] xfs: use xfs_ilock_data_map_shared in xfs_qm_dqiterate

[Christoph Hellwig]

[-- Attachment #1: xfs-quota-locking-2 --]
[-- Type: text/plain, Size: 1136 bytes --]

We might not have read in the extent list at this point, so make sure we
take the ilock exclusively if we have to do so.

Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: xfs/fs/xfs/xfs_qm.c
===================================================================
--- xfs.orig/fs/xfs/xfs_qm.c	2013-12-06 17:20:27.491331600 +0100
+++ xfs/fs/xfs/xfs_qm.c	2013-12-06 19:39:27.251160455 +0100
@@ -1193,16 +1193,18 @@ xfs_qm_dqiterate(
 	lblkno = 0;
 	maxlblkcnt = XFS_B_TO_FSB(mp, mp->m_super->s_maxbytes);
 	do {
+		uint		lock_mode;
+
 		nmaps = XFS_DQITER_MAP_SIZE;
 		/*
 		 * We aren't changing the inode itself. Just changing
 		 * some of its data. No new blocks are added here, and
 		 * the inode is never added to the transaction.
 		 */
-		xfs_ilock(qip, XFS_ILOCK_SHARED);
+		lock_mode = xfs_ilock_data_map_shared(qip);
 		error = xfs_bmapi_read(qip, lblkno, maxlblkcnt - lblkno,
 				       map, &nmaps, 0);
-		xfs_iunlock(qip, XFS_ILOCK_SHARED);
+		xfs_iunlock(qip, lock_mode);
 		if (error)
 			break;
 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 09/11] xfs: use xfs_ilock_attr_map_shared in xfs_attr_get

[Christoph Hellwig]

[-- Attachment #1: xfs-attr-locking --]
[-- Type: text/plain, Size: 946 bytes --]

We might not have read in the extent list at this point, so make sure we
take the ilock exclusively if we have to do so.

Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: xfs/fs/xfs/xfs_attr.c
===================================================================
--- xfs.orig/fs/xfs/xfs_attr.c	2013-12-06 17:20:27.447331601 +0100
+++ xfs/fs/xfs/xfs_attr.c	2013-12-06 19:41:05.119158446 +0100
@@ -164,6 +164,7 @@ xfs_attr_get(
 {
 	int		error;
 	struct xfs_name	xname;
+	uint		lock_mode;
 
 	XFS_STATS_INC(xs_attr_get);
 
@@ -174,9 +175,9 @@ xfs_attr_get(
 	if (error)
 		return error;
 
-	xfs_ilock(ip, XFS_ILOCK_SHARED);
+	lock_mode = xfs_ilock_attr_map_shared(ip);
 	error = xfs_attr_get_int(ip, &xname, value, valuelenp, flags);
-	xfs_iunlock(ip, XFS_ILOCK_SHARED);
+	xfs_iunlock(ip, lock_mode);
 	return(error);
 }
 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 10/11] xfs: use xfs_ilock_attr_map_shared in xfs_attr_list_int

[Christoph Hellwig]

[-- Attachment #1: xfs-attr-locking-2 --]
[-- Type: text/plain, Size: 1184 bytes --]

We might not have read in the extent list at this point, so make sure we
take the ilock exclusively if we have to do so.

Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: xfs/fs/xfs/xfs_attr_list.c
===================================================================
--- xfs.orig/fs/xfs/xfs_attr_list.c	2013-12-06 17:20:27.371331603 +0100
+++ xfs/fs/xfs/xfs_attr_list.c	2013-12-06 19:41:42.779157674 +0100
@@ -507,17 +507,17 @@ xfs_attr_list_int(
 {
 	int error;
 	xfs_inode_t *dp = context->dp;
+	uint		lock_mode;
 
 	XFS_STATS_INC(xs_attr_list);
 
 	if (XFS_FORCED_SHUTDOWN(dp->i_mount))
 		return EIO;
 
-	xfs_ilock(dp, XFS_ILOCK_SHARED);
-
 	/*
 	 * Decide on what work routines to call based on the inode size.
 	 */
+	lock_mode = xfs_ilock_attr_map_shared(dp);
 	if (!xfs_inode_hasattr(dp)) {
 		error = 0;
 	} else if (dp->i_d.di_aformat == XFS_DINODE_FMT_LOCAL) {
@@ -527,9 +527,7 @@ xfs_attr_list_int(
 	} else {
 		error = xfs_attr_node_list(context);
 	}
-
-	xfs_iunlock(dp, XFS_ILOCK_SHARED);
-
+	xfs_iunlock(dp, lock_mode);
 	return error;
 }
 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 11/11] xfs: assert that we hold the ilock for extent map access

[Christoph Hellwig]

[-- Attachment #1: iread_extents-assert --]
[-- Type: text/plain, Size: 2410 bytes --]

Make sure that xfs_bmapi_read has the ilock held in some way, and that
xfs_bmapi_write, xfs_bmapi_delay, xfs_bunmapi and xfs_iread_extents are
called with the ilock held exclusively.

Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: xfs/fs/xfs/xfs_bmap.c
===================================================================
--- xfs.orig/fs/xfs/xfs_bmap.c	2013-11-29 14:25:12.172459195 +0100
+++ xfs/fs/xfs/xfs_bmap.c	2013-12-05 10:03:28.243801633 +0100
@@ -3997,6 +3997,7 @@ xfs_bmapi_read(
 	ASSERT(*nmap >= 1);
 	ASSERT(!(flags & ~(XFS_BMAPI_ATTRFORK|XFS_BMAPI_ENTIRE|
 			   XFS_BMAPI_IGSTATE)));
+	ASSERT(xfs_isilocked(ip, XFS_ILOCK_SHARED|XFS_ILOCK_EXCL));
 
 	if (unlikely(XFS_TEST_ERROR(
 	    (XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_EXTENTS &&
@@ -4191,6 +4192,7 @@ xfs_bmapi_delay(
 	ASSERT(*nmap >= 1);
 	ASSERT(*nmap <= XFS_BMAP_MAX_NMAP);
 	ASSERT(!(flags & ~XFS_BMAPI_ENTIRE));
+	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
 
 	if (unlikely(XFS_TEST_ERROR(
 	    (XFS_IFORK_FORMAT(ip, XFS_DATA_FORK) != XFS_DINODE_FMT_EXTENTS &&
@@ -4484,6 +4486,7 @@ xfs_bmapi_write(
 	ASSERT(tp != NULL);
 	ASSERT(len > 0);
 	ASSERT(XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_LOCAL);
+	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
 
 	if (unlikely(XFS_TEST_ERROR(
 	    (XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_EXTENTS &&
Index: xfs/fs/xfs/xfs_inode_fork.c
===================================================================
--- xfs.orig/fs/xfs/xfs_inode_fork.c	2013-12-05 09:57:24.347809100 +0100
+++ xfs/fs/xfs/xfs_inode_fork.c	2013-12-05 09:59:04.767807039 +0100
@@ -431,6 +431,8 @@ xfs_iread_extents(
 	xfs_ifork_t	*ifp;
 	xfs_extnum_t	nextents;
 
+	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
+
 	if (unlikely(XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_BTREE)) {
 		XFS_ERROR_REPORT("xfs_iread_extents", XFS_ERRLEVEL_LOW,
 				 ip->i_mount);
Index: xfs/fs/xfs/xfs_bmap.c
===================================================================
--- xfs.orig/fs/xfs/xfs_bmap.c	2013-12-05 22:12:32.931838271 +0100
+++ xfs/fs/xfs/xfs_bmap.c	2013-12-05 22:35:59.175809412 +0100
@@ -5038,6 +5038,7 @@ xfs_bunmapi(
 	if (XFS_FORCED_SHUTDOWN(mp))
 		return XFS_ERROR(EIO);
 
+	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
 	ASSERT(len > 0);
 	ASSERT(nexts >= 0);
 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 01/11] xfs: no need to lock the inode in xfs_find_handle

[Dave Chinner]

On Fri, Dec 06, 2013 at 12:30:07PM -0800, Christoph Hellwig wrote:
> Both the inode number and the generation do not change on a live inode.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> Index: xfs/fs/xfs/xfs_ioctl.c
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_ioctl.c	2013-12-06 19:16:33.819188640 +0100
> +++ xfs/fs/xfs/xfs_ioctl.c	2013-12-06 19:17:04.087188019 +0100
> @@ -112,15 +112,11 @@ xfs_find_handle(
>  		memset(&handle.ha_fid, 0, sizeof(handle.ha_fid));
>  		hsize = sizeof(xfs_fsid_t);
>  	} else {
> -		int		lock_mode;
> -
> -		lock_mode = xfs_ilock_map_shared(ip);
>  		handle.ha_fid.fid_len = sizeof(xfs_fid_t) -
>  					sizeof(handle.ha_fid.fid_len);
>  		handle.ha_fid.fid_pad = 0;
>  		handle.ha_fid.fid_gen = ip->i_d.di_gen;
>  		handle.ha_fid.fid_ino = ip->i_ino;
> -		xfs_iunlock_map_shared(ip, lock_mode);
>  
>  		hsize = XFS_HSIZE(handle);
>  	}

Agreed.

Reviewed-by: Dave Chinner <dchinner@redhat.com>
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 02/11] xfs: remove xfs_iunlock_map_shared

[Dave Chinner]

On Fri, Dec 06, 2013 at 12:30:08PM -0800, Christoph Hellwig wrote:
> We can just use xfs_iunlock without any loss of clarity.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Makes sense, gets rid of an unecessary wrapper.

Reviewed-by: Dave Chinner <dchinner@redhat.com>
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 03/11] xfs: rename xfs_ilock_map_shared

[Dave Chinner]

On Fri, Dec 06, 2013 at 12:30:09PM -0800, Christoph Hellwig wrote:
> Make it clear that we're only locking against the extent map on the data
> fork.  Also clean the function up a little bit.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
.....
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_inode.c	2013-12-06 19:57:33.199138169 +0100
> +++ xfs/fs/xfs/xfs_inode.c	2013-12-06 19:58:41.667136764 +0100
> @@ -91,20 +91,15 @@ xfs_get_extsz_hint(
>   * xfs_iunlock() call.
>   */
>  uint
> -xfs_ilock_map_shared(
> -	xfs_inode_t	*ip)
> +xfs_ilock_data_map_shared(
> +	struct xfs_inode	*ip)
>  {
> -	uint	lock_mode;
> +	uint			lock_mode = XFS_ILOCK_SHARED;
>  
> -	if ((ip->i_d.di_format == XFS_DINODE_FMT_BTREE) &&
> -	    ((ip->i_df.if_flags & XFS_IFEXTENTS) == 0)) {
> +	if (ip->i_d.di_format == XFS_DINODE_FMT_BTREE &&
> +	    (ip->i_df.if_flags & XFS_IFEXTENTS) == 0)
>  		lock_mode = XFS_ILOCK_EXCL;
> -	} else {
> -		lock_mode = XFS_ILOCK_SHARED;
> -	}
> -
>  	xfs_ilock(ip, lock_mode);
> -
>  	return lock_mode;
>  }

While we are changing this, I think it makes sense to move it to
being a static inline function given how simple it is....

Otherwise it looks good.

Cheers,

Dave.

-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 04/11] xfs: add xfs_ilock_attr_map_shared

[Dave Chinner]

On Fri, Dec 06, 2013 at 12:30:10PM -0800, Christoph Hellwig wrote:
> Equivalent to xfs_ilock_data_map_shared, except for the attribute fork.
> 
> Make xfs_getbmap use it if called for the attribute fork instead of
> xfs_ilock_data_map_shared.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> Index: xfs/fs/xfs/xfs_bmap_util.c
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_bmap_util.c	2013-12-06 19:58:15.759137296 +0100
> +++ xfs/fs/xfs/xfs_bmap_util.c	2013-12-06 20:02:23.291132216 +0100
> @@ -617,22 +617,27 @@ xfs_getbmap(
>  		return XFS_ERROR(ENOMEM);
>  
>  	xfs_ilock(ip, XFS_IOLOCK_SHARED);
> -	if (whichfork == XFS_DATA_FORK && !(iflags & BMV_IF_DELALLOC)) {
> -		if (ip->i_delayed_blks || XFS_ISIZE(ip) > ip->i_d.di_size) {
> +	if (whichfork == XFS_DATA_FORK) {
> +		if (!(iflags & BMV_IF_DELALLOC) &&
> +		    (ip->i_delayed_blks || XFS_ISIZE(ip) > ip->i_d.di_size)) {
>  			error = -filemap_write_and_wait(VFS_I(ip)->i_mapping);
>  			if (error)
>  				goto out_unlock_iolock;
> +
> +			/*
> +			 * Even after flushing the inode, there can still be
> +			 * delalloc blocks on the inode beyond EOF due to
> +			 * speculative reallocation.  These are not removed

"speculative preallocation"

> Index: xfs/fs/xfs/xfs_inode.c
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_inode.c	2013-12-06 19:58:41.667136764 +0100
> +++ xfs/fs/xfs/xfs_inode.c	2013-12-06 20:00:44.535134243 +0100
> @@ -77,17 +77,18 @@ xfs_get_extsz_hint(
>  }
>  
>  /*
> - * This is a wrapper routine around the xfs_ilock() routine used to centralize
> - * some grungy code.  It is used in places that wish to lock the inode solely
> - * for reading the extents.  The reason these places can't just call
> - * xfs_ilock(SHARED) is that the inode lock also guards to bringing in of the
> - * extents from disk for a file in b-tree format.  If the inode is in b-tree
> - * format, then we need to lock the inode exclusively until the extents are read
> - * in.  Locking it exclusively all the time would limit our parallelism
> - * unnecessarily, though.  What we do instead is check to see if the extents
> - * have been read in yet, and only lock the inode exclusively if they have not.
> + * These two are wrapper routines around the xfs_ilock() routine used to
> + * centralize some grungy code.  They are used in places that wish to lock the
> + * inode solely for reading the extents.  The reason these places can't just
> + * call xfs_ilock(ip, XFS_ILOCK_SHARED) is that the inode lock also guards to
> + * bringing in of the extents from disk for a file in b-tree format.  If the
> + * inode is in b-tree format, then we need to lock the inode exclusively until
> + * the extents are read in.  Locking it exclusively all the time would limit
> + * our parallelism unnecessarily, though.  What we do instead is check to see
> + * if the extents have been read in yet, and only lock the inode exclusively
> + * if they have not.
>   *
> - * The function returns a value which should be given to the corresponding
> + * The functions return a value which should be given to the corresponding
>   * xfs_iunlock() call.
>   */
>  uint
> @@ -101,6 +102,19 @@ xfs_ilock_data_map_shared(
>  		lock_mode = XFS_ILOCK_EXCL;
>  	xfs_ilock(ip, lock_mode);
>  	return lock_mode;
> +}
> +
> +uint
> +xfs_ilock_attr_map_shared(
> +	struct xfs_inode	*ip)
> +{
> +	uint			lock_mode = XFS_ILOCK_SHARED;
> +
> +	if (ip->i_d.di_aformat == XFS_DINODE_FMT_BTREE &&
> +	    (ip->i_afp->if_flags & XFS_IFEXTENTS) == 0)
> +		lock_mode = XFS_ILOCK_EXCL;
> +	xfs_ilock(ip, lock_mode);
> +	return lock_mode;
>  }

Again, I think a static inline is appropriate here...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 05/11] xfs: reinstate the ilock in xfs_readdir

[Dave Chinner]

On Fri, Dec 06, 2013 at 12:30:11PM -0800, Christoph Hellwig wrote:
> From: Ben Myers <bpm@sgi.com>
> 
> Although it was removed in commit 051e7cd44ab8, ilock needs to be taken in
> xfs_readdir because we might have to read the extent list in from disk.  This
> keeps other threads from reading from or writing to the extent list while it is
> being read in and is still in a transitional state.
> 
> This has been associated with "Access to block zero" messages on directories
> with large numbers of extents resulting from excessive filesytem fragmentation,
> as well as extent list corruption.  Unfortunately no test case at this point.
> 
> Signed-off-by: Ben Myers <bpm@sgi.com>

Looks good.

Reviewed-by: Dave Chinner <dchinner@redhat.com>
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 06/11] xfs: take the ilock around xfs_bmapi_read in xfs_zero_remaining_bytes

[Dave Chinner]

On Fri, Dec 06, 2013 at 12:30:12PM -0800, Christoph Hellwig wrote:
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> Index: xfs/fs/xfs/xfs_bmap_util.c
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_bmap_util.c	2013-12-06 19:37:36.715162723 +0100
> +++ xfs/fs/xfs/xfs_bmap_util.c	2013-12-06 19:38:45.643161309 +0100
> @@ -1175,9 +1175,15 @@ xfs_zero_remaining_bytes(
>  	xfs_buf_unlock(bp);
>  
>  	for (offset = startoff; offset <= endoff; offset = lastoffset + 1) {
> +		uint lock_mode;
> +
>  		offset_fsb = XFS_B_TO_FSBT(mp, offset);
>  		nimap = 1;
> +
> +		lock_mode = xfs_ilock_data_map_shared(ip);
>  		error = xfs_bmapi_read(ip, offset_fsb, 1, &imap, &nimap, 0);
> +		xfs_iunlock(ip, lock_mode);
> +
>  		if (error || nimap < 1)
>  			break;
>  		ASSERT(imap.br_blockcount >= 1);

Looks good.

Reviewed-by: Dave Chinner <dchinner@redhat.com>

-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 07/11] xfs: use xfs_ilock_data_map_shared in xfs_qm_dqtobp

[Dave Chinner]

On Fri, Dec 06, 2013 at 12:30:13PM -0800, Christoph Hellwig wrote:
> We might not have read in the extent list at this point, so make sure we
> take the ilock exclusively if we have to do so.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> Index: xfs/fs/xfs/xfs_dquot.c
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_dquot.c	2013-12-06 17:20:27.571331599 +0100
> +++ xfs/fs/xfs/xfs_dquot.c	2013-12-06 19:39:42.307160146 +0100
> @@ -469,16 +469,17 @@ xfs_qm_dqtobp(
>  	struct xfs_mount	*mp = dqp->q_mount;
>  	xfs_dqid_t		id = be32_to_cpu(dqp->q_core.d_id);
>  	struct xfs_trans	*tp = (tpp ? *tpp : NULL);
> +	uint			lock_mode;
>  
>  	dqp->q_fileoffset = (xfs_fileoff_t)id / mp->m_quotainfo->qi_dqperchunk;
>  
> -	xfs_ilock(quotip, XFS_ILOCK_SHARED);
> +	lock_mode = xfs_ilock_data_map_shared(quotip);
>  	if (!xfs_this_quota_on(dqp->q_mount, dqp->dq_flags)) {
>  		/*
>  		 * Return if this type of quotas is turned off while we
>  		 * didn't have the quota inode lock.
>  		 */
> -		xfs_iunlock(quotip, XFS_ILOCK_SHARED);
> +		xfs_iunlock(quotip, lock_mode);
>  		return ESRCH;
>  	}
>  
> @@ -488,7 +489,7 @@ xfs_qm_dqtobp(
>  	error = xfs_bmapi_read(quotip, dqp->q_fileoffset,
>  			       XFS_DQUOT_CLUSTER_SIZE_FSB, &map, &nmaps, 0);
>  
> -	xfs_iunlock(quotip, XFS_ILOCK_SHARED);
> +	xfs_iunlock(quotip, lock_mode);
>  	if (error)
>  		return error;

Looks fine - it doesn't change the race condition w/ dquot creation
at all, so we can ignore that for now...

Reviewed-by: Dave Chinner <dchinner@redhat.com>

-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 08/11] xfs: use xfs_ilock_data_map_shared in xfs_qm_dqiterate

[Dave Chinner]

On Fri, Dec 06, 2013 at 12:30:14PM -0800, Christoph Hellwig wrote:
> We might not have read in the extent list at this point, so make sure we
> take the ilock exclusively if we have to do so.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> Index: xfs/fs/xfs/xfs_qm.c
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_qm.c	2013-12-06 17:20:27.491331600 +0100
> +++ xfs/fs/xfs/xfs_qm.c	2013-12-06 19:39:27.251160455 +0100
> @@ -1193,16 +1193,18 @@ xfs_qm_dqiterate(
>  	lblkno = 0;
>  	maxlblkcnt = XFS_B_TO_FSB(mp, mp->m_super->s_maxbytes);
>  	do {
> +		uint		lock_mode;
> +
>  		nmaps = XFS_DQITER_MAP_SIZE;
>  		/*
>  		 * We aren't changing the inode itself. Just changing
>  		 * some of its data. No new blocks are added here, and
>  		 * the inode is never added to the transaction.
>  		 */
> -		xfs_ilock(qip, XFS_ILOCK_SHARED);
> +		lock_mode = xfs_ilock_data_map_shared(qip);
>  		error = xfs_bmapi_read(qip, lblkno, maxlblkcnt - lblkno,
>  				       map, &nmaps, 0);
> -		xfs_iunlock(qip, XFS_ILOCK_SHARED);
> +		xfs_iunlock(qip, lock_mode);
>  		if (error)
>  			break;

looks good.

Reviewed-by: Dave Chinner <dchinner@redhat.com>
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 09/11] xfs: use xfs_ilock_attr_map_shared in xfs_attr_get

[Dave Chinner]

On Fri, Dec 06, 2013 at 12:30:15PM -0800, Christoph Hellwig wrote:
> We might not have read in the extent list at this point, so make sure we
> take the ilock exclusively if we have to do so.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> Index: xfs/fs/xfs/xfs_attr.c
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_attr.c	2013-12-06 17:20:27.447331601 +0100
> +++ xfs/fs/xfs/xfs_attr.c	2013-12-06 19:41:05.119158446 +0100
> @@ -164,6 +164,7 @@ xfs_attr_get(
>  {
>  	int		error;
>  	struct xfs_name	xname;
> +	uint		lock_mode;
>  
>  	XFS_STATS_INC(xs_attr_get);
>  
> @@ -174,9 +175,9 @@ xfs_attr_get(
>  	if (error)
>  		return error;
>  
> -	xfs_ilock(ip, XFS_ILOCK_SHARED);
> +	lock_mode = xfs_ilock_attr_map_shared(ip);
>  	error = xfs_attr_get_int(ip, &xname, value, valuelenp, flags);
> -	xfs_iunlock(ip, XFS_ILOCK_SHARED);
> +	xfs_iunlock(ip, lock_mode);
>  	return(error);
>  }

Yup, that should fix the assert problem. :)

Reviewed-by: Dave Chinner <dchinner@redhat.com>

-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 10/11] xfs: use xfs_ilock_attr_map_shared in xfs_attr_list_int

[Dave Chinner]

On Fri, Dec 06, 2013 at 12:30:16PM -0800, Christoph Hellwig wrote:
> We might not have read in the extent list at this point, so make sure we
> take the ilock exclusively if we have to do so.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> Index: xfs/fs/xfs/xfs_attr_list.c
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_attr_list.c	2013-12-06 17:20:27.371331603 +0100
> +++ xfs/fs/xfs/xfs_attr_list.c	2013-12-06 19:41:42.779157674 +0100
> @@ -507,17 +507,17 @@ xfs_attr_list_int(
>  {
>  	int error;
>  	xfs_inode_t *dp = context->dp;
> +	uint		lock_mode;
>  
>  	XFS_STATS_INC(xs_attr_list);
>  
>  	if (XFS_FORCED_SHUTDOWN(dp->i_mount))
>  		return EIO;
>  
> -	xfs_ilock(dp, XFS_ILOCK_SHARED);
> -
>  	/*
>  	 * Decide on what work routines to call based on the inode size.
>  	 */
> +	lock_mode = xfs_ilock_attr_map_shared(dp);
>  	if (!xfs_inode_hasattr(dp)) {
>  		error = 0;
>  	} else if (dp->i_d.di_aformat == XFS_DINODE_FMT_LOCAL) {
> @@ -527,9 +527,7 @@ xfs_attr_list_int(
>  	} else {
>  		error = xfs_attr_node_list(context);
>  	}
> -
> -	xfs_iunlock(dp, XFS_ILOCK_SHARED);
> -
> +	xfs_iunlock(dp, lock_mode);
>  	return error;
>  }

Looks good.

Reviewed-by: Dave Chinner <dchinner@redhat.com>

-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 11/11] xfs: assert that we hold the ilock for extent map access

[Dave Chinner]

On Fri, Dec 06, 2013 at 12:30:17PM -0800, Christoph Hellwig wrote:
> Make sure that xfs_bmapi_read has the ilock held in some way, and that
> xfs_bmapi_write, xfs_bmapi_delay, xfs_bunmapi and xfs_iread_extents are
> called with the ilock held exclusively.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> Index: xfs/fs/xfs/xfs_bmap.c
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_bmap.c	2013-11-29 14:25:12.172459195 +0100
> +++ xfs/fs/xfs/xfs_bmap.c	2013-12-05 10:03:28.243801633 +0100
> @@ -3997,6 +3997,7 @@ xfs_bmapi_read(
>  	ASSERT(*nmap >= 1);
>  	ASSERT(!(flags & ~(XFS_BMAPI_ATTRFORK|XFS_BMAPI_ENTIRE|
>  			   XFS_BMAPI_IGSTATE)));
> +	ASSERT(xfs_isilocked(ip, XFS_ILOCK_SHARED|XFS_ILOCK_EXCL));
>  
>  	if (unlikely(XFS_TEST_ERROR(
>  	    (XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_EXTENTS &&
> @@ -4191,6 +4192,7 @@ xfs_bmapi_delay(
>  	ASSERT(*nmap >= 1);
>  	ASSERT(*nmap <= XFS_BMAP_MAX_NMAP);
>  	ASSERT(!(flags & ~XFS_BMAPI_ENTIRE));
> +	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
>  
>  	if (unlikely(XFS_TEST_ERROR(
>  	    (XFS_IFORK_FORMAT(ip, XFS_DATA_FORK) != XFS_DINODE_FMT_EXTENTS &&
> @@ -4484,6 +4486,7 @@ xfs_bmapi_write(
>  	ASSERT(tp != NULL);
>  	ASSERT(len > 0);
>  	ASSERT(XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_LOCAL);
> +	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
>  
>  	if (unlikely(XFS_TEST_ERROR(
>  	    (XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_EXTENTS &&
> Index: xfs/fs/xfs/xfs_inode_fork.c
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_inode_fork.c	2013-12-05 09:57:24.347809100 +0100
> +++ xfs/fs/xfs/xfs_inode_fork.c	2013-12-05 09:59:04.767807039 +0100
> @@ -431,6 +431,8 @@ xfs_iread_extents(
>  	xfs_ifork_t	*ifp;
>  	xfs_extnum_t	nextents;
>  
> +	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
> +
>  	if (unlikely(XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_BTREE)) {
>  		XFS_ERROR_REPORT("xfs_iread_extents", XFS_ERRLEVEL_LOW,
>  				 ip->i_mount);
> Index: xfs/fs/xfs/xfs_bmap.c
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_bmap.c	2013-12-05 22:12:32.931838271 +0100
> +++ xfs/fs/xfs/xfs_bmap.c	2013-12-05 22:35:59.175809412 +0100
> @@ -5038,6 +5038,7 @@ xfs_bunmapi(
>  	if (XFS_FORCED_SHUTDOWN(mp))
>  		return XFS_ERROR(EIO);
>  
> +	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
>  	ASSERT(len > 0);
>  	ASSERT(nexts >= 0);

I can't think of anywhere else we need to add asserts for this, so
it seems fine to me.

Reviewed-by: Dave Chinner <dchinner@redhat.com>

-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 03/11] xfs: rename xfs_ilock_map_shared

[Christoph Hellwig]

On Mon, Dec 09, 2013 at 09:33:59AM +1100, Dave Chinner wrote:
> While we are changing this, I think it makes sense to move it to
> being a static inline function given how simple it is....

I actually tried that first, but with XFS_DINODE_FMT_BTREE in
fs/xfs/xfs_dinode.h, struct xfsicdinode in xfs_log_format.h
and the resulting dependencies it didn't seem workable.  My inode
reshuffle might provide relief eventually.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 03/11] xfs: rename xfs_ilock_map_shared

[Dave Chinner]

On Sun, Dec 08, 2013 at 11:03:16PM -0800, Christoph Hellwig wrote:
> On Mon, Dec 09, 2013 at 09:33:59AM +1100, Dave Chinner wrote:
> > While we are changing this, I think it makes sense to move it to
> > being a static inline function given how simple it is....
> 
> I actually tried that first, but with XFS_DINODE_FMT_BTREE in
> fs/xfs/xfs_dinode.h, struct xfsicdinode in xfs_log_format.h
> and the resulting dependencies it didn't seem workable.  My inode
> reshuffle might provide relief eventually.

Ok, makes sense to leave it like that for the moment.

Reviewed-by: Dave Chinner <dchinner@redhat.com>

-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 04/11] xfs: add xfs_ilock_attr_map_shared

[Christoph Hellwig]

On Mon, Dec 09, 2013 at 09:36:10AM +1100, Dave Chinner wrote:
> > +			/*
> > +			 * Even after flushing the inode, there can still be
> > +			 * delalloc blocks on the inode beyond EOF due to
> > +			 * speculative reallocation.  These are not removed
> 
> "speculative preallocation"

I just re-indented the comment, the wording is the original one.  Maybe
we'll get a commit that cares enought to fix it on the fly..

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 04/11] xfs: add xfs_ilock_attr_map_shared

[Ben Myers]

On Mon, Dec 09, 2013 at 10:16:12AM -0800, Christoph Hellwig wrote:
> On Mon, Dec 09, 2013 at 09:36:10AM +1100, Dave Chinner wrote:
> > > +			/*
> > > +			 * Even after flushing the inode, there can still be
> > > +			 * delalloc blocks on the inode beyond EOF due to
> > > +			 * speculative reallocation.  These are not removed
> > 
> > "speculative preallocation"
> 
> I just re-indented the comment, the wording is the original one.  Maybe
> we'll get a commit that cares enought to fix it on the fly..

Sure...

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 04/11] xfs: add xfs_ilock_attr_map_shared

[Dave Chinner]

On Mon, Dec 09, 2013 at 10:16:12AM -0800, Christoph Hellwig wrote:
> On Mon, Dec 09, 2013 at 09:36:10AM +1100, Dave Chinner wrote:
> > > +			/*
> > > +			 * Even after flushing the inode, there can still be
> > > +			 * delalloc blocks on the inode beyond EOF due to
> > > +			 * speculative reallocation.  These are not removed
> > 
> > "speculative preallocation"
> 
> I just re-indented the comment, the wording is the original one.  Maybe
> we'll get a commit that cares enought to fix it on the fly..

Actually, I checked that before commenting on it - the original
comment is correct:

 626                 /*
 627                  * even after flushing the inode, there can still be delalloc
 628                  * blocks on the inode beyond EOF due to speculative
 629                  * preallocation. These are not removed until the release
 630                  * function is called or the inode is inactivated. Hence we
 631                  * cannot assert here that ip->i_delayed_blks == 0.
 632                  */

I wouldn't have pointed at it if the original code had that
problem... :/

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 04/11] xfs: add xfs_ilock_attr_map_shared

[Ben Myers]

Dave,

On Tue, Dec 10, 2013 at 09:24:33AM +1100, Dave Chinner wrote:
> On Mon, Dec 09, 2013 at 10:16:12AM -0800, Christoph Hellwig wrote:
> > On Mon, Dec 09, 2013 at 09:36:10AM +1100, Dave Chinner wrote:
> > > > +			/*
> > > > +			 * Even after flushing the inode, there can still be
> > > > +			 * delalloc blocks on the inode beyond EOF due to
> > > > +			 * speculative reallocation.  These are not removed
> > > 
> > > "speculative preallocation"
> > 
> > I just re-indented the comment, the wording is the original one.  Maybe
> > we'll get a commit that cares enought to fix it on the fly..
> 
> Actually, I checked that before commenting on it - the original
> comment is correct:
> 
>  626                 /*
>  627                  * even after flushing the inode, there can still be delalloc
>  628                  * blocks on the inode beyond EOF due to speculative
>  629                  * preallocation. These are not removed until the release
>  630                  * function is called or the inode is inactivated. Hence we
>  631                  * cannot assert here that ip->i_delayed_blks == 0.
>  632                  */
> 
> I wouldn't have pointed at it if the original code had that
> problem... :/

If you want to extend your Reviewed-by: on this patch I will clean up the
spelling mistake and pull in this series.

-Ben

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 04/11] xfs: add xfs_ilock_attr_map_shared

[Christoph Hellwig]

On Tue, Dec 10, 2013 at 09:24:33AM +1100, Dave Chinner wrote:
> I wouldn't have pointed at it if the original code had that
> problem... :/

Strange.  I'd swear I haven't reworded anything.  I'll fix it back up.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 04/11] xfs: add xfs_ilock_attr_map_shared

[Ben Myers]

Hey Christoph,

On Tue, Dec 10, 2013 at 08:13:49AM -0800, Christoph Hellwig wrote:
> On Tue, Dec 10, 2013 at 09:24:33AM +1100, Dave Chinner wrote:
> > I wouldn't have pointed at it if the original code had that
> > problem... :/
> 
> Strange.  I'd swear I haven't reworded anything.  I'll fix it back up.

I'd like to pull this series in.  Are you still planning on reposting this patch?

Thanks,
	Ben

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 04/11 v2] xfs: add xfs_ilock_attr_map_shared

[Christoph Hellwig]

Equivalent to xfs_ilock_data_map_shared, except for the attribute fork.

Make xfs_getbmap use it if called for the attribute fork instead of
xfs_ilock_data_map_shared.

Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: xfs/fs/xfs/xfs_bmap_util.c
===================================================================
--- xfs.orig/fs/xfs/xfs_bmap_util.c	2013-12-18 11:14:52.587953376 +0100
+++ xfs/fs/xfs/xfs_bmap_util.c	2013-12-18 11:15:29.367952621 +0100
@@ -617,22 +617,27 @@ xfs_getbmap(
 		return XFS_ERROR(ENOMEM);
 
 	xfs_ilock(ip, XFS_IOLOCK_SHARED);
-	if (whichfork == XFS_DATA_FORK && !(iflags & BMV_IF_DELALLOC)) {
-		if (ip->i_delayed_blks || XFS_ISIZE(ip) > ip->i_d.di_size) {
+	if (whichfork == XFS_DATA_FORK) {
+		if (!(iflags & BMV_IF_DELALLOC) &&
+		    (ip->i_delayed_blks || XFS_ISIZE(ip) > ip->i_d.di_size)) {
 			error = -filemap_write_and_wait(VFS_I(ip)->i_mapping);
 			if (error)
 				goto out_unlock_iolock;
+
+			/*
+			 * Even after flushing the inode, there can still be
+			 * delalloc blocks on the inode beyond EOF due to
+			 * speculative preallocation.  These are not removed
+			 * until the release function is called or the inode
+			 * is inactivated.  Hence we cannot assert here that
+			 * ip->i_delayed_blks == 0.
+			 */
 		}
-		/*
-		 * even after flushing the inode, there can still be delalloc
-		 * blocks on the inode beyond EOF due to speculative
-		 * preallocation. These are not removed until the release
-		 * function is called or the inode is inactivated. Hence we
-		 * cannot assert here that ip->i_delayed_blks == 0.
-		 */
-	}
 
-	lock = xfs_ilock_data_map_shared(ip);
+		lock = xfs_ilock_data_map_shared(ip);
+	} else {
+		lock = xfs_ilock_attr_map_shared(ip);
+	}
 
 	/*
 	 * Don't let nex be bigger than the number of extents
Index: xfs/fs/xfs/xfs_inode.c
===================================================================
--- xfs.orig/fs/xfs/xfs_inode.c	2013-12-18 11:14:52.587953376 +0100
+++ xfs/fs/xfs/xfs_inode.c	2013-12-18 11:14:53.599953355 +0100
@@ -77,17 +77,18 @@ xfs_get_extsz_hint(
 }
 
 /*
- * This is a wrapper routine around the xfs_ilock() routine used to centralize
- * some grungy code.  It is used in places that wish to lock the inode solely
- * for reading the extents.  The reason these places can't just call
- * xfs_ilock(SHARED) is that the inode lock also guards to bringing in of the
- * extents from disk for a file in b-tree format.  If the inode is in b-tree
- * format, then we need to lock the inode exclusively until the extents are read
- * in.  Locking it exclusively all the time would limit our parallelism
- * unnecessarily, though.  What we do instead is check to see if the extents
- * have been read in yet, and only lock the inode exclusively if they have not.
+ * These two are wrapper routines around the xfs_ilock() routine used to
+ * centralize some grungy code.  They are used in places that wish to lock the
+ * inode solely for reading the extents.  The reason these places can't just
+ * call xfs_ilock(ip, XFS_ILOCK_SHARED) is that the inode lock also guards to
+ * bringing in of the extents from disk for a file in b-tree format.  If the
+ * inode is in b-tree format, then we need to lock the inode exclusively until
+ * the extents are read in.  Locking it exclusively all the time would limit
+ * our parallelism unnecessarily, though.  What we do instead is check to see
+ * if the extents have been read in yet, and only lock the inode exclusively
+ * if they have not.
  *
- * The function returns a value which should be given to the corresponding
+ * The functions return a value which should be given to the corresponding
  * xfs_iunlock() call.
  */
 uint
@@ -101,6 +102,19 @@ xfs_ilock_data_map_shared(
 		lock_mode = XFS_ILOCK_EXCL;
 	xfs_ilock(ip, lock_mode);
 	return lock_mode;
+}
+
+uint
+xfs_ilock_attr_map_shared(
+	struct xfs_inode	*ip)
+{
+	uint			lock_mode = XFS_ILOCK_SHARED;
+
+	if (ip->i_d.di_aformat == XFS_DINODE_FMT_BTREE &&
+	    (ip->i_afp->if_flags & XFS_IFEXTENTS) == 0)
+		lock_mode = XFS_ILOCK_EXCL;
+	xfs_ilock(ip, lock_mode);
+	return lock_mode;
 }
 
 /*
Index: xfs/fs/xfs/xfs_inode.h
===================================================================
--- xfs.orig/fs/xfs/xfs_inode.h	2013-12-18 11:14:52.587953376 +0100
+++ xfs/fs/xfs/xfs_inode.h	2013-12-18 11:14:53.599953355 +0100
@@ -338,6 +338,7 @@ void		xfs_iunlock(xfs_inode_t *, uint);
 void		xfs_ilock_demote(xfs_inode_t *, uint);
 int		xfs_isilocked(xfs_inode_t *, uint);
 uint		xfs_ilock_data_map_shared(struct xfs_inode *);
+uint		xfs_ilock_attr_map_shared(struct xfs_inode *);
 int		xfs_ialloc(struct xfs_trans *, xfs_inode_t *, umode_t,
 			   xfs_nlink_t, xfs_dev_t, prid_t, int,
 			   struct xfs_buf **, xfs_inode_t **);

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 04/11 v2] xfs: add xfs_ilock_attr_map_shared

[Ben Myers]

On Wed, Dec 18, 2013 at 02:14:39AM -0800, Christoph Hellwig wrote:
> Equivalent to xfs_ilock_data_map_shared, except for the attribute fork.
> 
> Make xfs_getbmap use it if called for the attribute fork instead of
> xfs_ilock_data_map_shared.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> Index: xfs/fs/xfs/xfs_bmap_util.c
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_bmap_util.c	2013-12-18 11:14:52.587953376 +0100
> +++ xfs/fs/xfs/xfs_bmap_util.c	2013-12-18 11:15:29.367952621 +0100
> @@ -617,22 +617,27 @@ xfs_getbmap(
>  		return XFS_ERROR(ENOMEM);
>  
>  	xfs_ilock(ip, XFS_IOLOCK_SHARED);
> -	if (whichfork == XFS_DATA_FORK && !(iflags & BMV_IF_DELALLOC)) {
> -		if (ip->i_delayed_blks || XFS_ISIZE(ip) > ip->i_d.di_size) {
> +	if (whichfork == XFS_DATA_FORK) {
> +		if (!(iflags & BMV_IF_DELALLOC) &&
> +		    (ip->i_delayed_blks || XFS_ISIZE(ip) > ip->i_d.di_size)) {
>  			error = -filemap_write_and_wait(VFS_I(ip)->i_mapping);
>  			if (error)
>  				goto out_unlock_iolock;
> +
> +			/*
> +			 * Even after flushing the inode, there can still be
> +			 * delalloc blocks on the inode beyond EOF due to
> +			 * speculative preallocation.  These are not removed

This one has a 'p'.

Reviewed-by: Ben Myers <bpm@sgi.com>

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs