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

From: Adrian Hunter
Date: Wed Apr 30 2008 - 03:09:26 EST


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?



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/