Re: [Patch] Robustly search for ZFS labels & uberblocks

[Vladimir 'φ-coder/phcoder' Serbinenko]

Could you please split this patch? In particular the removing of dprintf
takes to much of this patch and makes it unreadable. Note that we don't
comment out the code, we only remove it.
On 19.09.2011 20:45, Zachary Bedell wrote:
> Greetings,
>
> In working with ZFSonLinux, I found myself with a number of somewhat 
> inconstant pools which failed either when running grub-probe or at boot time. 
>  In all cases, these pools were importable by the ZFS driver and completed a 
> `zpool scrub` with no errors reported, but still Grub wasn't able to touch 
> the pools.  In most (but not all) cases, running `zpool scrub` made the pool 
> accessible to Grub again, though obviously that was less than helpful for 
> boot-time failures.
>
> I'm including an (unfortunately rather large) patch to Grub's ZFS code which 
> fixes several edge cases where Grub is unable to read a pool which is 
> otherwise valid as far as the full ZFS driver is concerned.  Changes include:
>
> ashift:
>
> * Support non-default values of ashift pool attribute.  When ashift is 
> increased beyond 10, the size of the uberblocks changes.  Scan to find 
> uberblocks must account for this and skip over the extra padding.  Based on 
> patch to Grub 0.97 by Hans Rosenfeld at http://www.illumos.org/issues/1303
>
>
> Label parsing changes:
>
> * Access the two end-device labels at label-size aligned location rather than 
> device_end-(2*label_size).  On-disk spec document incorrectly describes 
> end-label locations.  Adapted from Grub 0.97 behavior.
>
> * Scan all readable labels for uberblocks and accept the one with the highest 
> txg/create date.  Previously UB scan would stop if a valid UB was found in 
> Label 0 without checking for newer UB's in the other labels.  All labels must 
> be scanned as a more recent block may be found in the other labels in certain 
> circumstances.  This fixes a case where Grub would be unable to access a ZFS 
> system unless the pool was scrubbed first (but ZFS itself saw no problems & 
> scrub reported zero errors).
>
> * Verify that uberblock txg is greater than that of the label before 
> accepting the UB so that partially written UB's are not considered.
>
> * Verify that underlying data pointed by uberblock ub_rootbp has valid 
> checksums before accepting the UB.  This gives some possibility of booting 
> from a technically invalid pool and effectively falls back to older 
> uberblocks in a case where the correct uberblock is damaged.
>
> * Store pool uuid found during zfs_mount to grub_zfs_data in order to prevent 
> duplicated logic in zfs_uuid.
>
>
> Data integrity:
>
> * Verify checksum in grub_read_data before accepting block.  Attempt to read 
> backup DVA's if checksum on first fails.  Previously backup DVA's were 
> checked only if the physical read failed, not if bad data was read without 
> error.  This resulted in pools which were valid and readable by ZFS (and 
> transparently healed by ZFS' read behavior) being rejected by Grub.
>
>
> Logging/Debugging:
>
> * Provide more debugging output describing inconsistencies found in pool.
>
> * Remove superflous debugging output to reduce bootup time in verbose mode to 
> a mangeable level (~30min down to ~5min to boot w/ 'debug=all' in grub.cfg)
>
>
>
> I do apologies for the monolithic patch.  I took a second look at the changes 
> to try to pull certain elements apart, but I ended up in several situations 
> where the test pools I have snapshotted in VM's each hit two or more of the 
> issues above making testing in isolation difficult.  The total change set is 
> able to probe and boot from all of the odd (as well as all of the normal) 
> pools that I have on hand.  
>
> If remixing this patch in some way would help in integrating it, I'd welcome 
> any feedback on how to better present the changes.
>
> Best regards,
> Zac Bedell
>
>
>
>
> _______________________________________________
> Grub-devel mailing list
> address@hidden
> https://lists.gnu.org/mailman/listinfo/grub-devel


-- 
Regards
Vladimir 'φ-coder/phcoder' Serbinenko

signature.asc
Description: OpenPGP digital signature