Re: [PATCH 4/5] xfs: update metadata LSN in buffers during log recovery

On Mon, Aug 29, 2016 at 11:29:23AM +1000, Dave Chinner wrote:
> On Thu, Aug 11, 2016 at 01:11:06PM -0400, Brian Foster wrote:
> > @@ -2552,6 +2562,27 @@ xlog_recover_validate_buf_type(
> >  		xfs_warn(mp, warnmsg);
> >  		ASSERT(0);
> >  	}
> > +
> > +	/*
> > +	 * We must update the metadata LSN of the buffer as it is written out to
> > +	 * ensure that older transactions never replay over this one and corrupt
> > +	 * the buffer. This can occur if log recovery is interrupted at some
> > +	 * point after the current transaction completes, at which point a
> > +	 * subsequent mount starts recovery from the beginning.
> > +	 *
> > +	 * Write verifiers update the metadata LSN from log items attached to
> > +	 * the buffer. Therefore, initialize a bli purely to carry the LSN to
> > +	 * the verifier. We'll clean it up in our ->iodone() callback.
> > +	 */
> > +	if (bp->b_ops && current_lsn != NULLCOMMITLSN) {
> > +		struct xfs_buf_log_item	*bip;
> > +
> > +		ASSERT(!bp->b_iodone || bp->b_iodone == xlog_recover_iodone);
> > +		bp->b_iodone = xlog_recover_iodone;
> > +		xfs_buf_item_init(bp, mp);
> > +		bip = bp->b_fspriv;
> > +		bip->bli_item.li_lsn = current_lsn;
> > +	}
> >  }
> 
> Of, so now we have two things we do when current_lsn !=
> NULLCOMMITLSN. I'd change this to something like:
> 
> 
> 	ASSERT(bp->b_fspriv == NULL);
> 	if (current_lsn == NULLCOMMITLSN)
> 		return;
> 
> 	if (warn) {
> 		....
> 	}
> 
> 	if (!bp->b_ops)
> 		return
> 
> 	/* add buf_item */

Ok, I may still invert the bp->b_ops check as that seems more clear to
me for whatever reason, but otherwise that looks like a nice cleanup.
Thanks.

Brian

> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> [email protected]
> 
> _______________________________________________
> xfs mailing list
> [email protected]
> http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs