Re: [developer] Re: [openzfs/openzfs] 7361 Improve L2ARC filling (#189)

[Ben RUBSON]

> On 01 Dec 2016, at 01:33, Dan McDonald  wrote:
> 
>   git clone -b  https://github.com/omniti-labs/illumos-omnios
> 
>  is any one of r151014, r151018, r151020.  The "master" branch is 
> the OmniOS bloody source.

So attached is a patch against r151014.

It contains the PR we are talking about :
https://github.com/openzfs/openzfs/pull/189
And the following one, as it is let's say related :
https://github.com/openzfs/openzfs/pull/222

The only thing I was not able to so is the following :

+SYSCTL_INT(_vfs_zfs, OID_AUTO, arc_evict_l2_first, CTLFLAG_RW,
+&arc_evict_l2_first, 0, "first evict buffers from ARC which are in L2ARC");
+SYSCTL_INT(_vfs_zfs, OID_AUTO, arc_evict_l2_only, CTLFLAG_RW,
+&arc_evict_l2_only, 0, "only evict buffers from ARC which are in L2ARC");

These are the declarations of the syctls, for FreeBSB.
I'm not sure how it is done in OmniOS.

Defaults values in the patch are the following :
arc_evict_l2_first = B_TRUE
arc_evict_l2_only = B_FALSE

Perhaps you could simply hard-set both to B_TRUE and give it a try.

Ben




PR_189_222_ omnios151014.patch
Description: Binary data




---
openzfs-developer
Archives: https://www.listbox.com/member/archive/274414/=now
RSS Feed: https://www.listbox.com/member/archive/rss/274414/28015062-cce53afa
Modify Your Subscription: 
https://www.listbox.com/member/?member_id=28015062&id_secret=28015062-f966d51c
Powered by Listbox: http://www.listbox.com

Re: [developer] Re: [openzfs/openzfs] 7361 Improve L2ARC filling (#189)

[Paul B. Henson]

On Wed, Nov 30, 2016 at 07:33:59PM -0500, Dan McDonald wrote:

> While we do backport nontrivial ZFS features to LTS release like
> r151014, we don't do it all the time.  This is one that would need
> evaluation, and testing.  Since we have willing testers, it can be
> arranged.  (Esp. for Chip, who's helped us out before!)

To clarify, this feature hasn't been integrated into illumos yet; they
want to see more testing in production environments first. Chip is
offering to test in his environment as he believes the feature will help
his performance, but he needs the proposed feature available in omnios
r151014 binary form to test (and I assume he's not in a position to
build his own)... So at this point I don't think it's really a question
of officially backporting to LTS so much as just providing a test binary
to help provide feedback for the RTI process.

Although I suppose if Chip finds the feature rocks for his needs and it
ends up getting integrated it might turn into a backport request :).


---
openzfs-developer
Archives: https://www.listbox.com/member/archive/274414/=now
RSS Feed: https://www.listbox.com/member/archive/rss/274414/28015062-cce53afa
Modify Your Subscription: 
https://www.listbox.com/member/?member_id=28015062&id_secret=28015062-f966d51c
Powered by Listbox: http://www.listbox.com

Re: [developer] Re: [openzfs/openzfs] 7361 Improve L2ARC filling (#189)

[Dan McDonald]

> On Nov 30, 2016, at 6:31 PM, Paul B. Henson  wrote:
> 
> I can't speak definitively for Chip, but I believe you're talking about
> basically illumos head, and I think he's looking for a build from the patch
> applied against the source of omnios 15014 stable, which is *way*
> behind illumos head, to run/test on a production system. Dropping in
> binaries from such a recent build on top of such an old build
> probably wouldn't end well 8-/.

Yeah, Paul is right folks.

While we do backport nontrivial ZFS features to LTS release like r151014, we 
don't do it all the time.  This is one that would need evaluation, and testing. 
 Since we have willing testers, it can be arranged.  (Esp. for Chip, who's 
helped us out before!)

It can ALSO be arranged by checking out the appropriate branch:

git clone -b  https://github.com/omniti-labs/illumos-omnios

 is any one of r151014, r151018, r151020.  The "master" branch is the 
OmniOS bloody source.

Dan



---
openzfs-developer
Archives: https://www.listbox.com/member/archive/274414/=now
RSS Feed: https://www.listbox.com/member/archive/rss/274414/28015062-cce53afa
Modify Your Subscription: 
https://www.listbox.com/member/?member_id=28015062&id_secret=28015062-f966d51c
Powered by Listbox: http://www.listbox.com

Re: [developer] Re: [openzfs/openzfs] 7361 Improve L2ARC filling (#189)

[Paul B. Henson]

On Wed, Nov 30, 2016 at 01:58:19PM -0800, Prakash Surya wrote:
> I could probably create a tarball of the IPS repository that is created by
> the automated build step and/or of the build products themselves. The build
> is performed on an OmniOS r151020 system, and would be a build of the
> OpenZFS repository with this PR applied. Not sure if that would be helpful,
> though.

I can't speak definitively for Chip, but I believe you're talking about
basically illumos head, and I think he's looking for a build from the patch
applied against the source of omnios 15014 stable, which is *way*
behind illumos head, to run/test on a production system. Dropping in
binaries from such a recent build on top of such an old build
probably wouldn't end well 8-/.


> On Wed, Nov 30, 2016 at 1:35 PM, Paul B. Henson  wrote:
> 
> > On Wed, Nov 30, 2016 at 07:40:56AM -0600, Schweiss, Chip wrote:
> > > Sounds like you're no better prepared to pull this in to OmniOS than I
> > am.
> > >   I would need a patch against the latest r151014 to test this out.
> > 
> > You should try asking on the omnios list, Dan might hook you up :). He's
> > been known to publish alternate pkg repos or provide standalone binaries
> > for testing experimental features...


---
openzfs-developer
Archives: https://www.listbox.com/member/archive/274414/=now
RSS Feed: https://www.listbox.com/member/archive/rss/274414/28015062-cce53afa
Modify Your Subscription: 
https://www.listbox.com/member/?member_id=28015062&id_secret=28015062-f966d51c
Powered by Listbox: http://www.listbox.com

Re: [developer] Re: [openzfs/openzfs] 7361 Improve L2ARC filling (#189)

[Prakash Surya]

I could probably create a tarball of the IPS repository that is created by
the automated build step and/or of the build products themselves. The build
is performed on an OmniOS r151020 system, and would be a build of the
OpenZFS repository with this PR applied. Not sure if that would be helpful,
though.

On Wed, Nov 30, 2016 at 1:35 PM, Paul B. Henson  wrote:

> On Wed, Nov 30, 2016 at 07:40:56AM -0600, Schweiss, Chip wrote:
> > Sounds like you're no better prepared to pull this in to OmniOS than I
> am.
> >   I would need a patch against the latest r151014 to test this out.
> 
> You should try asking on the omnios list, Dan might hook you up :). He's
> been known to publish alternate pkg repos or provide standalone binaries
> for testing experimental features...
> 



---
openzfs-developer
Archives: https://www.listbox.com/member/archive/274414/=now
RSS Feed: https://www.listbox.com/member/archive/rss/274414/28015062-cce53afa
Modify Your Subscription: 
https://www.listbox.com/member/?member_id=28015062&id_secret=28015062-f966d51c
Powered by Listbox: http://www.listbox.com

Re: [developer] Re: [openzfs/openzfs] 7361 Improve L2ARC filling (#189)

[Paul B. Henson]

On Wed, Nov 30, 2016 at 07:40:56AM -0600, Schweiss, Chip wrote:
> Sounds like you're no better prepared to pull this in to OmniOS than I am.
>   I would need a patch against the latest r151014 to test this out.

You should try asking on the omnios list, Dan might hook you up :). He's
been known to publish alternate pkg repos or provide standalone binaries
for testing experimental features...


---
openzfs-developer
Archives: https://www.listbox.com/member/archive/274414/=now
RSS Feed: https://www.listbox.com/member/archive/rss/274414/28015062-cce53afa
Modify Your Subscription: 
https://www.listbox.com/member/?member_id=28015062&id_secret=28015062-f966d51c
Powered by Listbox: http://www.listbox.com

Re: [developer] Re: [openzfs/openzfs] 7361 Improve L2ARC filling (#189)

[Ben RUBSON]

Any pointer to the r151014 tree ?

Giving you a patch should be something easy :)

Ben

> On 30 Nov 2016, at 14:40, Schweiss, Chip  wrote:
> 
> Sounds like you're no better prepared to pull this in to OmniOS than I am.   
> I would need a patch against the latest r151014 to test this out.   
> 
> This along with a lot of other features that have been hanging in review 
> state for a long time is where OpenZFS really needs a testing branch.   Often 
> the best expert on the feature is the developer who wrote it and the code 
> really needs some real world testing to make sure it is sound.
> 
> -Chip
> 
> 
> On Wed, Nov 30, 2016 at 6:57 AM, Ben RUBSON  > wrote:
> Hi Chip,
> 
> Yes sounds like this PR should help you improving performance.
> You would also need PR #222 which assigns correct L2CACHE flag to prefetched 
> buffers :
> https://github.com/openzfs/openzfs/pull/222 
> 
> 
> Regarding the OmniOS build, unfortunately, I never used OmniOS, so I'm not 
> really comfortable with it :)
> I use ZFS with FreeBSD.
> However, what do you mean by making a build ? You need a patch file ? Against 
> which tree ?
> 
> Thank you !
> 
> Ben
> 
>> On 30 Nov 2016, at 13:11, Schweiss, Chip > > wrote:
>> 
>> Ben,
>> 
>> Would you be able make a build that I could test this on OmniOS r151014?   
>> I'd love to test this out.
>> 
>> This seems to be exactly the tuning knob I need in my environment.   All my 
>> pools are 0.4 - 1 PB in size with billions of files. Metadata performance is 
>> always an issue for us even with secondarycache=metadata.  It never seems to 
>> get enough metadata into L2ARC.
>> 
>> -Chip
>> 
>> On Tue, Nov 29, 2016 at 6:52 PM, Matthew Ahrens > > wrote:
>> @ahrens requested changes on this pull request.
>> 
>> Overall, I am not comfortable making this semantic change without more 
>> extensive real-world testing. I would suggest that you get someone who uses 
>> L2ARC in production on general-purpose workloads (or a wide variety of 
>> workloads) to at least review this code, and better yet to test it. 
>> Unfortunately I (and Delphix) do not use L2ARC so we aren't in a good 
>> position to test this.
>> 
>> In usr/src/uts/common/fs/zfs/arc.c 
>> :
>> 
>> > @@ -2931,9 +2943,34 @@ arc_evict_hdr(arc_buf_hdr_t *hdr, kmutex_t 
>> > *hash_lock)
>>  return (bytes_evicted);
>>  }
>>  
>> +/*
>> + * Based on l2arc_spa_list, returns true if the
>> + * given spa has an alive (!dead) L2 device,
>> + * false otherwise.
>> + */
>> +static boolean_t
>> +l2arc_alive(uint64_t spa)
>> Could you do this by walking the existing l2arc_dev_list? This mechanism 
>> seems quite complicated.
>> 
>> That said, I'd be concerned about the performance of this either way, it's 
>> in a pretty hot path.
>> 
>> —
>> You are receiving this because you are subscribed to this thread.
>> Reply to this email directly, view it on GitHub 
>> , or 
>> mute the thread 
>> .
>> 
>> 
>> 
>> 
>> 
>> 
>> 
>> 
>> 
>> 
>> 
>> 
>> 
>> 
> 
> 
> openzfs-developer | Archives 
>   
>  | 
> Modify  Your Subscription
> 



---
openzfs-developer
Archives: https://www.listbox.com/member/archive/274414/=now
RSS Feed: https://www.listbox.com/member/archive/rss/274414/28015062-cce53afa
Modify Your Subscription: 
https://www.listbox.com/member/?member_id=28015062&id_secret=28015062-f966d51c
Powered by Listbox: http://www.listbox.com

Re: [developer] Re: [openzfs/openzfs] 7361 Improve L2ARC filling (#189)

[Schweiss, Chip]

Sounds like you're no better prepared to pull this in to OmniOS than I am.
  I would need a patch against the latest r151014 to test this out.

This along with a lot of other features that have been hanging in review
state for a long time is where OpenZFS really needs a testing branch.
Often the best expert on the feature is the developer who wrote it and the
code really needs some real world testing to make sure it is sound.

-Chip


On Wed, Nov 30, 2016 at 6:57 AM, Ben RUBSON  wrote:

> Hi Chip,
>
> Yes sounds like this PR should help you improving performance.
> You would also need PR #222 which assigns correct L2CACHE flag to
> prefetched buffers :
> https://github.com/openzfs/openzfs/pull/222
>
> Regarding the OmniOS build, unfortunately, I never used OmniOS, so I'm not
> really comfortable with it :)
> I use ZFS with FreeBSD.
> However, what do you mean by making a build ? You need a patch file ?
> Against which tree ?
>
> Thank you !
>
> Ben
>
> On 30 Nov 2016, at 13:11, Schweiss, Chip  wrote:
>
> Ben,
>
> Would you be able make a build that I could test this on OmniOS r151014?
> I'd love to test this out.
>
> This seems to be exactly the tuning knob I need in my environment.   All
> my pools are 0.4 - 1 PB in size with billions of files. Metadata
> performance is always an issue for us even with secondarycache=metadata.
> It never seems to get enough metadata into L2ARC.
>
> -Chip
>
> On Tue, Nov 29, 2016 at 6:52 PM, Matthew Ahrens 
>  wrote:
>
>> *@ahrens* requested changes on this pull request.
>>
>> Overall, I am not comfortable making this semantic change without more
>> extensive real-world testing. I would suggest that you get someone who uses
>> L2ARC in production on general-purpose workloads (or a wide variety of
>> workloads) to at least review this code, and better yet to test it.
>> Unfortunately I (and Delphix) do not use L2ARC so we aren't in a good
>> position to test this.
>> --
>>
>> In usr/src/uts/common/fs/zfs/arc.c
>> :
>>
>> > @@ -2931,9 +2943,34 @@ arc_evict_hdr(arc_buf_hdr_t *hdr, kmutex_t 
>> > *hash_lock)
>>  return (bytes_evicted);
>>  }
>>
>> +/*
>> + * Based on l2arc_spa_list, returns true if the
>> + * given spa has an alive (!dead) L2 device,
>> + * false otherwise.
>> + */
>> +static boolean_t
>> +l2arc_alive(uint64_t spa)
>>
>> Could you do this by walking the existing l2arc_dev_list? This mechanism
>> seems quite complicated.
>>
>> That said, I'd be concerned about the performance of this either way,
>> it's in a pretty hot path.
>>
>> —
>> You are receiving this because you are subscribed to this thread.
>> Reply to this email directly, view it on GitHub
>> ,
>> or mute the thread
>> 
>> .
>>
>>
>>
>>
>>
>>
>>
>
> *openzfs-developer* | Archives
> 
>  |
> Modify
> 
> Your Subscription 
>



---
openzfs-developer
Archives: https://www.listbox.com/member/archive/274414/=now
RSS Feed: https://www.listbox.com/member/archive/rss/274414/28015062-cce53afa
Modify Your Subscription: 
https://www.listbox.com/member/?member_id=28015062&id_secret=28015062-f966d51c
Powered by Listbox: http://www.listbox.com

Re: [developer] Re: [openzfs/openzfs] 7361 Improve L2ARC filling (#189)

[Ben RUBSON]

Hi Chip,

Yes sounds like this PR should help you improving performance.
You would also need PR #222 which assigns correct L2CACHE flag to prefetched 
buffers :
https://github.com/openzfs/openzfs/pull/222 


Regarding the OmniOS build, unfortunately, I never used OmniOS, so I'm not 
really comfortable with it :)
I use ZFS with FreeBSD.
However, what do you mean by making a build ? You need a patch file ? Against 
which tree ?

Thank you !

Ben

> On 30 Nov 2016, at 13:11, Schweiss, Chip  wrote:
> 
> Ben,
> 
> Would you be able make a build that I could test this on OmniOS r151014?   
> I'd love to test this out.
> 
> This seems to be exactly the tuning knob I need in my environment.   All my 
> pools are 0.4 - 1 PB in size with billions of files. Metadata performance is 
> always an issue for us even with secondarycache=metadata.  It never seems to 
> get enough metadata into L2ARC.
> 
> -Chip
> 
> On Tue, Nov 29, 2016 at 6:52 PM, Matthew Ahrens  > wrote:
> @ahrens requested changes on this pull request.
> 
> Overall, I am not comfortable making this semantic change without more 
> extensive real-world testing. I would suggest that you get someone who uses 
> L2ARC in production on general-purpose workloads (or a wide variety of 
> workloads) to at least review this code, and better yet to test it. 
> Unfortunately I (and Delphix) do not use L2ARC so we aren't in a good 
> position to test this.
> 
> In usr/src/uts/common/fs/zfs/arc.c 
> :
> 
> > @@ -2931,9 +2943,34 @@ arc_evict_hdr(arc_buf_hdr_t *hdr, kmutex_t 
> > *hash_lock)
>   return (bytes_evicted);
>  }
>  
> +/*
> + * Based on l2arc_spa_list, returns true if the
> + * given spa has an alive (!dead) L2 device,
> + * false otherwise.
> + */
> +static boolean_t
> +l2arc_alive(uint64_t spa)
> Could you do this by walking the existing l2arc_dev_list? This mechanism 
> seems quite complicated.
> 
> That said, I'd be concerned about the performance of this either way, it's in 
> a pretty hot path.
> 
> —
> You are receiving this because you are subscribed to this thread.
> Reply to this email directly, view it on GitHub 
> , or 
> mute the thread 
> .
> 
> 
> 
> 
> 
> 
> 
> 
> openzfs-developer | Archives 
>   
>  | 
> Modify  Your Subscription
> 



---
openzfs-developer
Archives: https://www.listbox.com/member/archive/274414/=now
RSS Feed: https://www.listbox.com/member/archive/rss/274414/28015062-cce53afa
Modify Your Subscription: 
https://www.listbox.com/member/?member_id=28015062&id_secret=28015062-f966d51c
Powered by Listbox: http://www.listbox.com

Re: [developer] Re: [openzfs/openzfs] 7361 Improve L2ARC filling (#189)

[Schweiss, Chip]

Ben,

Would you be able make a build that I could test this on OmniOS r151014?
I'd love to test this out.

This seems to be exactly the tuning knob I need in my environment.   All my
pools are 0.4 - 1 PB in size with billions of files. Metadata performance
is always an issue for us even with secondarycache=metadata.  It never
seems to get enough metadata into L2ARC.

-Chip

On Tue, Nov 29, 2016 at 6:52 PM, Matthew Ahrens 
wrote:

> *@ahrens* requested changes on this pull request.
>
> Overall, I am not comfortable making this semantic change without more
> extensive real-world testing. I would suggest that you get someone who uses
> L2ARC in production on general-purpose workloads (or a wide variety of
> workloads) to at least review this code, and better yet to test it.
> Unfortunately I (and Delphix) do not use L2ARC so we aren't in a good
> position to test this.
> --
>
> In usr/src/uts/common/fs/zfs/arc.c
> :
>
> > @@ -2931,9 +2943,34 @@ arc_evict_hdr(arc_buf_hdr_t *hdr, kmutex_t 
> > *hash_lock)
>   return (bytes_evicted);
>  }
>
> +/*
> + * Based on l2arc_spa_list, returns true if the
> + * given spa has an alive (!dead) L2 device,
> + * false otherwise.
> + */
> +static boolean_t
> +l2arc_alive(uint64_t spa)
>
> Could you do this by walking the existing l2arc_dev_list? This mechanism
> seems quite complicated.
>
> That said, I'd be concerned about the performance of this either way, it's
> in a pretty hot path.
>
> —
> You are receiving this because you are subscribed to this thread.
> Reply to this email directly, view it on GitHub
> ,
> or mute the thread
> 
> .
> *openzfs-developer* | Archives
> 
>  |
> Modify
> 
> Your Subscription 
>
>



---
openzfs-developer
Archives: https://www.listbox.com/member/archive/274414/=now
RSS Feed: https://www.listbox.com/member/archive/rss/274414/28015062-cce53afa
Modify Your Subscription: 
https://www.listbox.com/member/?member_id=28015062&id_secret=28015062-f966d51c
Powered by Listbox: http://www.listbox.com