[patch] Prevent AIL lock contention during transaction completion

[patch] Prevent AIL lock contention during transaction completion

[David Chinner]

When hundreds of processors attempt to commit
transactions at the same time, they can contend on the AIL
lock when updating the tail LSN held in the in-core log
structure.

At the moment, the tail LSN is only needed when actually writing
out an iclog, so it really does not need to be updated on every
single transaction completion - only those that result in switching
iclogs and flushing them to disk.

The result is that we reduce the number oftimes we need to grab the
AIL lock and the log grant lock by up to two orders of magnitude
on large processor count machines. The problem has previously been
hidden by AIL lock contention walking the AIL list, which has
recently been solved.

Signed-off-by: Dave Chinner <dgc@sgi.com>
---
 fs/xfs/xfs_log.c |   15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

Index: 2.6.x-xfs-new/fs/xfs/xfs_log.c
===================================================================
--- 2.6.x-xfs-new.orig/fs/xfs/xfs_log.c	2008-01-21 16:06:27.187549816 +1100
+++ 2.6.x-xfs-new/fs/xfs/xfs_log.c	2008-01-21 16:16:51.804146394 +1100
@@ -2815,15 +2815,13 @@ xlog_state_put_ticket(xlog_t	    *log,
  *
  */
 STATIC int
-xlog_state_release_iclog(xlog_t		*log,
-			 xlog_in_core_t	*iclog)
+xlog_state_release_iclog(
+	xlog_t		*log,
+	xlog_in_core_t	*iclog)
 {
 	int		sync = 0;	/* do we sync? */
 
-	xlog_assign_tail_lsn(log->l_mp);
-
 	spin_lock(&log->l_icloglock);
-
 	if (iclog->ic_state & XLOG_STATE_IOERROR) {
 		spin_unlock(&log->l_icloglock);
 		return XFS_ERROR(EIO);
@@ -2835,13 +2833,14 @@ xlog_state_release_iclog(xlog_t		*log,
 
 	if (--iclog->ic_refcnt == 0 &&
 	    iclog->ic_state == XLOG_STATE_WANT_SYNC) {
+		/* update tail before writing to iclog */
+		xlog_assign_tail_lsn(log->l_mp);
 		sync++;
 		iclog->ic_state = XLOG_STATE_SYNCING;
 		iclog->ic_header.h_tail_lsn = cpu_to_be64(log->l_tail_lsn);
 		xlog_verify_tail_lsn(log, iclog, log->l_tail_lsn);
 		/* cycle incremented when incrementing curr_block */
 	}
-
 	spin_unlock(&log->l_icloglock);
 
 	/*
@@ -2851,11 +2850,9 @@ xlog_state_release_iclog(xlog_t		*log,
 	 * this iclog has consistent data, so we ignore IOERROR
 	 * flags after this point.
 	 */
-	if (sync) {
+	if (sync)
 		return xlog_sync(log, iclog);
-	}
 	return 0;
-
 }	/* xlog_state_release_iclog */
 
 

Re: [patch] Prevent AIL lock contention during transaction completion

[Timothy Shimmin]

Hi Dave,

So all cosmetic except for moving of xlog_assign_tail_lsn().

Looking at the code the l_tail_lsn is used by more than just
when we are writing out the iclog.
Certainly, that is where we set the h_tail_lsn in the iclog
header, so we can find the tail later on during mount/recovery.

However, we also use l_tail_lsn when trying to work out
how much space is left in the log
i.e.
- xlog_space_left(), xlog_grant_push_tail(),
   xlog_grant_log_space(), xlog_regrant_write_log_space()

I guess this could mean that we may fail to update the l_tail_lsn
now if we don't sync the iclog (not in want-sync state etc..)
and so there could be more space
in the log than we realise until a bit later.
Maybe not a big deal.
Not sure if this really happens though or not.

Looking who assigns to l_tail_lsn (apart from initialisation
and recovery) we have xlog_assign_tail_lsn and xfs_log_move_tail.
And (apart from recovery) xlog_assign_tail_lsn is called by our
xlog_state_release_iclog.
So I presume the other place where we update the l_tail_lsn in
general is in calls to xfs_log_move_tail.
And xfs_log_move_tail is called by:
* xfs_trans_update_ail, xfs_trans_delete_ail,
   (xfs_trans_unlocked_item and xlog_ungrant_log_space who call
    xfs_log_move_tail call it with param 1 which doesn't modify
    l_tail_lsn)
I would have thought update_ail and delete_ail would cover the
changes to the ail and hence what the new min item in the ail list
is and hence the change in the tail.
In the case of an empty AIL, I guess it needs to use l_last_sync_lsn
which is what xlog_assign_tail_lsn gives you that xfs_log_move_tail
doesn't.

--Tim

David Chinner wrote:
> When hundreds of processors attempt to commit
> transactions at the same time, they can contend on the AIL
> lock when updating the tail LSN held in the in-core log
> structure.
> 
> At the moment, the tail LSN is only needed when actually writing
> out an iclog, so it really does not need to be updated on every
> single transaction completion - only those that result in switching
> iclogs and flushing them to disk.
> 
> The result is that we reduce the number oftimes we need to grab the
> AIL lock and the log grant lock by up to two orders of magnitude
> on large processor count machines. The problem has previously been
> hidden by AIL lock contention walking the AIL list, which has
> recently been solved.
> 
> Signed-off-by: Dave Chinner <dgc@sgi.com>
> ---
>  fs/xfs/xfs_log.c |   15 ++++++---------
>  1 file changed, 6 insertions(+), 9 deletions(-)
> 
> Index: 2.6.x-xfs-new/fs/xfs/xfs_log.c
> ===================================================================
> --- 2.6.x-xfs-new.orig/fs/xfs/xfs_log.c	2008-01-21 16:06:27.187549816 +1100
> +++ 2.6.x-xfs-new/fs/xfs/xfs_log.c	2008-01-21 16:16:51.804146394 +1100
> @@ -2815,15 +2815,13 @@ xlog_state_put_ticket(xlog_t	    *log,
>   *
>   */
>  STATIC int
> -xlog_state_release_iclog(xlog_t		*log,
> -			 xlog_in_core_t	*iclog)
> +xlog_state_release_iclog(
> +	xlog_t		*log,
> +	xlog_in_core_t	*iclog)
>  {
>  	int		sync = 0;	/* do we sync? */
>  
> -	xlog_assign_tail_lsn(log->l_mp);
> -
>  	spin_lock(&log->l_icloglock);
> -
>  	if (iclog->ic_state & XLOG_STATE_IOERROR) {
>  		spin_unlock(&log->l_icloglock);
>  		return XFS_ERROR(EIO);
> @@ -2835,13 +2833,14 @@ xlog_state_release_iclog(xlog_t		*log,
>  
>  	if (--iclog->ic_refcnt == 0 &&
>  	    iclog->ic_state == XLOG_STATE_WANT_SYNC) {
> +		/* update tail before writing to iclog */
> +		xlog_assign_tail_lsn(log->l_mp);
>  		sync++;
>  		iclog->ic_state = XLOG_STATE_SYNCING;
>  		iclog->ic_header.h_tail_lsn = cpu_to_be64(log->l_tail_lsn);
>  		xlog_verify_tail_lsn(log, iclog, log->l_tail_lsn);
>  		/* cycle incremented when incrementing curr_block */
>  	}
> -
>  	spin_unlock(&log->l_icloglock);
>  
>  	/*
> @@ -2851,11 +2850,9 @@ xlog_state_release_iclog(xlog_t		*log,
>  	 * this iclog has consistent data, so we ignore IOERROR
>  	 * flags after this point.
>  	 */
> -	if (sync) {
> +	if (sync)
>  		return xlog_sync(log, iclog);
> -	}
>  	return 0;
> -
>  }	/* xlog_state_release_iclog */
>  
>  

Re: [patch] Prevent AIL lock contention during transaction completion

[David Chinner]

On Wed, Jan 23, 2008 at 06:12:08PM +1100, Timothy Shimmin wrote:
> Hi Dave,
> 
> So all cosmetic except for moving of xlog_assign_tail_lsn().
> 
> Looking at the code the l_tail_lsn is used by more than just
> when we are writing out the iclog.
> Certainly, that is where we set the h_tail_lsn in the iclog
> header, so we can find the tail later on during mount/recovery.

Right, but the critical usage here is to update it before it
gets written into the iclog.

> However, we also use l_tail_lsn when trying to work out
> how much space is left in the log
> i.e.
> - xlog_space_left(), xlog_grant_push_tail(),
>   xlog_grant_log_space(), xlog_regrant_write_log_space()
> 
> I guess this could mean that we may fail to update the l_tail_lsn
> now if we don't sync the iclog (not in want-sync state etc..)
> and so there could be more space
> in the log than we realise until a bit later.
> Maybe not a big deal.
> Not sure if this really happens though or not.

All correct.

> 
> Looking who assigns to l_tail_lsn (apart from initialisation
> and recovery) we have xlog_assign_tail_lsn and xfs_log_move_tail.
> And (apart from recovery) xlog_assign_tail_lsn is called by our
> xlog_state_release_iclog.
> So I presume the other place where we update the l_tail_lsn in
> general is in calls to xfs_log_move_tail.

Correct.

> And xfs_log_move_tail is called by:
> * xfs_trans_update_ail, xfs_trans_delete_ail,
>   (xfs_trans_unlocked_item and xlog_ungrant_log_space who call
>    xfs_log_move_tail call it with param 1 which doesn't modify
>    l_tail_lsn)
> I would have thought update_ail and delete_ail would cover the
> changes to the ail and hence what the new min item in the ail list
> is and hence the change in the tail.

Right - the tail gets moved by (re)moving the tail object in the AIL

> In the case of an empty AIL, I guess it needs to use l_last_sync_lsn
> which is what xlog_assign_tail_lsn gives you that xfs_log_move_tail
> doesn't.

No, if a value of zero is passed to xfs_log_move_tail(), the lsn is
grabbed from l_last_sync_lsn, so when we remove the last object from
the AIL from xfs_trans_delete_ail() we set the l_tail_lsn to l_last_sync_lsn,
same as xlog_assign_tail_lsn does.

IOWs, xlog_assign_tail_lsn() could really be considered redundant as
the I/O completion keeps the l_tail_lsn up to date via the AIL manipulation
functions. I decided to split the difference and ensure that what is
written to the iclog does not change by calling it just before the
tail lsn is written into the header....

IOWs, I don't think calling xlog_assign_tail_lsn() in
xlog_state_release_iclog() ever changes the l_tail_lsn because
it is always kept up to date via the notifications in the AIL
code.....

Cheers,

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

Re: [patch] Prevent AIL lock contention during transaction completion

[Timothy Shimmin]

David Chinner & Tim wrote:
>> And xfs_log_move_tail is called by:
>> * xfs_trans_update_ail, xfs_trans_delete_ail,
>>   (xfs_trans_unlocked_item and xlog_ungrant_log_space who call
>>    xfs_log_move_tail call it with param 1 which doesn't modify
>>    l_tail_lsn)
>> I would have thought update_ail and delete_ail would cover the
>> changes to the ail and hence what the new min item in the ail list
>> is and hence the change in the tail.
> 
> Right - the tail gets moved by (re)moving the tail object in the AIL
> 
>> In the case of an empty AIL, I guess it needs to use l_last_sync_lsn
>> which is what xlog_assign_tail_lsn gives you that xfs_log_move_tail
>> doesn't.
> 
> No, if a value of zero is passed to xfs_log_move_tail(), the lsn is
> grabbed from l_last_sync_lsn, so when we remove the last object from
> the AIL from xfs_trans_delete_ail() we set the l_tail_lsn to l_last_sync_lsn,
> same as xlog_assign_tail_lsn does.
> 
Yep, sorry missed that bit (we do the same in both).

> IOWs, xlog_assign_tail_lsn() could really be considered redundant as
> the I/O completion keeps the l_tail_lsn up to date via the AIL manipulation
> functions. I decided to split the difference and ensure that what is
> written to the iclog does not change by calling it just before the
> tail lsn is written into the header....
> 
> IOWs, I don't think calling xlog_assign_tail_lsn() in
> xlog_state_release_iclog() ever changes the l_tail_lsn because
> it is always kept up to date via the notifications in the AIL
> code.....
> 
Exactly.
When looking, I just couldn't see what calling xlog_assign_tail_lsn() is
going to do for us considering every time we change the AIL (from
metadata io completions etc) we update the l_tail_lsn immediately.
Which led me to look at what differences there could be (and found
a wrong difference ;-).
So do we really need to call xlog_assign_tail_lsn() then?
Or are we just being conservative in case we missed something?

--Tim

Re: [patch] Prevent AIL lock contention during transaction completion

[David Chinner]

On Fri, Jan 25, 2008 at 05:51:33PM +1100, Timothy Shimmin wrote:
> So do we really need to call xlog_assign_tail_lsn() then?
> Or are we just being conservative in case we missed something?

Conservative - the last thing I want is to introduce a subtle
difference to the tail lsn in the log record because we didn't
update it immediately before writing it to disk. I think we are
probably safe removing it, but lets leave that until we got some
wider test coverage on this change first....

Cheers,

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

Re: [patch] Prevent AIL lock contention during transaction completion

[David Chinner]

On Fri, Jan 25, 2008 at 06:42:35PM +1100, David Chinner wrote:
> On Fri, Jan 25, 2008 at 05:51:33PM +1100, Timothy Shimmin wrote:
> > So do we really need to call xlog_assign_tail_lsn() then?
> > Or are we just being conservative in case we missed something?
> 
> Conservative - the last thing I want is to introduce a subtle
> difference to the tail lsn in the log record because we didn't
> update it immediately before writing it to disk. I think we are
> probably safe removing it, but lets leave that until we got some
> wider test coverage on this change first....

Tim - did you finish the review of this? Testing on the 2048p
machine appears to have been successful, so I'm just waiting
on review ACKs now....

Cheers,

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

Re: [patch] Prevent AIL lock contention during transaction completion

[Timothy Shimmin]

David Chinner wrote:
> On Fri, Jan 25, 2008 at 06:42:35PM +1100, David Chinner wrote:
>> On Fri, Jan 25, 2008 at 05:51:33PM +1100, Timothy Shimmin wrote:
>>> So do we really need to call xlog_assign_tail_lsn() then?
>>> Or are we just being conservative in case we missed something?
>> Conservative - the last thing I want is to introduce a subtle
>> difference to the tail lsn in the log record because we didn't
>> update it immediately before writing it to disk. I think we are
>> probably safe removing it, but lets leave that until we got some
>> wider test coverage on this change first....
> 
> Tim - did you finish the review of this? Testing on the 2048p
> machine appears to have been successful, so I'm just waiting
> on review ACKs now....
> 
> Cheers,
> 
> Dave.

Yep, that was an ACK.

--Tim