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

[devZer0]

i have a large zfs backup server hosting millions of files being transferred to 
via rsync. most of the runtime is taken from rsync comparing files by 
timestamp. metadata is being re-read again and again on every backup (there is 
only few incremental transfer).  that puts unnecessary stress on the disks and 
is so much slower, so please consider adding an optimization param like this 
which helps keeping metadata completely in l2arc, as it`s too big to fit in 
ram. 

anyhow, i`m curious why the metadata in ram takes that much space at all. how 
much metadata is kept in ram for each file? it seems it`s much more than i 
would guess. i would guess it`s <<1kb/file, but from what i observe it`s more...

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/openzfs/openzfs/pull/189#issuecomment-350455544
--
openzfs-developer
Archives: 
https://openzfs.topicbox.com/groups/developer/discussions/Tf640c727e00d72ef-M67b31ec6eb6479fdeca3e0c5
Powered by Topicbox: https://topicbox.com

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

[Ben RUBSON]

Ooooh you approved #222 but closed this one... :)

> I would consider a simpler semantic change like "evict blocks in L2ARC 
> first". 

This is exactly what this change does with `arc_evict_l2_first = B_TRUE ` :)
It will prefer buffers which are already in L2ARC, but if it does not found 
such enough buffers to reach the target, it will then continue with other 
buffers (...).

> the change is too risky to take as-is

(...) So there is no risk at all compared to the current behaviour, as it is 
the current behaviour + a preference in buffers to choose.

The step above `arc_evict_l2_only = B_TRUE`, disabled by default (so set to 
`B_FALSE`), is for advertised users who want more than simply **preferring** 
buffers which are already in L2ARC.
It allows to **only** evict buffers which are already in L2ARC. Of course, if 
it can't find such buffers at all, it will then evict other buffers up to the 
given target, to avoid memory pressure.
It's then a more aggressive setting, but fitted with a security valve.

> With sufficient testing

In production on 3 backup & storage servers for a year now with :
`arc_evict_l2_only = B_TRUE` : the more aggressive setting ;
`primarycache=metadata` & `primarycache=all` : I tested both ;
`secondarycache=metadata` : it gives even more work to the algorithm to find 
buffers !

This change gives a very nice and constant performance improvement, [as show in 
the bug report](https://www.illumos.org/issues/7361).

In addition is is quite small in size.

Did I manage to convince you ? :)

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/openzfs/openzfs/pull/189#issuecomment-324660376
--
openzfs-developer
Archives: 
https://openzfs.topicbox.com/groups/developer/discussions/T122a323c2623f54c-M707a596cf617fbd7b5f71a8e
Powered by Topicbox: https://topicbox.com

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

[Matthew Ahrens]

Closed #189.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/openzfs/openzfs/pull/189#event-1218736634
--
openzfs-developer
Archives: 
https://openzfs.topicbox.com/groups/developer/discussions/T122a323c2623f54c-M5678b94ebe3f668cb3492461
Powered by Topicbox: https://topicbox.com

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

[Ben RUBSON]

Good news (at least for this change !), thank you @prakashsurya  

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/openzfs/openzfs/pull/189#issuecomment-324371112
--
openzfs-developer
Archives: 
https://openzfs.topicbox.com/groups/developer/discussions/T122a323c2623f54c-M1b5e2d36ad936408dc0c083b
Powered by Topicbox: https://topicbox.com

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

[Prakash Surya]

@benrubson I don't think that failure is due to this change; rather, think that 
failure is due to a prior commit: 
https://github.com/openzfs/openzfs/commit/d28671a3b094af696bea87f52272d4c4d89321c7

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/openzfs/openzfs/pull/189#issuecomment-324354962
--
openzfs-developer
Archives: 
https://openzfs.topicbox.com/groups/developer/discussions/T122a323c2623f54c-Mf66a04b93993c80e296d640e
Powered by Topicbox: https://topicbox.com

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

[Ben RUBSON]

Thank you @prakashsurya for having launched the test.
One of the 67 `ztest` commands failed. Seems to be related to ZIL, whereas my 
PR concentrates on ARC. Is is then an "expected" failure ? Perhaps give the 
test another chance ?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/openzfs/openzfs/pull/189#issuecomment-324229068
--
openzfs-developer
Archives: 
https://openzfs.topicbox.com/groups/developer/discussions/T122a323c2623f54c-Mc33c340c254b999c38560de3
Powered by Topicbox: https://topicbox.com

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

[Ben RUBSON]

Yes kOT, you're right, finally I just reworked this PR into a unique commit, 
which looks nicer  

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/openzfs/openzfs/pull/189#issuecomment-322173432
--
openzfs-developer
Archives: 
https://openzfs.topicbox.com/groups/developer/discussions/T122a323c2623f54c-M9b757da272660bfd62034224
Powered by Topicbox: https://topicbox.com

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

[kernelOfTruth aka. kOT, Gentoo user]

This looks more like a ```git merge branch``` than a git rebase of a commit on 
top of a branch to a newer master branch ❓ 

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/openzfs/openzfs/pull/189#issuecomment-322068232
--
openzfs-developer
Archives: 
https://openzfs.topicbox.com/groups/developer/discussions/T122a323c2623f54c-M0e1dd7218cce96b29dd1ae83
Powered by Topicbox: https://topicbox.com

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

[Ben RUBSON]

@benrubson pushed 1 commit.

3b95a85  Merge branch 'master' into master


-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/openzfs/openzfs/pull/189/files/a166fc26c4e2a4c4886476f86b762cf0d873c911..3b95a859d4c4474a40d1e70e757406c437ad451f

--
openzfs-developer
Archives: 
https://openzfs.topicbox.com/groups/developer/discussions/T122a323c2623f54c-M678fef756061207391a29edd
Powered by Topicbox: https://topicbox.com

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,
+_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,
+_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_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_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_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_secret=28015062-f966d51c
Powered by Listbox: http://www.listbox.com