1 сентября 2016 г. 4:40:26 CEST, Matthew Ahrens <[email protected]> пишет: >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 >>> >> >> *illumos-developer* | Archives >> <https://www.listbox.com/member/archive/182179/=now> >> <https://www.listbox.com/member/archive/rss/182179/27166212-fc0e512f> >| >> Modify >> <https://www.listbox.com/member/?&;> >> Your Subscription <http://www.listbox.com> >> > >
A short question about the design: can you please elaborate just how is it backwards-compatible? ;-) For example, if I boot into an older BE or LiveCD and write something to the pool, there will be TXGs that are newer than the mintxg but written with potentially buggy software. Or are there failsafes like storing a chain of (mintxg;maxtxg) pairs written only by the bugless version (and a new chain link starts if the largest stored maxtxg is not the current txg at pool import)? Or would this pool just not import (r/w) into older kernels anymore, due to the new feature? Thanks, Jim -- Typos courtesy of K-9 Mail on my Samsung Android ------------------------------------------- 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
