All of lore.kernel.org
 help / color / mirror / Atom feed
From: Brian Foster <bfoster@redhat.com>
To: Mark Tinguely <tinguely@sgi.com>
Cc: xfs@oss.sgi.com
Subject: Re: [PATCH 5/5] xfs: fix cil push sequence after log recovery
Date: Mon, 7 Jul 2014 11:26:41 -0400	[thread overview]
Message-ID: <20140707152641.GC4123@laptop.bfoster> (raw)
In-Reply-To: <20140702144139.978978390@sgi.com>

On Wed, Jul 02, 2014 at 09:32:11AM -0500, Mark Tinguely wrote:
> The CIL pushes are marked complete with transaction
> tickets and should be in the the correct sequence order.
> The back end of the cil push code uses the ctx->commit_lsn
> to make sure all previous pushes are complete before adding
> the commit ticket for the current cil push. Because
> xlog_cil_init_post_recovery sets the ctx->commit_lsn,
> the later pushes can incorrectly think that the first
> sequence push is complete and allow out of order cil
> completion records to be written to the log. If the
> system crashes, the log will be replayed in the
> wrong order.
> 
> Signed-off-by: Mark Tinguely <tinguely@sgi.com>
> ---
>  fs/xfs/xfs_log_cil.c |    2 --
>  1 file changed, 2 deletions(-)
> 
> Index: b/fs/xfs/xfs_log_cil.c
> ===================================================================
> --- a/fs/xfs/xfs_log_cil.c
> +++ b/fs/xfs/xfs_log_cil.c
> @@ -78,8 +78,6 @@ xlog_cil_init_post_recovery(
>  {
>  	log->l_cilp->xc_ctx->ticket = xlog_cil_ticket_alloc(log);
>  	log->l_cilp->xc_ctx->sequence = 1;
> -	log->l_cilp->xc_ctx->commit_lsn = xlog_assign_lsn(log->l_curr_cycle,
> -								log->l_curr_block);

So we set ctx->commit_lsn here, this ctx is open for business and
sometime later it's committed. If a subsequent ctx is pushed before this
one has committed, commit_lsn is already set and thus the wait checks in
xlog_cil_push(), etc. are bypassed.

The fix seems logical to me, though I'm curious if there was some
original reason for setting commit_lsn here (it looks like this and the
xlog_wait() bits both go back to the original delayed logging commit).

It also seems that the dependence on l_curr_cycle and l_curr_block is
the only reason for the existence of this post-recovery function. Can we
move the ticket alloc and kill it if the commit_lsn assignment goes
away?

Brian

>  }
>  
>  /*
> 
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

      reply	other threads:[~2014-07-07 15:26 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-07-02 14:32 [PATCH 0/5] Misc controversial patches Mark Tinguely
2014-07-02 14:32 ` [PATCH 1/5] xfs: remove efi from AIL in log recovery Mark Tinguely
2014-07-07 14:30   ` Brian Foster
2014-07-07 15:29     ` Mark Tinguely
2014-07-07 23:44       ` Dave Chinner
2014-07-02 14:32 ` [PATCH 2/5] xfs: free the EFI entries from AIL on forced shutdown Mark Tinguely
2014-07-02 14:32 ` [PATCH 3/5] xfs: free the list of recovery items on error Mark Tinguely
2014-07-02 14:32 ` [PATCH 4/5] xfs: free inodes on log recovery error Mark Tinguely
2014-07-07 14:30   ` Brian Foster
2014-07-07 15:18     ` Mark Tinguely
2014-07-09  9:02   ` Christoph Hellwig
2014-07-02 14:32 ` [PATCH 5/5] xfs: fix cil push sequence after log recovery Mark Tinguely
2014-07-07 15:26   ` Brian Foster [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20140707152641.GC4123@laptop.bfoster \
    --to=bfoster@redhat.com \
    --cc=tinguely@sgi.com \
    --cc=xfs@oss.sgi.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.