While I no longer have an economic horse in this race, I absolutely concur Bryan. Correctness trumps other concerns IMO.
Sent from my iPhone > On Aug 31, 2016, at 4:51 PM, Bryan Cantrill <[email protected]> wrote: > > > So, I have a different perspective. Given that the failure mode here is > severe (namely, silent data corruption) I think our collective disposition > needs to be to ignore hole birth on zfs send by default. To claim that this > is heavy handed (that is, that the hole birth optimization is so essential to > the performance of so many that disabling it would be debilitating) is also > to effectively claim that this condition is common and that silent data > corruption is therefore rampant -- which should in turn strengthen our > resolve to eliminate it, regardless the cost. For whatever it's worth, for > our use case we believe the optimization afforded by tracking hole birth is > broadly immaterial -- and that disabling this optimization has similarly > little effect on our systems. > > Either way, the system should be correct first -- especially given the acute > failure mode -- and hole birth should be ignored by zfs send until we know > that this work is entirely correct. > > - Bryan > >> On Wed, Aug 31, 2016 at 8:58 AM, Paul Dagnelie <[email protected]> wrote: >> I think that the best approach, from my perspective, is to have an >> off-by-default flag that lets you ignore the effects of hole birth. While >> we're doing that, we should keep trying to fix all of these issues; we've >> made good progress, and I suspect there aren't many issues left. I think >> this approach does at least an ok job of addressing the needs of everyone >> affected: >> 1) Developers can keep developing, and this approach has a fairly low >> implementation cost >> 2) Users new to ZFS will get to use the feature by default, and will be >> unaffected by any previous hole_birth bugs, because they have no data from >> before the fixes >> 3) Users who already have ZFS but didn't have corrupt data will get to use >> the feature by default, and also will be unaffected by earlier bugs because >> their data is not corrupt. >> 4) Users who already have ZFS and do have corrupt data are probably aware of >> the issue; they will hopefully be paying enough attention to modify the flag >> when they upgrade to the new version. They won't be able to use hole_birth >> until all their old data falls out of whatever retention policies they have >> set up, or they recreate it. >> >> A flag (undocumented?) to zfs send that forced the use of hole_birth might >> not be a bad idea, because it would let group 4 take advantage of the >> feature on new data they create. The only people this approach doesn't help >> are users who have corrupted data, but aren't aware of it; if we think >> that's a large population, we might want to take a different approach, like >> the "hole_birth_fixed" featureflag. >> >> On Wed, Aug 31, 2016 at 4:02 AM, Rich <[email protected]> wrote: >>> Hi all, >>> I wanted to prod this discussion back to life again, given the existence of >>> two more hole_birth flaws in the intervening period, namely Illumos #7176 >>> and ZoL #4996. >>> >>> So, if we'd opted for a feature flag to mark "fixed" before this point, >>> we'd be in a position of needing another one (or a mechanism to push the >>> enabled_txg forward) to indicate that "actually, after THIS POINT, we don't >>> think there's any more known bugs." >>> >>> I dislike the idea of whack-a-mole on revisions that are "safe", or manual >>> intervention to detect that, but I'm not sure there's a nicer solution than >>> either that or defaulting to discarding hole_birth metadata without an >>> explicit flag to zfs send like -e or -L. >>> >>> If anyone has any nicer suggestions, or thoughts on how to better handle >>> this recurring problem, I'd love to have a better answer to this. >>> >>> Thanks, >>> - Rich >>> >>>> On Fri, Jul 8, 2016 at 1:31 PM, Boris <[email protected]> wrote: >>>> So, as we found out a short time ago, this is actually fixed in ZoL as >>>> well, but after the 0.6.5.7 >>>> >>>> From: Rich <[email protected]> >>>> Sent: Friday, July 8, 2016 12:37:52 PM >>>> To: [email protected] >>>> Cc: developer; Developer Lists Illumos >>>> >>>> Subject: Re: [developer] Improvements to 6513 handling >>>> >>>> Hi Boris, >>>> I now have working code that implements this feature, and defaults to >>>> ignoring hole_birth data for sends if this feature is not enabled; I'm >>>> going to post patches for it after I've tested it on both ZoL and illumos. >>>> >>>> The latter portion of the above is easily changed, but "always correct and >>>> marginally less efficient for old data" seemed a better default than >>>> "usually correct". >>>> >>>> For a filesystem that prides itself on not allowing silent corruption, >>>> requiring manual detection and intervention for correctness seems >>>> unreasonable to me. >>>> >>>> - Rich >>>> >>>>> On Fri, Jul 8, 2016 at 12:15 PM, Boris <[email protected]> wrote: >>>>> Hi, Rich, >>>>> >>>>> >>>>> >>>>> I agree that unconditional switch using the tunable is heavy if done 'as >>>>> a matter of standard practice' as opposed to 'for a short time, to fixup >>>>> the known corrupted backups'. >>>>> >>>>> >>>>> >>>>> To clarify the earlier suggestions, people with data affected by the bug >>>>> can do two things: >>>>> >>>>> >>>>> >>>>> 1) install the code with 6513 fix and the patch with the tunable, then >>>>> temporarily turn off the hole birth optimization, resend the 'difference' >>>>> (a selected subset of incrementals) affected by the problem, then turn >>>>> the optimization back on >>>>> >>>>> 2) install the code with 6513 fix without the patch, do a full send of >>>>> the affected snapshots >>>>> >>>>> >>>>> >>>>> For 2) the non-incremental send would need to happen only once per the >>>>> affected snapshot lineage. Once the missed holes are re-instated with the >>>>> full send, the new fixed code will perform proper incremental sends. >>>>> >>>>> >>>>> >>>>> 1) is potentially more optimal in terms of resources (network bw, etc.) >>>>> >>>>> 2) is potentially simpler from the operational standpoint, does not >>>>> require building/installing patched code, twiddling the tunables, etc. >>>>> >>>>> >>>>> >>>>> Boris. >>>>> >>>>> >>>>> >>>>> >>>>> >>>>> >>>>> >>>>> From: Rich <[email protected]> >>>>> Sent: Friday, July 8, 2016 11:26:27 AM >>>>> To: developer >>>>> Cc: Developer Lists Illumos >>>>> Subject: Re: [developer] Improvements to 6513 handling >>>>> >>>>> Hi Boris, >>>>> A full send of the affected snapshots should be safe, AIUI - but that >>>>> means people would need to do non-incremental snapshot sends to be >>>>> certain of not hitting this bug, which becomes increasingly infeasible as >>>>> your datasets grow. >>>>> >>>>> If we're looking for the simplest solution without risk of data >>>>> corruption, unconditionally ignoring the hole_birth data for doing a zfs >>>>> send fits the bill, but seems a bit heavy-handed. >>>>> >>>>> This seemed like the best way to permit people to safely send older >>>>> datasets while also permitting use of the hole_birth data going forward. >>>>> >>>>> - Rich >>>>> >>>>>> On Fri, Jul 8, 2016 at 11:03 AM, Boris <[email protected]> wrote: >>>>>> Hi, Rich, >>>>>> >>>>>> >>>>>> >>>>>> perhaps there is a simpler solution here. >>>>>> >>>>>> >>>>>> >>>>>> I think for the datasets affected by this feature, a full (not >>>>>> incremental) send of the source snapshot that has some holes that have >>>>>> not been transmitted by the faulty incremental send code, should fix the >>>>>> issue, as far as the on-disk layout is concerned. >>>>>> >>>>>> >>>>>> >>>>>> Boris. >>>>>> >>>>>> From: Rich <[email protected]> >>>>>> Sent: Thursday, July 7, 2016 8:30:54 PM >>>>>> To: developer; Developer Lists Illumos >>>>>> Subject: [developer] Improvements to 6513 handling >>>>>> >>>>>> Hi all, >>>>>> >>>>>> So, ZFS on Linux just noticed it was getting bitten by what ultimately >>>>>> turned out to be Illumos #6513, partially filled holes losing birth time. >>>>>> >>>>>> Implementing that fix removes this problem for new data, but on all >>>>>> platforms, this doesn't help data already written on existing pools, >>>>>> getting munged silently in incremental sends forever. >>>>>> >>>>>> pcd pointed out that a relatively trivial workaround would be possible >>>>>> by simply ignoring the hole_birth metadata with something like a global >>>>>> tunable, but that seems too heavy-handed to me - either you're disabling >>>>>> the feature everywhere because you don't know when you can start >>>>>> trusting the birth times, or you're risking silent mangling of affected >>>>>> files forever. >>>>>> >>>>>> I'd like to suggest using a read-compatible feature, call it something >>>>>> like hole_birth_fix, in conjunction with the enabled_txg feature, to >>>>>> permit a reasonable default of ignoring hole_birth information before >>>>>> the hole_birth_fix feature was enabled, but still permitting use of it >>>>>> afterward. >>>>>> >>>>>> This has the unfortunate behavior of breaking write support if you >>>>>> enable hole_birth_fix and then try to go back to a prior codebase, but I >>>>>> can't think of a reasonable way to avoid this. >>>>>> >>>>>> I filed illumos #7175 to track this proposal - I'll happily write the >>>>>> code to implement this shortly. >>>>>> >>>>>> (Apologies if I've over-CCed or missed someone I should be asking for >>>>>> comment, I've not done this workflow before.) >>>>>> >>>>>> - Rich >> >> >> >> -- >> Paul Dagnelie > > openzfs-developer | Archives | Modify Your Subscription ------------------------------------------- openzfs-developer Archives: https://www.listbox.com/member/archive/274414/=now RSS Feed: https://www.listbox.com/member/archive/rss/274414/28015062-cce53afa Modify Your Subscription: https://www.listbox.com/member/?member_id=28015062&id_secret=28015062-f966d51c Powered by Listbox: http://www.listbox.com
