Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

3525 persistent l2arc. #2672

Closed
wants to merge 1 commit into from
Closed

3525 persistent l2arc. #2672

wants to merge 1 commit into from

Conversation

yshui
Copy link
Contributor

@yshui yshui commented Sep 7, 2014

For details, see: https://www.illumos.org/issues/3525

Close #925

Ported-by: Yuxuan Shui yshuiv7@gmail.com

For details, see: https://www.illumos.org/issues/3525

v2: Change two KM_SLEEP to KM_PUSHPAGE.
v3: Change one more KM_SLEEP.
v4: Fix log buffer alignment in l2arc_dev_log_commit.
v5: Fix style.
v6: l2arc vdev can go away, remove ASSERT in l2arc_spa_rebuild_start.

Close #925

Ported-by: Yuxuan Shui <yshuiv7@gmail.com>
@edillmann edillmann mentioned this pull request Sep 7, 2014
@yshui
Copy link
Contributor Author

yshui commented Sep 8, 2014

The failed test, is it a deadlock during module loading? The log is a little bit confusing..

@behlendorf
Copy link
Contributor

@yshui Sometimes the buildbot results can sometimes be a little vague when there's a failure. For these cases usually the best place to look is in the dmesg log. This is always run as the last build step so you can effectively get the console log from the system.

In this case we seem to have hit an unexpected memory allocation failure which prevented the modules from loading. That's almost certainly unrelated this this patch.

Do you know what the status is for this change on the Illumos side?

@yshui
Copy link
Contributor Author

yshui commented Sep 8, 2014

@behlendorf The status on the issue tracker is 'Feedback', although that has been the same for nearly a year now...

@kernelOfTruth
Copy link
Contributor

works fine here,

after a reboot I usually have to manually mount the zpool containing my /home

and after the zpool was imported - the state of L2ARC has been preserved

Thanks a lot !

@sempervictus
Copy link
Contributor

This seems to have some issues with #2484 as evidenced by http://pastebin.com/1xuCjyi9.
Pool consists of 6 1T WD greens in a Z2, with 7th spare, and a 64G L2ARC.
Import hangs for minutes, the spinners show ~11/MB/s read per disk (~60 for the pool) and then this happens, hangs ZFS entirely, requires a reboot, etc.
With the l2arc vdev not present (its an encrypted SSD, so not presenting the decrypted space keeps ZFS' paws off of it while i import), the import appears to take even longer - there's ~1T of actual data in those ZVOLs at present, and its been flusing 60MB/s through the poor ARC (which keeps showing 100% miss) for ~20m.
Running this patch on a number of systems, that particular one seems to hate me, otherwise, no issues so far...

@behlendorf behlendorf added Type: Feature Feature request or new feature Difficulty - Hard labels Oct 8, 2014
@behlendorf behlendorf added this to the 0.8.0 milestone Oct 8, 2014
@kernelOfTruth
Copy link
Contributor

FYI:

after every few boots l2arc gets corrupted ("faulted") and the cache needs to be removed and re-added

haven't figured out the pattern yet

this didn't happen before - some new changes from the upstream master or some of the additional added patches seem to break it

patches used before: #2351 #2672 #2753 #2484 ( #2484 now replaced by #2129 )

additional added patches recently: #2784 #2786

could also be that the order in which the partitions are getting unmounted and/or closed during shutdown has changed ... (not very probable - but this already has led to issues with minor checksum issues in the past on a mirrored zpool)

@dswartz
Copy link
Contributor

dswartz commented Oct 16, 2014

I have this happen as well.

"kernelOfTruth aka. kOT, Gentoo user" notifications@github.com wrote:

FYI:

after every few boots l2arc gets corrupted ("faulted") and the cache needs to be removed and re-added

haven't figured out the pattern yet

this didn't happen before - some new changes from the upstream master or some of the additional added patches seem to break it

patches used before: #2351 #2672 #2753 #2484 ( #2484 now replaced by #2129 )

additional patches: #2784 #2786

could also be that the order in which the partitions are getting unmounted and/or closed during shutdown has changed ... (not very probable - but this already has led to issues with minor checksum issues in the past on a mirrored zpool)


Reply to this email directly or view it on GitHub.

{"@context":"http://schema.org","@type":"EmailMessage","description":"View this Pull Request on GitHub","action":{"@type":"ViewAction","url":"https://github.com/zfsonlinux/zfs/pull/2672#issuecomment-59421155","name":"View Pull Request"}}

@kernelOfTruth
Copy link
Contributor

seems like even only booting up Windows without logging in or doing any administration (from time to time I'm dual-booting into Windows 8.1) is enough to corrupt the partition (!)

so Windows is to blame in my case :/

l2arc here is seated on an luks-encrypted partition on the SSD

I'll observe if there are other causes to this ...

@yshui
Copy link
Contributor Author

yshui commented Oct 17, 2014

@kernelOfTruth Can you post the content of /proc/spl/kstat/zfs/arcstats when zfs failed to rebuild l2arc?

@algragon
Copy link

@kernelOfTruth
When merging #2672 on top of #2129 I got warnings about incompatible pointer types.
Maybe the l2arc gets corrupted right away instead of "at reboots", because the l2arc* functions pass around something unexpected?

--- zfs.make.vmalloc.log 2014-10-17 13:36:41.487493228 +0200
+++ zfs.make.vmalloc+l2arc.log 2014-10-17 13:41:58.584008391 +0200
@@ -338,6 +338,18 @@
CC zprop_common.lo
CC abd.lo
CC arc.lo
+../../module/zfs/arc.c: In function 'l2arc_uberblock_read':
+../../module/zfs/arc.c:6566:6: warning: passing argument 5 of 'zio_read_phys' from incompatible pointer type [enabled by default]
+../../include/sys/zio.h:492:15: note: expected 'struct abd_t *' but argument is of type 'struct l2arc_uberblock_phys_t *'
+../../module/zfs/arc.c: In function 'l2arc_log_prefetch':
+../../module/zfs/arc.c:6770:6: warning: passing argument 5 of 'zio_read_phys' from incompatible pointer type [enabled by default]
+../../include/sys/zio.h:492:15: note: expected 'struct abd_t *' but argument is of type 'uint8_t *'
+../../module/zfs/arc.c: In function 'l2arc_dev_uberblock_update':
+../../module/zfs/arc.c:6813:6: warning: passing argument 5 of 'zio_write_phys' from incompatible pointer type [enabled by default]
+../../include/sys/zio.h:497:15: note: expected 'struct abd_t *' but argument is of type 'struct l2arc_uberblock_phys_t *'
+../../module/zfs/arc.c: In function 'l2arc_dev_log_commit':
+../../module/zfs/arc.c:6883:6: warning: passing argument 5 of 'zio_write_phys' from incompatible pointer type [enabled by default]
+../../include/sys/zio.h:497:15: note: expected 'struct abd_t *' but argument is of type 'uint8_t *'
CC blkptr.lo
CC bplist.lo
CC bpobj.lo
@@ -569,6 +581,18 @@
LD [M] /home/robert/gits/zfs/zfs/module/zcommon/zcommon.o
CC [M] /home/robert/gits/zfs/zfs/module/zfs/../../module/zfs/abd.o
CC [M] /home/robert/gits/zfs/zfs/module/zfs/../../module/zfs/arc.o
+/home/robert/gits/zfs/zfs/module/zfs/../../module/zfs/arc.c: In function ‘l2arc_uberblock_read’:
+/home/robert/gits/zfs/zfs/module/zfs/../../module/zfs/arc.c:6566:6: warning: passing argument 5 of ‘zio_read_phys’ from incompatible pointer type [enabled by default]
+/home/robert/gits/zfs/zfs/include/sys/zio.h:492:15: note: expected ‘struct abd_t *’ but argument is of type ‘struct l2arc_uberblock_phys_t *’
+/home/robert/gits/zfs/zfs/module/zfs/../../module/zfs/arc.c: In function ‘l2arc_log_prefetch’:
+/home/robert/gits/zfs/zfs/module/zfs/../../module/zfs/arc.c:6770:6: warning: passing argument 5 of ‘zio_read_phys’ from incompatible pointer type [enabled by default]
+/home/robert/gits/zfs/zfs/include/sys/zio.h:492:15: note: expected ‘struct abd_t *’ but argument is of type ‘uint8_t *’
+/home/robert/gits/zfs/zfs/module/zfs/../../module/zfs/arc.c: In function ‘l2arc_dev_uberblock_update’:
+/home/robert/gits/zfs/zfs/module/zfs/../../module/zfs/arc.c:6813:6: warning: passing argument 5 of ‘zio_write_phys’ from incompatible pointer type [enabled by default]
+/home/robert/gits/zfs/zfs/include/sys/zio.h:497:15: note: expected ‘struct abd_t *’ but argument is of type ‘struct l2arc_uberblock_phys_t *’
+/home/robert/gits/zfs/zfs/module/zfs/../../module/zfs/arc.c: In function ‘l2arc_dev_log_commit’:
+/home/robert/gits/zfs/zfs/module/zfs/../../module/zfs/arc.c:6883:6: warning: passing argument 5 of ‘zio_write_phys’ from incompatible pointer type [enabled by default]
+/home/robert/gits/zfs/zfs/include/sys/zio.h:497:15: note: expected ‘struct abd_t *’ but argument is of type ‘uint8_t *’
CC [M] /home/robert/gits/zfs/zfs/module/zfs/../../module/zfs/blkptr.o
CC [M] /home/robert/gits/zfs/zfs/module/zfs/../../module/zfs/bplist.o
CC [M] /home/robert/gits/zfs/zfs/module/zfs/../../module/zfs/bpobj.o

@yshui
Copy link
Contributor Author

yshui commented Oct 21, 2014

@algragon You probably should not use this with #2129.

@kernelOfTruth
Copy link
Contributor

@yshui sure will do, but currently it seems to work fine
it clearly works - otherwise working with git and kernel sources would be awfully slow ;)

I'm glad it does - besides that this SSD doesn't survive that huge amount of write/read circles (it's already 1/3 of what's allowed - thus I'm concerned if it has to be wiped when l2arc is failing/faulting)

seems like I can trigger it with the following (will have to try after next reboot):

  • either shutting down or rebooting
  • mount points aren't automatically populated - thus: zpool status -v
    ==> from time to time, or when after having booted into windows: the mentioned error message;
    ==> this was a habit since in the past I had checksum errors with both devices which seems to have been caused by the kernel, now with 3.17-based kernel it doesn't seem to appear anymore
  • BUT: when doing zfs mount -a ; the zvols are getting mounted/loaded and l2arc continues to work fine, not faulting

will observe more ...

thanks

@algragon
Copy link

@yshui Yes, I decided to skip #2672 for now, being more interested in getting ZoL to work with 32-bit.
I just saw @kernelOfTruth post his patchset in #2129#issuecomment-57924154 when I started to look into building my own zfs module and tried it.

I guess depending on which patchset gets mainlined first, the other one will have to be adjusted.

@maci0
Copy link
Contributor

maci0 commented Oct 21, 2014

@algragon whats your specific usecase for 32-bit anyway? just curious.

@algragon
Copy link

@maci0 I got a little fileserver at home with an Atom D525, which only has 32-bit, and I would like to convert the mdadm-raid1/ext4 to zfs. When I tried the mainline 0.6.3 module for ubuntu 14.04, I got lots of vmalloc-related stalls/hangs, so I'm testing a patched version of ZoL.
I know, one should use ecc-ram with zfs...

@josla972
Copy link

josla972 commented Nov 1, 2014

I would like to try this out. So I cloned and made a local branch from 0.6.3, I then fetched and merged yshui:illumos-3525 into this branch, and built with the following commands:

./configure --prefix=/usr --build=x86_64-pc-linux-gnu --host=x86_64-pc-linux-gnu --mandir=/usr/share/man --infodir=/usr/share/info --datadir=/usr/share --sysconfdir=/etc --localstatedir=/var/lib --libdir=/usr/lib64 --disable-silent-rules --disable-dependency-tracking --docdir=/usr/share/doc/zfs-0.6.3 --enable-shared --disable-static --bindir=/bin --sbindir=/sbin --with-config=user --with-linux= --with-linux-obj= --with-udevdir=/lib/udev --with-blkid --disable-debug
make -j5
make install
./configure --prefix=/usr --build=x86_64-pc-linux-gnu --host=x86_64-pc-linux-gnu --mandir=/usr/share/man --infodir=/usr/share/info --datadir=/usr/share --sysconfdir=/etc --localstatedir=/var/lib --libdir=/usr/lib64 --disable-dependency-tracking --docdir=/usr/share/doc/zfs-kmod-0.6.3 --bindir=/bin --sbindir=/sbin --with-config=kernel --with-linux=/usr/src/linux --with-linux-obj=/usr/src/linux --with-spl=/usr/src/spl-0.6.3 --with-spl-obj=/usr/src/spl-0.6.3/3.14.14-gentoo --disable-debug
make -j5
make install

L2ARC does not seem to be saved between reboots, not even when it exceeds 350 MB. Do I have to re-create my pool? Removing the cache device and re-adding it did not solve this issue.

@kernelOfTruth
Copy link
Contributor

@josla972 just go with (zfs) master and/or 0.6.3 and apply the following patch to it:
https://github.com/zfsonlinux/zfs/pull/2672.diff
that has always worked for me

both with

zpool export & zfs import

and

zfs umount -a && zfs mount -a

l2arc's state is preserved

@josla972
Copy link

josla972 commented Nov 1, 2014

The result of what I did is the same code-wise. zpool export will not work very well since its my root system we are talking about. I am unsure if systemd does an export or not on reboot, but I think so since zfs services are enabled.

How do you verify that preservation works? Looking at zpool iostat -v, I notice that the allocated amount of MiB is reset to 43.9M (probably the files needed for boot are cached) after a reboot:

Then after playing around a bit and observing that the allocated cache has increased to, say, 350 M, I do a reboot and notice that it still is reset to 43.9M.

@kernelOfTruth
Copy link
Contributor

before a reboot - it e.g. is at:

cache                 -      -      -      -      -      -
  intelSSD180     10.7G  97.9G      2      0  16.7K  45.7K

after a reboot (zfs mount -a)
it's

cache                 -      -      -      -      -      -
  intelSSD180     10.7G  97.9G      2      0  16.7K  45.7K

and/or grows

when the l2arc fails and I had to remove & re-add it

it's usually at

cache                 -      -      -      -      -      -
  intelSSD180     0G  107.9G      2      0  16.7K  45.7K

or something similar

@josla972 what you're currently seeing - that's the behavior I had before applying this patch
not sure what provokes this reset to empty state

@josla972
Copy link

josla972 commented Nov 2, 2014

Since you seem to be a Gentoo user just like me, I could try to reproduce the way you installed the patch. I fear that the paths are not correct when I build with make inside a development directory instead of letting emerge handle it all. Did you let emerge handle it (overlay/edit ebuilt etc) or did you just manually apply the patch and built and installed zfs from a separate dir?

@kernelOfTruth
Copy link
Contributor

I could have let portage handle it but the other patches I use have rejects - so generally doing it manually directly via portage-tree and /var/tmp/portage:

cd /usr/portage/sys-fs/zfs-kmod/
ebuild zfs-kmod-9999.ebuild unpack

(in another terminal window)

cd /var/tmp/portage/sys-fs/zfs-kmod-9999/work/zfs-kmod-9999/
patch -p1 < /usr/src/sources/zfs_patches/zfs/03.10.2014_Move\ ARC\ data\ buffers\ out\ of\ vmalloc_2129/03.10.2014_Move\ ARC\ data\ buffers\ out\ of\ vmalloc_2129.diff
chown -R portage:portage /var/tmp/portage/
ebuild zfs-kmod-9999.ebuild compile install qmerge

not sure if it's really necessary - but in case scripts change - also do the same for sys-fs/zfs

@josla972
Copy link

josla972 commented Nov 3, 2014

Thank you. I got it working now with the zfs-kmod-9999! I failed to apply the patch to 0.6.3 though. I hope this is stable enough.

@kerberizer
Copy link

illumos 3525 is now in code review.

@behlendorf
Copy link
Contributor

Closing. We'll pick up this change once it's merged in to illumos.

@sempervictus
Copy link
Contributor

@kpande: do you have a branch up with the rebase somewhere? Seems a bit less than trivial at first glance :)

@lundman
Copy link
Contributor

lundman commented Jul 8, 2019

I put it into
https://github.com/openzfsonosx/zfs/tree/persist_l2arc

which compiles and runs - however, there are two calls that needed to be changed into abd* calls that I was not sure of. If someone with more abd knowledge than I could check it over...

@gamanakis
Copy link
Contributor

I wanted to experiment with this, so I rebased it on current master. It compiles and runs, no errors. Also no errors/warnings in dmesg.
It can be found in:
https://github.com/gamanakis/zfs/tree/persist_l2arc

@yshui
Copy link
Contributor Author

yshui commented Oct 25, 2019

@behlendorf What's the upstream status of l2arc right now? Is there any news you can share?

@tycho
Copy link
Contributor

tycho commented Oct 25, 2019

I wanted to experiment with this, so I rebased it on current master. It compiles and runs, no errors. Also no errors/warnings in dmesg.
It can be found in:
https://github.com/gamanakis/zfs/tree/persist_l2arc

Big thank you for doing this! Having a cold L2ARC after kernel upgrades/reboots has been the biggest pain point for my setup for a while, so your work is greatly appreciated.

Is there a bug/feature bounty tracker for ZOL? I'd like to post a bounty to prioritize getting this feature integrated, if it will help.

@gamanakis
Copy link
Contributor

Although my rebase runs with no apparent problems, it doesn't make the L2ARC persistent.
Testing with:
fio --ioengine=libaio --direct=1 --name=test --bs=2M --size=1G --readwrite=randread --runtime=600 --time_based --iodepth=64
Monitoring with:
zpool iostat -v 2
After exporting and reimporting the pool, read speeds are lower and iostat reports the cache is empty.

@gamanakis
Copy link
Contributor

gamanakis commented Nov 4, 2019

I think I am making slow progress on this. Pushed new commits in:
https://github.com/gamanakis/zfs/tree/persist_l2arc
It compiles and it actually makes the L2ARC persistent!
Only downside so far is that upon unloading the zfs module (after exporting the pool) it complains that:
Objects remaining in abd_t on __kmem_cache_shutdown().

Further testing would be greatly appreciated.
This is strictly experimental. Do not use in production.
If @yshui, @skiselkov, @lundman or anybody else could take a look I would greatly appreciate it.

@PrivatePuffin
Copy link
Contributor

What's the upstream status of l2arc right now? Is there any news you can share?

@yshui You are in luck: Illumnos has been cordially trashed as upstream, so you would very well be able to finish this now :)

@yshui
Copy link
Contributor Author

yshui commented Nov 11, 2019

@gamanakis
Copy link
Contributor

gamanakis commented Nov 12, 2019

Each abd_get_from_buf() call allocates memory that is not freed by zio_wait() later on. There are several in this patch, 2 in blk_commit(), 1 in dev_hdr_read() and dev_hdr_update(), 1 in blk_read() and 1 in blk_prefetch. In each one of these functions, the abd_t has to be freed properly. This is not trivial to do as it requires a proper zio structure with a callback function.
I am still looking at the code to figure out how this works, but I think this is what is going on.

And then of course, encryption would have to be implemented...

@gamanakis
Copy link
Contributor

I just pushed a new commit addressing those issues, in the quickest way. No error messages anymore.

@gamanakis
Copy link
Contributor

I implemented encryption, seems to be working fine. Just pushed the new commit. I would appreciate testing!

@PrivatePuffin
Copy link
Contributor

@gamanakis If you are the one actually developing it, you can make your own PR here.
It's a nogo if everything gets done outside of the bounds of this repo, it messes things up big time.

Great work btw, how much do you think you have left?

@gamanakis
Copy link
Contributor

gamanakis commented Nov 13, 2019

Will happen soon, I am working on cancelling the rebuild in l2arc_remove_vdev.

@PrivatePuffin
Copy link
Contributor

@gamanakis Awesome, love your dev attitude! :)

@gamanakis
Copy link
Contributor

New pull request submitted:
#9582

@ahrens ahrens mentioned this pull request Nov 13, 2019
12 tasks
@ahrens
Copy link
Member

ahrens commented Nov 13, 2019

Superseded by #9582

@ahrens ahrens closed this Nov 13, 2019
behlendorf pushed a commit that referenced this pull request Apr 10, 2020
This commit makes the L2ARC persistent across reboots. We implement
a light-weight persistent L2ARC metadata structure that allows L2ARC
contents to be recovered after a reboot. This significantly eases the
impact a reboot has on read performance on systems with large caches.

Reviewed-by: Matthew Ahrens <mahrens@delphix.com>
Reviewed-by: George Wilson <gwilson@delphix.com>
Reviewed-by: Ryan Moeller <ryan@iXsystems.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Co-authored-by: Saso Kiselkov <skiselkov@gmail.com>
Co-authored-by: Jorgen Lundman <lundman@lundman.net>
Co-authored-by: George Amanakis <gamanakis@gmail.com>
Ported-by: Yuxuan Shui <yshuiv7@gmail.com>
Signed-off-by: George Amanakis <gamanakis@gmail.com>
Closes #925 
Closes #1823 
Closes #2672 
Closes #3744 
Closes #9582
jsai20 pushed a commit to jsai20/zfs that referenced this pull request Mar 30, 2021
This commit makes the L2ARC persistent across reboots. We implement
a light-weight persistent L2ARC metadata structure that allows L2ARC
contents to be recovered after a reboot. This significantly eases the
impact a reboot has on read performance on systems with large caches.

Reviewed-by: Matthew Ahrens <mahrens@delphix.com>
Reviewed-by: George Wilson <gwilson@delphix.com>
Reviewed-by: Ryan Moeller <ryan@iXsystems.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Co-authored-by: Saso Kiselkov <skiselkov@gmail.com>
Co-authored-by: Jorgen Lundman <lundman@lundman.net>
Co-authored-by: George Amanakis <gamanakis@gmail.com>
Ported-by: Yuxuan Shui <yshuiv7@gmail.com>
Signed-off-by: George Amanakis <gamanakis@gmail.com>
Closes openzfs#925 
Closes openzfs#1823 
Closes openzfs#2672 
Closes openzfs#3744 
Closes openzfs#9582
@PrivatePuffin
Copy link
Contributor

@tycho Bounty was mentioned in this thread, so if interested in crowd-funding zfs please see: #13397

Necro’ing old shit is not appreciated

@jittygitty
Copy link

@Ornias1993 Apologies to anyone upset, was hoping it wouldn't be a nuisance. I had posted in a few tickets where "Bounty" had been discussed in a positive light ( @tyco ), thinking there would be interest in participating in the crowd funding issue I linked.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature Feature request or new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

allow for persistent l2arc