On Thu, Jun 13, 2019 at 11:06:09AM -0700, Darrick J. Wong wrote:
> On Thu, Jun 13, 2019 at 12:27:06PM -0400, Brian Foster wrote:
> > On Tue, Jun 11, 2019 at 11:47:44PM -0700, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> > >
> > > Create a new iterator function to simplify walking inodes in an XFS
> > > filesystem. This new iterator will replace the existing open-coded
> > > walking that goes on in various places.
> > >
> > > Signed-off-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> > > ---
> > > fs/xfs/Makefile | 1
> > > fs/xfs/libxfs/xfs_ialloc_btree.c | 36 +++
> > > fs/xfs/libxfs/xfs_ialloc_btree.h | 3
> > > fs/xfs/xfs_itable.c | 5
> > > fs/xfs/xfs_itable.h | 8 +
> > > fs/xfs/xfs_iwalk.c | 418 ++++++++++++++++++++++++++++++++++++++
> > > fs/xfs/xfs_iwalk.h | 19 ++
> > > fs/xfs/xfs_trace.h | 40 ++++
> > > 8 files changed, 524 insertions(+), 6 deletions(-)
> > > create mode 100644 fs/xfs/xfs_iwalk.c
> > > create mode 100644 fs/xfs/xfs_iwalk.h
> > >
> > >
> > ...
> > > diff --git a/fs/xfs/xfs_iwalk.c b/fs/xfs/xfs_iwalk.c
> > > new file mode 100644
> > > index 000000000000..49289588413f
> > > --- /dev/null
> > > +++ b/fs/xfs/xfs_iwalk.c
> > > @@ -0,0 +1,418 @@
> > ...
> > > +/* Allocate memory for a walk. */
> > > +STATIC int
> > > +xfs_iwalk_alloc(
> > > + struct xfs_iwalk_ag *iwag)
> > > +{
> > > + size_t size;
> > > +
> > > + ASSERT(iwag->recs == NULL);
> > > + iwag->nr_recs = 0;
> > > +
> > > + /* Allocate a prefetch buffer for inobt records. */
> > > + size = iwag->sz_recs * sizeof(struct xfs_inobt_rec_incore);
> > > + iwag->recs = kmem_alloc(size, KM_MAYFAIL);
> > > + if (iwag->recs == NULL)
> > > + return -ENOMEM;
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +/* Free memory we allocated for a walk. */
> > > +STATIC void
> > > +xfs_iwalk_free(
> > > + struct xfs_iwalk_ag *iwag)
> > > +{
> > > + kmem_free(iwag->recs);
> >
> > It might be a good idea to ->recs = NULL here since the alloc call
> > asserts for that (if any future code happens to free and realloc the
> > recs buffer for whatever reason).
Fixed.
> > > +}
> > > +
> > ...
> > > +/* Walk all inodes in a single AG, from @iwag->startino to the end of the AG. */
> > > +STATIC int
> > > +xfs_iwalk_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);
> > > +
> > > + while (!error && has_more) {
> > > + 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;
> > > +
> > > + /* No allocated inodes in this chunk; skip it. */
> > > + if (irec->ir_freecount == irec->ir_count) {
> > > + error = xfs_btree_increment(cur, 0, &has_more);
> > > + if (error)
> > > + break;
> > > + continue;
> > > + }
> > > +
> > > + /*
> > > + * Start readahead for this inode chunk in anticipation of
> > > + * walking the inodes.
> > > + */
> > > + xfs_bulkstat_ichunk_ra(mp, agno, irec);
> > > +
> > > + /*
> > > + * 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.
> > > + */
> > > + has_more = true;
> >
> > has_more should always be true if we get here right? If so, perhaps
> > better to replace this with ASSERT(has_more).
Right; fixed.
> > > + error = xfs_iwalk_run_callbacks(iwag, agno, &cur, &agi_bp,
> > > + &has_more);
> > > + }
> > > +
> > > + if (iwag->nr_recs == 0 || error)
> > > + goto out;
> > > +
> > > + /* Walk the unprocessed records in the cache. */
> > > + error = xfs_iwalk_run_callbacks(iwag, agno, &cur, &agi_bp, &has_more);
> > > +
> > > +out:
> > > + xfs_iwalk_del_inobt(tp, &cur, &agi_bp, error);
> > > + return error;
> > > +}
> > > +
> > > +/*
> > > + * Given the number of inodes to prefetch, set the number of inobt records that
> > > + * we cache in memory, which controls the number of inodes we try to read
> > > + * ahead.
> > > + */
> > > +static inline void
> > > +xfs_iwalk_set_prefetch(
> > > + struct xfs_iwalk_ag *iwag,
> > > + unsigned int max_prefetch)
> > > +{
> > > + /*
> > > + * Default to 4096 bytes' worth of inobt records; this should be plenty
> > > + * of inodes to read ahead. This number was chosen so that the cache
> > > + * is never more than a single memory page and the amount of inode
> > > + * readahead is limited to to 16k inodes regardless of CPU:
> > > + *
> > > + * 4096 bytes / 16 bytes per inobt record = 256 inobt records
> > > + * 256 inobt records * 64 inodes per record = 16384 inodes
> > > + * 16384 inodes * 512 bytes per inode(?) = 8MB of inode readahead
> > > + */
> > > + iwag->sz_recs = 4096 / sizeof(struct xfs_inobt_rec_incore);
> > > +
> >
> > So we decided not to preserve current readahead behavior in this patch?
>
> I sent this patch before I received your reply. :(
...and hit send before replying to everything.
--D
> The current version of this patch restores the (4 * PAGE_SIZE) behavior,
> and a new patch immediately afterwards replaces it with better logic.
> "better" is where we allow prefetch up to 2048 inodes and use the
> (admittedly sparse) amount of information gathered so far about average
> inode chunk free factors to guess at how many inobt records to cache.
>
> > > + /*
> > > + * If the caller gives us a desired prefetch amount, round it up to
> > > + * an even inode chunk and cap it as defined previously.
> > > + */
> > > + if (max_prefetch) {
> > > + unsigned int nr;
> > > +
> > > + nr = round_up(max_prefetch, XFS_INODES_PER_CHUNK) /
> > > + XFS_INODES_PER_CHUNK;
> > > + iwag->sz_recs = min_t(unsigned int, iwag->sz_recs, nr);
> >
> > This is comparing the record count calculated above with max_prefetch,
> > which the rounding just above suggests is in inodes. BTW, could we add a
> > one line /* prefetch in inodes */ comment on the max_prefetch parameter
> > line at the top of the function?
>
> I renamed the parameter "inode_records", FWIW.
> >
> > Aside from those nits the rest looks good to me.
>
> <nod> Thanks for review!
>
> (Oh, more replies are slowly wandering in...)
>
> --D
>
> >
> > Brian
> >
> > > + }
> > > +
> > > + /*
> > > + * Allocate enough space to prefetch at least two records so that we
> > > + * can cache both the inobt record where the iwalk started and the next
> > > + * record. This simplifies the AG inode walk loop setup code.
> > > + */
> > > + iwag->sz_recs = max_t(unsigned int, iwag->sz_recs, 2);
> > > +}
> > > +
> > > +/*
> > > + * Walk all inodes in the filesystem starting from @startino. The @iwalk_fn
> > > + * will be called for each allocated inode, being passed the inode's number and
> > > + * @data. @max_prefetch controls how many inobt records' worth of inodes we
> > > + * try to readahead.
> > > + */
> > > +int
> > > +xfs_iwalk(
> > > + struct xfs_mount *mp,
> > > + struct xfs_trans *tp,
> > > + xfs_ino_t startino,
> > > + xfs_iwalk_fn iwalk_fn,
> > > + unsigned int max_prefetch,
> > > + void *data)
> > > +{
> > > + struct xfs_iwalk_ag iwag = {
> > > + .mp = mp,
> > > + .tp = tp,
> > > + .iwalk_fn = iwalk_fn,
> > > + .data = data,
> > > + .startino = startino,
> > > + };
> > > + 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);
> > > + error = xfs_iwalk_alloc(&iwag);
> > > + if (error)
> > > + return error;
> > > +
> > > + for (; agno < mp->m_sb.sb_agcount; agno++) {
> > > + error = xfs_iwalk_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
> > > new file mode 100644
> > > index 000000000000..9e762e31dadc
> > > --- /dev/null
> > > +++ b/fs/xfs/xfs_iwalk.h
> > > @@ -0,0 +1,19 @@
> > > +// SPDX-License-Identifier: GPL-2.0+
> > > +/*
> > > + * Copyright (C) 2019 Oracle. All Rights Reserved.
> > > + * Author: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> > > + */
> > > +#ifndef __XFS_IWALK_H__
> > > +#define __XFS_IWALK_H__
> > > +
> > > +/* Walk all inodes in the filesystem starting from @startino. */
> > > +typedef int (*xfs_iwalk_fn)(struct xfs_mount *mp, struct xfs_trans *tp,
> > > + xfs_ino_t ino, void *data);
> > > +/* Return values for xfs_iwalk_fn. */
> > > +#define XFS_IWALK_CONTINUE (XFS_ITER_CONTINUE)
> > > +#define XFS_IWALK_ABORT (XFS_ITER_ABORT)
> > > +
> > > +int xfs_iwalk(struct xfs_mount *mp, struct xfs_trans *tp, xfs_ino_t startino,
> > > + xfs_iwalk_fn iwalk_fn, unsigned int max_prefetch, void *data);
> > > +
> > > +#endif /* __XFS_IWALK_H__ */
> > > diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
> > > index 2464ea351f83..f9bb1d50bc0e 100644
> > > --- a/fs/xfs/xfs_trace.h
> > > +++ b/fs/xfs/xfs_trace.h
> > > @@ -3516,6 +3516,46 @@ DEFINE_EVENT(xfs_inode_corrupt_class, name, \
> > > DEFINE_INODE_CORRUPT_EVENT(xfs_inode_mark_sick);
> > > DEFINE_INODE_CORRUPT_EVENT(xfs_inode_mark_healthy);
> > >
> > > +TRACE_EVENT(xfs_iwalk_ag,
> > > + TP_PROTO(struct xfs_mount *mp, xfs_agnumber_t agno,
> > > + xfs_agino_t startino),
> > > + TP_ARGS(mp, agno, startino),
> > > + TP_STRUCT__entry(
> > > + __field(dev_t, dev)
> > > + __field(xfs_agnumber_t, agno)
> > > + __field(xfs_agino_t, startino)
> > > + ),
> > > + TP_fast_assign(
> > > + __entry->dev = mp->m_super->s_dev;
> > > + __entry->agno = agno;
> > > + __entry->startino = startino;
> > > + ),
> > > + TP_printk("dev %d:%d agno %d startino %u",
> > > + MAJOR(__entry->dev), MINOR(__entry->dev), __entry->agno,
> > > + __entry->startino)
> > > +)
> > > +
> > > +TRACE_EVENT(xfs_iwalk_ag_rec,
> > > + TP_PROTO(struct xfs_mount *mp, xfs_agnumber_t agno,
> > > + struct xfs_inobt_rec_incore *irec),
> > > + TP_ARGS(mp, agno, irec),
> > > + TP_STRUCT__entry(
> > > + __field(dev_t, dev)
> > > + __field(xfs_agnumber_t, agno)
> > > + __field(xfs_agino_t, startino)
> > > + __field(uint64_t, freemask)
> > > + ),
> > > + TP_fast_assign(
> > > + __entry->dev = mp->m_super->s_dev;
> > > + __entry->agno = agno;
> > > + __entry->startino = irec->ir_startino;
> > > + __entry->freemask = irec->ir_free;
> > > + ),
> > > + TP_printk("dev %d:%d agno %d startino %u freemask 0x%llx",
> > > + MAJOR(__entry->dev), MINOR(__entry->dev), __entry->agno,
> > > + __entry->startino, __entry->freemask)
> > > +)
> > > +
> > > #endif /* _TRACE_XFS_H */
> > >
> > > #undef TRACE_INCLUDE_PATH
> > >
