On Fri, Mar 02, 2018 at 09:35:27AM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@xxxxxxxxxx>
>
> Brian Foster reported an oops like:
>
> BUG: unable to handle kernel NULL pointer dereference at 0000000000000008
> IP: xlog_cil_push+0x184/0x400 [xfs]
> ....
> Workqueue: xfs-cil/dm-3 xlog_cil_push_work [xfs]
> RIP: 0010:xlog_cil_push+0x184/0x400 [xfs]
> ....
>
> This was caused by the log item size calculation return that there
> were no log vectors to write on a normal buffer. This should only
> occur for ordered buffers - the exception handling had never been
> executed for a non-ordered buffer. This mean we ended up with
> a log item marked dirty (XFS_LID_DIRTY) but with no log vectors
> attached that got added to the CIL. When the CIL was pushed, it
> tripped over the null log vector on the dirty log item when trying
> to chain the log vectors that needed to be written to the log.
>
> Fix this by clearing the XFS_LID_DIRTY flag on the log item if
> nothing gets formatted. This will prevent the log item from being
> added incorrectly to the CIL, and hence avoid the crash
Is there a test case for this, or just intermittent crashes?
> Reported-by: Brian Foster <bfoster@xxxxxxxxxx>
> Signed-Off-By: Dave Chinner <dchinner@xxxxxxxxxx>
> ---
> fs/xfs/xfs_log_cil.c | 27 ++++++++++++++++++++-------
> 1 file changed, 20 insertions(+), 7 deletions(-)
>
> diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
> index 61ab5c0a4c45..c1ecd7698100 100644
> --- a/fs/xfs/xfs_log_cil.c
> +++ b/fs/xfs/xfs_log_cil.c
> @@ -345,9 +345,16 @@ xlog_cil_insert_format_items(
> if (shadow->lv_buf_len == XFS_LOG_VEC_ORDERED)
> ordered = true;
>
> - /* Skip items that do not have any vectors for writing */
> - if (!shadow->lv_niovecs && !ordered)
> + /*
> + * Skip items that do not have any vectors for writing. These
> + * log items must now be considered clean in this transaction,
> + * so clear the XFS_LID_DIRTY flag to ensure it doesn't get
> + * added to the CIL by mistake.
> + */
> + if (!shadow->lv_niovecs && !ordered) {
> + lidp->lid_flags &= ~XFS_LID_DIRTY;
> continue;
> + }
>
> /* compare to existing item size */
> old_lv = lip->li_lv;
> @@ -486,6 +493,14 @@ xlog_cil_insert_items(
> if (!(lidp->lid_flags & XFS_LID_DIRTY))
> continue;
>
> + /*
> + * Log items added to the CIL must have a formatted log vector
> + * attached to them. This is a requirement of the log vector
> + * chaining used separate the log items from the changes that
^^^^^^^^^^^^^^^^^^
/me trips over this part of the comment, is this supposed to say
'...chaining using separate log items...' or '...used to separate the log
items...'?
Codewise I /think/ I understand what this is trying to do...
--D
> + * need to be written to the log in xlog_cil_push().
> + */
> + ASSERT(lip->li_lv);
> +
> /*
> * Only move the item if it isn't already at the tail. This is
> * to prevent a transient list_empty() state when reinserting
> @@ -655,6 +670,7 @@ xlog_cil_push(
> struct xfs_log_vec lvhdr = { NULL };
> xfs_lsn_t commit_lsn;
> xfs_lsn_t push_seq;
> + struct xfs_log_item *item;
>
> if (!cil)
> return 0;
> @@ -722,11 +738,8 @@ xlog_cil_push(
> */
> lv = NULL;
> num_iovecs = 0;
> - while (!list_empty(&cil->xc_cil)) {
> - struct xfs_log_item *item;
> -
> - item = list_first_entry(&cil->xc_cil,
> - struct xfs_log_item, li_cil);
> + while ((item = list_first_entry_or_null(&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;
> --
> 2.16.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
