Re: [RFC PATCH 26/26] UBIFS: include FS to compilation

[Adrian Hunter <ext-adrian.hunter@nokia.com>]

Christoph Hellwig wrote:
> On Mon, Apr 28, 2008 at 10:10:07AM +0300, Adrian Hunter wrote:
>>>> So instead of arguing about this you really ought to look at what
>>>> SLUB, for example, does. It's perfectly okay to have _debugging
>>>> checks_ compiled out (stuff like verify_inode and such) but at the
>>>> assertion level it makes no sense whatsoever!
>>> Yes, having this in filesystems is not very nice.
>> A technical reason would be more compelling than "not very nice".
> 
> These checks are if properly maintained not harmful for the code,
> but they make the code less readable and there's of course the chance
> that they get out of sync.  That's why the term "not very nice" is
> entirely apropinquate here.

What would make them more readable?  We (ok mostly me) are not enthusiastic
about BUG_ON for the following reasons:
	1. When testing and debugging you lose the opportunity to get more
	information and have to reboot the test platform
	2. When the system is in the hands of consumers we don't want it
	to oops under any circumstances.  This is particularly the case
	when looking at things like BUG_ON(!PageLocked(page)).  In an
	embedded system, single CPU, preemption disabled, even in the
	unlikely event this happens, the system would probably get away
	with it.
	3. We don't have many people using UBIFS so hoping they catch
	BUGs is less realistic.  It is also bad for us as we try to
	persuade people of the merits of UBIFS.  Consequently our focus
	is on our own testing (which brings us back to point 1).
	4. We have had a couple of situations with JFFS2 where BUG_ON was
	used incorrectly to handle errors that didn't look like they
	could happen but it turned out they could.  I.e. an error code
	should have been returned and the system allowed to continue.
	In short BUG_ON can be bad all by itself.

Perhaps if we called it dbg_check() instead of ubifs_assert() ?

As for things getting out of sync, anyone making changes to UBIFS, either
without testing or testing without the debug checks turned on, is taking
a much much bigger risk than just letting the checks get out of sync.

I am not sure it is really reasonable to compare the debugging needs of
SLUB with those of UBIFS because UBIFS has a much lower visibility and
usage.  There are far fewer eyes looking at the code and far fewer people
using it.  Since the burden of testing really falls on just a few of us,
we are keen to have lots of checks.

> 
>>> If someone feels
>>> very strong about interface assertation we should add them at the
>>> method level boundary so that the interface is verified for all
>>> filesystems.
>>>
>> The checks are not valid for all file systems.
>>
>> The point of the checks as preconditions is lost if they moved elsewhere.
>>
>> VFS code paths are not simple, so the suggestion is impractical anyway.
> 
> Most of these checks are indeed generic.  Those that arise from special
> filesystem invariants like not having unmapped buffers due to
> implementing ->page_mkwrite should for now be checked in the filesystem,
> although I'd like to make sure this is true for all filesystems
> long-term.

We use both PagePrivate and PageChecked differently to EXT3 etc.

>From the original example, that just leaves PageLocked, but lots of kernel
functions seem to check that, so why not UBIFS too?