* [PATCH] xfs/log: protect xc_cil in xlog_cil_push() @ 2019-10-30 6:29 Pingfan Liu 2019-10-30 12:53 ` Brian Foster 0 siblings, 1 reply; 9+ messages in thread From: Pingfan Liu @ 2019-10-30 6:29 UTC (permalink / raw) To: linux-xfs; +Cc: Pingfan Liu, Darrick J. Wong, linux-fsdevel 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); /* * initialise the new context and attach it to the CIL. Then attach -- 2.7.5 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] xfs/log: protect xc_cil in xlog_cil_push() 2019-10-30 6:29 [PATCH] xfs/log: protect xc_cil in xlog_cil_push() Pingfan Liu @ 2019-10-30 12:53 ` Brian Foster 2019-10-30 13:33 ` Pingfan Liu 0 siblings, 1 reply; 9+ messages in thread From: Brian Foster @ 2019-10-30 12:53 UTC (permalink / raw) To: Pingfan Liu; +Cc: linux-xfs, Darrick J. Wong, linux-fsdevel 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 > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] xfs/log: protect xc_cil in xlog_cil_push() 2019-10-30 12:53 ` Brian Foster @ 2019-10-30 13:33 ` Pingfan Liu 2019-10-30 13:37 ` [PATCH] xfs/log: protect the logging content under xc_ctx_lock Pingfan Liu 2019-10-31 21:25 ` [PATCH] xfs/log: protect xc_cil in xlog_cil_push() Dave Chinner 0 siblings, 2 replies; 9+ messages in thread From: Pingfan Liu @ 2019-10-30 13:33 UTC (permalink / raw) To: Brian Foster; +Cc: linux-xfs, Darrick J. Wong, linux-fsdevel 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] xfs/log: protect the logging content under xc_ctx_lock 2019-10-30 13:33 ` Pingfan Liu @ 2019-10-30 13:37 ` Pingfan Liu 2019-10-30 16:48 ` Darrick J. Wong 2019-10-31 21:40 ` Dave Chinner 2019-10-31 21:25 ` [PATCH] xfs/log: protect xc_cil in xlog_cil_push() Dave Chinner 1 sibling, 2 replies; 9+ messages in thread From: Pingfan Liu @ 2019-10-30 13:37 UTC (permalink / raw) To: linux-xfs; +Cc: Pingfan Liu, Darrick J. Wong, Brian Foster, linux-fsdevel 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 Survive this race issue by putting under the protection of xc_ctx_lock. Meanwhile the xc_cil_lock can be dropped as xc_ctx_lock does it against xlog_cil_insert_items() Signed-off-by: Pingfan Liu <kernelfans@gmail.com> Cc: "Darrick J. Wong" <darrick.wong@oracle.com> Cc: Brian Foster <bfoster@redhat.com> To: linux-xfs@vger.kernel.org Cc: linux-fsdevel@vger.kernel.org --- fs/xfs/xfs_log_cil.c | 35 +++++++++++++++++++---------------- 1 file changed, 19 insertions(+), 16 deletions(-) diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c index 004af09..f8df3b5 100644 --- a/fs/xfs/xfs_log_cil.c +++ b/fs/xfs/xfs_log_cil.c @@ -723,22 +723,6 @@ 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; - - item = list_first_entry(&cil->xc_cil, - struct xfs_log_item, li_cil); - list_del_init(&item->li_cil); - if (!ctx->lv_chain) - ctx->lv_chain = item->li_lv; - else - lv->lv_next = item->li_lv; - lv = item->li_lv; - 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 @@ -783,6 +767,25 @@ xlog_cil_push( up_write(&cil->xc_ctx_lock); /* + * cil->xc_cil_lock around this loop can be dropped, since xc_ctx_lock + * protects us against xlog_cil_insert_items(). + */ + while (!list_empty(&cil->xc_cil)) { + struct xfs_log_item *item; + + item = list_first_entry(&cil->xc_cil, + struct xfs_log_item, li_cil); + list_del_init(&item->li_cil); + if (!ctx->lv_chain) + ctx->lv_chain = item->li_lv; + else + lv->lv_next = item->li_lv; + lv = item->li_lv; + item->li_lv = NULL; + num_iovecs += lv->lv_niovecs; + } + + /* * Build a checkpoint transaction header and write it to the log to * begin the transaction. We need to account for the space used by the * transaction header here as it is not accounted for in xlog_write(). -- 2.7.5 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] xfs/log: protect the logging content under xc_ctx_lock 2019-10-30 13:37 ` [PATCH] xfs/log: protect the logging content under xc_ctx_lock Pingfan Liu @ 2019-10-30 16:48 ` Darrick J. Wong 2019-10-31 3:48 ` Pingfan Liu 2019-10-31 21:40 ` Dave Chinner 1 sibling, 1 reply; 9+ messages in thread From: Darrick J. Wong @ 2019-10-30 16:48 UTC (permalink / raw) To: Pingfan Liu; +Cc: linux-xfs, Brian Foster, linux-fsdevel 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 > > Survive this race issue by putting under the protection of xc_ctx_lock. > Meanwhile the xc_cil_lock can be dropped as xc_ctx_lock does it against > xlog_cil_insert_items() How did you trigger this race? Is there a test case to reproduce, or did you figure this out via code inspection? --D > > Signed-off-by: Pingfan Liu <kernelfans@gmail.com> > Cc: "Darrick J. Wong" <darrick.wong@oracle.com> > Cc: Brian Foster <bfoster@redhat.com> > To: linux-xfs@vger.kernel.org > Cc: linux-fsdevel@vger.kernel.org > --- > fs/xfs/xfs_log_cil.c | 35 +++++++++++++++++++---------------- > 1 file changed, 19 insertions(+), 16 deletions(-) > > diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c > index 004af09..f8df3b5 100644 > --- a/fs/xfs/xfs_log_cil.c > +++ b/fs/xfs/xfs_log_cil.c > @@ -723,22 +723,6 @@ 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; > - > - item = list_first_entry(&cil->xc_cil, > - struct xfs_log_item, li_cil); > - list_del_init(&item->li_cil); > - if (!ctx->lv_chain) > - ctx->lv_chain = item->li_lv; > - else > - lv->lv_next = item->li_lv; > - lv = item->li_lv; > - 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 > @@ -783,6 +767,25 @@ xlog_cil_push( > up_write(&cil->xc_ctx_lock); > > /* > + * cil->xc_cil_lock around this loop can be dropped, since xc_ctx_lock > + * protects us against xlog_cil_insert_items(). > + */ > + while (!list_empty(&cil->xc_cil)) { > + struct xfs_log_item *item; > + > + item = list_first_entry(&cil->xc_cil, > + struct xfs_log_item, li_cil); > + list_del_init(&item->li_cil); > + if (!ctx->lv_chain) > + ctx->lv_chain = item->li_lv; > + else > + lv->lv_next = item->li_lv; > + lv = item->li_lv; > + item->li_lv = NULL; > + num_iovecs += lv->lv_niovecs; > + } > + > + /* > * Build a checkpoint transaction header and write it to the log to > * begin the transaction. We need to account for the space used by the > * transaction header here as it is not accounted for in xlog_write(). > -- > 2.7.5 > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] xfs/log: protect the logging content under xc_ctx_lock 2019-10-30 16:48 ` Darrick J. Wong @ 2019-10-31 3:48 ` Pingfan Liu 0 siblings, 0 replies; 9+ messages in thread From: Pingfan Liu @ 2019-10-31 3:48 UTC (permalink / raw) To: Darrick J. Wong; +Cc: linux-xfs, Brian Foster, linux-fsdevel On Wed, Oct 30, 2019 at 09:48:25AM -0700, Darrick J. Wong 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 > > > > Survive this race issue by putting under the protection of xc_ctx_lock. > > Meanwhile the xc_cil_lock can be dropped as xc_ctx_lock does it against > > xlog_cil_insert_items() > > How did you trigger this race? Is there a test case to reproduce, or > did you figure this out via code inspection? > Via code inspection. To hit this bug, the condition is hard to meet: a broken transA is written to disk, then system encounters a failure before transB is written. Only if this happens, the recovery will bring us to a broken context. Regards, Pingfan ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] xfs/log: protect the logging content under xc_ctx_lock 2019-10-30 13:37 ` [PATCH] xfs/log: protect the logging content under xc_ctx_lock Pingfan Liu 2019-10-30 16:48 ` Darrick J. Wong @ 2019-10-31 21:40 ` Dave Chinner 2019-11-01 3:39 ` Pingfan Liu 1 sibling, 1 reply; 9+ messages in thread From: Dave Chinner @ 2019-10-31 21:40 UTC (permalink / raw) To: Pingfan Liu; +Cc: linux-xfs, Darrick J. Wong, Brian Foster, linux-fsdevel 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.... When CPU B modifies item1, it modifies the item and logs the new changes to the log item. It does not modify the log vector that might be attached to the log item from a previous change. The log vector is only updated during transaction commit, so the changes being made in transaction on CPU B are private to that transaction until they are committed, formatted into log vectors and inserted into the CIL under the CIL context lock. > Survive this race issue by putting under the protection of xc_ctx_lock. > Meanwhile the xc_cil_lock can be dropped as xc_ctx_lock does it against > xlog_cil_insert_items() > > Signed-off-by: Pingfan Liu <kernelfans@gmail.com> > Cc: "Darrick J. Wong" <darrick.wong@oracle.com> > Cc: Brian Foster <bfoster@redhat.com> > To: linux-xfs@vger.kernel.org > Cc: linux-fsdevel@vger.kernel.org > --- > fs/xfs/xfs_log_cil.c | 35 +++++++++++++++++++---------------- > 1 file changed, 19 insertions(+), 16 deletions(-) > > diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c > index 004af09..f8df3b5 100644 > --- a/fs/xfs/xfs_log_cil.c > +++ b/fs/xfs/xfs_log_cil.c > @@ -723,22 +723,6 @@ 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; > - > - item = list_first_entry(&cil->xc_cil, > - struct xfs_log_item, li_cil); > - list_del_init(&item->li_cil); > - if (!ctx->lv_chain) > - ctx->lv_chain = item->li_lv; > - else > - lv->lv_next = item->li_lv; > - lv = item->li_lv; > - 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 > @@ -783,6 +767,25 @@ xlog_cil_push( > up_write(&cil->xc_ctx_lock); ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ We don't hold the CIL context lock anymore.... > > /* > + * cil->xc_cil_lock around this loop can be dropped, since xc_ctx_lock > + * protects us against xlog_cil_insert_items(). > + */ > + while (!list_empty(&cil->xc_cil)) { > + struct xfs_log_item *item; > + > + item = list_first_entry(&cil->xc_cil, > + struct xfs_log_item, li_cil); > + list_del_init(&item->li_cil); > + if (!ctx->lv_chain) > + ctx->lv_chain = item->li_lv; > + else > + lv->lv_next = item->li_lv; > + lv = item->li_lv; > + item->li_lv = NULL; > + num_iovecs += lv->lv_niovecs; > + } So this is completely unserialised now. i.e. even if there was a problem like you suggest, this modification doesn't do what you say it does. Cheers, Dave. -- Dave Chinner david@fromorbit.com ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] xfs/log: protect the logging content under xc_ctx_lock 2019-10-31 21:40 ` Dave Chinner @ 2019-11-01 3:39 ` Pingfan Liu 0 siblings, 0 replies; 9+ messages in thread From: Pingfan Liu @ 2019-11-01 3:39 UTC (permalink / raw) To: Dave Chinner; +Cc: linux-xfs, Darrick J. Wong, Brian Foster, linux-fsdevel 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] xfs/log: protect xc_cil in xlog_cil_push() 2019-10-30 13:33 ` Pingfan Liu 2019-10-30 13:37 ` [PATCH] xfs/log: protect the logging content under xc_ctx_lock Pingfan Liu @ 2019-10-31 21:25 ` Dave Chinner 1 sibling, 0 replies; 9+ messages in thread From: Dave Chinner @ 2019-10-31 21:25 UTC (permalink / raw) To: Pingfan Liu; +Cc: Brian Foster, linux-xfs, Darrick J. Wong, linux-fsdevel 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-11-01 3:39 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-10-30 6:29 [PATCH] xfs/log: protect xc_cil in xlog_cil_push() Pingfan Liu 2019-10-30 12:53 ` Brian Foster 2019-10-30 13:33 ` Pingfan Liu 2019-10-30 13:37 ` [PATCH] xfs/log: protect the logging content under xc_ctx_lock Pingfan Liu 2019-10-30 16:48 ` Darrick J. Wong 2019-10-31 3:48 ` Pingfan Liu 2019-10-31 21:40 ` Dave Chinner 2019-11-01 3:39 ` Pingfan Liu 2019-10-31 21:25 ` [PATCH] xfs/log: protect xc_cil in xlog_cil_push() Dave Chinner
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).