ZFS_DEBUG + enable ZFS ASSERTS with INVARIANTS any objections?

[Justin T. Gibbs]

On Jul 1, 2013, at 6:04 AM, "Steven Hartland" <killing at multiplay.co.uk> wrote:

> 
> ----- Original Message ----- From: "Justin T. Gibbs" <gibbs at FreeBSD.org>
> To: "Steven Hartland" <smh at freebsd.org>
> Cc: <zfs-devel at FreeBSD.org>
> Sent: Monday, July 01, 2013 4:06 AM
> Subject: Re: ZFS_DEBUG + enable ZFS ASSERTS with INVARIANTS any objections?
> 
> 
>> On Jun 29, 2013, at 5:47 PM, "Steven Hartland" <smh at freebsd.org> wrote:
>>> The attached patch adds a ZFS_DEBUG option as well
>>> as enabling ZFS ASSERTS by default when the kernel
>>> is compiled with INVARIANTS.
>>> This should enable us to get better test coverage
>>> from CURRENT users and has already enabled me to
>>> catch a number of issues when testing code + fix
>>> for invalid ASSERT removed by r252390.
>>> So the question is there any objections to this
>>> patch?
>>>  Regards
>>>  Steve<zfs-debug.patch>_______________________________________________
>>> zfs-devel at freebsd.org mailing list
>>> http://lists.freebsd.org/mailman/listinfo/zfs-devel
>>> To unsubscribe, send any mail to "zfs-devel-unsubscribe at freebsd.org"
>> The patch seems to be only a partial solution. If DEBUG is the
>> OpenSolaris equivalent of our INVARIANTS, shouldnt DEBUG be defined
>> in all contexts when INVARIANTS is enabled?  Otherwise, ASSERTS
>> that depend on state maintained in #if DEBUG code in a .c file will
>> unexpectedly fail.
> 
> From what I've seen ZFS code maintains additional state under ZFS_DEBUG
> not DEBUG although ZFS_DEBUG is itself triggered by DEBUG when compiling
> kernel code.
> 
> This is why there's a few extra #ifdef ZFS_DEBUG's added so that ASSERTS
> which rely on this information aren't triggered.

Yes, I understand this.  But there is no guarantee that this will
continue to be true for future imports of ZFS or DTrace.  I'm just
trying to advocate for something that is easier to maintain than
what you propose.


>> Just leaving DEBUG defined upon exit of sys/debug.h isn't enough.
>> It appears that dtrace.c uses DEBUG without including (at least
>> directly) this header.  This usage (just like INVARIANTS) should
>> be supported.
> 
> This indeed could be done, however as I'm not a user of dtrace its
> not something I'm really setup to implement / test currently.

The goal of turning on INVARIANTS in head is to get crowd sourced
coverage of diagnostic code.  If this diagnostic code is currently
broken, it will get noticed and fixed long before it hits production
systems.

We use DTrace at Spectra on ZFS_DEBUG kernels (so -DDEBUG is enabled
in all "OpenSolaris" modules).  We haven't noticed new bugs caused
by this.  I haven't tested on head recently, but if it does happen
to be broken there, it would get noticed which would be a good
thing! :-)

>> In your patch, everyone does gets DEBUG if ZFS_DEBUG is set.  While
>> convenient for us ZFS developers, this should also work for someone
>> working on DTrace, or a bug in the OpenSolaris nvpair code, etc.
>> So I'd argue that the option is misnamed for at least the OpenSolaris
>> module.
> 
> Maybe its best to remove the ZFS_DEBUG flag for the time being, as
> the key change is to get ASSERTS enabled under INVARIANTS?
> 
>> On possible solution to this problem would be to grep opt_global.h
>> from within the Makefile for OpenSolaris-ish modules:
>> INVARIANTS_ENABLED!=   grep INVARIANTS ${KERNBUILDDIR}/opt_global.h || true
>> .if !empty(INVARIANTS_ENABLED)
>> CFLAGS += -DDEBUG
>> .endif
>> If ZFS_DEBUG were also made a global option, similar logic could
>> fail the ZFS module build if ZFS_DEBUG is enabled without INVARIANTS.
> 
> I'm not sure we want DEBUG enabled in modules triggered by INVARIANTS
> as this does add quite a bit more than just additional ASSERT checks,
> which as mentioned above is what the patch is designed to achieve.
> 
> My reason for keeping it just to ASSERTs / VERIFYs was to gain the
> main benefit without enabling significant extra code.

I would be okay with this if we were simply taking advantage of
defined idioms in the OpenSolairs code for different levels of
"diagnostic cost".  Right now we have DEBUG and ZFS_DEBUG.  You're
proposing "half DEBUG", but since this doesn't exist upstream, it
is a loaded gun waiting to go off on a future import (when an ASSERT
is added that depends on #ifdef DEBUG .c file code).

Folks already know that INVARIANTS compromises performance.  I doubt
the cost of turning on DEBUG would outweigh the benefit of finally
having this code turned on in head.

> Perhaps adding an additional global flag such as OPENSOLARIS_DEBUG
> in addition to enabling ASSERT checks is the way to go?

Only if you can upstream it.

>   Regards
>   Steve

--
Justin