Project

General

Profile

Bug #8731

ASSERT3U(nui64s, <=, UINT16_MAX) fails for large blocks

Added by Andriy Gapon almost 2 years ago. Updated over 1 year ago.

Status:
Closed
Priority:
Normal
Assignee:
-
Category:
zfs - Zettabyte File System
Start date:
2017-10-26
Due date:
% Done:

100%

Estimated time:
Difficulty:
Medium
Tags:
needs-triage

Description

annotate_ecksum() asserts that nui64s, calculated as nui64s = size / sizeof (uint64_t), is not greater than UINT16_MAX.
This restriction is needed because histograms of incorrectly set and cleared bits have 16 bit counters and if the buffer consists of too many 64-bit words, then a counter can potentially overflow producing an incorrect result.
When the largest buffer size was 128KB the greatest value of nui64s was 16K, well within the limit.
But now we have support for large buffers and for buffer sizes of 512KB and above the restriction is violated.

Per discussion with Matt Ahrens there are two ways to fix this problem.

One is to clip the histogram counters at UINT16_MAX to avoid the wrap-around.
So, even if the values are clipped they still give an idea about the scale of the problem and the pattern of the corruption.

The other is to extend the counter type to 32 bits.
This should cover all supported buffer sizes.
But there is a concern about consumers of the histograms.

History

#1

Updated by Andriy Gapon almost 2 years ago

This PR https://github.com/openzfs/openzfs/pull/483 implements the second approach.

I tried to analyze the potential impact.
Both histogram arrays currently have size of 128 bytes each (64 bit positions * 2 bytes).
They are posted as nvlist elements of type DATA_TYPE_UINT16_ARRAY.
So, changing the type to uint32_t would double array sizes to 256 bytes.
That value does not seem like it's crossing any threshold. I think that the
nvlist and fm code should be able to deal with the larger size without any issues.
As to the consumers, I could not find any in-tree consumer that specifically
looks for those two fields, bad_set_histogram and bad_cleared_histogram.
Any code code that processes nvlists in a generic way should be able to deal
with the type change. And any code, if any at all, that expects those elements
to be DATA_TYPE_UINT16_ARRAY should simply fail to extract the arrays.
So, at the very least, the change should not cause any silent misinterpretation
of the data.

#2

Updated by Electric Monk over 1 year ago

  • Status changed from New to Closed
  • % Done changed from 0 to 100

git commit a6c1eb3c08094a6db69aa1dc6315bc814e82e79c

commit  a6c1eb3c08094a6db69aa1dc6315bc814e82e79c
Author: Andriy Gapon <avg@FreeBSD.org>
Date:   2018-01-24T16:10:02.000Z

    8731 ASSERT3U(nui64s, <=, UINT16_MAX) fails for large blocks
    Reviewed by: Matthew Ahrens <mahrens@delphix.com>
    Approved by: Dan McDonald <danmcd@joyent.com>

Also available in: Atom PDF