Re: [PATCH 03/19] xfs: refactor log recovery item dispatch for pass2 readhead functions

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Sat, Apr 25, 2020 at 11:19:29AM -0700, Christoph Hellwig wrote:
> On Tue, Apr 21, 2020 at 07:06:21PM -0700, Darrick J. Wong wrote:
> >  typedef enum xlog_recover_reorder (*xlog_recover_reorder_fn)(
> >  		struct xlog_recover_item *item);
> > +typedef void (*xlog_recover_ra_pass2_fn)(struct xlog *log,
> > +		struct xlog_recover_item *item);
> 
> Same comment for this one - or the other two following.

<nod> I'll skip the typedefs.

> > +/*
> > + * This structure is used during recovery to record the buf log items which
> > + * have been canceled and should not be replayed.
> > + */
> > +struct xfs_buf_cancel {
> > +	xfs_daddr_t		bc_blkno;
> > +	uint			bc_len;
> > +	int			bc_refcount;
> > +	struct list_head	bc_list;
> > +};
> > +
> > +struct xfs_buf_cancel *xlog_peek_buffer_cancelled(struct xlog *log,
> > +		xfs_daddr_t blkno, uint len, unsigned short flags);
> 
> None of the callers moved in this patch even needs the xfs_buf_cancel
> structure, it just uses the return value as a boolean.  I think they
> all should be switched to use xlog_check_buffer_cancelled instead, which
> means that struct xfs_buf_cancel and xlog_peek_buffer_cancelled can stay
> private in xfs_log_recovery.c (and later xfs_buf_item.c).

They now all use your new xlog_buf_readahead function.

> > +STATIC void
> > +xlog_recover_buffer_ra_pass2(
> > +	struct xlog                     *log,
> > +	struct xlog_recover_item        *item)
> > +{
> > +	struct xfs_buf_log_format	*buf_f = item->ri_buf[0].i_addr;
> > +	struct xfs_mount		*mp = log->l_mp;
> > +
> > +	if (xlog_peek_buffer_cancelled(log, buf_f->blf_blkno,
> > +			buf_f->blf_len, buf_f->blf_flags)) {
> > +		return;
> > +	}
> > +
> > +	xfs_buf_readahead(mp->m_ddev_targp, buf_f->blf_blkno,
> > +				buf_f->blf_len, NULL);
> 
> Why not:
> 
> 	if (!xlog_peek_buffer_cancelled(log, buf_f->blf_blkno,
> 			buf_f->blf_len, buf_f->blf_flags)) {
> 		xfs_buf_readahead(mp->m_ddev_targp, buf_f->blf_blkno,
> 				buf_f->blf_len, NULL);
> 	}
> 
> > +STATIC void
> > +xlog_recover_dquot_ra_pass2(
> > +	struct xlog			*log,
> > +	struct xlog_recover_item	*item)
> > +{
> > +	struct xfs_mount	*mp = log->l_mp;
> > +	struct xfs_disk_dquot	*recddq;
> > +	struct xfs_dq_logformat	*dq_f;
> > +	uint			type;
> > +	int			len;
> > +
> > +
> > +	if (mp->m_qflags == 0)
> 
> Double empty line above.
> 
> > +	if (xlog_peek_buffer_cancelled(log, dq_f->qlf_blkno, len, 0))
> > +		return;
> > +
> > +	xfs_buf_readahead(mp->m_ddev_targp, dq_f->qlf_blkno, len,
> > +			  &xfs_dquot_buf_ra_ops);
> 
> Same comment as above, no real need for the early return here.
> 
> > +	if (xlog_peek_buffer_cancelled(log, ilfp->ilf_blkno, ilfp->ilf_len, 0))
> > +		return;
> > +
> > +	xfs_buf_readahead(mp->m_ddev_targp, ilfp->ilf_blkno,
> > +				ilfp->ilf_len, &xfs_inode_buf_ra_ops);
> 
> Here again.

Changed to xlog_buf_readahead.

> 
> > -	unsigned short			flags)
> > +	unsigned short		flags)
> 
> Spurious whitespace change.
> 
> >  		case XLOG_RECOVER_PASS2:
> > -			xlog_recover_ra_pass2(log, item);
> > +			if (item->ri_type && item->ri_type->ra_pass2_fn)
> > +				item->ri_type->ra_pass2_fn(log, item);
> 
> Shouldn't we ensure eatly on that we always have a valid ->ri_type?

Item sorting will bail out on unrecognized log item types, in which case
we will discard the transaction (and all its items) and abort the mount,
right?  That should suffice to ensure that we always have a valid
ri_type long before we get to starting readahead for pass 2.

--D