Re: [PATCH 1/3] xfs: always do log forces via the workqueue

[Brian Foster <bfoster@redhat.com>]

On Sat, Feb 22, 2014 at 09:21:06AM +1100, Dave Chinner wrote:
> On Fri, Feb 21, 2014 at 10:04:37AM -0500, Brian Foster wrote:
> > On 02/19/2014 07:23 PM, Dave Chinner wrote:
> > > On Wed, Feb 19, 2014 at 01:24:54PM -0500, Brian Foster wrote:
> > >> On 02/18/2014 11:16 PM, Dave Chinner wrote:
> > >>> From: Dave Chinner <dchinner@redhat.com>
> > >>>
> > ...
...
> > 
> > General follow up question - what makes not taking xc_ctx_lock anywhere
> > in here safe in the first place? In the current implementation, if the
> > push has already been queued (note that we flush before we take the
> > spinlock and check the push sequence) and we get into the ctx wait
> > sequence, isn't it possible to see xc_committing before the ctx we're
> > pushing is even added?
> 
> The waiting is serialised on the push lock, not the context lock.
> 
> The context lock is used to serialise addition to a CIL context with
> the against the pushing of that sequence. Triggering a push of a CIL
> context does not need to be serialised addition to the CIL, nor
> directly against the push of the CIL. A blocking push needs to be
> serialised against the checkpoint of a CIL context to the iclog,
> which is a different thing altogether.
> 
> Hence we don't want to use the xc_ctx_lock for this - it is already
> a contended lock and we don't want to hold off commits into a new
> sequence while we wait for a previous sequence to finish pushing.
> 
> Yes, there are potential races in the exist code. They are fixed by
> this patch.
> 

Ok, thanks.

> > With this patch, what prevents us from seeing the updated
> > xc_current_sequence and thus skipping the restart (xc_current_sequence
> > isn't updated under the spinlock) before the pushed ctx has been added
> > to xc_committing?
> 
> The fact that the patch moves the xc_current_sequence update under
> the the push_lock avoids this. i.e. it is now only updated atomically
> with adding the context to the committing list. Both are now
> explicitly updated at the same time, so you can't see a sequence
> number greater than what you might find on the list...
> 

Ah, right. I was reading through your patch and the original code to
understand it better and lost the fact that you moved
xc_current_sequence under spinlock (e.g., my assumption above about it
not updated under lock is incorrect). That clears that up. Thanks for the
explanations.

Reviewed-by: Brian Foster <bfoster@redhat.com>

Brian

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com

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