Re: [RFC] ext4-delayed-allocation.patch

[Andrew Morton <akpm@osdl.org>]

On Fri, 22 Dec 2006 23:28:32 +0300
Alex Tomas <alex@clusterfs.com> wrote:

> +/*
> + * With EXT4_WB_SKIP_SMALL defined the patch will try to avoid
> + * small I/Os ignoring ->writepages() if mapping hasn't enough
> + * contig. dirty pages
> + */
> +#define EXT4_WB_SKIP_SMALL__
> +
> +#define WB_ASSERT(__x__) if (!(__x__)) BUG();

This is unused.  Please kill.

> +#define WB_DEBUG__
> +#ifdef WB_DEBUG
> +#define wb_debug(fmt,a...)	printk(fmt, ##a);
> +#else
> +#define wb_debug(fmt,a...)
> +#endif

It's tiresome for each piece of kernel code to implement private debug
macros.  Why not use pr_debug()?

In general, this patch adds a mountain of code which I suspect just
shouldn't be in a filesystem.  It should be library code in mm/ or fs/. 
It'll take some thought and refactoring and definition of new
address_space_operations entries, etc.  But burying all this inside a
single filesystem is just The Wrong Thing To Do.

I am not inclined to review the code in detail because the lack of suitable
code comments would make that task much larger and significantly less
useful than it should be.  Please spend a day or so commenting the code in
a similar manner to other parts of ext4 and jbd2.  When doing so, put
yourself in the position of an experienced kernel developer who seeks to
understand what the code is doing and why it is doing it.  Skilful
commenting is essential if the code is to be maintainable and it has an
immediate impact upon the code's quality.  Every non-trivial function
should have an introductory comment which tells the reader why this
function exists, what it does and, if not obvious, how it does it.  Don't
bother with kernel-doc comments - for some reason they tend to be useless
for code conprehension.

I shouldn't need to sit here scratching my head and wondering why the heck
a writepages function is running __set_page_dirty_nobuffers().

Thanks.