On Wed, Jun 26, 2019 at 01:47:04PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
>
> In examining a flame graph of time spent running xfs_scrub on various
> filesystems, I noticed that we spent nearly 7% of the total runtime on
> allocating a zeroed 65k buffer for every SCRUB_TYPE_XATTR invocation.
> We do this even if none of the attribute values were anywhere near 64k
> in size, even if there were no attribute blocks to check space on, and
> even if it just turns out there are no attributes at all.
>
> Therefore, rearrange the xattr buffer setup code to support reallocating
> with a bigger buffer and redistribute the callers of that function so
> that we only allocate memory just prior to needing it, and only allocate
> as much as we need. If we can't get memory with the ILOCK held we'll
> bail out with EDEADLOCK which will allocate the maximum memory.
>
> Signed-off-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> ---
> fs/xfs/scrub/attr.c | 67 ++++++++++++++++++++++++++++++++++++++++++++-------
> fs/xfs/scrub/attr.h | 6 ++++-
> 2 files changed, 63 insertions(+), 10 deletions(-)
>
>
> diff --git a/fs/xfs/scrub/attr.c b/fs/xfs/scrub/attr.c
> index c20b6da1db84..09081d8ab34b 100644
> --- a/fs/xfs/scrub/attr.c
> +++ b/fs/xfs/scrub/attr.c
...
> @@ -47,10 +53,23 @@ xchk_setup_xattr_buf(
> sz = 3 * sizeof(long) * BITS_TO_LONGS(sc->mp->m_attr_geo->blksize);
> sz = max_t(size_t, sz, value_size);
>
> - sc->buf = kmem_zalloc_large(sz, KM_SLEEP);
> - if (!sc->buf)
> + /*
> + * If there's already a buffer, figure out if we need to reallocate it
> + * to accomdate a larger size.
accommodate
Otherwise looks good:
Reviewed-by: Brian Foster <bfoster@xxxxxxxxxx>
> + */
> + if (ab) {
> + if (sz <= ab->sz)
> + return 0;
> + kmem_free(ab);
> + sc->buf = NULL;
> + }
> +
> + ab = kmem_zalloc_large(sizeof(*ab) + sz, flags);
> + if (!ab)
> return -ENOMEM;
>
> + ab->sz = sz;
> + sc->buf = ab;
> return 0;
> }
>
> @@ -62,9 +81,16 @@ xchk_setup_xattr(
> {
> int error;
>
> - error = xchk_setup_xattr_buf(sc, XATTR_SIZE_MAX);
> - if (error)
> - return error;
> + /*
> + * We failed to get memory while checking attrs, so this time try to
> + * get all the memory we're ever going to need. Allocate the buffer
> + * without the inode lock held, which means we can sleep.
> + */
> + if (sc->flags & XCHK_TRY_HARDER) {
> + error = xchk_setup_xattr_buf(sc, XATTR_SIZE_MAX, KM_SLEEP);
> + if (error)
> + return error;
> + }
>
> return xchk_setup_inode_contents(sc, ip, 0);
> }
> @@ -115,6 +141,19 @@ xchk_xattr_listent(
> return;
> }
>
> + /*
> + * Try to allocate enough memory to extrat the attr value. If that
> + * doesn't work, we overload the seen_enough variable to convey
> + * the error message back to the main scrub function.
> + */
> + error = xchk_setup_xattr_buf(sx->sc, valuelen, KM_MAYFAIL);
> + if (error == -ENOMEM)
> + error = -EDEADLOCK;
> + if (error) {
> + context->seen_enough = error;
> + return;
> + }
> +
> args.flags = ATTR_KERNOTIME;
> if (flags & XFS_ATTR_ROOT)
> args.flags |= ATTR_ROOT;
> @@ -128,7 +167,7 @@ xchk_xattr_listent(
> args.hashval = xfs_da_hashname(args.name, args.namelen);
> args.trans = context->tp;
> args.value = xchk_xattr_valuebuf(sx->sc);
> - args.valuelen = XATTR_SIZE_MAX;
> + args.valuelen = valuelen;
>
> error = xfs_attr_get_ilocked(context->dp, &args);
> if (error == -EEXIST)
> @@ -281,16 +320,26 @@ xchk_xattr_block(
> struct xfs_attr_leafblock *leaf = bp->b_addr;
> struct xfs_attr_leaf_entry *ent;
> struct xfs_attr_leaf_entry *entries;
> - unsigned long *usedmap = xchk_xattr_usedmap(ds->sc);
> + unsigned long *usedmap;
> char *buf_end;
> size_t off;
> __u32 last_hashval = 0;
> unsigned int usedbytes = 0;
> unsigned int hdrsize;
> int i;
> + int error;
>
> if (*last_checked == blk->blkno)
> return 0;
> +
> + /* Allocate memory for block usage checking. */
> + error = xchk_setup_xattr_buf(ds->sc, 0, KM_MAYFAIL);
> + if (error == -ENOMEM)
> + return -EDEADLOCK;
> + if (error)
> + return error;
> + usedmap = xchk_xattr_usedmap(ds->sc);
> +
> *last_checked = blk->blkno;
> bitmap_zero(usedmap, mp->m_attr_geo->blksize);
>
> diff --git a/fs/xfs/scrub/attr.h b/fs/xfs/scrub/attr.h
> index 27e879aeaafc..13a1d2e8424d 100644
> --- a/fs/xfs/scrub/attr.h
> +++ b/fs/xfs/scrub/attr.h
> @@ -10,6 +10,9 @@
> * Temporary storage for online scrub and repair of extended attributes.
> */
> struct xchk_xattr_buf {
> + /* Size of @buf, in bytes. */
> + size_t sz;
> +
> /*
> * Memory buffer -- either used for extracting attr values while
> * walking the attributes; or for computing attr block bitmaps when
> @@ -62,6 +65,7 @@ xchk_xattr_dstmap(
> BITS_TO_LONGS(sc->mp->m_attr_geo->blksize);
> }
>
> -int xchk_setup_xattr_buf(struct xfs_scrub *sc, size_t value_size);
> +int xchk_setup_xattr_buf(struct xfs_scrub *sc, size_t value_size,
> + xfs_km_flags_t flags);
>
> #endif /* __XFS_SCRUB_ATTR_H__ */
>
