| Message ID | 1572416980-25274-1-git-send-email-kernelfans@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xfs/log: protect xc_cil in xlog_cil_push() | expand |
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 >
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
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.
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
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(+)