ZFS_DEBUG + enable ZFS ASSERTS with INVARIANTS any objections?

[Steven Hartland]

----- 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.

> 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.

> 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.

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

    Regards
    Steve

================================================
This e.mail is private and confidential between Multiplay (UK) Ltd. and the person or entity to whom it is addressed. In the event of misdirection, the recipient is prohibited from using, copying, printing or otherwise disseminating it or any information contained in it. 

In the event of misdirection, illegible or incomplete transmission please telephone +44 845 868 1337
or return the E.mail to postmaster at multiplay.co.uk.