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/21175001-8b3b9e0a> |
> Modify
> <https://www.listbox.com/member/?&;>
> Your Subscription <http://www.listbox.com>
>
-------------------------------------------
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
