----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.csiden.org/r/267/#review882 -----------------------------------------------------------
Looking forward to seeing this in illumos! I'd like George Wilson to also review this after you respond to my feedback. usr/src/uts/common/fs/zfs/arc.c (line 994) <https://reviews.csiden.org/r/267/#comment701> given that you're padding the other structures in the hope of being able to reconstruct the L2ARC even after future on-disk format changes -- it would probably make sense to add a couple words of padding here too (and would bring the size of this up to a power of 2, though maybe that doesn't matter) usr/src/uts/common/fs/zfs/arc.c (lines 1041 - 1051) <https://reviews.csiden.org/r/267/#comment702> why aren't we using the l2arc_log_blkptr_t? usr/src/uts/common/fs/zfs/arc.c (line 1085) <https://reviews.csiden.org/r/267/#comment703> this still needs updating, I think usr/src/uts/common/fs/zfs/arc.c (line 1156) <https://reviews.csiden.org/r/267/#comment704> consider using a static inline func usr/src/uts/common/fs/zfs/arc.c (lines 6220 - 6221) <https://reviews.csiden.org/r/267/#comment705> cstyle: parens around return value usr/src/uts/common/fs/zfs/arc.c (lines 7027 - 7028) <https://reviews.csiden.org/r/267/#comment706> shouldn't this be P2ROUNDUP(), in case sizeof(...) is e.g. 520? Or better yet, vdev_psize_to_asize()? usr/src/uts/common/fs/zfs/arc.c (line 7063) <https://reviews.csiden.org/r/267/#comment710> could we rename this member "l2ad_rebuild_needed"? or _wanted? usr/src/uts/common/fs/zfs/arc.c (line 7207) <https://reviews.csiden.org/r/267/#comment707> why only in kernel? can this not be exercised by ztest? usr/src/uts/common/fs/zfs/arc.c (line 7208) <https://reviews.csiden.org/r/267/#comment708> why record the did rather than the kthread_t? usr/src/uts/common/fs/zfs/arc.c (line 7209) <https://reviews.csiden.org/r/267/#comment709> I think we can make this part of spa_proc usr/src/uts/common/fs/zfs/arc.c (line 7283) <https://reviews.csiden.org/r/267/#comment712> I think that the daddr should already be ashift-aligned, so this should be "daddr + vdev_psize_to_asize(PSIZE())" usr/src/uts/common/fs/zfs/arc.c (line 7285) <https://reviews.csiden.org/r/267/#comment711> It would be nice to somehow make it more explicit that for log blocks, the concept of ASIZE (how much space is allocated for this block) is equal to vdev_psize_to_asize(PSIZE). Maybe even add a macro for LBP_GET_ASIZE() that does this (you'd have to pass in the dev as well)? usr/src/uts/common/fs/zfs/arc.c (line 7290) <https://reviews.csiden.org/r/267/#comment713> maybe first you could ASSERT3U() that sizeof (lb_ptrs) is the same as sizeof (dh_start_lbps)? that way if we change one we will find where to change the other. usr/src/uts/common/fs/zfs/arc.c (lines 7328 - 7329) <https://reviews.csiden.org/r/267/#comment714> how about also (or instead), zfs_dbgmsg()? usr/src/uts/common/fs/zfs/arc.c (lines 7340 - 7341) <https://reviews.csiden.org/r/267/#comment715> I'm confused by the use of PSIZE() here. Why would we care what the compressed size is at this point, given that we are using a l2arc_log_blk_phys_t*, which is uncompressed. usr/src/uts/common/fs/zfs/arc.c (line 7405) <https://reviews.csiden.org/r/267/#comment716> 'hdr' meaning dev->l2ad_dev_hdr? usr/src/uts/common/fs/zfs/arc.c (lines 7424 - 7425) <https://reviews.csiden.org/r/267/#comment717> not very clean to be releasing a lock that the caller acqired. And what about the other return paths that don't drop the lock? Can this be restructured such that the caller drops its lock if we return an error? usr/src/uts/common/fs/zfs/arc.c (line 7433) <https://reviews.csiden.org/r/267/#comment718> logically, it would make sense to check the magic, then the checksum, then the guid. That way we could differentiate between disk corruption and it being from a different pool. That said, isn't there a zfs label on the cache device that ensures we wouldn't open it unless it's part of this pool? usr/src/uts/common/fs/zfs/arc.c (line 7576) <https://reviews.csiden.org/r/267/#comment721> we never have a partially-full log block? usr/src/uts/common/fs/zfs/arc.c (line 7640) <https://reviews.csiden.org/r/267/#comment723> if it's ADDR_UNSET, how can the rest of this work? usr/src/uts/common/fs/zfs/arc.c (line 7647) <https://reviews.csiden.org/r/267/#comment722> I don't understand "connect the l2hdr to the hdr" usr/src/uts/common/fs/zfs/arc.c (line 7679) <https://reviews.csiden.org/r/267/#comment719> these are not prefetch i/os (see ARC_FLAG_PREFETCH), so please use a different name here (and when describing this in comments) usr/src/uts/common/fs/zfs/arc.c (lines 7698 - 7707) <https://reviews.csiden.org/r/267/#comment720> not sure why we need a wrapper function for this? usr/src/uts/common/fs/zfs/arc.c (line 7768) <https://reviews.csiden.org/r/267/#comment727> why not set the BP's byteorder bit? (and use that instead of checking the magic) usr/src/uts/common/fs/zfs/arc.c (line 7774) <https://reviews.csiden.org/r/267/#comment724> can this be moved into the macro? usr/src/uts/common/fs/zfs/arc.c (line 7776) <https://reviews.csiden.org/r/267/#comment725> oh, the PSIZE is the asize? so we don't need to do vdev_psize_to_asize() when reading it? usr/src/uts/common/fs/zfs/arc.c (line 7779) <https://reviews.csiden.org/r/267/#comment726> how about DMU_OTN_UINT64_METADATA? usr/src/uts/common/fs/zfs/arc.c (lines 7845 - 7846) <https://reviews.csiden.org/r/267/#comment729> how about &hdr->dh_spa_guid ? usr/src/uts/common/fs/zfs/arc.c (line 7872) <https://reviews.csiden.org/r/267/#comment730> PSIZE here is not ASIZE? seems we should be consistent. usr/src/uts/common/fs/zfs/arc.c (line 7880) <https://reviews.csiden.org/r/267/#comment731> That is not a valid dmu_object_type_t. I'd suggest either using MDU_OTN_UINT8_{META}DATA, or explicitly document this to be the arc_buf_contents_t (and put a comment by arc_buf_contents_t explaining that it's used on disk and therefore can't be changed!) usr/src/uts/common/fs/zfs/spa.c (lines 1521 - 1524) <https://reviews.csiden.org/r/267/#comment732> I'm always suspicious of the config, because it can be provided by unreliable sources (userland, zpool.cache, outdated labels). Do we know that this is from the config in the MOS (which is reliable)? If not, I guess if do_rebuild is wrong, it isn't the end of the world, but I predict bugs of the form "I installed persistent l2arc bits but it wasn't persisted across reboot [when something weird happened with the config]". - Matthew Ahrens On Nov. 5, 2015, 3:31 p.m., Saso Kiselkov wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.csiden.org/r/267/ > ----------------------------------------------------------- > > (Updated Nov. 5, 2015, 3:31 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/fs/zfs.h bc9f057dd1361ae73a12375515abacd0fed820d2 > 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/sys/spa.h > 7ac78390338a44f7b7658017e1ae8fcc9beb89d6 > usr/src/uts/common/fs/zfs/sys/arc.h > 899b72114b9909098080a5d6bbad1a60808f030c > 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 > > 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
