You could change the commit message to:
"This commit makes the L2ARC persistent across reboots. It is largely
based on 3525 commit on Illumos"

The rest is already in the Authored and ported by tags.

That way it complies with the stylerule a little more.

edit
I've since come to realise i'm a retard and the style rule is 75 PER LINE, not 75 total. Please ignore this.

Quick work fixing the commit message!
Seems only some space-tabs issues and the sort are left now :)
http://build.zfsonlinux.org/builders/Ubuntu%2017.10%20x86_64%20%28STYLE%29/builds/18872/steps/shell_2/logs/stdio

@Ornias1993 I accidentally deleted your comment regarding disabling persistent L2ARC. I guess we could make it an option the user can set, ie a zfs property?

@gamanakis No worries that was me, I didnt notice I already submitted it and afterwards scrolled up and noted you already had a quite easy/nice hook to disable persistent L2Arc :)

(not a property yet though, so maybe that can be added indeed )

Awesome, thanks for picking this up!

based on 3525 commit on Illumos

I don't see that commit in illumos. Could you specify which codebase you started from? Is it #2672 ?

@ahrens My fault of being unclear with my advice to change the commit message. @gamanakis also accidentily changed the PR summary.
this was the original (that should be put back, in the PR summary but left out of the commit summary):

This commit makes the L2ARC persistent across reboots. It is largely
based on 3525 commit on Illumos by Saso Kiselkov, which was later
ported by Yuxuan Shui to zfsonlinux, and rebased to current master
with fixes by myself.

With regards to the Illumos commit...
I see the mistake now, it is based on a Ilumos Issue, not commit.
https://www.illumos.org/issues/3525

@ahrens Yes, I picked up from #2672

Thanks for clarifying.

@behlendorf Do you have anyone in mind to request a review from?

@gamanakis thanks for moving this work forward! I haven't had a chance to take a careful look at this PR to provide any detailed feedback. But when skimming I noticed that it doesn't appear to add any new test cases. One of the next steps for this PR would be add some to the tests/zfs-tests/tests/functional directory to verify everything is working as intended.

@behlendorf I will work on that. Meanwhile I did a minor cleanup and implemented this as a zpool property, so that the user can set it. Is it ok if I push (or force-push?) those commits in this PR?

@gamanakis Usually those changes can get pushed to the same repo no problem, certainly before formal review :)

Thanks for picking this up.

What is the point of another property?

@scineram I think @gamanakis means the earlier discussed option to enable/disable the persistance

@gamanakis Just as headsup: lookup if your git email on your machine is the same as the one setup on github. it doesn't recognise your commits as linked to your github account. you might want too ;)

@gamanakis You might need to add your property to the test suite too, test errors:
zpool_get_002_pos
Found zpool features not in the zpool_get test config 59/60.

I pushed a final functional commit finalizing the zpool property, and adding man pages.
I am working on the tests now.

6b1a0aa: Rebase to master, update tests, simplify the logic between dh_evict and l2ad_evict.

b8ef435: Cover one more case in l2arc_log_blkptr_valid() and keep cksum related code together in zdb.c.

8bdb6fe: Simplify l2arc_log_blkptr_valid() and do not evict a cache device when onlining with !l2arc_rebuild_enabled.

I've been testing this day-to-day on my desktop machine for a couple of months now. It's been behaving well.

Every few reboots it'll drop the cache and I'll need to warm it again, but since I'm rebuilding openzfs with fresher versions of this patch regularly I'm assuming this is expected as the cache header/format/version/validation changes.

@adamdmoss Thank you for testing. I have been making some changes that could cause this. I am about to push a final one (hopefully) to cover some corner cases.

60c9ca3: avoid infinite loops after restoring a log block in l2arc_rebuild(), update zdb.c logic to reflect this, rebase to master.

217790d: Make code more concise in zdb.c, improve l2arc_log_blkptr_valid() to detect evicted blocks like this:

l2ad_start =================================== l2ad_end
                               ^             ^
                      l2ad_hand     l2ad_evict
           ------^           ^----------------
           lbp_daddr      payload_start

Been getting what looks like infinite recursion since updating to current HEAD of persist_l2arc_unified_review (79fbfb6):

[   97.177110] Call Trace:
[   97.177136]  l2arc_evict+0x354/0x480 [zfs]
[   97.177160]  l2arc_evict+0x354/0x480 [zfs]
(many many repetitions removed)
[   97.180363]  l2arc_evict+0x354/0x480 [zfs]
[   97.180386]  l2arc_evict+0x354/0x480 [zfs]
[   97.180410]  l2arc_feed_thread+0x296/0x1780 [zfs]
[   97.180413]  ? kfree+0x1f7/0x210
[   97.180416]  ? __schedule+0x2b0/0x670
[   97.180439]  ? l2arc_remove_vdev+0x1f0/0x1f0 [zfs]
[   97.180445]  thread_generic_wrapper+0x74/0x90 [spl]
[   97.180447]  kthread+0x121/0x140
[   97.180451]  ? __thread_exit+0x20/0x20 [spl]
[   97.180452]  ? kthread_park+0xb0/0xb0
[   97.180454]  ret_from_fork+0x1f/0x40

@adamdmoss Please do not test persist_l2arc_unified_review. That branch contains not up-to-date experimental pushes. The one this PR is based upon is persist_l2arc_unified. Thank you.

While testing this PR I found a subtle bug in the L2ARC implementation, unrelated to this PR:
l2arc_feed_thread runs periodically like this: l2arc_write_size -> l2arc_evict(size) -> l2arc_write_buffers(size)

Assume l2ad_hand is approaching l2ad_end. l2arc_write_buffers will reset l2ad_hand at its end taking into account the write size calculated at l2arc_write_size at the start of this cycle. However if the user increases l2arc_write_{max,boost} this will happen in the next cycle when l2arc_write_size runs again. Now if l2arc_write_buffers does not have enough space to perform its writes (as it didn't account for the new write size), zio_write_phys panics.

This is unlikely to happen in a production environment as l2arc_write_{max,boost} are not constantly changed, and the cache size exceeds those parameters. However, it can be fixed by moving resetting of l2ad_hand to l2arc_evict so that the new write size is always accounted for.

I will make a new PR (#10154), and then we can rebase this one on that.

0ab68d3: Addresses a comment from @ahrens. Comment on padding of log entries, add a CTASSERT on the size of log blocks, set the size of log entries to 64 bytes (from 128) and size of log blocks to 64K (from 128K). Reduce the maximum amount of log entries in a log block to 1022 (from 1023). Also improve the tests.

eb7e847: Rebase to master, resolve a conflict in a man page.

842a51d: Rebase to master. No functional changes. Incorporate #10154 and employ the newly introduced get_arcstat function in the tests.

As mentioned on the March 30th OpenZFS call there's still time to provide additional review feedback on this feature PR. Let's target getting this merged by April 10th, so if you have input please provide it by then.

f005091: No functional change. Improve the implementation of #10154 and add comments.

1ee5bac: Make L2BLK_*_COMPRESS use SPA_COMPRESSBITS, like BP_*_COMPRESS. Again no functional changes.

e5ef3fd: Minor style changes consolidating code in tests, zdb.c and arc.c. Also give the 007 test more time to retrieve the amount of log blocks using zdb after issuing the off-lining of the cache device. Report the amount of restored blocks in dbgmsg if the rebuild was successful.

303f413: Fix the placement of DTRACE_PROBE4 in l2arc_evict().

412d6f5: Prevent unnecessary iteration of l2arc_evict() if we evict all buffers.

fbf8134: If we encounter a log block with invalid cksum in zdb.c, print out its pointer.

e044ce2: Report the number of restored log blocks in dbgmsg even when the rebuild was aborted prematurely.

b74fa37: Minor fix in zdb.c when dumping log entries.

Still behaving well FWIW. :)

Merged. Thanks to everyone who helped get this done!

Thanks for your hard work on this, folks! This is going to be a great benefit to several machines I maintain with ZOL.

why is this patch enforcing lz4 compression, no matter which compression was selected? its also uses lz4 if compression is disabled.

@BrainSlayer the persistent L2ARC does use LZ4 for the compression of the metadata (log blocks). Otherwise the L2ARC buffers (L2ARC data) use whatever compression the filesystem uses.

@BrainSlayer you can see what compression algorithm is used for L2ARC data by inspecting the metadata with:
zdb -lll device | less
This produces for example:

lb[   1]        le[   5]        DVA asize: 105472, vdev: 0, offset: 2484060160
|                               birth: 19
|                               lsize: 131072
|                               psize: 105472
|                               compr: 15
|                               type: 1
|                               protected: 0
|                               prefetch: 0
|                               address: 663780352
|

Which tells you: the 5th buffer (le[5]) referenced by the first log block (lb[1]) is using compression algorithm (compr) 15, which in master is lz4 (found in zio_compress.h) because I set the filesystem compression to lz4. If you run this with the ZSTD PR merged in, and in a filesystem where zstd is enabled you will see 16 (the value for zstd in zio_compress.h) in that PR.

the question is more why you enforce lz4 and why not using the configured algorithm (or even no compression if disabled)

@BrainSlayer L2ARC devices can cache from different filesystems with different compression algorithms. Doing what you suggested makes it very complex and prone to bugs. After all only the metadata on L2ARC used for restoring the buffers use LZ4, the actual cached buffers (which were fetched from ARC) still use the compression used for the filesystem.

@BrainSlayer actually your suggestion is impossible with the current implementation. The metadata (log blocks) consist of up to 1022 entries which refer to buffers cached from ARC. Each of those 1022 cached buffers may be using a different compression algorithm, dependent on which filesystem it comes from. Ie the log block can contain entries referencing buffers, some of which are LZ4, some ZSTD, some with compression disabled. So which compression algorithm would the log block use?
This is the reason the compression for log blocks (again these are the metadata on L2ARC only) was fixed to LZ4.

okay. thanks for explaination. i will consider this in my rework of the zstd patch. so your own patch might be already good enough

Is it possible for l2arc to cache data even if it's still in ram and not evicted(duplicated cache)? For now, if the ram is still enough to hold cache, the data cached cannot be retrieved between reboots through persistent l2arc. It's hard to accept the most frequently used data in ram cannot be accelerated by persistent l2arc.

@fedstryale yes it is. Try setting l2arc_headroom = 0 in current master. That way the L2ARC will scan the full ARC lists for cacheable buffers, long before they are evicted. This is also documented in the zpoolconcepts.8 man page.