Re: Review: Reduce in-core superblock lock contention near ENOSPC

[Lachlan McIlroy <lachlan@sgi.com>]

David Chinner wrote:
> On Thu, Nov 30, 2006 at 06:03:40PM +0000, Lachlan McIlroy wrote:
> 
>>Dave,
>>
>>Could you have changed the SB_LOCK from a spinlock to a blocking
>>mutex and have achieved a similar effect?
> 
> 
> Sort of - it would still be inefficient and wouldn't help solve the
> underlying causes of contention.  Also, everything else that uses
> the SB_LOCK would now have a sleep point where there wasn't one
> previously. If we are nesting the SB_LOCK somewhere else inside a
> another spinlock (not sure if we are) then we can't sleep. I'd
> prefer not to change the semantics of such a lock if I can avoid it.

That's fair enough and I can't disagree with you.  I think the SB_LOCK
was/is being abused anyway and was used too genericly (if there's such
a word).  Using separate locks for specific purposes like you've done
with the new mutex is a great start to cleaning this code up.

> 
> I think the slow path code is somewhat clearer with a separate
> mutex - it clearly documents the serialisation barrier that
> the slow path uses and allows us to do slow path checks on the
> per-cpu counters without needing the SB_LOCK.

It's certainly an improvement over the original code.

> 
> It also means that in future, we can slowly remove the need for
> holding the SB_LOCK across the entire rebalance operation and only
> use it when referencing the global superblock fields during
> the rebalance.

Sounds good.

> 
> If the need arises, it also means we can move to a mutex per counter
> so we can independently rebalance different types of counters at the
> same time (which we can't do right now).

That seems so obvious - I'm surprised we can't do it now.

> 
> 
>>Has this change had much testing on a large machine?
> 
> 
> 8p is the largest I've run it on (junkbond) and it's been ENOSPC
> tested on a 2.7GB/s filesystem (junkbond once again) as well
> as one single, slow disks.
> 
> I've tried and tried to get the ppl that reported the problem to
> test this fix but no luck so far (this bug has been open for months
> and most of that time has been me waiting for someone to run a
> test). I've basically got sick of waiting and I just want to
> move this on. It's already too late for sles10sp1 because of
> the lack of response.

If it's important to them they'll test it.  If the change doesn't fix
their problem then I'm sure we'll hear from them again.

> 
> 
>>These changes wouldn't apply cleanly to tot (3 hunks failed in
>>xfs_mount.c) but I couldn't see why.
> 
> 
> Whitespace issue? Try setting:
> 
> $ export QUILT_PATCH_OPTS="--ignore-whitespace"
> 
> I'll apply the patch to a separate tree and see if I hit the same
> problem....
> 
> 
>>The changes look fine to me, couple of comments below.
>>
>>Lachlan
>>
>>
>>@@ -1479,9 +1479,11 @@ xfs_mod_incore_sb_batch(xfs_mount_t *mp,
>> 		case XFS_SBS_IFREE:
>> 		case XFS_SBS_FDBLOCKS:
>> 			if (!(mp->m_flags & XFS_MOUNT_NO_PERCPU_SB)) {
>>-				status = xfs_icsb_modify_counters_locked(mp,
>>+				XFS_SB_UNLOCK(mp, s);
>>+				status = xfs_icsb_modify_counters(mp,
>> 							msbp->msb_field,
>> 							msbp->msb_delta, 
>> 							rsvd);
>>+				s = XFS_SB_LOCK(mp);
>> 				break;
>> 			}
>> 			/* FALLTHROUGH */
>>
>>Is it safe to be releasing the SB_LOCK?
> 
> 
> Yes.
> 
> 
>>Is it assumed that the
>>superblock wont change while we process the list of xfs_mod_sb
>>structures?
> 
> 
> No. We are applying deltas - it doesn't matter if other deltas are
> applied at the same time by other callers because in the end all
> the deltas get applied and it adds up to the same thing.

Okay.

> 
> 
>>@@ -1515,11 +1517,12 @@ xfs_mod_incore_sb_batch(xfs_mount_t *mp,
>> 			case XFS_SBS_IFREE:
>> 			case XFS_SBS_FDBLOCKS:
>> 				if (!(mp->m_flags & XFS_MOUNT_NO_PERCPU_SB)) 
>> 				{
>>-					status =
>>-					    
>>xfs_icsb_modify_counters_locked(mp,
>>+					XFS_SB_UNLOCK(mp, s);
>>+					status = xfs_icsb_modify_counters(mp,
>> 							msbp->msb_field,
>> 							-(msbp->msb_delta),
>> 							rsvd);
>>+					s = XFS_SB_LOCK(mp);
>> 					break;
>> 				}
>> 				/* FALLTHROUGH */
>>
>>Same as above.
> 
> 
> Ditto ;)
> 
> Thanks for looking at this, Lachlan.
> 
> Cheers,
> 
> Dave.