Page MenuHomeFreeBSD

Speed up vdev_geom_open_by_guids
ClosedPublic

Authored by asomers on Apr 29 2016, 10:53 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Nov 24, 10:53 AM
Unknown Object (File)
Thu, Nov 21, 11:07 AM
Unknown Object (File)
Thu, Nov 21, 11:07 AM
Unknown Object (File)
Thu, Nov 21, 11:07 AM
Unknown Object (File)
Thu, Nov 21, 11:07 AM
Unknown Object (File)
Thu, Nov 21, 11:07 AM
Unknown Object (File)
Thu, Nov 21, 11:06 AM
Unknown Object (File)
Thu, Nov 21, 10:44 AM
Subscribers

Details

Summary

Speed up vdev_geom_open_by_guids

Speedup is hard to measure because the only time vdev_geom_open_by_guids gets
called on many drives at the same time is during boot. But with vdev_geom_open
hacked to always call vdev_geom_open_by_guids, operations like "zpool create"
speed up by 65%.

sys/cddl/contrib/opensolaris/uts/common/fs/zfs/vdev_geom.c

  • Read all of a vdev's labels in parallel instead of sequentially.
  • In vdev_geom_read_config, don't read the entire label, including the
	  uberblock.  That's a waste of RAM.  Just read the vdev config nvlist.
	  Reduces the IO and RAM involved with tasting from 1MB to 448KB.
Test Plan

ZFS test suite

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
No Lint Coverage
Unit
No Test Coverage
Build Status
Buildable 3635
Build 3675: arc lint + arc unit

Event Timeline

asomers retitled this revision from to Speed up vdev_geom_open_by_guids.
asomers updated this object.
asomers edited the test plan for this revision. (Show Details)
asomers added reviewers: avg, mav, smh.

I'll commit this Friday morning if I don't get any feedback by then.

sys/cddl/contrib/opensolaris/uts/common/fs/zfs/vdev_geom.c
355

Use kmem_zalloc instead of kmem_alloc + bzero?

379

Spacing on =

asomers marked 2 inline comments as done.

Incorporate feedback from smh

sys/cddl/contrib/opensolaris/uts/common/fs/zfs/vdev_geom.c
368

Pretty sure this isn't needed given its a bzero under the covers and g_alloc_bio specifies M_ZERO.

375

Any reason why j++ isn't in the for?

430

This looks odd as error is loop invariant, bug?

asomers added inline comments.
sys/cddl/contrib/opensolaris/uts/common/fs/zfs/vdev_geom.c
368

Revision 295707 says we should always use g_reset_bio after g_alloc_bio because g_reset_bio might not always be bzero.

375

Nope.

430

Yes.

sys/cddl/contrib/opensolaris/uts/common/fs/zfs/vdev_geom.c
368

That sounds like a very odd change as double zeroing is quite an overhead, also doesn't correspond with what was committed as only existing bzero's where changed no addition of g_reset_bio after existing g_alloc_bio / g_new_bio.

If its required by those calls then they should do it, not rely on every single caller performing the two calls?

You can remove the g_reset_bio() call. It looks to be not needed since you aren't reusing bios and resetting them with bzero.

sys/cddl/contrib/opensolaris/uts/common/fs/zfs/vdev_geom.c
368

This call to g_reset_bios is bogus. You just allocated the bio, yo don't need to call it unless you *REUSE* the bio later.

asomers marked an inline comment as done.

Incorporate feedback from smh and imp

sys/cddl/contrib/opensolaris/uts/common/fs/zfs/vdev_geom.c
380

Do I correctly parse this line as returning an overall failure when at least one command fails?
I think that that makes the code less robust: if reading any single of 4 vdev labels fails for whatever reason (e.g. failing hardware), then we would ignore the successfully read labels.

sys/cddl/contrib/opensolaris/uts/common/fs/zfs/vdev_geom.c
380

I'm afraid that you're correct. It's not completely straightforward to fix since there isn't a 1-1 relationship between commands and bios. I'll try to come up with a fix.

make vdev_geom_io return separate errors for each command instead of a single
overall error.

avg edited edge metadata.

LGTM

This revision is now accepted and ready to land.May 17 2016, 8:47 AM
This revision was automatically updated to reflect the committed changes.