Re: [PATCH] xfs/log: protect xc_cil in xlog_cil_push()

[Dave Chinner <david@fromorbit.com>]

On Wed, Oct 30, 2019 at 09:33:27PM +0800, Pingfan Liu wrote:
> On Wed, Oct 30, 2019 at 08:53:16AM -0400, Brian Foster wrote:
> > On Wed, Oct 30, 2019 at 02:29:40PM +0800, Pingfan Liu wrote:
> > > xlog_cil_push() is the reader and writer of xc_cil, and should be protected
> > > against xlog_cil_insert_items().
> > > 
> > > Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
> > > Cc: "Darrick J. Wong" <darrick.wong@oracle.com>
> > > To: linux-xfs@vger.kernel.org
> > > Cc: linux-fsdevel@vger.kernel.org
> > > ---
> > >  fs/xfs/xfs_log_cil.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
> > > index ef652abd..004af09 100644
> > > --- a/fs/xfs/xfs_log_cil.c
> > > +++ b/fs/xfs/xfs_log_cil.c
> > > @@ -723,6 +723,7 @@ xlog_cil_push(
> > >  	 */
> > >  	lv = NULL;
> > >  	num_iovecs = 0;
> > > +	spin_lock(&cil->xc_cil_lock);
> > >  	while (!list_empty(&cil->xc_cil)) {
> > >  		struct xfs_log_item	*item;
> > >  
> > > @@ -737,6 +738,7 @@ xlog_cil_push(
> > >  		item->li_lv = NULL;
> > >  		num_iovecs += lv->lv_niovecs;
> > >  	}
> > > +	spin_unlock(&cil->xc_cil_lock);
> > 
> > The majority of this function executes under exclusive ->xc_ctx_lock.
> > xlog_cil_insert_items() runs with the ->xc_ctx_lock taken in read mode.
> > The ->xc_cil_lock spinlock is used in the latter case to protect the
> > list under concurrent transaction commits.
> > 
> I think the logic of xc_ctx_lock should be at a higher level of file
> system. But on the fundamental level, reader and writer should be
> protected against each other. And there is no protection for the list
> ops here.

Yes there is. The locking here is complex and unique, so takes some
understanding.

These are two different sets of operations that are being serialised
- high level operation is that transaction commits can run
concurrently (and must for performance), while CIL pushes must run
exclusively (for correctness).

So, yes, there is only one data structure we are accessing here and
it has two locks protecting it. They _nest_ to provide different
levels of exclusion: multiple producers vs single consumer via a
rwsem, and producer vs producer via a spin lock inside the shared
rwsem context. i.e.:

commit 1		commit 2		push

down_read(ctx_lock)
			down_read(ctx_lock)
						down_write(ctx_lock)
						<blocks>
spin_lock(cil_lock)
add to CIL		spin_lock(cil_lock)
			<spins>
spin_unlock(cil_lock)
			<gets cil_lock)
			add to CIL
up_read(ctx_lock)
			spin_unlock(cil_lock)
			up_read(ctx_lock)
						<gets ctx_lock>
						Processes CIL

As you can see, the CIL can only be accessed by a single thread at a
time, despite the fact there are multiple locks involved.

And the implied unlock->lock memory barriers ensure that list state
does not leak incorrectly between the different contexts....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com