xfs/log: protect xc_cil in xlog_cil_push()
diff mbox series

Message ID 1572416980-25274-1-git-send-email-kernelfans@gmail.com
State New
Headers show
Series
  • xfs/log: protect xc_cil in xlog_cil_push()
Related show

Commit Message

Pingfan Liu Oct. 30, 2019, 6:29 a.m. UTC
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(+)

Comments

Brian Foster Oct. 30, 2019, 12:53 p.m. UTC | #1
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.

Brian

>  
>  	/*
>  	 * initialise the new context and attach it to the CIL. Then attach
> -- 
> 2.7.5
>
Pingfan Liu Oct. 30, 2019, 1:33 p.m. UTC | #2
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.

BTW, even spinlock is not enough to protect integrity of a trans, and I
think another patch should be involved. I will send the extra patch,
which is applied on this one.

Thanks for your kindly review.

Regards,
	Pingfan
Dave Chinner Oct. 31, 2019, 9:25 p.m. UTC | #3
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.

Patch
diff mbox series

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);
 
 	/*
 	 * initialise the new context and attach it to the CIL. Then attach