On Tue, Aug 26, 2014 at 11:21:38AM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@xxxxxxxxxx>
>
> Clean up xlog_recover_process_data() structure in preparation for
> fixing the allocationa nd freeing context of the transaction being
> recovered.
>
> Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
> ---
> fs/xfs/xfs_log_recover.c | 151 ++++++++++++++++++++++++++---------------------
> 1 file changed, 84 insertions(+), 67 deletions(-)
>
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index 01becbb..1970732f 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -3531,12 +3531,78 @@ out:
> }
>
> STATIC int
> -xlog_recover_unmount_trans(
> - struct xlog *log)
> +xlog_recovery_process_ophdr(
> + struct xlog *log,
> + struct hlist_head rhash[],
> + struct xlog_rec_header *rhead,
> + struct xlog_op_header *ohead,
> + xfs_caddr_t dp,
> + xfs_caddr_t lp,
> + int pass)
> {
> - /* Do nothing now */
> - xfs_warn(log->l_mp, "%s: Unmount LR", __func__);
> - return 0;
> + struct xlog_recover *trans;
> + xlog_tid_t tid;
> + int error;
> + unsigned long hash;
> + uint flags;
> + unsigned int hlen;
> +
> + hlen = be32_to_cpu(ohead->oh_len);
> + tid = be32_to_cpu(ohead->oh_tid);
> + hash = XLOG_RHASH(tid);
> + trans = xlog_recover_find_tid(&rhash[hash], tid);
> + if (!trans) {
> + /* add new tid if this is a new transaction */
> + if (ohead->oh_flags & XLOG_START_TRANS) {
> + xlog_recover_new_tid(&rhash[hash], tid,
> + be64_to_cpu(rhead->h_lsn));
> + }
> + return 0;
> + }
> +
Overall this looks pretty good to me. I wonder if we can clean this up
to separate state management from error detection while we're at it. I
don't see any reason this code to find trans has to be up here.
> + error = -EIO;
> + if (dp + hlen > lp) {
> + xfs_warn(log->l_mp, "%s: bad length 0x%x", __func__, hlen);
> + WARN_ON(1);
> + goto out_free;
> + }
> +
> + flags = ohead->oh_flags & ~XLOG_END_TRANS;
> + if (flags & XLOG_WAS_CONT_TRANS)
> + flags &= ~XLOG_CONTINUE_TRANS;
> +
/* we should find a trans for anything other than a start op */
trans = xlog_recover_find_tid(&rhash[hash], tid);
if (((ohead->oh_flags & XLOG_START_TRANS) && trans) ||
(!(ohead->oh_flags & XLOG_START_TRANS) && !trans)) {
xfs_warn(log->l_mp, "%s: bad transaction 0x%x oh_flags 0x%x trans %p",
__func__, tid, ohead->oh_flags, trans);
ASSERT(0);
return -EIO;
}
Maybe returning error here is not the right thing to do because we want
the recovery to proceed. We could still dump a warning and return 0
though.
> + switch (flags) {
> + /* expected flag values */
> + case 0:
> + case XLOG_CONTINUE_TRANS:
> + error = xlog_recover_add_to_trans(log, trans, dp, hlen);
> + break;
> + case XLOG_WAS_CONT_TRANS:
> + error = xlog_recover_add_to_cont_trans(log, trans, dp, hlen);
> + break;
> + case XLOG_COMMIT_TRANS:
> + error = xlog_recover_commit_trans(log, trans, pass);
> + break;
> +
> + /* unexpected flag values */
> + case XLOG_UNMOUNT_TRANS:
> + xfs_warn(log->l_mp, "%s: Unmount LR", __func__);
> + error = 0;
> + break;
> + case XLOG_START_TRANS:
> + xfs_warn(log->l_mp, "%s: bad transaction 0x%x", __func__, tid);
> + ASSERT(0);
> + break;
xlog_recover_new_tid(&rhash[hash], tid, be64_to_cpu(rhead->h_lsn)
error = 0;
break;
Brian
> + default:
> + xfs_warn(log->l_mp, "%s: bad flag 0x%x", __func__, flags);
> + ASSERT(0);
> + break;
> + }
> +
> +out_free:
> + if (error)
> + xlog_recover_free_trans(trans);
> + return error;
> }
>
> /*
> @@ -3556,14 +3622,10 @@ xlog_recover_process_data(
> xfs_caddr_t dp,
> int pass)
> {
> + struct xlog_op_header *ohead;
> xfs_caddr_t lp;
> int num_logops;
> - xlog_op_header_t *ohead;
> - xlog_recover_t *trans;
> - xlog_tid_t tid;
> int error;
> - unsigned long hash;
> - uint flags;
>
> lp = dp + be32_to_cpu(rhead->h_len);
> num_logops = be32_to_cpu(rhead->h_num_logops);
> @@ -3573,69 +3635,24 @@ xlog_recover_process_data(
> return -EIO;
>
> while ((dp < lp) && num_logops) {
> - ASSERT(dp + sizeof(xlog_op_header_t) <= lp);
> - ohead = (xlog_op_header_t *)dp;
> - dp += sizeof(xlog_op_header_t);
> + ASSERT(dp + sizeof(struct xlog_op_header) <= lp);
> +
> + ohead = (struct xlog_op_header *)dp;
> + dp += sizeof(*ohead);
> +
> if (ohead->oh_clientid != XFS_TRANSACTION &&
> ohead->oh_clientid != XFS_LOG) {
> xfs_warn(log->l_mp, "%s: bad clientid 0x%x",
> - __func__, ohead->oh_clientid);
> + __func__, ohead->oh_clientid);
> ASSERT(0);
> return -EIO;
> }
> - tid = be32_to_cpu(ohead->oh_tid);
> - hash = XLOG_RHASH(tid);
> - trans = xlog_recover_find_tid(&rhash[hash], tid);
> - if (trans == NULL) { /* not found; add new tid */
> - if (ohead->oh_flags & XLOG_START_TRANS)
> - xlog_recover_new_tid(&rhash[hash], tid,
> - be64_to_cpu(rhead->h_lsn));
> - } else {
> - if (dp + be32_to_cpu(ohead->oh_len) > lp) {
> - xfs_warn(log->l_mp, "%s: bad length 0x%x",
> - __func__, be32_to_cpu(ohead->oh_len));
> - WARN_ON(1);
> - return -EIO;
> - }
> - flags = ohead->oh_flags & ~XLOG_END_TRANS;
> - if (flags & XLOG_WAS_CONT_TRANS)
> - flags &= ~XLOG_CONTINUE_TRANS;
> - switch (flags) {
> - case XLOG_COMMIT_TRANS:
> - error = xlog_recover_commit_trans(log,
> - trans, pass);
> - break;
> - case XLOG_UNMOUNT_TRANS:
> - error = xlog_recover_unmount_trans(log);
> - break;
> - case XLOG_WAS_CONT_TRANS:
> - error = xlog_recover_add_to_cont_trans(log,
> - trans, dp,
> - be32_to_cpu(ohead->oh_len));
> - break;
> - case XLOG_START_TRANS:
> - xfs_warn(log->l_mp, "%s: bad transaction",
> - __func__);
> - ASSERT(0);
> - error = -EIO;
> - break;
> - case 0:
> - case XLOG_CONTINUE_TRANS:
> - error = xlog_recover_add_to_trans(log, trans,
> - dp, be32_to_cpu(ohead->oh_len));
> - break;
> - default:
> - xfs_warn(log->l_mp, "%s: bad flag 0x%x",
> - __func__, flags);
> - ASSERT(0);
> - error = -EIO;
> - break;
> - }
> - if (error) {
> - xlog_recover_free_trans(trans);
> - return error;
> - }
> - }
> +
> + error = xlog_recovery_process_ophdr(log, rhash, rhead, ohead,
> + dp, lp, pass);
> + if (error)
> + return error;
> +
> dp += be32_to_cpu(ohead->oh_len);
> num_logops--;
> }
> --
> 2.0.0
>
> _______________________________________________
> xfs mailing list
> xfs@xxxxxxxxxxx
> http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
