Hey all, I've posted the PR here: https://github.com/openzfs/openzfs/pull/188 . I'm also pinging the ZoL community via the PR where they integrated their own version of this patch; theirs is disabled by default, so they may want to discuss things in the PR or here. Just a heads up.
On Thu, Sep 1, 2016 at 2:44 PM, Bryan Cantrill <[email protected]> wrote: > > Hey Paul, > > Definitely share your concern about dead code, and I think Matt has a > potentially interesting solution to allow for working around past bugs > (though I do wonder how it will fare with features that have several bugs > over time; should it be versioned?), but it also seems like we have > consensus that whatever its ultimate fate, this feature should be disabled > for now with an on-by-default tunable. Given that you already have a patch > that can apply cleanly to illumos, I would say move forward with it -- and > thank you! > > - Bryan > > > On Wed, Aug 31, 2016 at 8:03 PM, Paul Dagnelie <[email protected]> wrote: > >> It sounds like people agree that we should definitely take strong action >> on this, and that an off-by-default flag isn't enough. That's totally >> fine; correctness should definitely be job 1, especially for something that >> people rely on as much as their backup utilities. My only real issue with >> the on-by-default an approach is that it would be easy for this to lead to >> the feature effectively becoming dead code, which would be better off >> removed than present but largely unused and untested. However, it sounds >> like Matt's solution solves that nicely, by disabling the feature until we >> know it's good, and then transparently activating it afterwards. If people >> like Matt's approach, I think an on-by-default kernel tunable to ignore >> hole birth that we push out ASAP would be a good start. I know ZoL has >> already implemented one (and it's going into 0.6.5.8), and I have a version >> of it for DelphixOS that should apply cleanly to illumos. I could start >> work on testing and pushing that immediately. We could then take the time >> to work out all the hole birth issue, implement the backwards-compatible >> featureflags, and push those upstream without being rushed by having the >> hole_birth issues live in the wild hanging over us. >> >> On Wed, Aug 31, 2016 at 7:40 PM, Matthew Ahrens <[email protected]> >> wrote: >> >>> I agree that correctness is job #1, including in this "corner case" of >>> accidentally leaving a hole unpunched when sending/receiving. >>> >>> We should add a kernel tunable to ignore the hole birth. This is >>> low-effort and low-risk, and allows those "in the know" to immediately >>> resolve their problem. I would be open to having this be set to disable >>> hole birth behavior by default until we can implement the more complete >>> solution described below. >>> >>> I know we have improved the performance of handling holes in >>> send/receive since the hole birth feature was first introduced, and I >>> appreciate the data points that turning it off doesn't seem to hurt. But >>> I'd like to see/do a more thorough analysis before throwing it out >>> altogether (though it would be a relief if we could do so, given how much >>> headache this feature has caused). In particular, incremental sends of a >>> large zvol with lots of large, discontiguous holes will result in lots of >>> FREE records without the hole_birth feature. In the worst case, each >>> modified block could result in 8KB of data and 156KB of FREE records (312 >>> bytes/record * 512, if every other block is a hole in a 128KB indirect >>> block (which has 1024 block pointers)). So hopefully most cases are not >>> close to the worst one :-) >>> >>> As a more complete, permanent solution, I would like to propose this as >>> the first use case of a new type of feature flag: the "transparent, fully >>> backward compatible feature flag". The problem this solves is the need to >>> know last time old software wrote to pool, because old software does not >>> maintain certain data structures (e.g. does not correctly maintain hole >>> BP’s birth times - this could also be used for other things like additional >>> space accounting). >>> >>> In this case, there was a bug that caused hole BP’s birth times to not >>> be maintained correctly. Holes with birth times that were written with the >>> buggy software can not be trusted, but we can trust holes that were written >>> with newer software. By adding a fully-backwards-compatible feature flag >>> to the new (fixed) software, we can say “since TXG X, we have only used new >>> software that correctly maintains the holes birth times, and therefore we >>> can trust hole birth times that are > X (and when sending from a TXG > X, >>> birth times of 0 can be trusted)". >>> >>> Note: no user action is required (e.g. “enabling” the feature), and the >>> feature is not listed in the UI, and is not documented. In other words, >>> this is purely an implementation detail. >>> >>> Here's a brief design of how this could be implemented: >>> >>> New on-disk structures: >>> - TXG range (mintxg, maxtxg) in which only new software has written to >>> pool >>> - stored per backwards-compatible feature >>> - stored in object in MOS >>> >>> new behaviors: >>> - when open pool: >>> - if object does not mention this feature, or if this feature’s >>> maxtxg != last written txg, store new range (mintxg = maxtxg = current_txg) >>> - when sync each txg: >>> - assert maxtxg == current_txg - 1 >>> - store maxtxg = current_txg >>> - query: when was feature enabled: return “mintxg”. We know that new >>> software has been continuously in use since this txg. >>> >>> --matt >>> >>> On Wed, 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 >>>>> >>>> >>>> >>> >> >> >> -- >> Paul Dagnelie >> > > *illumos-developer* | Archives > <https://www.listbox.com/member/archive/182179/=now> > <https://www.listbox.com/member/archive/rss/182179/25088305-e49d3582> | > Modify > <https://www.listbox.com/member/?&;> > Your Subscription <http://www.listbox.com> > -- Paul Dagnelie ------------------------------------------- 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
