Re: [PATCH] xfs/log: protect the logging content under xc_ctx_lock

[Pingfan Liu <kernelfans@gmail.com>]

On Fri, Nov 01, 2019 at 08:40:31AM +1100, Dave Chinner wrote:
> On Wed, Oct 30, 2019 at 09:37:11PM +0800, Pingfan Liu wrote:
> > xc_cil_lock is not enough to protect the integrity of a trans logging.
> > Taking the scenario:
> >   cpuA                                 cpuB                          cpuC
> > 
> >   xlog_cil_insert_format_items()
> > 
> >   spin_lock(&cil->xc_cil_lock)
> >   link transA's items to xc_cil,
> >      including item1
> >   spin_unlock(&cil->xc_cil_lock)
> >                                                                       xlog_cil_push() fetches transA's item under xc_cil_lock
> >                                        issue transB, modify item1
> >                                                                       xlog_write(), but now, item1 contains content from transB and we have a broken transA
> 
> TL;DR: 1. log vectors. 2. CIL context lock exclusion.
> 
> When CPU A formats the item during commit, it copies all the changes
> into a list of log vectors, and that is attached to the log item
> and the item is added to the CIL. The item is then unlocked. This is
> done with the CIL context lock held excluding CIL pushes.
> 
> When CPU C pushes on the CIL, it detatches the -log vectors- from
> the log item and removes the item from the CIL. This is done hold
> the CIL context lock, excluding transaction commits from modifying
> the CIL log vector list. It then formats the -log vectors- into the
> journal by passing them to xlog_write().  It does not use log items
> for this, and because the log vector list has been isolated and is
> now private to the push context, we don't need to hold any locks
> anymore to call xlog_write....
Yes. I failed to realize it. The critical "item->li_lv = NULL" in
xlog_cil_push(), which isolates the vectors and free of new
modification even after releasing xc_ctx_lock.
[...]
> >  	 * initialise the new context and attach it to the CIL. Then attach
> > @@ -783,6 +767,25 @@ xlog_cil_push(
> >  	up_write(&cil->xc_ctx_lock);
>         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> We don't hold the CIL context lock anymore....
> 
Doze on it, make a mistaken reverse recognition of the up/down meaning.

Thank you for very patient and detailed explain. I get a full
understanding now.

Regards,
	Pingfan