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

[Pingfan Liu <kernelfans@gmail.com>]

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