On Fri, Jul 05, 2019 at 10:52:46AM -0400, Brian Foster wrote: > On Wed, Jun 26, 2019 at 01:47:10PM -0700, Darrick J. Wong wrote: > > From: Darrick J. Wong <darrick.wong@xxxxxxxxxx> > > > > The xattr scrubber functions use the temporary memory buffer either for > > storing bitmaps or for testing if attribute value extraction works. The > > bitmap code always zeroes what it needs and the value extraction merely > > sets the buffer contents (we never read the contents, we just look for > > return codes), so it's not necessary to waste CPU time zeroing on > > allocation. > > > > If we don't need to zero the buffer because we never look at the result, > that suggests we don't need to populate it in the first place right? We still need to read the attr value into the buffer (at least for remote attr values) because scrub doesn't otherwise check the remote attribute block header. We never read the contents (because the contents are just arbitrary bytes) but we do need to be able to catch an EFSCORRUPTED if, say, the attribute dabtree points at a corrupt block. > > A flame graph analysis showed that we were spending 7% of a xfs_scrub > > run (the whole program, not just the attr scrubber itself) allocating > > and zeroing 64k segments needlessly. > > > > How much does this patch help? About 1-2% I think. FWIW the "7%" figure represents the smallest improvement I saw in runtimes, where allocation ate 1-2% of the runtime and zeroing accounts for the rest (~5-6%). Practically speaking, when I retested with NVME flash instead of spinning rust then the improvement jumped to 15-20% overall. > > Signed-off-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx> > > --- > > fs/xfs/scrub/attr.c | 7 ++++++- > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > > > diff --git a/fs/xfs/scrub/attr.c b/fs/xfs/scrub/attr.c > > index 09081d8ab34b..d3a6f3dacf0d 100644 > > --- a/fs/xfs/scrub/attr.c > > +++ b/fs/xfs/scrub/attr.c > > @@ -64,7 +64,12 @@ xchk_setup_xattr_buf( > > sc->buf = NULL; > > } > > > > - ab = kmem_zalloc_large(sizeof(*ab) + sz, flags); > > + /* > > + * Allocate the big buffer. We skip zeroing it because that added 7% > > + * to the scrub runtime and all the users were careful never to read > > + * uninitialized contents. > > + */ > > Ok, that suggests the 7% hit was due to zeroing (where the commit log > says "allocating and zeroing"). Either way, we probably don't need such > details in the code. Can we tweak the comment to something like: > > /* > * Don't zero the buffer on allocation to avoid runtime overhead. All > * users must be careful never to read uninitialized contents. > */ Ok, I'll do that. Thanks for all the review! :) --D > > With that: > > Reviewed-by: Brian Foster <bfoster@xxxxxxxxxx> > > > + ab = kmem_alloc_large(sizeof(*ab) + sz, flags); > > if (!ab) > > return -ENOMEM; > > > >
