[PATCH 0/3] clean up superblock modification helpers

[PATCH 0/3] clean up superblock modification helpers

[Christoph Hellwig]

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

[PATCH 1/3] [PATCH 1/3] xfs: remove XFS_MOUNT_NO_PERCPU_SB

[Christoph Hellwig]

[-- Attachment #1: xfs-kill-XFS_MOUNT_NO_PERCPU_SB --]
[-- Type: text/plain, Size: 3761 bytes --]

Fail the mount if we can't allocate memory for the per-CPU counters.
This is consistent with how we handle everything else in the mount
path and makes the superblock counter modification a lot simpler.

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

Index: xfs/fs/xfs/linux-2.6/xfs_super.c
===================================================================
--- xfs.orig/fs/xfs/linux-2.6/xfs_super.c	2010-09-29 12:37:32.562557720 +0900
+++ xfs/fs/xfs/linux-2.6/xfs_super.c	2010-09-29 12:38:33.154557727 +0900
@@ -1517,8 +1517,9 @@ xfs_fs_fill_super(
 	if (error)
 		goto out_free_fsname;
 
-	if (xfs_icsb_init_counters(mp))
-		mp->m_flags |= XFS_MOUNT_NO_PERCPU_SB;
+	error = xfs_icsb_init_counters(mp);
+	if (error)
+		goto out_close_devices;
 
 	error = xfs_readsb(mp, flags);
 	if (error)
@@ -1579,6 +1580,7 @@ xfs_fs_fill_super(
 	xfs_freesb(mp);
  out_destroy_counters:
 	xfs_icsb_destroy_counters(mp);
+ out_close_devices:
 	xfs_close_devices(mp);
  out_free_fsname:
 	xfs_free_fsname(mp);
Index: xfs/fs/xfs/xfs_mount.c
===================================================================
--- xfs.orig/fs/xfs/xfs_mount.c	2010-09-29 12:37:39.846557721 +0900
+++ xfs/fs/xfs/xfs_mount.c	2010-09-29 12:39:12.376557727 +0900
@@ -1856,12 +1856,8 @@ xfs_mod_incore_sb(
 	case XFS_SBS_ICOUNT:
 	case XFS_SBS_IFREE:
 	case XFS_SBS_FDBLOCKS:
-		if (!(mp->m_flags & XFS_MOUNT_NO_PERCPU_SB)) {
-			status = xfs_icsb_modify_counters(mp, field,
-							delta, rsvd);
-			break;
-		}
-		/* FALLTHROUGH */
+		status = xfs_icsb_modify_counters(mp, field, delta, rsvd);
+		break;
 #endif
 	default:
 		spin_lock(&mp->m_sb_lock);
@@ -1910,15 +1906,12 @@ xfs_mod_incore_sb_batch(xfs_mount_t *mp,
 		case XFS_SBS_ICOUNT:
 		case XFS_SBS_IFREE:
 		case XFS_SBS_FDBLOCKS:
-			if (!(mp->m_flags & XFS_MOUNT_NO_PERCPU_SB)) {
-				spin_unlock(&mp->m_sb_lock);
-				status = xfs_icsb_modify_counters(mp,
-							msbp->msb_field,
-							msbp->msb_delta, rsvd);
-				spin_lock(&mp->m_sb_lock);
-				break;
-			}
-			/* FALLTHROUGH */
+			spin_unlock(&mp->m_sb_lock);
+			status = xfs_icsb_modify_counters(mp,
+						msbp->msb_field,
+						msbp->msb_delta, rsvd);
+			spin_lock(&mp->m_sb_lock);
+			break;
 #endif
 		default:
 			status = xfs_mod_incore_sb_unlocked(mp,
@@ -1948,16 +1941,13 @@ xfs_mod_incore_sb_batch(xfs_mount_t *mp,
 			case XFS_SBS_ICOUNT:
 			case XFS_SBS_IFREE:
 			case XFS_SBS_FDBLOCKS:
-				if (!(mp->m_flags & XFS_MOUNT_NO_PERCPU_SB)) {
-					spin_unlock(&mp->m_sb_lock);
-					status = xfs_icsb_modify_counters(mp,
-							msbp->msb_field,
-							-(msbp->msb_delta),
-							rsvd);
-					spin_lock(&mp->m_sb_lock);
-					break;
-				}
-				/* FALLTHROUGH */
+				spin_unlock(&mp->m_sb_lock);
+				status = xfs_icsb_modify_counters(mp,
+						msbp->msb_field,
+						-(msbp->msb_delta),
+						rsvd);
+				spin_lock(&mp->m_sb_lock);
+				break;
 #endif
 			default:
 				status = xfs_mod_incore_sb_unlocked(mp,
Index: xfs/fs/xfs/xfs_mount.h
===================================================================
--- xfs.orig/fs/xfs/xfs_mount.h	2010-09-29 12:37:50.782557721 +0900
+++ xfs/fs/xfs/xfs_mount.h	2010-09-29 12:39:18.410558090 +0900
@@ -231,8 +231,6 @@ typedef struct xfs_mount {
 #define XFS_MOUNT_DIRSYNC	(1ULL << 21)	/* synchronous directory ops */
 #define XFS_MOUNT_COMPAT_IOSIZE	(1ULL << 22)	/* don't report large preferred
 						 * I/O size in stat() */
-#define XFS_MOUNT_NO_PERCPU_SB	(1ULL << 23)	/* don't use per-cpu superblock
-						   counters */
 #define XFS_MOUNT_FILESTREAMS	(1ULL << 24)	/* enable the filestreams
 						   allocator */
 #define XFS_MOUNT_NOATTR2	(1ULL << 25)	/* disable use of attr2 format */

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

[PATCH 2/3] [PATCH 2/3] xfs: do not use xfs_mod_incore_sb for per-cpu counters

[Christoph Hellwig]

[-- Attachment #1: xfs-dont-use-xfs_mod_incore_sb-for-percpu-counters --]
[-- Type: text/plain, Size: 7703 bytes --]

Export xfs_icsb_modify_counters and always use it for modifying the per-cpu
counters.  Remove support for per-cpu counters from xfs_mod_incore_sb to
simplify it.

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

Index: xfs/fs/xfs/xfs_bmap.c
===================================================================
--- xfs.orig/fs/xfs/xfs_bmap.c	2010-09-29 13:05:08.239557721 +0900
+++ xfs/fs/xfs/xfs_bmap.c	2010-09-29 13:07:23.403557720 +0900
@@ -614,7 +614,7 @@ xfs_bmap_add_extent(
 			nblks += cur->bc_private.b.allocated;
 		ASSERT(nblks <= da_old);
 		if (nblks < da_old)
-			xfs_mod_incore_sb(ip->i_mount, XFS_SBS_FDBLOCKS,
+			xfs_icsb_modify_counters(ip->i_mount, XFS_SBS_FDBLOCKS,
 				(int64_t)(da_old - nblks), rsvd);
 	}
 	/*
@@ -1078,8 +1078,8 @@ xfs_bmap_add_extent_delay_real(
 		temp2 = xfs_bmap_worst_indlen(ip, temp2);
 		diff = (int)(temp + temp2 - startblockval(PREV.br_startblock) -
 			(cur ? cur->bc_private.b.allocated : 0));
-		if (diff > 0 &&
-		    xfs_mod_incore_sb(ip->i_mount, XFS_SBS_FDBLOCKS, -((int64_t)diff), rsvd)) {
+		if (diff > 0 && xfs_icsb_modify_counters(ip->i_mount,
+				XFS_SBS_FDBLOCKS, -((int64_t)diff), rsvd)) {
 			/*
 			 * Ick gross gag me with a spoon.
 			 */
@@ -1089,7 +1089,7 @@ xfs_bmap_add_extent_delay_real(
 					temp--;
 					diff--;
 					if (!diff ||
-					    !xfs_mod_incore_sb(ip->i_mount,
+					    !xfs_icsb_modify_counters(ip->i_mount,
 						    XFS_SBS_FDBLOCKS, -((int64_t)diff), rsvd))
 						break;
 				}
@@ -1097,7 +1097,7 @@ xfs_bmap_add_extent_delay_real(
 					temp2--;
 					diff--;
 					if (!diff ||
-					    !xfs_mod_incore_sb(ip->i_mount,
+					    !xfs_icsb_modify_counters(ip->i_mount,
 						    XFS_SBS_FDBLOCKS, -((int64_t)diff), rsvd))
 						break;
 				}
@@ -1766,7 +1766,7 @@ xfs_bmap_add_extent_hole_delay(
 	}
 	if (oldlen != newlen) {
 		ASSERT(oldlen > newlen);
-		xfs_mod_incore_sb(ip->i_mount, XFS_SBS_FDBLOCKS,
+		xfs_icsb_modify_counters(ip->i_mount, XFS_SBS_FDBLOCKS,
 			(int64_t)(oldlen - newlen), rsvd);
 		/*
 		 * Nothing to do for disk quota accounting here.
@@ -3111,9 +3111,10 @@ xfs_bmap_del_extent(
 	 * Nothing to do for disk quota accounting here.
 	 */
 	ASSERT(da_old >= da_new);
-	if (da_old > da_new)
-		xfs_mod_incore_sb(mp, XFS_SBS_FDBLOCKS, (int64_t)(da_old - da_new),
-			rsvd);
+	if (da_old > da_new) {
+		xfs_icsb_modify_counters(mp, XFS_SBS_FDBLOCKS,
+			(int64_t)(da_old - da_new), rsvd);
+	}
 done:
 	*logflagsp = flags;
 	return error;
@@ -4526,13 +4527,13 @@ xfs_bmapi(
 							-((int64_t)extsz), (flags &
 							XFS_BMAPI_RSVBLOCKS));
 				} else {
-					error = xfs_mod_incore_sb(mp,
+					error = xfs_icsb_modify_counters(mp,
 							XFS_SBS_FDBLOCKS,
 							-((int64_t)alen), (flags &
 							XFS_BMAPI_RSVBLOCKS));
 				}
 				if (!error) {
-					error = xfs_mod_incore_sb(mp,
+					error = xfs_icsb_modify_counters(mp,
 							XFS_SBS_FDBLOCKS,
 							-((int64_t)indlen), (flags &
 							XFS_BMAPI_RSVBLOCKS));
@@ -4542,7 +4543,7 @@ xfs_bmapi(
 							(int64_t)extsz, (flags &
 							XFS_BMAPI_RSVBLOCKS));
 					else if (error)
-						xfs_mod_incore_sb(mp,
+						xfs_icsb_modify_counters(mp,
 							XFS_SBS_FDBLOCKS,
 							(int64_t)alen, (flags &
 							XFS_BMAPI_RSVBLOCKS));
@@ -5206,7 +5207,7 @@ xfs_bunmapi(
 					ip, -((long)del.br_blockcount), 0,
 					XFS_QMOPT_RES_RTBLKS);
 			} else {
-				xfs_mod_incore_sb(mp, XFS_SBS_FDBLOCKS,
+				xfs_icsb_modify_counters(mp, XFS_SBS_FDBLOCKS,
 						(int64_t)del.br_blockcount, rsvd);
 				(void)xfs_trans_reserve_quota_nblks(NULL,
 					ip, -((long)del.br_blockcount), 0,
Index: xfs/fs/xfs/xfs_fsops.c
===================================================================
--- xfs.orig/fs/xfs/xfs_fsops.c	2010-09-29 13:04:46.122557720 +0900
+++ xfs/fs/xfs/xfs_fsops.c	2010-09-29 13:05:00.030557720 +0900
@@ -596,7 +596,8 @@ out:
 		 * the extra reserve blocks from the reserve.....
 		 */
 		int error;
-		error = xfs_mod_incore_sb(mp, XFS_SBS_FDBLOCKS, fdblks_delta, 0);
+		error = xfs_icsb_modify_counters(mp, XFS_SBS_FDBLOCKS,
+						 fdblks_delta, 0);
 		if (error == ENOSPC)
 			goto retry;
 	}
Index: xfs/fs/xfs/xfs_mount.c
===================================================================
--- xfs.orig/fs/xfs/xfs_mount.c	2010-09-29 13:02:19.389557773 +0900
+++ xfs/fs/xfs/xfs_mount.c	2010-09-29 13:14:08.511557720 +0900
@@ -52,16 +52,11 @@ STATIC void	xfs_icsb_balance_counter(xfs
 						int);
 STATIC void	xfs_icsb_balance_counter_locked(xfs_mount_t *, xfs_sb_field_t,
 						int);
-STATIC int	xfs_icsb_modify_counters(xfs_mount_t *, xfs_sb_field_t,
-						int64_t, int);
 STATIC void	xfs_icsb_disable_counter(xfs_mount_t *, xfs_sb_field_t);
-
 #else
 
 #define xfs_icsb_balance_counter(mp, a, b)		do { } while (0)
 #define xfs_icsb_balance_counter_locked(mp, a, b)	do { } while (0)
-#define xfs_icsb_modify_counters(mp, a, b, c)		do { } while (0)
-
 #endif
 
 static const struct {
@@ -1843,28 +1838,18 @@ xfs_mod_incore_sb_unlocked(
  */
 int
 xfs_mod_incore_sb(
-	xfs_mount_t	*mp,
-	xfs_sb_field_t	field,
-	int64_t		delta,
-	int		rsvd)
+	struct xfs_mount	*mp,
+	xfs_sb_field_t		field,
+	int64_t			delta,
+	int			rsvd)
 {
-	int	status;
+	int			status;
 
-	/* check for per-cpu counters */
-	switch (field) {
-#ifdef HAVE_PERCPU_SB
-	case XFS_SBS_ICOUNT:
-	case XFS_SBS_IFREE:
-	case XFS_SBS_FDBLOCKS:
-		status = xfs_icsb_modify_counters(mp, field, delta, rsvd);
-		break;
-#endif
-	default:
-		spin_lock(&mp->m_sb_lock);
-		status = xfs_mod_incore_sb_unlocked(mp, field, delta, rsvd);
-		spin_unlock(&mp->m_sb_lock);
-		break;
-	}
+	ASSERT(field < XFS_SBS_ICOUNT || field > XFS_SBS_FDBLOCKS);
+
+	spin_lock(&mp->m_sb_lock);
+	status = xfs_mod_incore_sb_unlocked(mp, field, delta, rsvd);
+	spin_unlock(&mp->m_sb_lock);
 
 	return status;
 }
Index: xfs/fs/xfs/xfs_mount.h
===================================================================
--- xfs.orig/fs/xfs/xfs_mount.h	2010-09-29 13:02:19.401557721 +0900
+++ xfs/fs/xfs/xfs_mount.h	2010-09-29 13:14:29.509557697 +0900
@@ -90,6 +90,8 @@ extern void	xfs_icsb_reinit_counters(str
 extern void	xfs_icsb_destroy_counters(struct xfs_mount *);
 extern void	xfs_icsb_sync_counters(struct xfs_mount *, int);
 extern void	xfs_icsb_sync_counters_locked(struct xfs_mount *, int);
+extern int	xfs_icsb_modify_counters(struct xfs_mount *, xfs_sb_field_t,
+						int64_t, int);
 
 #else
 #define xfs_icsb_init_counters(mp)		(0)
@@ -97,6 +99,8 @@ extern void	xfs_icsb_sync_counters_locke
 #define xfs_icsb_reinit_counters(mp)		do { } while (0)
 #define xfs_icsb_sync_counters(mp, flags)	do { } while (0)
 #define xfs_icsb_sync_counters_locked(mp, flags) do { } while (0)
+#define xfs_icsb_modify_counters(mp, field, delta, rsvd) \
+	xfs_mod_incore_sb(mp, field, delta, rsvd)
 #endif
 
 typedef struct xfs_mount {
Index: xfs/fs/xfs/xfs_trans.c
===================================================================
--- xfs.orig/fs/xfs/xfs_trans.c	2010-09-29 13:07:34.260557720 +0900
+++ xfs/fs/xfs/xfs_trans.c	2010-09-29 13:14:08.521557720 +0900
@@ -696,7 +696,7 @@ xfs_trans_reserve(
 	 * fail if the count would go below zero.
 	 */
 	if (blocks > 0) {
-		error = xfs_mod_incore_sb(tp->t_mountp, XFS_SBS_FDBLOCKS,
+		error = xfs_icsb_modify_counters(tp->t_mountp, XFS_SBS_FDBLOCKS,
 					  -((int64_t)blocks), rsvd);
 		if (error != 0) {
 			current_restore_flags_nested(&tp->t_pflags, PF_FSTRANS);
@@ -767,7 +767,7 @@ undo_log:
 
 undo_blocks:
 	if (blocks > 0) {
-		(void) xfs_mod_incore_sb(tp->t_mountp, XFS_SBS_FDBLOCKS,
+		(void) xfs_icsb_modify_counters(tp->t_mountp, XFS_SBS_FDBLOCKS,
 					 (int64_t)blocks, rsvd);
 		tp->t_blk_res = 0;
 	}

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

[PATCH 3/3] [PATCH 3/3] xfs: do not use xfs_mod_incore_sb_batch for per-cpu counters

[Christoph Hellwig]

[-- Attachment #1: xfs-simplify-icsb-updates --]
[-- Type: text/plain, Size: 6946 bytes --]

Update the per-cpu counters manually in xfs_trans_unreserve_and_mod_sb and
remove support for per-cpu counters from xfs_mod_incore_sb_batch to
simplify it.  And added benefit is that we don't have to take m_sb_lock
for transactions that only modify per-cpu counters.

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

Index: xfs/fs/xfs/xfs_mount.c
===================================================================
--- xfs.orig/fs/xfs/xfs_mount.c	2010-09-29 13:14:08.511557720 +0900
+++ xfs/fs/xfs/xfs_mount.c	2010-09-29 14:09:39.419557721 +0900
@@ -1866,84 +1866,51 @@ xfs_mod_incore_sb(
  * will be backed out and EINVAL will be returned.
  */
 int
-xfs_mod_incore_sb_batch(xfs_mount_t *mp, xfs_mod_sb_t *msb, uint nmsb, int rsvd)
+xfs_mod_incore_sb_batch(
+	struct xfs_mount	*mp,
+	xfs_mod_sb_t		*msb,
+	uint			nmsb,
+	int			rsvd)
 {
-	int		status=0;
-	xfs_mod_sb_t	*msbp;
+	xfs_mod_sb_t		*msbp = &msb[0];
+	int			status = 0;
 
 	/*
-	 * Loop through the array of mod structures and apply each
-	 * individually.  If any fail, then back out all those
-	 * which have already been applied.  Do all of this within
-	 * the scope of the m_sb_lock so that all of the changes will
-	 * be atomic.
+	 * Loop through the array of mod structures and apply each individually.
+	 * If any fail, then back out all those which have already been applied.
+	 * Do all of this within the scope of the m_sb_lock so that all of the
+	 * changes will be atomic.
 	 */
 	spin_lock(&mp->m_sb_lock);
-	msbp = &msb[0];
 	for (msbp = &msbp[0]; msbp < (msb + nmsb); msbp++) {
+		ASSERT(msbp->msb_field < XFS_SBS_ICOUNT ||
+		       msbp->msb_field > XFS_SBS_FDBLOCKS);
+
 		/*
 		 * Apply the delta at index n.  If it fails, break
 		 * from the loop so we'll fall into the undo loop
 		 * below.
 		 */
-		switch (msbp->msb_field) {
-#ifdef HAVE_PERCPU_SB
-		case XFS_SBS_ICOUNT:
-		case XFS_SBS_IFREE:
-		case XFS_SBS_FDBLOCKS:
-			spin_unlock(&mp->m_sb_lock);
-			status = xfs_icsb_modify_counters(mp,
-						msbp->msb_field,
-						msbp->msb_delta, rsvd);
-			spin_lock(&mp->m_sb_lock);
-			break;
-#endif
-		default:
-			status = xfs_mod_incore_sb_unlocked(mp,
-						msbp->msb_field,
-						msbp->msb_delta, rsvd);
-			break;
-		}
-
-		if (status != 0) {
-			break;
-		}
+		status = xfs_mod_incore_sb_unlocked(mp, msbp->msb_field,
+						    msbp->msb_delta, rsvd);
+		if (status)
+			goto unwind;
 	}
+	spin_unlock(&mp->m_sb_lock);
+	return 0;
 
+unwind:
 	/*
-	 * If we didn't complete the loop above, then back out
-	 * any changes made to the superblock.  If you add code
-	 * between the loop above and here, make sure that you
-	 * preserve the value of status. Loop back until
-	 * we step below the beginning of the array.  Make sure
-	 * we don't touch anything back there.
+	 * If we didn't complete the loop above, then back out any changes
+	 * made to the superblock.  If you add code between the loop above
+	 * and here, make sure that you preserve the value of status.
+	 * Loop back until we step below the beginning of the array.  Make
+	 * sure we don't touch anything back there.
 	 */
-	if (status != 0) {
-		msbp--;
-		while (msbp >= msb) {
-			switch (msbp->msb_field) {
-#ifdef HAVE_PERCPU_SB
-			case XFS_SBS_ICOUNT:
-			case XFS_SBS_IFREE:
-			case XFS_SBS_FDBLOCKS:
-				spin_unlock(&mp->m_sb_lock);
-				status = xfs_icsb_modify_counters(mp,
-						msbp->msb_field,
-						-(msbp->msb_delta),
-						rsvd);
-				spin_lock(&mp->m_sb_lock);
-				break;
-#endif
-			default:
-				status = xfs_mod_incore_sb_unlocked(mp,
-							msbp->msb_field,
-							-(msbp->msb_delta),
-							rsvd);
-				break;
-			}
-			ASSERT(status == 0);
-			msbp--;
-		}
+	while (--msbp >= msb) {
+		status = xfs_mod_incore_sb_unlocked(mp, msbp->msb_field,
+						    -(msbp->msb_delta), rsvd);
+		ASSERT(status == 0);
 	}
 	spin_unlock(&mp->m_sb_lock);
 	return status;
Index: xfs/fs/xfs/xfs_trans.c
===================================================================
--- xfs.orig/fs/xfs/xfs_trans.c	2010-09-29 13:14:08.521557720 +0900
+++ xfs/fs/xfs/xfs_trans.c	2010-09-29 13:21:58.164557721 +0900
@@ -1017,55 +1017,61 @@ xfs_trans_unreserve_and_mod_sb(
 	int		rsvd;
 	int64_t		blkdelta = 0;
 	int64_t		rtxdelta = 0;
+	int64_t		idelta = 0;
+	int64_t		ifreedelta = 0;
 
 	msbp = msb;
 	rsvd = (tp->t_flags & XFS_TRANS_RESERVE) != 0;
 
-	/* calculate free blocks delta */
+	/* calculate deltas */
 	if (tp->t_blk_res > 0)
 		blkdelta = tp->t_blk_res;
-
 	if ((tp->t_fdblocks_delta != 0) &&
 	    (xfs_sb_version_haslazysbcount(&mp->m_sb) ||
 	     (tp->t_flags & XFS_TRANS_SB_DIRTY)))
 	        blkdelta += tp->t_fdblocks_delta;
 
-	if (blkdelta != 0) {
-		msbp->msb_field = XFS_SBS_FDBLOCKS;
-		msbp->msb_delta = blkdelta;
-		msbp++;
-	}
-
-	/* calculate free realtime extents delta */
 	if (tp->t_rtx_res > 0)
 		rtxdelta = tp->t_rtx_res;
-
 	if ((tp->t_frextents_delta != 0) &&
 	    (tp->t_flags & XFS_TRANS_SB_DIRTY))
 		rtxdelta += tp->t_frextents_delta;
 
+	if (xfs_sb_version_haslazysbcount(&mp->m_sb) ||
+	     (tp->t_flags & XFS_TRANS_SB_DIRTY)) {
+		idelta = tp->t_icount_delta;
+		ifreedelta = tp->t_ifree_delta;
+	}
+
+	/* apply the per-cpu counters */
+	if (blkdelta) {
+		error = xfs_icsb_modify_counters(mp, XFS_SBS_FDBLOCKS,
+						 blkdelta, rsvd);
+		if (error)
+			goto out;
+	}
+
+	if (idelta) {
+		error = xfs_icsb_modify_counters(mp, XFS_SBS_ICOUNT,
+						 idelta, rsvd);
+		if (error)
+			goto out_undo_fdblocks;
+	}
+
+	if (ifreedelta) {
+		error = xfs_icsb_modify_counters(mp, XFS_SBS_IFREE,
+						 ifreedelta, rsvd);
+		if (error)
+			goto out_undo_icount;
+	}
+
+	/* apply remaining deltas */
 	if (rtxdelta != 0) {
 		msbp->msb_field = XFS_SBS_FREXTENTS;
 		msbp->msb_delta = rtxdelta;
 		msbp++;
 	}
 
-	/* apply remaining deltas */
-
-	if (xfs_sb_version_haslazysbcount(&mp->m_sb) ||
-	     (tp->t_flags & XFS_TRANS_SB_DIRTY)) {
-		if (tp->t_icount_delta != 0) {
-			msbp->msb_field = XFS_SBS_ICOUNT;
-			msbp->msb_delta = tp->t_icount_delta;
-			msbp++;
-		}
-		if (tp->t_ifree_delta != 0) {
-			msbp->msb_field = XFS_SBS_IFREE;
-			msbp->msb_delta = tp->t_ifree_delta;
-			msbp++;
-		}
-	}
-
 	if (tp->t_flags & XFS_TRANS_SB_DIRTY) {
 		if (tp->t_dblocks_delta != 0) {
 			msbp->msb_field = XFS_SBS_DBLOCKS;
@@ -1115,8 +1121,24 @@ xfs_trans_unreserve_and_mod_sb(
 	if (msbp > msb) {
 		error = xfs_mod_incore_sb_batch(tp->t_mountp, msb,
 			(uint)(msbp - msb), rsvd);
-		ASSERT(error == 0);
+		if (error)
+			goto out_undo_ifreecount;
 	}
+
+	return;
+
+out_undo_ifreecount:
+	if (ifreedelta)
+		xfs_icsb_modify_counters(mp, XFS_SBS_IFREE, ifreedelta, rsvd);
+out_undo_icount:
+	if (idelta)
+		xfs_icsb_modify_counters(mp, XFS_SBS_ICOUNT, idelta, rsvd);
+out_undo_fdblocks:
+	if (blkdelta)
+		xfs_icsb_modify_counters(mp, XFS_SBS_FDBLOCKS, -blkdelta, rsvd);
+out:
+	ASSERT(error = 0);
+	return;
 }
 
 /*

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

Re: [PATCH 3/3] [PATCH 3/3] xfs: do not use xfs_mod_incore_sb_batch for per-cpu counters

[Dave Chinner]

On Wed, Sep 29, 2010 at 03:22:24AM -0400, Christoph Hellwig wrote:
> Update the per-cpu counters manually in xfs_trans_unreserve_and_mod_sb and
> remove support for per-cpu counters from xfs_mod_incore_sb_batch to
> simplify it.  And added benefit is that we don't have to take m_sb_lock
> for transactions that only modify per-cpu counters.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
....

> +
> +	return;
> +
> +out_undo_ifreecount:
> +	if (ifreedelta)
> +		xfs_icsb_modify_counters(mp, XFS_SBS_IFREE, ifreedelta, rsvd);
> +out_undo_icount:
> +	if (idelta)
> +		xfs_icsb_modify_counters(mp, XFS_SBS_ICOUNT, idelta, rsvd);

These two should be -ifreedelta and -idelta, right?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

Re: [PATCH 1/3] [PATCH 1/3] xfs: remove XFS_MOUNT_NO_PERCPU_SB

[Dave Chinner]

On Wed, Sep 29, 2010 at 03:22:22AM -0400, Christoph Hellwig wrote:
> Fail the mount if we can't allocate memory for the per-CPU counters.
> This is consistent with how we handle everything else in the mount
> path and makes the superblock counter modification a lot simpler.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks good.  I had an "XXX: kill XFS_MOUNT_NO_PERCPU_SB?" in my
patch, so no complaints about this from me.

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

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

Re: [PATCH 2/3] [PATCH 2/3] xfs: do not use xfs_mod_incore_sb for per-cpu counters

[Dave Chinner]

On Wed, Sep 29, 2010 at 03:22:23AM -0400, Christoph Hellwig wrote:
> Export xfs_icsb_modify_counters and always use it for modifying the per-cpu
> counters.  Remove support for per-cpu counters from xfs_mod_incore_sb to
> simplify it.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks good. Couple of things, though.

>  			nblks += cur->bc_private.b.allocated;
>  		ASSERT(nblks <= da_old);
>  		if (nblks < da_old)
> -			xfs_mod_incore_sb(ip->i_mount, XFS_SBS_FDBLOCKS,
> +			xfs_icsb_modify_counters(ip->i_mount, XFS_SBS_FDBLOCKS,
>  				(int64_t)(da_old - nblks), rsvd);
>  	}
>  	/*
> @@ -1078,8 +1078,8 @@ xfs_bmap_add_extent_delay_real(
>  		temp2 = xfs_bmap_worst_indlen(ip, temp2);
>  		diff = (int)(temp + temp2 - startblockval(PREV.br_startblock) -
>  			(cur ? cur->bc_private.b.allocated : 0));
> -		if (diff > 0 &&
> -		    xfs_mod_incore_sb(ip->i_mount, XFS_SBS_FDBLOCKS, -((int64_t)diff), rsvd)) {
> +		if (diff > 0 && xfs_icsb_modify_counters(ip->i_mount,
> +				XFS_SBS_FDBLOCKS, -((int64_t)diff), rsvd)) {

Not sure I like the indenting of the second line. I'd prefer the
parameters to have a little more indent or use three lines...

>  int
>  xfs_mod_incore_sb(
> -	xfs_mount_t	*mp,
> -	xfs_sb_field_t	field,
> -	int64_t		delta,
> -	int		rsvd)
> +	struct xfs_mount	*mp,
> +	xfs_sb_field_t		field,
> +	int64_t			delta,
> +	int			rsvd)
>  {
> -	int	status;
> +	int			status;
>  
> -	/* check for per-cpu counters */
> -	switch (field) {
> -#ifdef HAVE_PERCPU_SB
> -	case XFS_SBS_ICOUNT:
> -	case XFS_SBS_IFREE:
> -	case XFS_SBS_FDBLOCKS:
> -		status = xfs_icsb_modify_counters(mp, field, delta, rsvd);
> -		break;
> -#endif
> -	default:
> -		spin_lock(&mp->m_sb_lock);
> -		status = xfs_mod_incore_sb_unlocked(mp, field, delta, rsvd);
> -		spin_unlock(&mp->m_sb_lock);
> -		break;
> -	}
> +	ASSERT(field < XFS_SBS_ICOUNT || field > XFS_SBS_FDBLOCKS);

That assert will cause issues with:

> @@ -97,6 +99,8 @@ extern void	xfs_icsb_sync_counters_locke
>  #define xfs_icsb_reinit_counters(mp)		do { } while (0)
>  #define xfs_icsb_sync_counters(mp, flags)	do { } while (0)
>  #define xfs_icsb_sync_counters_locked(mp, flags) do { } while (0)
> +#define xfs_icsb_modify_counters(mp, field, delta, rsvd) \
> +	xfs_mod_incore_sb(mp, field, delta, rsvd)
>  #endif

UP builds. Perhaps a CONFIG_SMP only assert? Given that the per-cpu
counter rework I'm doing doesn't have a different code path for
UP vs SMP, it'd only be a temporary concern....

>  undo_blocks:
>  	if (blocks > 0) {
> -		(void) xfs_mod_incore_sb(tp->t_mountp, XFS_SBS_FDBLOCKS,
> +		(void) xfs_icsb_modify_counters(tp->t_mountp, XFS_SBS_FDBLOCKS,
>  					 (int64_t)blocks, rsvd);

You can kill the (void) cast there.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

Re: [PATCH 2/3] [PATCH 2/3] xfs: do not use xfs_mod_incore_sb for per-cpu counters

[Christoph Hellwig]

On Wed, Sep 29, 2010 at 09:39:14PM +1000, Dave Chinner wrote:
> Not sure I like the indenting of the second line. I'd prefer the
> parameters to have a little more indent or use three lines...

That whole area needs some larger refactoring / reformatting work.
I'll see if I can ad danother patch for that.

> > +	ASSERT(field < XFS_SBS_ICOUNT || field > XFS_SBS_FDBLOCKS);
> 
> That assert will cause issues with:
> 
> > @@ -97,6 +99,8 @@ extern void	xfs_icsb_sync_counters_locke
> >  #define xfs_icsb_reinit_counters(mp)		do { } while (0)
> >  #define xfs_icsb_sync_counters(mp, flags)	do { } while (0)
> >  #define xfs_icsb_sync_counters_locked(mp, flags) do { } while (0)
> > +#define xfs_icsb_modify_counters(mp, field, delta, rsvd) \
> > +	xfs_mod_incore_sb(mp, field, delta, rsvd)
> >  #endif
> 
> UP builds. Perhaps a CONFIG_SMP only assert? Given that the per-cpu
> counter rework I'm doing doesn't have a different code path for
> UP vs SMP, it'd only be a temporary concern....

Indeed, it should be conditional.

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

Re: [PATCH 3/3] [PATCH 3/3] xfs: do not use xfs_mod_incore_sb_batch for per-cpu counters

[Christoph Hellwig]

On Wed, Sep 29, 2010 at 09:27:20PM +1000, Dave Chinner wrote:
> > +out_undo_ifreecount:
> > +	if (ifreedelta)
> > +		xfs_icsb_modify_counters(mp, XFS_SBS_IFREE, ifreedelta, rsvd);
> > +out_undo_icount:
> > +	if (idelta)
> > +		xfs_icsb_modify_counters(mp, XFS_SBS_ICOUNT, idelta, rsvd);
> 
> These two should be -ifreedelta and -idelta, right?

Indeed, fixed.

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

Re: [PATCH 1/3] [PATCH 1/3] xfs: remove XFS_MOUNT_NO_PERCPU_SB

[Alex Elder]

On Wed, 2010-09-29 at 03:22 -0400, Christoph Hellwig wrote:
> plain text document attachment (xfs-kill-XFS_MOUNT_NO_PERCPU_SB)
> Fail the mount if we can't allocate memory for the per-CPU counters.
> This is consistent with how we handle everything else in the mount
> path and makes the superblock counter modification a lot simpler.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks good.

Reviewed-by: Alex Elder <aelder@sgi.com>


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

Re: [PATCH 2/3] [PATCH 2/3] xfs: do not use xfs_mod_incore_sb for per-cpu counters

[Alex Elder]

On Wed, 2010-09-29 at 03:22 -0400, Christoph Hellwig wrote:
> plain text document attachment
> (xfs-dont-use-xfs_mod_incore_sb-for-percpu-counters)
> Export xfs_icsb_modify_counters and always use it for modifying the per-cpu
> counters.  Remove support for per-cpu counters from xfs_mod_incore_sb to
> simplify it.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks good.

Reviewed-by: Alex Elder <aelder@sgi.com>


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

Re: [PATCH 3/3] [PATCH 3/3] xfs: do not use xfs_mod_incore_sb_batch for per-cpu counters

[Alex Elder]

On Wed, 2010-09-29 at 03:22 -0400, Christoph Hellwig wrote:
> plain text document attachment (xfs-simplify-icsb-updates)
> Update the per-cpu counters manually in xfs_trans_unreserve_and_mod_sb and
> remove support for per-cpu counters from xfs_mod_incore_sb_batch to
> simplify it.  And added benefit is that we don't have to take m_sb_lock
> for transactions that only modify per-cpu counters.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Two undo's have the wrong sign, but if that's fixed this
looks good.

Reviewed-by: Alex Elder <aelder@sgi.com> 

. . .

> Index: xfs/fs/xfs/xfs_trans.c
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_trans.c	2010-09-29 13:14:08.521557720 +0900
> +++ xfs/fs/xfs/xfs_trans.c	2010-09-29 13:21:58.164557721 +0900
> @@ -1017,55 +1017,61 @@ xfs_trans_unreserve_and_mod_sb(
. . .
> +
> +	/* apply the per-cpu counters */
> +	if (blkdelta) {
> +		error = xfs_icsb_modify_counters(mp, XFS_SBS_FDBLOCKS,
> +						 blkdelta, rsvd);
> +		if (error)
> +			goto out;
> +	}
> +
> +	if (idelta) {
> +		error = xfs_icsb_modify_counters(mp, XFS_SBS_ICOUNT,
> +						 idelta, rsvd);
> +		if (error)
> +			goto out_undo_fdblocks;
> +	}
> +
> +	if (ifreedelta) {
> +		error = xfs_icsb_modify_counters(mp, XFS_SBS_IFREE,
> +						 ifreedelta, rsvd);
> +		if (error)
> +			goto out_undo_icount;
> +	}
> +
> +	/* apply remaining deltas */
>  
. . .
> +	return;
> +
> +out_undo_ifreecount:
> +	if (ifreedelta)
> +		xfs_icsb_modify_counters(mp, XFS_SBS_IFREE, ifreedelta, rsvd);

*negative* ifreedelta

> +out_undo_icount:
> +	if (idelta)
> +		xfs_icsb_modify_counters(mp, XFS_SBS_ICOUNT, idelta, rsvd);

*negative* idelta

> +out_undo_fdblocks:
> +	if (blkdelta)
> +		xfs_icsb_modify_counters(mp, XFS_SBS_FDBLOCKS, -blkdelta, rsvd);
> +out:



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

[PATCH 1/3] [PATCH 1/3] xfs: remove XFS_MOUNT_NO_PERCPU_SB

[Christoph Hellwig]

[-- Attachment #1: xfs-kill-XFS_MOUNT_NO_PERCPU_SB --]
[-- Type: text/plain, Size: 3761 bytes --]

Fail the mount if we can't allocate memory for the per-CPU counters.
This is consistent with how we handle everything else in the mount
path and makes the superblock counter modification a lot simpler.

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

Index: xfs/fs/xfs/linux-2.6/xfs_super.c
===================================================================
--- xfs.orig/fs/xfs/linux-2.6/xfs_super.c	2010-09-29 20:44:11.711357120 +0900
+++ xfs/fs/xfs/linux-2.6/xfs_super.c	2010-09-30 08:11:46.119709459 +0900
@@ -1517,8 +1517,9 @@ xfs_fs_fill_super(
 	if (error)
 		goto out_free_fsname;
 
-	if (xfs_icsb_init_counters(mp))
-		mp->m_flags |= XFS_MOUNT_NO_PERCPU_SB;
+	error = xfs_icsb_init_counters(mp);
+	if (error)
+		goto out_close_devices;
 
 	error = xfs_readsb(mp, flags);
 	if (error)
@@ -1579,6 +1580,7 @@ xfs_fs_fill_super(
 	xfs_freesb(mp);
  out_destroy_counters:
 	xfs_icsb_destroy_counters(mp);
+ out_close_devices:
 	xfs_close_devices(mp);
  out_free_fsname:
 	xfs_free_fsname(mp);
Index: xfs/fs/xfs/xfs_mount.c
===================================================================
--- xfs.orig/fs/xfs/xfs_mount.c	2010-09-29 20:44:11.716357120 +0900
+++ xfs/fs/xfs/xfs_mount.c	2010-09-30 08:11:34.660709458 +0900
@@ -1856,12 +1856,8 @@ xfs_mod_incore_sb(
 	case XFS_SBS_ICOUNT:
 	case XFS_SBS_IFREE:
 	case XFS_SBS_FDBLOCKS:
-		if (!(mp->m_flags & XFS_MOUNT_NO_PERCPU_SB)) {
-			status = xfs_icsb_modify_counters(mp, field,
-							delta, rsvd);
-			break;
-		}
-		/* FALLTHROUGH */
+		status = xfs_icsb_modify_counters(mp, field, delta, rsvd);
+		break;
 #endif
 	default:
 		spin_lock(&mp->m_sb_lock);
@@ -1910,15 +1906,12 @@ xfs_mod_incore_sb_batch(xfs_mount_t *mp,
 		case XFS_SBS_ICOUNT:
 		case XFS_SBS_IFREE:
 		case XFS_SBS_FDBLOCKS:
-			if (!(mp->m_flags & XFS_MOUNT_NO_PERCPU_SB)) {
-				spin_unlock(&mp->m_sb_lock);
-				status = xfs_icsb_modify_counters(mp,
-							msbp->msb_field,
-							msbp->msb_delta, rsvd);
-				spin_lock(&mp->m_sb_lock);
-				break;
-			}
-			/* FALLTHROUGH */
+			spin_unlock(&mp->m_sb_lock);
+			status = xfs_icsb_modify_counters(mp,
+						msbp->msb_field,
+						msbp->msb_delta, rsvd);
+			spin_lock(&mp->m_sb_lock);
+			break;
 #endif
 		default:
 			status = xfs_mod_incore_sb_unlocked(mp,
@@ -1948,16 +1941,13 @@ xfs_mod_incore_sb_batch(xfs_mount_t *mp,
 			case XFS_SBS_ICOUNT:
 			case XFS_SBS_IFREE:
 			case XFS_SBS_FDBLOCKS:
-				if (!(mp->m_flags & XFS_MOUNT_NO_PERCPU_SB)) {
-					spin_unlock(&mp->m_sb_lock);
-					status = xfs_icsb_modify_counters(mp,
-							msbp->msb_field,
-							-(msbp->msb_delta),
-							rsvd);
-					spin_lock(&mp->m_sb_lock);
-					break;
-				}
-				/* FALLTHROUGH */
+				spin_unlock(&mp->m_sb_lock);
+				status = xfs_icsb_modify_counters(mp,
+						msbp->msb_field,
+						-(msbp->msb_delta),
+						rsvd);
+				spin_lock(&mp->m_sb_lock);
+				break;
 #endif
 			default:
 				status = xfs_mod_incore_sb_unlocked(mp,
Index: xfs/fs/xfs/xfs_mount.h
===================================================================
--- xfs.orig/fs/xfs/xfs_mount.h	2010-09-29 20:44:11.723357120 +0900
+++ xfs/fs/xfs/xfs_mount.h	2010-09-30 08:11:34.669709458 +0900
@@ -231,8 +231,6 @@ typedef struct xfs_mount {
 #define XFS_MOUNT_DIRSYNC	(1ULL << 21)	/* synchronous directory ops */
 #define XFS_MOUNT_COMPAT_IOSIZE	(1ULL << 22)	/* don't report large preferred
 						 * I/O size in stat() */
-#define XFS_MOUNT_NO_PERCPU_SB	(1ULL << 23)	/* don't use per-cpu superblock
-						   counters */
 #define XFS_MOUNT_FILESTREAMS	(1ULL << 24)	/* enable the filestreams
 						   allocator */
 #define XFS_MOUNT_NOATTR2	(1ULL << 25)	/* disable use of attr2 format */

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

Re: [PATCH 1/3] [PATCH 1/3] xfs: remove XFS_MOUNT_NO_PERCPU_SB

[Alex Elder]

On Wed, 2010-09-29 at 22:25 -0400, Christoph Hellwig wrote:

> Fail the mount if we can't allocate memory for the per-CPU counters.
> This is consistent with how we handle everything else in the mount
> path and makes the superblock counter modification a lot simpler.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks good.

Reviewed-by: Alex Elder <aelder@sgi.com>



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