Review: fix block reservation to work with per-cpu counters

Review: fix block reservation to work with per-cpu counters

[David Chinner]

Currently, XFS_IOC_SET_RESBLKS will not work properly when
per-cpu superblock counters are enabled. Reservations can be lost
silently as they are applied to the incore superblock instead of
the currently active counters.

Rather than try to shoe-horn the current reservation code into
the per-cpu counters or vice-versa, we lock the superblock
and snap the current counter state and work on that number.
Once we work out exactly how much we need to "allocate" to
the reserved area, we drop the lock and call xfs_mod_incore_sb()
which will do all the right things w.r.t to the counter state.

If we fail to get as much as we want (i.e. ENOSPC is returned)
we go back to the start and try to allocate as much of what is
left.

Comments?

Cheers,

Dave.
-- 
Dave Chinner
Principal Engineer
SGI Australian Software Group

---
 fs/xfs/xfs_fsops.c  |   54 +++++++++++++++++++++++++++++++++++++++++++++++-----
 fs/xfs/xfs_mount.c  |   16 ++-------------
 fs/xfs/xfs_mount.h  |    2 -
 fs/xfs/xfs_vfsops.c |    2 -
 4 files changed, 54 insertions(+), 20 deletions(-)

Index: 2.6.x-xfs-new/fs/xfs/xfs_fsops.c
===================================================================
--- 2.6.x-xfs-new.orig/fs/xfs/xfs_fsops.c	2006-12-12 12:05:20.000000000 +1100
+++ 2.6.x-xfs-new/fs/xfs/xfs_fsops.c	2006-12-22 00:30:53.770384646 +1100
@@ -460,7 +460,7 @@ xfs_fs_counts(
 {
 	unsigned long	s;
 
-	xfs_icsb_sync_counters_lazy(mp);
+	xfs_icsb_sync_counters_flags(mp, XFS_ICSB_LAZY_COUNT);
 	s = XFS_SB_LOCK(mp);
 	cnt->freedata = mp->m_sb.sb_fdblocks - XFS_ALLOC_SET_ASIDE(mp);
 	cnt->freertx = mp->m_sb.sb_frextents;
@@ -491,7 +491,7 @@ xfs_reserve_blocks(
 	__uint64_t              *inval,
 	xfs_fsop_resblks_t      *outval)
 {
-	__int64_t		lcounter, delta;
+	__int64_t		lcounter, delta, fdblks_delta;
 	__uint64_t		request;
 	unsigned long		s;
 
@@ -504,17 +504,35 @@ xfs_reserve_blocks(
 	}
 
 	request = *inval;
+
+	/*
+	 * With per-cpu counters, this becomes an interesting
+	 * problem. we needto work out if we are freeing or allocation
+	 * blocks first, then we can do the modification as necessary.
+	 *
+	 * We do this under the XFS_SB_LOCK so that if we are near
+	 * ENOSPC, we will hold out any changes while we work out
+	 * what to do. This means that the amount of free space can
+	 * change while we do this, so we need to retry if we end up
+	 * trying to reserve more space than is available.
+	 *
+	 * We also use the xfs_mod_incore_sb() interface so that we
+	 * don't have to care about whether per cpu counter are
+	 * enabled, disabled or even compiled in....
+	 */
+retry:
 	s = XFS_SB_LOCK(mp);
+	xfs_icsb_sync_counters_flags(mp, XFS_ICSB_SB_LOCKED);
 
 	/*
 	 * If our previous reservation was larger than the current value,
 	 * then move any unused blocks back to the free pool.
 	 */
-
+	fdblks_delta = 0;
 	if (mp->m_resblks > request) {
 		lcounter = mp->m_resblks_avail - request;
 		if (lcounter  > 0) {		/* release unused blocks */
-			mp->m_sb.sb_fdblocks += lcounter;
+			fdblks_delta = lcounter;
 			mp->m_resblks_avail -= lcounter;
 		}
 		mp->m_resblks = request;
@@ -522,24 +540,50 @@ xfs_reserve_blocks(
 		__int64_t	free;
 
 		free =  mp->m_sb.sb_fdblocks - XFS_ALLOC_SET_ASIDE(mp);
+		if (!free)
+			goto out; /* ENOSPC and fdblks_delta = 0 */
+
 		delta = request - mp->m_resblks;
 		lcounter = free - delta;
 		if (lcounter < 0) {
 			/* We can't satisfy the request, just get what we can */
 			mp->m_resblks += free;
 			mp->m_resblks_avail += free;
+			fdblks_delta = -free;
 			mp->m_sb.sb_fdblocks = XFS_ALLOC_SET_ASIDE(mp);
 		} else {
+			fdblks_delta = -delta;
 			mp->m_sb.sb_fdblocks =
 				lcounter + XFS_ALLOC_SET_ASIDE(mp);
 			mp->m_resblks = request;
 			mp->m_resblks_avail += delta;
 		}
 	}
-
+out:
 	outval->resblks = mp->m_resblks;
 	outval->resblks_avail = mp->m_resblks_avail;
 	XFS_SB_UNLOCK(mp, s);
+
+	if (fdblks_delta) {
+		/*
+		 * If we are putting blocks back here, m_resblks_avail is
+		 * already at it's max so this will put it in the free pool.
+		 *
+		 * If we need space, we'll either succeed in getting it
+		 * from the free block count or we'll get an enospc. If
+		 * we get a ENOSPC, it means things changed while we were
+		 * calculating fdblks_delta and so we should try again to
+		 * see if there is anything left to reserve.
+		 *
+		 * Don't set the reserved flag here - we don't want to reserve
+		 * the extra reserve blocks from the reserve.....
+		 */
+		int error;
+		error = xfs_mod_incore_sb(mp, XFS_SBS_FDBLOCKS, fdblks_delta, 0);
+		if (error == ENOSPC)
+			goto retry;
+	}
+
 	return 0;
 }
 
Index: 2.6.x-xfs-new/fs/xfs/xfs_mount.c
===================================================================
--- 2.6.x-xfs-new.orig/fs/xfs/xfs_mount.c	2006-12-12 18:02:03.000000000 +1100
+++ 2.6.x-xfs-new/fs/xfs/xfs_mount.c	2006-12-21 22:53:35.669131775 +1100
@@ -1963,8 +1963,8 @@ xfs_icsb_enable_counter(
 	xfs_icsb_unlock_all_counters(mp);
 }
 
-STATIC void
-xfs_icsb_sync_counters_int(
+void
+xfs_icsb_sync_counters_flags(
 	xfs_mount_t	*mp,
 	int		flags)
 {
@@ -1996,17 +1996,7 @@ STATIC void
 xfs_icsb_sync_counters(
 	xfs_mount_t	*mp)
 {
-	xfs_icsb_sync_counters_int(mp, 0);
-}
-
-/*
- * lazy addition used for things like df, background sb syncs, etc
- */
-void
-xfs_icsb_sync_counters_lazy(
-	xfs_mount_t	*mp)
-{
-	xfs_icsb_sync_counters_int(mp, XFS_ICSB_LAZY_COUNT);
+	xfs_icsb_sync_counters_flags(mp, 0);
 }
 
 /*
Index: 2.6.x-xfs-new/fs/xfs/xfs_mount.h
===================================================================
--- 2.6.x-xfs-new.orig/fs/xfs/xfs_mount.h	2006-12-20 22:59:33.000000000 +1100
+++ 2.6.x-xfs-new/fs/xfs/xfs_mount.h	2006-12-21 22:52:35.596932143 +1100
@@ -312,7 +312,7 @@ typedef struct xfs_icsb_cnts {
 #define XFS_ICSB_LAZY_COUNT	(1 << 1)	/* accuracy not needed */
 
 extern int	xfs_icsb_init_counters(struct xfs_mount *);
-extern void	xfs_icsb_sync_counters_lazy(struct xfs_mount *);
+extern void	xfs_icsb_sync_counters_flags(struct xfs_mount *, int);
 
 #else
 #define xfs_icsb_init_counters(mp)	(0)
Index: 2.6.x-xfs-new/fs/xfs/xfs_vfsops.c
===================================================================
--- 2.6.x-xfs-new.orig/fs/xfs/xfs_vfsops.c	2006-12-12 15:40:58.000000000 +1100
+++ 2.6.x-xfs-new/fs/xfs/xfs_vfsops.c	2006-12-22 00:28:42.851623181 +1100
@@ -815,7 +815,7 @@ xfs_statvfs(
 
 	statp->f_type = XFS_SB_MAGIC;
 
-	xfs_icsb_sync_counters_lazy(mp);
+	xfs_icsb_sync_counters_flags(mp, XFS_ICSB_LAZY_COUNT);
 	s = XFS_SB_LOCK(mp);
 	statp->f_bsize = sbp->sb_blocksize;
 	lsize = sbp->sb_logstart ? sbp->sb_logblocks : 0;

Re: Review: fix block reservation to work with per-cpu counters

[Christoph Hellwig]

On Mon, Jan 08, 2007 at 05:10:40PM +1100, David Chinner wrote:
> Currently, XFS_IOC_SET_RESBLKS will not work properly when
> per-cpu superblock counters are enabled. Reservations can be lost
> silently as they are applied to the incore superblock instead of
> the currently active counters.
> 
> Rather than try to shoe-horn the current reservation code into
> the per-cpu counters or vice-versa, we lock the superblock
> and snap the current counter state and work on that number.
> Once we work out exactly how much we need to "allocate" to
> the reserved area, we drop the lock and call xfs_mod_incore_sb()
> which will do all the right things w.r.t to the counter state.
> 
> If we fail to get as much as we want (i.e. ENOSPC is returned)
> we go back to the start and try to allocate as much of what is
> left.
> 
> Comments?

Sounds okay.  Reservations shouldn't be frequent enough for this
to have a performance impact.

Re: Review: fix block reservation to work with per-cpu counters

[Kevin Jamieson]

This fails to build on non-SMP systems:

  CC [M]  fs/xfs/xfs_fsops.o
fs/xfs/xfs_fsops.c: In function 'xfs_fs_counts':
fs/xfs/xfs_fsops.c:463: warning: implicit declaration of function 'xfs_icsb_sync_counters_flags'
fs/xfs/xfs_fsops.c:463: error: 'XFS_ICSB_LAZY_COUNT' undeclared (first use in this function)
fs/xfs/xfs_fsops.c:463: error: (Each undeclared identifier is reported only once
fs/xfs/xfs_fsops.c:463: error: for each function it appears in.)
fs/xfs/xfs_fsops.c: In function 'xfs_reserve_blocks':
fs/xfs/xfs_fsops.c:525: error: 'XFS_ICSB_SB_LOCKED' undeclared (first use in this function)
make[1]: *** [fs/xfs/xfs_fsops.o] Error 1
make: *** [fs/xfs/xfs.ko] Error 2


The define for not HAVE_PERCPU_SB needs to be changed:


Index: fs/xfs/xfs_mount.h
===================================================================
RCS file: /cvs/linux-2.6-xfs/fs/xfs/xfs_mount.h,v
retrieving revision 1.232
diff -u -r1.232 xfs_mount.h
--- fs/xfs/xfs_mount.h	10 Jan 2007 14:42:52 -0000	1.232
+++ fs/xfs/xfs_mount.h	15 Jan 2007 21:13:09 -0000
@@ -311,7 +311,7 @@
 
 #else
 #define xfs_icsb_init_counters(mp)	(0)
-#define xfs_icsb_sync_counters_lazy(mp)	do { } while (0)
+#define xfs_icsb_sync_counters_flags(mp, flags)	do { } while (0)
 #endif
 
 typedef struct xfs_mount {

Re: Review: fix block reservation to work with per-cpu counters

[David Chinner]

On Mon, Jan 15, 2007 at 01:31:22PM -0800, Kevin Jamieson wrote:
> This fails to build on non-SMP systems:
> 
>  CC [M]  fs/xfs/xfs_fsops.o
> fs/xfs/xfs_fsops.c: In function 'xfs_fs_counts':
> fs/xfs/xfs_fsops.c:463: warning: implicit declaration of function 
> 'xfs_icsb_sync_counters_flags'
> fs/xfs/xfs_fsops.c:463: error: 'XFS_ICSB_LAZY_COUNT' undeclared (first use 
> in this function)
> fs/xfs/xfs_fsops.c:463: error: (Each undeclared identifier is reported only 
> once
> fs/xfs/xfs_fsops.c:463: error: for each function it appears in.)
> fs/xfs/xfs_fsops.c: In function 'xfs_reserve_blocks':
> fs/xfs/xfs_fsops.c:525: error: 'XFS_ICSB_SB_LOCKED' undeclared (first use 
> in this function)
> make[1]: *** [fs/xfs/xfs_fsops.o] Error 1
> make: *** [fs/xfs/xfs.ko] Error 2
> 
> 
> The define for not HAVE_PERCPU_SB needs to be changed:

Sorry about that - my fault. This fix looks good - I'll try to find some
time to check this in (I'm at LCA right now and have limited net
access).

Chatz, maybe you could check this in?

Cheers,

Dave.
-- 
Dave Chinner
Principal Engineer
SGI Australian Software Group

Re: Review: fix block reservation to work with per-cpu counters

[David Chatterton]

David Chinner wrote:
> On Mon, Jan 15, 2007 at 01:31:22PM -0800, Kevin Jamieson wrote:
>> This fails to build on non-SMP systems:
>>
>>  CC [M]  fs/xfs/xfs_fsops.o
>> fs/xfs/xfs_fsops.c: In function 'xfs_fs_counts':
>> fs/xfs/xfs_fsops.c:463: warning: implicit declaration of function 
>> 'xfs_icsb_sync_counters_flags'
>> fs/xfs/xfs_fsops.c:463: error: 'XFS_ICSB_LAZY_COUNT' undeclared (first use 
>> in this function)
>> fs/xfs/xfs_fsops.c:463: error: (Each undeclared identifier is reported only 
>> once
>> fs/xfs/xfs_fsops.c:463: error: for each function it appears in.)
>> fs/xfs/xfs_fsops.c: In function 'xfs_reserve_blocks':
>> fs/xfs/xfs_fsops.c:525: error: 'XFS_ICSB_SB_LOCKED' undeclared (first use 
>> in this function)
>> make[1]: *** [fs/xfs/xfs_fsops.o] Error 1
>> make: *** [fs/xfs/xfs.ko] Error 2
>>
>>
>> The define for not HAVE_PERCPU_SB needs to be changed:
> 
> Sorry about that - my fault. This fix looks good - I'll try to find some
> time to check this in (I'm at LCA right now and have limited net
> access).
> 
> Chatz, maybe you could check this in?
> 

Done.

David

-- 
David Chatterton
XFS Engineering Manager
SGI Australia