> On Nov. 5, 2015, 1:14 a.m., Matthew Ahrens wrote: > > usr/src/uts/common/fs/zfs/arc.c, line 1029 > > <https://reviews.csiden.org/r/267/diff/3/?file=18198#file18198line1029> > > > > It would be reasonable to not handle byte swapping - if you move the > > pool (& l2arc) to a different endian, you can deal with losing the contents > > of the l2arc. Not sure if it saves us much code but definitely saves > > testing!
It literally saves two lines. At such a low line count, I'd wager even I can write bug-free code. Yes, this might rarely be used, but this might end up hurting portability especially on the rarer platforms (not necessarily Illumos-supported ones - think Linux). > On Nov. 5, 2015, 1:14 a.m., Matthew Ahrens wrote: > > usr/src/uts/common/fs/zfs/arc.c, line 1031 > > <https://reviews.csiden.org/r/267/diff/3/?file=18198#file18198line1031> > > > > should we add a version field (string?) here so that we can easily > > change the l2arc on-disk format and detect that we need to start over? Or > > did you have an alternate plan for how we will do that? I had originally planned to use feature flags and/or changing the magic, but if you insist, I'll add a field. > On Nov. 5, 2015, 1:14 a.m., Matthew Ahrens wrote: > > usr/src/uts/common/fs/zfs/arc.c, line 1069 > > <https://reviews.csiden.org/r/267/diff/3/?file=18198#file18198line1069> > > > > Given that it isn't a big deal to wipe the l2arc and start over, do we > > really need to reserve padding for future expansion? Seems like we can > > just change this and lose the contents of the l2arc once. Actually, with persistent L2ARC, it is becoming quite a deal to lose it, since it can significantly affect performance (especially considering we plan on offloading most DDT reading on there). Another feature we plan to implement is mirrored L2ARC - this is to just underscore how important it is keeping the L2ARC around. So I disagree that it isn't a big deal. Sometimes there's no way around it, but I'd like to minimize those cases. To that end, we could add a version field in here as well (or use my originally planned mechanism, change magic). > On Nov. 5, 2015, 1:14 a.m., Matthew Ahrens wrote: > > usr/src/uts/common/fs/zfs/arc.c, lines 1092-1096 > > <https://reviews.csiden.org/r/267/diff/3/?file=18198#file18198line1092> > > > > this still works OK with the new SPA_MAXBLOCKSIZE of 16MB? It does, but pushes the minimum size limit of rebuildable L2ARC devices up to ~51 GB. With the old SPA_MAXBLOCKSIZE the limit was 400 MB. I can try to make this switchable based on whether largeblock support is used on the pool. But I'll think about whether this limit makes sense anyhow. I basically only put it in to avoid people testing it on laughably small L2ARC devices (e.g. loopback to a 128mb file). > On Nov. 5, 2015, 1:14 a.m., Matthew Ahrens wrote: > > usr/src/uts/common/fs/zfs/arc.c, lines 1102-1104 > > <https://reviews.csiden.org/r/267/diff/3/?file=18198#file18198line1102> > > > > This is ~50GB by my calculation: > > 3 * 16MB * 1023 Precisely. I'll think about whether we actually need this. > On Nov. 5, 2015, 1:14 a.m., Matthew Ahrens wrote: > > usr/src/uts/common/fs/zfs/arc.c, lines 1108-1109 > > <https://reviews.csiden.org/r/267/diff/3/?file=18198#file18198line1108> > > > > see other comment about byte swapping I'd argue for it. It's only two extra lines of code and I'm certain it's bug-free. > On Nov. 5, 2015, 1:14 a.m., Matthew Ahrens wrote: > > usr/src/uts/common/fs/zfs/arc.c, line 1133 > > <https://reviews.csiden.org/r/267/diff/3/?file=18198#file18198line1133> > > > > why not have these take the "whole field", like callers would pass > > &lbp->lbp_prop as the argument. You're right. Fixed. > On Nov. 5, 2015, 1:14 a.m., Matthew Ahrens wrote: > > usr/src/uts/common/fs/zfs/arc.c, lines 1134-1137 > > <https://reviews.csiden.org/r/267/diff/3/?file=18198#file18198line1134> > > > > use SPA_LSIZEBITS Done. > On Nov. 5, 2015, 1:14 a.m., Matthew Ahrens wrote: > > usr/src/uts/common/fs/zfs/arc.c, lines 1138-1141 > > <https://reviews.csiden.org/r/267/diff/3/?file=18198#file18198line1138> > > > > use SPA_PSIZEBITS (unless you don't intend this to be the same, > > conceptually) Done. > On Nov. 5, 2015, 1:14 a.m., Matthew Ahrens wrote: > > usr/src/uts/common/fs/zfs/arc.c, lines 1142-1145 > > <https://reviews.csiden.org/r/267/diff/3/?file=18198#file18198line1142> > > > > There's only 7 bits in compress field in BP now. Ah, ok, this was changed after I wrote this. Changed this over to 7 bits. > On Nov. 5, 2015, 1:14 a.m., Matthew Ahrens wrote: > > usr/src/uts/common/fs/zfs/arc.c, lines 1155-1167 > > <https://reviews.csiden.org/r/267/diff/3/?file=18198#file18198line1155> > > > > what's the neumonic for "add"? the obvious variable name would be > > "lbp" or _lbp. Yeah, I have no idea what it was supposed to be myself. Changed it to lbp. > On Nov. 5, 2015, 1:14 a.m., Matthew Ahrens wrote: > > usr/src/uts/common/fs/zfs/arc.c, line 5800 > > <https://reviews.csiden.org/r/267/diff/3/?file=18198#file18198line5800> > > > > When we reconstruct, are we able to set the write head back to > > approximately where it was before reboot? Yes we do, it's in l2arc_rebuild(). We set the write hand to the next block following the last log block. > On Nov. 5, 2015, 1:14 a.m., Matthew Ahrens wrote: > > usr/src/uts/common/fs/zfs/sys/spa.h, line 467 > > <https://reviews.csiden.org/r/267/diff/3/?file=18201#file18201line467> > > > > The similar macros in this file don't need NOTREACHED. Why do we need > > it here? Probably a leftover from some older development testing. Removed & lint didn't complain. > On Nov. 5, 2015, 1:14 a.m., Matthew Ahrens wrote: > > usr/src/uts/common/sys/debug.h, line 31 > > <https://reviews.csiden.org/r/267/diff/3/?file=18204#file18204line31> > > > > doesn't look like you actually changed anything in this file. This was a result of the merge of the new hash functions. In both that and persistent L2ARC I had used CTASSERT, so in either case, I had to add the CTASSERT define to debug.h. Illumos happened to merge the new hash function work before persitent L2ARC, while in Nexenta's repo, it's the reverse, hence the lingering commit message. I've removed this line from the illumos upstream, as I'm sure any downstream-directed merge conflicts will be trivial to resolve. - Saso ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.csiden.org/r/267/#review867 ----------------------------------------------------------- On Oct. 31, 2015, 2:47 p.m., Saso Kiselkov wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.csiden.org/r/267/ > ----------------------------------------------------------- > > (Updated Oct. 31, 2015, 2:47 p.m.) > > > Review request for OpenZFS Developer Mailing List and Christopher Siden. > > > Repository: illumos-gate > > > Description > ------- > > This is an upstream port of the Persistent L2ARC feature from Nexenta's repo. > > > Diffs > ----- > > usr/src/uts/common/sys/debug.h 8efc8956b67543c11c012c2185896441c66b546c > usr/src/uts/common/fs/zfs/sys/arc.h > 899b72114b9909098080a5d6bbad1a60808f030c > usr/src/uts/common/fs/zfs/vdev_label.c > f0924ab1e66eaa678540da8925995da6e0e2a29c > usr/src/uts/common/fs/zfs/vdev.c 1c57fce4dcee909b164353181dcd8e2a29ed7946 > usr/src/uts/common/fs/zfs/spa.c 95a6b0fae7760e8a1e8cfc1e657dc22fd9ef3245 > usr/src/uts/common/fs/zfs/arc.c 52f582be1633a8d462105e068ae9679c04753d24 > usr/src/lib/libzpool/common/sys/zfs_context.h > 9e4d8ed0b8ec42be75bb93f44602ac99e907cf00 > usr/src/uts/common/sys/fs/zfs.h bc9f057dd1361ae73a12375515abacd0fed820d2 > usr/src/uts/common/fs/zfs/sys/spa.h > 7ac78390338a44f7b7658017e1ae8fcc9beb89d6 > > Diff: https://reviews.csiden.org/r/267/diff/ > > > Testing > ------- > > Been running in Nexenta's QA process for the past 2+ months. > > > Thanks, > > Saso Kiselkov > >
_______________________________________________ developer mailing list [email protected] http://lists.open-zfs.org/mailman/listinfo/developer
