Re: [PATCH 10/10] xfs: refactor INUMBERS to use iwalk functions

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

On Tue, Jun 11, 2019 at 11:08:51AM -0400, Brian Foster wrote:
> On Tue, Jun 04, 2019 at 02:50:33PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Now that we have generic functions to walk inode records, refactor the
> > INUMBERS implementation to use it.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  fs/xfs/xfs_ioctl.c   |   20 ++++--
> >  fs/xfs/xfs_ioctl.h   |    2 +
> >  fs/xfs/xfs_ioctl32.c |   35 ++++------
> >  fs/xfs/xfs_itable.c  |  168 ++++++++++++++++++++------------------------------
> >  fs/xfs/xfs_itable.h  |   22 +------
> >  fs/xfs/xfs_iwalk.c   |  161 ++++++++++++++++++++++++++++++++++++++++++++++--
> >  fs/xfs/xfs_iwalk.h   |   12 ++++
> 
> It looks like there's a decent amount of xfs_iwalk code changes in this
> patch. That should probably be a separate patch, at minimum to have a
> separate changelog to document the changes/updates required for
> inumbers.

<nod> I'll break out the iwalk changes into a separate patch so that
this one only has the changes needed to wire up the ioctl.

> >  7 files changed, 262 insertions(+), 158 deletions(-)
> > 
> > 
> ...
> > diff --git a/fs/xfs/xfs_itable.c b/fs/xfs/xfs_itable.c
> > index 06abe5c9c0ee..bade54d6ac64 100644
> > --- a/fs/xfs/xfs_itable.c
> > +++ b/fs/xfs/xfs_itable.c
> > @@ -259,121 +259,85 @@ xfs_bulkstat(
> >  	return error;
> >  }
> >  
> > -int
> > -xfs_inumbers_fmt(
> > -	void			__user *ubuffer, /* buffer to write to */
> > -	const struct xfs_inogrp	*buffer,	/* buffer to read from */
> > -	long			count,		/* # of elements to read */
> > -	long			*written)	/* # of bytes written */
> > +struct xfs_inumbers_chunk {
> > +	inumbers_fmt_pf		formatter;
> > +	struct xfs_ibulk	*breq;
> > +};
> > +
> > +/*
> > + * INUMBERS
> > + * ========
> > + * This is how we export inode btree records to userspace, so that XFS tools
> > + * can figure out where inodes are allocated.
> > + */
> > +
> > +/*
> > + * Format the inode group structure and report it somewhere.
> > + *
> > + * Similar to xfs_bulkstat_one_int, lastino is the inode cursor as we walk
> > + * through the filesystem so we move it forward unless there was a runtime
> > + * error.  If the formatter tells us the buffer is now full we also move the
> > + * cursor forward and abort the walk.
> > + */
> > +STATIC int
> > +xfs_inumbers_walk(
> > +	struct xfs_mount	*mp,
> > +	struct xfs_trans	*tp,
> > +	xfs_agnumber_t		agno,
> > +	const struct xfs_inobt_rec_incore *irec,
> > +	void			*data)
> >  {
> > -	if (copy_to_user(ubuffer, buffer, count * sizeof(*buffer)))
> > -		return -EFAULT;
> > -	*written = count * sizeof(*buffer);
> > -	return 0;
> > +	struct xfs_inogrp	inogrp = {
> > +		.xi_startino	= XFS_AGINO_TO_INO(mp, agno, irec->ir_startino),
> > +		.xi_alloccount	= irec->ir_count - irec->ir_freecount,
> > +		.xi_allocmask	= ~irec->ir_free,
> > +	};
> 
> Not related to this patch, but I'm wondering if we should be using
> xfs_inobt_irec_to_allocmask() here to mask off holes from the resulting
> allocation bitmap. Eh, I guess it's misleading either way..

Holes were supposed to be marked in ir_free also, right?

So (assuming the irec isn't corrupt) we should be protected against
reporting a hole as an "allocated" inode, right?

> > +	struct xfs_inumbers_chunk *ic = data;
> > +	xfs_agino_t		agino;
> > +	int			error;
> > +
> > +	error = ic->formatter(ic->breq, &inogrp);
> > +	if (error && error != XFS_IBULK_BUFFER_FULL)
> > +		return error;
> > +	if (error == XFS_IBULK_BUFFER_FULL)
> > +		error = XFS_INOBT_WALK_ABORT;
> > +
> > +	agino = irec->ir_startino + XFS_INODES_PER_CHUNK;
> > +	ic->breq->startino = XFS_AGINO_TO_INO(mp, agno, agino);
> > +	return error;
> >  }
> >  
> ...
> > diff --git a/fs/xfs/xfs_iwalk.c b/fs/xfs/xfs_iwalk.c
> > index c4a9c4c246b7..3a35d1cf7e14 100644
> > --- a/fs/xfs/xfs_iwalk.c
> > +++ b/fs/xfs/xfs_iwalk.c
> ...
> > @@ -286,7 +298,7 @@ xfs_iwalk_ag_start(
> >  	 * have to deal with tearing down the cursor to walk the records.
> >  	 */
> >  	error = xfs_iwalk_grab_ichunk(*curpp, agino, &icount,
> > -			&iwag->recs[iwag->nr_recs]);
> > +			&iwag->recs[iwag->nr_recs], trim);
> 
> Hmm, it looks like we could lift the lookup from xfs_iwalk_grab_ichunk()
> up into xfs_iwalk_ag_start() and avoid needing to pass trim down
> multiple levels. In fact, if we're not trimming the record we don't need
> to grab the record at all in this path. We could do the lookup (setting
> has_more) then bounce right up to the core walker algorithm, right? If
> so, that seems a bit cleaner in terms of only using special cased code
> when it's actually necessary.

Right.

> 
> >  	if (error)
> >  		return error;
> >  	if (icount)
> ...
> > @@ -561,3 +574,135 @@ xfs_iwalk_threaded(
> ...
> > +/*
> > + * Walk all inode btree records in a single AG, from @iwag->startino to the end
> > + * of the AG.
> > + */
> > +STATIC int
> > +xfs_inobt_walk_ag(
> > +	struct xfs_iwalk_ag		*iwag)
> > +{
> > +	struct xfs_mount		*mp = iwag->mp;
> > +	struct xfs_trans		*tp = iwag->tp;
> > +	struct xfs_buf			*agi_bp = NULL;
> > +	struct xfs_btree_cur		*cur = NULL;
> > +	xfs_agnumber_t			agno;
> > +	xfs_agino_t			agino;
> > +	int				has_more;
> > +	int				error = 0;
> > +
> > +	/* Set up our cursor at the right place in the inode btree. */
> > +	agno = XFS_INO_TO_AGNO(mp, iwag->startino);
> > +	agino = XFS_INO_TO_AGINO(mp, iwag->startino);
> > +	error = xfs_iwalk_ag_start(iwag, agno, agino, &cur, &agi_bp, &has_more,
> > +			false);
> > +
> > +	while (!error && has_more && !xfs_pwork_want_abort(&iwag->pwork)) {
> > +		struct xfs_inobt_rec_incore	*irec;
> > +
> > +		cond_resched();
> > +
> > +		/* Fetch the inobt record. */
> > +		irec = &iwag->recs[iwag->nr_recs];
> > +		error = xfs_inobt_get_rec(cur, irec, &has_more);
> > +		if (error || !has_more)
> > +			break;
> > +
> > +		/*
> > +		 * If there's space in the buffer for more records, increment
> > +		 * the btree cursor and grab more.
> > +		 */
> > +		if (++iwag->nr_recs < iwag->sz_recs) {
> > +			error = xfs_btree_increment(cur, 0, &has_more);
> > +			if (error || !has_more)
> > +				break;
> > +			continue;
> > +		}
> > +
> > +		/*
> > +		 * Otherwise, we need to save cursor state and run the callback
> > +		 * function on the cached records.  The run_callbacks function
> > +		 * is supposed to return a cursor pointing to the record where
> > +		 * we would be if we had been able to increment like above.
> > +		 */
> > +		error = xfs_iwalk_run_callbacks(iwag, xfs_inobt_walk_ag_recs,
> > +				agno, &cur, &agi_bp, &has_more);
> > +	}
> > +
> > +	xfs_iwalk_del_inobt(tp, &cur, &agi_bp, error);
> > +
> > +	/* Walk any records left behind in the cache. */
> > +	if (iwag->nr_recs == 0 || error || xfs_pwork_want_abort(&iwag->pwork))
> > +		return error;
> > +
> > +	return xfs_inobt_walk_ag_recs(iwag);
> > +}
> 
> Similar comments apply here as for the previous xfs_iwalk_ag() patch.
> Though looking at it, the only differences here are the lack of free
> inode check, readahead and the callback function (particularly once you
> consider the partial completion refactoring we discussed earlier). I
> think this could all be generalized with a single flag such that there's
> no need for separate xfs_[inobt|iwalk]_ag() functions.

Yep.  Already refactoring that. :)

> Hmmm.. perhaps we could use a flag or separate function pointers in
> struct xfs_iwalk_ag to accomplish the same thing all the way up through
> the record walker functions. IOW, xfs_iwalk_ag_recs() looks like it
> could call either callback based on which is defined in the
> xfs_iwalk_ag.

<nod>

> This could all be done as a separate patch of course, if that's easier.

I might just go back and remove the function pointer from run_callbacks
in the earlier patches...

> 
> > +
> > +/*
> > + * Walk all inode btree records in the filesystem starting from @startino.  The
> > + * @inobt_walk_fn will be called for each btree record, being passed the incore
> > + * record and @data.  @max_prefetch controls how many inobt records we try to
> > + * cache ahead of time.
> > + */
> > +int
> > +xfs_inobt_walk(
> > +	struct xfs_mount	*mp,
> > +	struct xfs_trans	*tp,
> > +	xfs_ino_t		startino,
> > +	xfs_inobt_walk_fn	inobt_walk_fn,
> > +	unsigned int		max_prefetch,
> > +	void			*data)
> > +{
> > +	struct xfs_iwalk_ag	iwag = {
> > +		.mp		= mp,
> > +		.tp		= tp,
> > +		.inobt_walk_fn	= inobt_walk_fn,
> > +		.data		= data,
> > +		.startino	= startino,
> > +		.pwork		= XFS_PWORK_SINGLE_THREADED,
> > +	};
> > +	xfs_agnumber_t		agno = XFS_INO_TO_AGNO(mp, startino);
> > +	int			error;
> > +
> > +	ASSERT(agno < mp->m_sb.sb_agcount);
> > +
> > +	xfs_iwalk_set_prefetch(&iwag, max_prefetch * XFS_INODES_PER_CHUNK);
> 
> A brief comment above this line would be helpful. Something like:
> 
> 	/* translate inumbers record count to inode count */

Done.  Thanks for the review!

--D

> Brian
> 
> > +	error = xfs_iwalk_alloc(&iwag);
> > +	if (error)
> > +		return error;
> > +
> > +	for (; agno < mp->m_sb.sb_agcount; agno++) {
> > +		error = xfs_inobt_walk_ag(&iwag);
> > +		if (error)
> > +			break;
> > +		iwag.startino = XFS_AGINO_TO_INO(mp, agno + 1, 0);
> > +	}
> > +
> > +	xfs_iwalk_free(&iwag);
> > +	return error;
> > +}
> > diff --git a/fs/xfs/xfs_iwalk.h b/fs/xfs/xfs_iwalk.h
> > index 76d8f87a39ef..20bee93d4676 100644
> > --- a/fs/xfs/xfs_iwalk.h
> > +++ b/fs/xfs/xfs_iwalk.h
> > @@ -18,4 +18,16 @@ int xfs_iwalk_threaded(struct xfs_mount *mp, xfs_ino_t startino,
> >  		xfs_iwalk_fn iwalk_fn, unsigned int max_prefetch, bool poll,
> >  		void *data);
> >  
> > +/* Walk all inode btree records in the filesystem starting from @startino. */
> > +typedef int (*xfs_inobt_walk_fn)(struct xfs_mount *mp, struct xfs_trans *tp,
> > +				 xfs_agnumber_t agno,
> > +				 const struct xfs_inobt_rec_incore *irec,
> > +				 void *data);
> > +/* Return value (for xfs_inobt_walk_fn) that aborts the walk immediately. */
> > +#define XFS_INOBT_WALK_ABORT	(XFS_IWALK_ABORT)
> > +
> > +int xfs_inobt_walk(struct xfs_mount *mp, struct xfs_trans *tp,
> > +		xfs_ino_t startino, xfs_inobt_walk_fn inobt_walk_fn,
> > +		unsigned int max_prefetch, void *data);
> > +
> >  #endif /* __XFS_IWALK_H__ */
> >