review request: 5027 zfs large block support

Discussion:

Matthew Ahrens via illumos-zfs

2014-07-21 04:34:59 UTC

https://reviews.csiden.org/r/51/diff/#

Note: this change is not part of the Delphix product, and therefore has had
less rigorous testing than most of the changes I submit. I would
appreciate a thorough code review and also any testing others can offer.
Diffs can be downloaded via the link above, or from
https://github.com/ahrens/illumos/tree/largeblock

--matt

-------------------------------------------
illumos-zfs
Archives: https://www.listbox.com/member/archive/182191/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182191/23047029-187a0c8d
Modify Your Subscription: https://www.listbox.com/member/?member_id=23047029&id_secret=23047029-2e85923f
Powered by Listbox: http://www.listbox.com

Matthew Ahrens via illumos-zfs

2014-09-04 04:58:43 UTC

Permalink

Ping for additional reviewers or testing?

--matt

Post by Matthew Ahrens via illumos-zfs
https://reviews.csiden.org/r/51/diff/#
Note: this change is not part of the Delphix product, and therefore has
had less rigorous testing than most of the changes I submit. I would
appreciate a thorough code review and also any testing others can offer.
Diffs can be downloaded via the link above, or from
https://github.com/ahrens/illumos/tree/largeblock
--matt

-------------------------------------------
illumos-zfs
Archives: https://www.listbox.com/member/archive/182191/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182191/23047029-187a0c8d
Modify Your Subscription: https://www.listbox.com/member/?member_id=23047029&id_secret=23047029-2e85923f
Powered by Listbox: http://www.listbox.com

Richard Elling

2014-09-08 15:08:57 UTC

Permalink

HI Matt,
I don't see anything of concern in the approach or the code. But it is unclear
to me if it has been tested to 16MB in ztest or the zfs test suite?

-- richard

Post by Matthew Ahrens via illumos-zfs
https://reviews.csiden.org/r/51/diff/#
Note: this change is not part of the Delphix product, and therefore has had less rigorous testing than most of the changes I submit. I would appreciate a thorough code review and also any testing others can offer. Diffs can be downloaded via the link above, or from https://github.com/ahrens/illumos/tree/largeblock
--matt
_______________________________________________
developer mailing list
http://lists.open-zfs.org/mailman/listinfo/developer

--

Richard.Elling-Za1XIjzIzjxBQ5RWgiF5GgC/***@public.gmane.org
+1-760-896-4422

Alek Pinchuk via illumos-zfs

2014-09-10 18:24:38 UTC

Permalink

Hello Matt,

This looks good to me.

-Alek

On Mon, Sep 8, 2014 at 8:08 AM, Richard Elling

Post by Richard Elling
HI Matt,
I don't see anything of concern in the approach or the code. But it is unclear
to me if it has been tested to 16MB in ztest or the zfs test suite?
-- richard
https://reviews.csiden.org/r/51/diff/#
Note: this change is not part of the Delphix product, and therefore has had
less rigorous testing than most of the changes I submit. I would appreciate
a thorough code review and also any testing others can offer. Diffs can be
downloaded via the link above, or from
https://github.com/ahrens/illumos/tree/largeblock
--matt
_______________________________________________
developer mailing list
http://lists.open-zfs.org/mailman/listinfo/developer
--
+1-760-896-4422
_______________________________________________
developer mailing list
http://lists.open-zfs.org/mailman/listinfo/developer

Matthew Ahrens via illumos-zfs

2014-09-16 05:24:25 UTC

Permalink

On Mon, Sep 8, 2014 at 8:08 AM, Richard Elling <

Post by Richard Elling
HI Matt,
I don't see anything of concern in the approach or the code. But it is unclear
to me if it has been tested to 16MB in ztest or the zfs test suite?

I have updated ztest to test up to 1MB. I've manually tested up to 16MB,
by changing the tunable. I haven't added anything to the test suite. Not
ideal, but better than nothing?

--matt

Post by Richard Elling
-- richard
https://reviews.csiden.org/r/51/diff/#
Note: this change is not part of the Delphix product, and therefore has
had less rigorous testing than most of the changes I submit. I would
appreciate a thorough code review and also any testing others can offer.
Diffs can be downloaded via the link above, or from
https://github.com/ahrens/illumos/tree/largeblock
--matt
_______________________________________________
developer mailing list
http://lists.open-zfs.org/mailman/listinfo/developer
--
+1-760-896-4422
_______________________________________________
developer mailing list
http://lists.open-zfs.org/mailman/listinfo/developer

Richard Elling

2014-09-16 16:34:14 UTC

Permalink

Hi Matt,

Post by Richard Elling
HI Matt,
I don't see anything of concern in the approach or the code. But it is unclear
to me if it has been tested to 16MB in ztest or the zfs test suite?

I have updated ztest to test up to 1MB. I've manually tested up to 16MB, by changing the tunable. I haven't added anything to the test suite. Not ideal, but better than nothing?

ok, the sweet spot seems to be 1MB

-- richard

--matt

Post by Richard Elling
-- richard

Post by Matthew Ahrens via illumos-zfs
https://reviews.csiden.org/r/51/diff/#
Note: this change is not part of the Delphix product, and therefore has had less rigorous testing than most of the changes I submit. I would appreciate a thorough code review and also any testing others can offer. Diffs can be downloaded via the link above, or from https://github.com/ahrens/illumos/tree/largeblock
--matt
_______________________________________________
developer mailing list
http://lists.open-zfs.org/mailman/listinfo/developer

--
+1-760-896-4422
_______________________________________________
developer mailing list
http://lists.open-zfs.org/mailman/listinfo/developer

Matthew Ahrens via illumos-zfs

2014-09-16 05:19:31 UTC

Permalink

Thanks to my 3 reviewers so far: Saso, Josef, and George.

I discovered a problem with the way we were setting the ds_largeblocks flag
and associated on-disk flag DS_FIELD_LARGE_BLOCKS. Previously we only set
it on the filesystem whose "recordsize" property was increased, ignoring
the fact that it could be inherited.

I reworked this so that we set ds_largeblocks when we first sync out a
large block in each dataset. Because ds_largeblocks is now set reactively,
we can not use it to determine the maximum block size allowed in the
filesystem (it won't be set before the first large block is written). It
turns out most places don't need to know. Most of them can use
spa_maxblocksize(), which is based on whether the feature is enabled. The
few that do need per-filesystem ideas of the max block size now use the
recordsize property.

Also, I updated ztest to use block sizes up to 1MB.

I plan to submit an RTI once George gives me a ship-it. Let me know if
anyone else would like more time to review or test these changes.

--matt

-------------------------------------------
illumos-zfs
Archives: https://www.listbox.com/member/archive/182191/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182191/23047029-187a0c8d
Modify Your Subscription: https://www.listbox.com/member/?member_id=23047029&id_secret=23047029-2e85923f
Powered by Listbox: http://www.listbox.com

Matthew Ahrens via illumos-zfs

2014-09-16 05:26:09 UTC

Permalink

Post by Matthew Ahrens via illumos-zfs

Thanks to my 3 reviewers so far: Saso, Josef, and George.

And Richard and Alek! Thanks guys!

--matt

-------------------------------------------
illumos-zfs
Archives: https://www.listbox.com/member/archive/182191/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182191/23047029-187a0c8d
Modify Your Subscription: https://www.listbox.com/member/?member_id=23047029&id_secret=23047029-2e85923f
Powered by Listbox: http://www.listbox.com