Remove l_flushsema
Subsystems (2)
PATCH 1/1 Remove l_flushsema
The l_flushsema doesn't exactly have completion semantics, nor mutex semantics. It's used as a list of tasks which are waiting to be notified that a flush has completed. It was also being used in a way that was potentially racy, depending on the semaphore implementation.
By using a waitqueue instead of a semaphore we avoid the need for a separate counter, since we know we just need to wake everything on the queue.
Signature · 5 lines
I've only given this light testing, it could use some more. fs/xfs/xfs_log.c | 19 +++++++------------ fs/xfs/xfs_log_priv.h | 6 ++---- 2 files changed, 9 insertions(+), 16 deletions(-)
fs/xfs/xfs_log.c +7 -12
@@ -1228,7 +1228,7 @@ xlog_alloc_log(xfs_mount_t *mp,spin_lock_init (&log->l_icloglock);spin_lock_init (&log->l_grant_lock); -initnsema (&log->l_flushsema,0 ,"ic-flush" ); +init_waitqueue_head (&log->l_flush_wq);/* log record size must be multiple of BBSIZE; see xlog_rec_header_t */ ASSERT ((XFS_BUF_SIZE (bp) &BBMASK ) ==0 );
@@ -1573,7 +1573,6 @@ xlog_dealloc_log(xlog_t *log)kmem_free (iclog,sizeof (xlog_in_core_t)); iclog = next_iclog; } -freesema (&log->l_flushsema);spinlock_destroy (&log->l_icloglock);spinlock_destroy (&log->l_grant_lock);
@@ -2278,14 +2277,9 @@ xlog_state_do_callback( }#endif - flushcnt =0 ; -if (log->l_iclog->ic_state & (XLOG_STATE_ACTIVE |XLOG_STATE_IOERROR )) { - flushcnt = log->l_flushcnt; - log->l_flushcnt =0 ; - } +if (log->l_iclog->ic_state & (XLOG_STATE_ACTIVE |XLOG_STATE_IOERROR )) +wake_up_all (&log->l_flush_wq);spin_unlock (&log->l_icloglock); -while (flushcnt--) -vsema (&log->l_flushsema); }/* xlog_state_do_callback */
@@ -2385,12 +2379,13 @@ restart: iclog = log->l_iclog;if (! (iclog->ic_state ==XLOG_STATE_ACTIVE )) { - log->l_flushcnt++; +DEFINE_WAIT (wait); +prepare_to_wait (&log->l_flush_wq, &wait,TASK_UNINTERRUPTIBLE );spin_unlock (&log->l_icloglock);xlog_trace_iclog (iclog,XLOG_TRACE_SLEEP_FLUSH );XFS_STATS_INC (xs_log_noiclogs); -/* Ensure that log writes happen */ -psema (&log->l_flushsema,PINOD ); +/* Wait for log writes to have flushed */ +schedule (); goto restart; }ASSERT (iclog->ic_state ==XLOG_STATE_ACTIVE );
fs/xfs/xfs_log_priv.h +2 -4
@@ -423,10 +423,8 @@ typedef struct log {int l_logBBsize;/* size of log in BB chunks */ /* The following block of fields are changed while holding icloglock */ -sema_t l_flushsema____cacheline_aligned_in_smp ; -/* iclog flushing semaphore */ -int l_flushcnt;/* # of procs waiting on this -* sema */ +wait_queue_head_t l_flush_wq____cacheline_aligned_in_smp ; +/* waiting for iclog flush */ int l_covered_state;/* state of "covering disk * log entries" */ xlog_in_core_t *l_iclog;/* head log queue */
Signature · 4 lines
Intel are signing my paycheques ... these opinions are still mine "Bill, look, we understand that you're interested in selling us this operating system, but compare it to ours. We can't possibly take such a retrograde step."
Quoted 7 lines · Matthew Wilcox
fs/xfs/xfs_log.c +2
-1@@ -2385,12 +2379,13 @@ restart: iclog = log->l_iclog;if (! (iclog->ic_state ==XLOG_STATE_ACTIVE )) { - log->l_flushcnt++; +DEFINE_WAIT (wait); +prepare_to_wait (&log->l_flush_wq, &wait,TASK_UNINTERRUPTIBLE );
fs/xfs/xfs_log.c restart: · line 2391
Quoted 10 lines
fs/xfs/xfs_log.c +2
-2spin_unlock (&log->l_icloglock);xlog_trace_iclog (iclog,XLOG_TRACE_SLEEP_FLUSH );XFS_STATS_INC (xs_log_noiclogs); -/* Ensure that log writes happen */ -psema (&log->l_flushsema,PINOD ); +/* Wait for log writes to have flushed */ +schedule (); goto restart; }ASSERT (iclog->ic_state ==XLOG_STATE_ACTIVE );
Christoph points out that this is missing a call to finish_wait() after schedule() and he's absolutely right. It only matters if the task was woken up by something other than being on this wait queue, so my testing didn't notice it missing.
Signature · 4 lines
Intel are signing my paycheques ... these opinions are still mine "Bill, look, we understand that you're interested in selling us this operating system, but compare it to ours. We can't possibly take such a retrograde step."
Quoted 9 lines · Matthew Wilcox
The
l_flushsemadoesn't exactly have completion semantics, nor mutex semantics. It's used as a list of tasks which are waiting to be notified that a flush has completed. It was also being used in a way that was potentially racy, depending on the semaphore implementation.By using a waitqueue instead of a semaphore we avoid the need for a separate counter, since we know we just need to wake everything on the queue.
Looks good at first glance. thanks for doing this, Matthew. I've been swamped the last couple of days so I haven't had a chance to do this myself....
Quoted 5 lines
Signed-off-by Matthew Wilcox <[email protected]>Signature · 1 line
I've only given this light testing, it could use some more.
Yeah, I've pulled it into my qa tree so it'll get some shaking down. If it survives for a while, I'll push it into the xfs tree. One comment, though:
Quoted 4 lines
fs/xfs/xfs_log.c +0
-0@@ -2278,14 +2277,9 @@ xlog_state_do_callback( }#endif
fs/xfs/xfs_log.c xlog_state_do_callback( · line 2288
Quoted 10 lines
fs/xfs/xfs_log.c +2
-7- flushcnt =0 ; -if (log->l_iclog->ic_state & (XLOG_STATE_ACTIVE |XLOG_STATE_IOERROR )) { - flushcnt = log->l_flushcnt; - log->l_flushcnt =0 ; - } +if (log->l_iclog->ic_state & (XLOG_STATE_ACTIVE |XLOG_STATE_IOERROR )) +wake_up_all (&log->l_flush_wq);spin_unlock (&log->l_icloglock); -while (flushcnt--) -vsema (&log->l_flushsema);
The only thing that I'm concerned about here is that this will substantially increase the time the l_icloglock is held. This is a severely contended lock on large cpu count machines and putting the wakeup inside this lock will increase the hold time.
I guess I can address this by adding a new lock for the waitqueue in a separate patch set.
Hmmm - CONFIG_XFS_DEBUG builds break in the xfs-dev tree with this patch (in the xfs kdb module). I'll fix this up as well.
Cheers,
Dave.
Signature · 3 lines
Dave Chinner Principal Engineer SGI Australian Software Group
Quoted 7 lines · David Chinner
The only thing that I'm concerned about here is that this will substantially increase the time the
l_icloglockis held. This is a severely contended lock on large cpu count machines and putting the wakeup inside this lock will increase the hold time.I guess I can address this by adding a new lock for the waitqueue in a separate patch set.
waitqueues are loked internally and don't need synchronization. With a little bit of re-arranging the code the wake_up could probably be moved out of the critical section.
Quoted 6 lines · Christoph Hellwig
Quoted 5 lines · David Chinner
The only thing that I'm concerned about here is that this will substantially increase the time the
l_icloglockis held. This is a severely contended lock on large cpu count machines and putting the wakeup inside this lock will increase the hold time.
Quoted 6 lines
Quoted 2 lines
I guess I can address this by adding a new lock for the waitqueue in a separate patch set.
waitqueues are loked internally and don't need synchronization. With a little bit of re-arranging the code the
wake_upcould probably be moved out of the critical section.
Yeah, I just realised that myself and was about to reply as such....
I'll move the wakeup outside the lock.
Cheers,
Dave.
Signature · 3 lines
Dave Chinner Principal Engineer SGI Australian Software Group
Quoted 7 lines · David Chinner
Quoted 3 lines
waitqueues are loked internally and don't need synchronization. With a little bit of re-arranging the code the
wake_upcould probably be moved out of the critical section.Yeah, I just realised that myself and was about to reply as such....
I'll move the wakeup outside the lock.
Below is the version I have now. One of the rare cases where using sv_t actually cleans up the code (althoug the whole sv_ family should probably loose some arguments).
fs/xfs/xfs_log.c +13 -16
@@ -1228,7 +1228,7 @@ xlog_alloc_log(xfs_mount_t *mp,spin_lock_init (&log->l_icloglock);spin_lock_init (&log->l_grant_lock); -initnsema (&log->l_flushsema,0 ,"ic-flush" ); +sv_init (&log->l_flush_wait,0 ,"flush_wait" );/* log record size must be multiple of BBSIZE; see xlog_rec_header_t */ ASSERT ((XFS_BUF_SIZE (bp) &BBMASK ) ==0 );
@@ -1573,7 +1573,6 @@ xlog_dealloc_log(xlog_t *log)kmem_free (iclog,sizeof (xlog_in_core_t)); iclog = next_iclog; } -freesema (&log->l_flushsema);spinlock_destroy (&log->l_icloglock);spinlock_destroy (&log->l_grant_lock);
@@ -2097,6 +2096,7 @@ xlog_state_do_callback(int funcdidcallbacks;/* flag: function did callbacks */ int repeats;/* for issuing console warnings if * looping too many times */ +int wake =0 ;spin_lock (&log->l_icloglock); first_iclog = iclog = log->l_iclog;
@@ -2278,15 +2278,13 @@ xlog_state_do_callback( }#endif - flushcnt =0 ; -if (log->l_iclog->ic_state & (XLOG_STATE_ACTIVE |XLOG_STATE_IOERROR )) { - flushcnt = log->l_flushcnt; - log->l_flushcnt =0 ; - } +if (log->l_iclog->ic_state & (XLOG_STATE_ACTIVE |XLOG_STATE_IOERROR )) + wake =1 ;spin_unlock (&log->l_icloglock); -while (flushcnt--) -vsema (&log->l_flushsema); -}/* xlog_state_do_callback */ + +if (wake) +sv_broadcast (&log->l_flush_wait); +}/*
@@ -2384,16 +2382,15 @@ restart: } iclog = log->l_iclog; -if (! (iclog->ic_state ==XLOG_STATE_ACTIVE )) { - log->l_flushcnt++; -spin_unlock (&log->l_icloglock); +if (iclog->ic_state !=XLOG_STATE_ACTIVE ) {xlog_trace_iclog (iclog,XLOG_TRACE_SLEEP_FLUSH );XFS_STATS_INC (xs_log_noiclogs); -/* Ensure that log writes happen */ -psema (&log->l_flushsema,PINOD ); + +/* Wait for log writes to have flushed */ +sv_wait (&log->l_flush_wait,0 , &log->l_icloglock,0 ); goto restart; } -ASSERT (iclog->ic_state ==XLOG_STATE_ACTIVE ); + head = &iclog->ic_header;atomic_inc (&iclog->ic_refcnt);/* prevents sync */
fs/xfs/xfs_log_priv.h +2 -4
@@ -423,10 +423,8 @@ typedef struct log {int l_logBBsize;/* size of log in BB chunks */ /* The following block of fields are changed while holding icloglock */ -sema_t l_flushsema____cacheline_aligned_in_smp ; -/* iclog flushing semaphore */ -int l_flushcnt;/* # of procs waiting on this -* sema */ +sv_t l_flush_wait____cacheline_aligned_in_smp ; +/* waiting for iclog flush */ int l_covered_state;/* state of "covering disk * log entries" */ xlog_in_core_t *l_iclog;/* head log queue */
fs/xfs/xfsidbg.c +2 -2
@@ -5829,8 +5829,8 @@ xfsidbg_xlog(xlog_t *log) };kdb_printf ("xlog at 0x%p\n" , log); -kdb_printf ("&flushsm: 0x%p flushcnt: %d ICLOG: 0x%p \n" , - &log->l_flushsema, log->l_flushcnt, log->l_iclog); +kdb_printf ("&flush_wait: 0x%p ICLOG: 0x%p \n" , + &log->l_flush_wait, log->l_iclog);kdb_printf ("&icloglock: 0x%p tail_lsn: %s last_sync_lsn: %s \n" , &log->l_icloglock,xfs_fmtlsn (&log->l_tail_lsn),xfs_fmtlsn (&log->l_last_sync_lsn));
Quoted 5 lines · Christoph Hellwig
Quoted 4 lines · David Chinner
Quoted 3 lines
waitqueues are loked internally and don't need synchronization. With a little bit of re-arranging the code the
wake_upcould probably be moved out of the critical section.
Quoted 7 lines
Quoted 3 lines
Yeah, I just realised that myself and was about to reply as such....
I'll move the wakeup outside the lock.
Below is the version I have now. One of the rare cases where using
sv_tactually cleans up the code (althoug the wholesv_family should probably loose some arguments).
Yep, much cleaner. Who's signoff goes on this?
Cheers,
Dave.
Signature · 3 lines
Dave Chinner Principal Engineer SGI Australian Software Group
Quoted 3 lines · David Chinner
Quoted 1 line
probably loose some arguments).
Yep, much cleaner. Who's signoff goes on this?
You can have mine:
but I think it's till essentially willy's and he should be credited for it.
Quoted 11 lines · Christoph Hellwig
Quoted 3 lines · David Chinner
Quoted 1 line
probably loose some arguments).
Yep, much cleaner. Who's signoff goes on this?
You can have mine:
Signed-off-by Christoph Hellwig <[email protected]>but I think it's till essentially willy's and he should be credited for it.
I'm fine with adding my S-o-B to this version:
Here's a little twist on the idea to avoid the thundering herd. A vigorous review of this might not be a bad idea -- the idea is to only wake up sleeping processes when there seems to be enough space in the log to make it worthwhile. So there's a few places where we unlock the l_icloglock and jump back to restart; I didn't add an sv_signal there. But there should be an sv_signal before each exit from the function, and I think I've done that.
fs/xfs/xfs_log.c +7 -2
@@ -2282,7 +2282,7 @@ xlog_state_do_callback(if (log->l_iclog->ic_state & (XLOG_STATE_ACTIVE |XLOG_STATE_IOERROR )) wait =1 ;spin_unlock (&log->l_icloglock); -sv_broadcast (&log->l_flush_wait); +sv_signal (&log->l_flush_wait); }/* xlog_state_do_callback */
@@ -2377,6 +2377,7 @@ restart:spin_lock (&log->l_icloglock);if (XLOG_FORCED_SHUTDOWN (log)) {spin_unlock (&log->l_icloglock); +sv_signal (&log->l_flush_wait);return XFS_ERROR (EIO ); }
@@ -2425,8 +2426,11 @@ restart:/* If I'm the only one writing to this iclog, sync it to disk */ if (atomic_read (&iclog->ic_refcnt) ==1 ) {spin_unlock (&log->l_icloglock); -if ((error =xlog_state_release_iclog (log, iclog))) + error =xlog_state_release_iclog (log, iclog); +if (error) { +sv_signal (&log->l_flush_wait);return error; + } }else {atomic_dec (&iclog->ic_refcnt);spin_unlock (&log->l_icloglock);
@@ -2452,6 +2456,7 @@ restart:ASSERT (iclog->ic_offset <= iclog->ic_size);spin_unlock (&log->l_icloglock); +sv_signal (&log->l_flush_wait); *logoffsetp = log_offset;return 0 ;
Signature · 4 lines
Intel are signing my paycheques ... these opinions are still mine "Bill, look, we understand that you're interested in selling us this operating system, but compare it to ours. We can't possibly take such a retrograde step."
Quoted 17 lines · Matthew Wilcox
Quoted 11 lines · Christoph Hellwig
Quoted 3 lines · David Chinner
Quoted 1 line
probably loose some arguments).
Yep, much cleaner. Who's signoff goes on this?
You can have mine:
Signed-off-by Christoph Hellwig <[email protected]>but I think it's till essentially willy's and he should be credited for it.
I'm fine with adding my S-o-B to this version:
Signed-off-by Matthew Wilcox <[email protected]>
Quoted 7 lines
Here's a little twist on the idea to avoid the thundering herd. A vigorous review of this might not be a bad idea -- the idea is to only wake up sleeping processes when there seems to be enough space in the log to make it worthwhile. So there's a few places where we unlock the
l_icloglockand jump back to restart; I didn't add ansv_signalthere. But there should be ansv_signalbefore each exit from the function, and I think I've done that.
That might work. I'll have to look at it more detail later and do some performance testing when I'm not so busy with other stuff.
FWIW, in all the error or shutdown cases, it may as well be a broadcast as every subsequent process through this code will get the same error. i.e. once a log error occurs, the filesystem gets shut down....
Cheers,
Dave.
Signature · 3 lines
Dave Chinner Principal Engineer SGI Australian Software Group
Quoted 10 lines · David Chinner
Quoted 9 lines · Christoph Hellwig
Quoted 7 lines · David Chinner
The only thing that I'm concerned about here is that this will substantially increase the time the
l_icloglockis held. This is a severely contended lock on large cpu count machines and putting the wakeup inside this lock will increase the hold time.I guess I can address this by adding a new lock for the waitqueue in a separate patch set.
Quoted 7 lines
Quoted 3 lines
waitqueues are loked internally and don't need synchronization. With a little bit of re-arranging the code the
wake_upcould probably be moved out of the critical section.Yeah, I just realised that myself and was about to reply as such....
I'll move the wakeup outside the lock.
I can't tell whether this race matters ... probably not:
N processes come in and queue up waiting for the flush xlog_state_do_callback() is called
it unlocks the spinlock
a new task comes in and takes the spinlock wakeups happen
ie do we care about 'fairness' here, or is it OK for a new task to jump the queue?
Signature · 4 lines
Intel are signing my paycheques ... these opinions are still mine "Bill, look, we understand that you're interested in selling us this operating system, but compare it to ours. We can't possibly take such a retrograde step."
Quoted 21 lines · Matthew Wilcox
Quoted 10 lines · David Chinner
Quoted 9 lines · Christoph Hellwig
Quoted 7 lines · David Chinner
The only thing that I'm concerned about here is that this will substantially increase the time the
l_icloglockis held. This is a severely contended lock on large cpu count machines and putting the wakeup inside this lock will increase the hold time.I guess I can address this by adding a new lock for the waitqueue in a separate patch set.
Quoted 7 lines
Quoted 3 lines
waitqueues are loked internally and don't need synchronization. With a little bit of re-arranging the code the
wake_upcould probably be moved out of the critical section.Yeah, I just realised that myself and was about to reply as such....
I'll move the wakeup outside the lock.
I can't tell whether this race matters ... probably not:
Quoted 8 lines
N processes come in and queue up waiting for the flush
xlog_state_do_callback()is called
it unlocks the spinlock
a new task comes in and takes the spinlock wakeups happenie do we care about 'fairness' here, or is it OK for a new task to jump the queue?
This has always been a possibility here. However, this deep inside the log code I don't think it really matters because the waiters have log space reservations. In overload conditions, fairness is handled when obtaining a reservation via a ordered ticket queue (see xlog_grant_log_space()). Thundering herds tend to be thinned to smaller bursts by this queue, too...
Cheers,
Dave.
Signature · 3 lines
Dave Chinner Principal Engineer SGI Australian Software Group