Re: -mm -> 2.6.13 merge status

Re: -mm -> 2.6.13 merge status

[Andi Kleen]

Andrew Morton <akpm@osdl.org> writes:

> perfctr
>
>    Not yet, but getting closer.  The PPC64 guys still need to sort out a
>    few interface issues with Mikael.  We might be able to fit this into
>    2.6.13 if people get a move on.

So the problems IA64 had with this are resolved now? 

Unfortunately there is a perfmon for i386/x86-64 implementation
floating around now (with some unmergeable stuff but might be fixable) 
which is kind of competing now :/

> reiser4
> 
>     Merge it, I guess.
> 
>     The patches still contain all the reiser4-specific namespace
>     enhancements, only it is disabled, so it is effectively dead code.  Maybe
>     we should ask that it actually be removed?

Has there been actually any serious review on this? 
Last time I looked there was a lot of very ugly code in there.

Also I'm not sure things like comming with an own profiler
and spinlock debugger are really acceptable. At least this stuff
should be removed too.

-Andi

Re: -mm -> 2.6.13 merge status

[Hans Reiser]

vs and zam, please comment on what we get from our profiler and spinlock
debugger that the standard tools don't supply.  I am sure you have a
reason, but now is the time to articulate it.

We would like to keep the disabled code in there until we have a chance
to prove (or fail to prove) that cycle detection can be resolved
effectively, and then with a solution in hand argue its merits.

Hans

Andi Kleen wrote:

> Also I'm not sure things like comming with an own profiler
>
>and spinlock debugger are really acceptable. At least this stuff
>should be removed too.
>
>-Andi
>
>-
>To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html
>Please read the FAQ at  http://www.tux.org/lkml/
>
>
>  
>

Re: -mm -> 2.6.13 merge status

[Andi Kleen]

On Tue, Jun 21, 2005 at 11:44:55AM -0700, Hans Reiser wrote:
> vs and zam, please comment on what we get from our profiler and spinlock
> debugger that the standard tools don't supply.  I am sure you have a
> reason, but now is the time to articulate it.
> 
> We would like to keep the disabled code in there until we have a chance
> to prove (or fail to prove) that cycle detection can be resolved
> effectively, and then with a solution in hand argue its merits.

How about the review of your code base? Has reiser4 ever been
fully reviewed by people outside your group? 

Normally full review is a requirement for merging.

-Andi

Re: -mm -> 2.6.13 merge status

[Christoph Hellwig]

On Tue, Jun 21, 2005 at 09:56:43PM +0200, Andi Kleen wrote:
> On Tue, Jun 21, 2005 at 11:44:55AM -0700, Hans Reiser wrote:
> > vs and zam, please comment on what we get from our profiler and spinlock
> > debugger that the standard tools don't supply.  I am sure you have a
> > reason, but now is the time to articulate it.
> > 
> > We would like to keep the disabled code in there until we have a chance
> > to prove (or fail to prove) that cycle detection can be resolved
> > effectively, and then with a solution in hand argue its merits.
> 
> How about the review of your code base? Has reiser4 ever been
> fully reviewed by people outside your group? 

I don't think so.  Everyone used the previous criteria of the broken
core changes, broken filesystem semantics and it's own useless abtraction
layer (*) as an excuse not to look deeply at this huge mess yet.

(*) which isn't gone yet.  and I need to look again if the core changes
are okay yet.  And the broken sematics should go completely aswell, if
you want to reintroduce them it needs to happen at the VFS level anyway.

Re: -mm -> 2.6.13 merge status

[Hans Reiser]

Andi Kleen wrote:

>On Tue, Jun 21, 2005 at 11:44:55AM -0700, Hans Reiser wrote:
>  
>
>>vs and zam, please comment on what we get from our profiler and spinlock
>>debugger that the standard tools don't supply.  I am sure you have a
>>reason, but now is the time to articulate it.
>>
>>We would like to keep the disabled code in there until we have a chance
>>to prove (or fail to prove) that cycle detection can be resolved
>>effectively, and then with a solution in hand argue its merits.
>>    
>>
>
>How about the review of your code base? Has reiser4 ever been
>fully reviewed by people outside your group? 
>
>Normally full review is a requirement for merging.
>  
>
V4 has a mailing list, and a large number of testers who read the code
and comment on it.   V4 has been reviewed and tested much more than V3
was before merging.   Given that we sent it in quite some time ago, your
suggestion that an additional review by unspecified additional others be
a requirement for merging seems untimely.  Do you see my point of view
on this?

I would however enjoy receiving coding suggestions at ANY time.  We
don't get as much of that as I would like.   I would in particular love
to have you Andi Kleen do a full review of V4 if you could be that
generous with your time, as I liked much of the advice you gave us on V3. 

Unspecified others doing a review, well, who knows, I will surely take
the time to consider what is said by them though..... 

I would prefer to not get reviews from authors of other filesystems who
prefer their own code, skim through our code without taking the time to
grok our philosophy and approach in depth, and then complain that our
code is different from what they chose to write, and think that our
changing to be like them should be mandated.  I will not name names here....

Some of the suggestions on our mailing list are great, some reflect a
lack of 5 years working with our code, perhaps I should feed our mailing
list into the linux kernel mailing list so that people on the kernel
mailing list are more aware that we exist and are active?

>-Andi
>
>
>  
>

Re: -mm -> 2.6.13 merge status

[Jeff Garzik]

Hans Reiser wrote:
> V4 has a mailing list, and a large number of testers who read the code
> and comment on it.   V4 has been reviewed and tested much more than V3
> was before merging.   Given that we sent it in quite some time ago, your
> suggestion that an additional review by unspecified additional others be
> a requirement for merging seems untimely.  Do you see my point of view
> on this?
> 
> I would however enjoy receiving coding suggestions at ANY time.  We
> don't get as much of that as I would like.   I would in particular love
> to have you Andi Kleen do a full review of V4 if you could be that
> generous with your time, as I liked much of the advice you gave us on V3. 
> 
> Unspecified others doing a review, well, who knows, I will surely take
> the time to consider what is said by them though..... 
> 
> I would prefer to not get reviews from authors of other filesystems who
> prefer their own code, skim through our code without taking the time to
> grok our philosophy and approach in depth, and then complain that our
> code is different from what they chose to write, and think that our
> changing to be like them should be mandated.  I will not name names here....


The Linux system isn't the greatest in the world, but here's reality: 
when a merge is imminent, a lot more attention is paid.

Andrew regularly uses this knowledge of human psychology to his (and 
Linux's) benefit :)

The MAJOR downside is that merge-stopping issues are sometimes ignored 
until an upstream merge is imminent.

If you want to get your code merged, you gotta work with the system, and 
LISTEN to the feedback.

	Jeff, who doesn't have a filesystem axe to grind

Re: -mm -> 2.6.13 merge status

[Andi Kleen]

On Tue, Jun 21, 2005 at 06:38:07PM -0700, Hans Reiser wrote:
> V4 has a mailing list, and a large number of testers who read the code
> and comment on it.   V4 has been reviewed and tested much more than V3
> was before merging.   Given that we sent it in quite some time ago, your
> suggestion that an additional review by unspecified additional others be
> a requirement for merging seems untimely.  Do you see my point of view
> on this?

The point of the merge review is that people who are familiar with the existing
Linux code double check that the way your code interfacts
with the rest of the kernel is sane, does not have obvious bugs and follows the 
existing good practice. 

Once the code is in mainline it will get maintained and fixed when needed, 
but to make this possible without undue work on the mainline hackers it is needed
to do a full review first. 

AFAIK reiserfs has not gotten such a full review yet.

Also good reviewers are rare so it is not a good idea to be picky here.

> Unspecified others doing a review, well, who knows, I will surely take
> the time to consider what is said by them though..... 
> 
> I would prefer to not get reviews from authors of other filesystems who
> prefer their own code, skim through our code without taking the time to
> grok our philosophy and approach in depth, and then complain that our
> code is different from what they chose to write, and think that our
> changing to be like them should be mandated.  I will not name names here....

Someone qualified to review a new file system for inclusion will have need necessary 
some experience in Linux file systems, and that can be hardly gotten without ever 
having touched one.  So you will have to live with other file system authors 
commenting on your code.

The main philosophy that is of concern here is also the philosophy of the 
core VFS and other kernel interfaces.

> Some of the suggestions on our mailing list are great, some reflect a
> lack of 5 years working with our code, perhaps I should feed our mailing
> list into the linux kernel mailing list so that people on the kernel
> mailing list are more aware that we exist and are active?

Nobody doubts that you are active. Just there are doubts that your
code follows the Linux coding standards enough to not be a undue
mainteance burden in mainline.  A review with the following changes
could probably fix that.

-Andi

Re: -mm -> 2.6.13 merge status

[Hans Reiser]

Andi Kleen wrote:

>
> Just there are doubts that your
>code follows the Linux coding standards enough to not be a undue
>mainteance burden in mainline. 
>
We get only a few bugfixes from outsiders, and the rest are done by us. 
The customers who buy licenses in addition to the GPL from us for
hundreds of thousands of dollars tend to make remarks to the effect of
"we licensed your code for more money in part because it was way easier
to work on than XXX linux filesystem".

I like feedback on our code, and I particularly like feedback from a Mr.
Andi Kleen, but there is no need to tie it to merging.  If, however, it
serves as an effective excuse to get some of your time allocated by SuSE
management, sure, go for it.;-)

Hans

Re: -mm -> 2.6.13 merge status

[Jeff Garzik]

Hans Reiser wrote:
> I like feedback on our code, and I particularly like feedback from a Mr.
> Andi Kleen, but there is no need to tie it to merging.  If, however, it
> serves as an effective excuse to get some of your time allocated by SuSE
> management, sure, go for it.;-)


All merges of new code go like this.  You've been around here for a 
while, this should not be a shock.

"Hans' team says its good stuff" is not a criteria for merging.

	Jeff

Re: -mm -> 2.6.13 merge status

[Hans Reiser]

Jeff Garzik wrote:

>
>
> "Hans' team says its good stuff" is not a criteria for merging.
>
>    

Try benchmarking it.  Maybe benchmarks mean more than our
chattering..... at least to the users.....

Re: -mm -> 2.6.13 merge status

[Jeff Garzik]

Hans Reiser wrote:
> Jeff Garzik wrote:
>>"Hans' team says its good stuff" is not a criteria for merging.

> Try benchmarking it.  Maybe benchmarks mean more than our
> chattering..... at least to the users.....

Still not a criteria for merging.

We have to care about the code behind the benchmarks.

	Jeff

Re: -mm -> 2.6.13 merge status

[Pekka Enberg]

Hi Hans,

On 6/22/05, Hans Reiser <reiser@namesys.com> wrote:
> I would in particular love to have you Andi Kleen do a full review of V4
> if you could be that generous with your time, as I liked much of the
> advice you gave us on V3.

Well, I am not Andi Kleen and this is not even in the ballpark of a full
review but here goes...

P.S. Would it be possible to have a version without the plugin stuff
submitted (and perhaps file as directory)? It would make much more
sense for the rest of us to review reiser4 without the most controversial
bits and get the bits that people agree on merged.

                                     Pekka

> --- /dev/null	2003-09-23 21:59:22.000000000 +0400
> +++ linux-2.6.11-vs/fs/reiser4/pool.c	2005-06-03 17:49:38.668204642 +0400
> +/* initialise new pool */
> +reiser4_internal void
> +reiser4_init_pool(reiser4_pool * pool /* pool to initialise */ ,
> +		  size_t obj_size /* size of objects in @pool */ ,
> +		  int num_of_objs /* number of preallocated objects */ ,
> +		  char *data /* area for preallocated objects */ )
> +{
> +	reiser4_pool_header *h;
> +	int i;
> +
> +	assert("nikita-955", pool != NULL);

These assertion codes are meaningless to the rest of us so please drop
them.

> --- /dev/null	2003-09-23 21:59:22.000000000 +0400
> +++ linux-2.6.11-vs/fs/reiser4/type_safe_hash.h	2005-06-03 17:49:38.751209197 +0400
> @@ -0,0 +1,320 @@
> +/* Copyright 2001, 2002, 2003 by Hans Reiser, licensing governed by
> + * reiser4/README */
> +
> +/* A hash table class that uses hash chains (singly-linked) and is
> +   parametrized to provide type safety.  */

This belongs to include/linux, not reiser4.

> --- /dev/null	2003-09-23 21:59:22.000000000 +0400
> +++ linux-2.6.11-vs/fs/reiser4/type_safe_list.h	2005-06-03 17:49:38.755209417 +0400
> @@ -0,0 +1,436 @@
> +/* Copyright 2001, 2002, 2003 by Hans Reiser, licensing governed by reiser4/README */
> +
> +#ifndef __REISER4_TYPE_SAFE_LIST_H__
> +#define __REISER4_TYPE_SAFE_LIST_H__
> +
> +#include "debug.h"
> +/* A circular doubly linked list that differs from the previous
> +   <linux/list.h> implementation because it is parametrized to provide
> +   type safety.  This data structure is also useful as a queue or stack.

This belongs to include/linux, not reiser4.

> --- /dev/null	2003-09-23 21:59:22.000000000 +0400
> +++ linux-2.6.11-vs/fs/reiser4/vfs_ops.c	2005-06-03 17:51:28.110210237 +0400
> +/* ->get_sb() method of file_system operations. */
> +static struct super_block *
> +reiser4_get_sb(struct file_system_type *fs_type	/* file
> +						 * system
> +						 * type */ ,
> +	       int flags /* flags */ ,
> +	       const char *dev_name /* device name */ ,
> +	       void *data /* mount options */ )

Please drop the useless parameter comments.

> +/*
> + * Initialization stages for reiser4.
> + *
> + * These enumerate various things that have to be done during reiser4
> + * startup. Initialization code (init_reiser4()) keeps track of what stage was
> + * reached, so that proper undo can be done if error occurs during
> + * initialization.
> + */
> +typedef enum {
> +	INIT_NONE,               /* nothing is initialized yet */
> +	INIT_INODECACHE,         /* inode cache created */
> +	INIT_CONTEXT_MGR,        /* list of active contexts created */
> +	INIT_ZNODES,             /* znode slab created */
> +	INIT_PLUGINS,            /* plugins initialized */
> +	INIT_PLUGIN_SET,         /* psets initialized */
> +	INIT_TXN,                /* transaction manager initialized */
> +	INIT_FAKES,              /* fake inode initialized */
> +	INIT_JNODES,             /* jnode slab initialized */
> +	INIT_EFLUSH,             /* emergency flush initialized */
> +	INIT_FQS,                /* flush queues initialized */
> +	INIT_DENTRY_FSDATA,      /* dentry_fsdata slab initialized */
> +	INIT_FILE_FSDATA,        /* file_fsdata slab initialized */
> +	INIT_D_CURSOR,           /* d_cursor suport initialized */
> +	INIT_FS_REGISTERED,      /* reiser4 file system type registered */
> +} reiser4_init_stage;

Please use regular gotos instead. This is a silly hack especially since you
don't have release function for all of the stages.

> +reiser4_internal void reiser4_handle_error(void)
> +{
> +	struct super_block *sb = reiser4_get_current_sb();
> +
> +	if ( !sb )
> +		return;
> +	reiser4_status_write(REISER4_STATUS_DAMAGED, 0, "Filesystem error occured");
> +	switch ( get_super_private(sb)->onerror ) {
> +	case 0:
> +		reiser4_panic("foobar-42", "Filesystem error occured\n");
> +	case 1:
> +		if ( sb->s_flags & MS_RDONLY )
> +			return;
> +		sb->s_flags |= MS_RDONLY;
> +		break;
> +	case 2:
> +		machine_restart(NULL);

Probably not something you should do at fs level.

> +/* free this znode */
> +reiser4_internal void
> +zfree(znode * node /* znode to free */ )
> +{
> +	assert("nikita-465", node != NULL);
> +	assert("nikita-2120", znode_page(node) == NULL);
> +	assert("nikita-2301", owners_list_empty(&node->lock.owners));
> +	assert("nikita-2302", requestors_list_empty(&node->lock.requestors));
> +	assert("nikita-2663", capture_list_is_clean(ZJNODE(node)) && NODE_LIST(ZJNODE(node)) == NOT_CAPTURED);
> +	assert("nikita-2773", !JF_ISSET(ZJNODE(node), JNODE_EFLUSH));
> +	assert("nikita-3220", list_empty(&ZJNODE(node)->jnodes));
> +	assert("nikita-3293", !znode_is_right_connected(node));
> +	assert("nikita-3294", !znode_is_left_connected(node));
> +	assert("nikita-3295", node->left == NULL);
> +	assert("nikita-3296", node->right == NULL);

Are all these still required? Seems bit too defensive for the kernel.

> +
> +
> +	/* not yet phash_jnode_destroy(ZJNODE(node)); */
> +
> +	/* poison memory. */
> +	ON_DEBUG(memset(node, 0xde, sizeof *node));

Poisoning belongs to slab, not fs.

> +/* allocate fresh znode */
> +reiser4_internal znode *
> +zalloc(int gfp_flag /* allocation flag */ )
> +{
> +	znode *node;
> +
> +	node = kmem_cache_alloc(znode_slab, gfp_flag);
> +	return node;
> +}

Please drop this useless wrapper.

> +reiser4_internal void
> +znode_remove(znode * node /* znode to remove */ , reiser4_tree * tree)
> +{
> +	assert("nikita-2108", node != NULL);
> +	assert("nikita-470", node->c_count == 0);
> +	assert("zam-879", rw_tree_is_write_locked(tree));
> +
> +	/* remove reference to this znode from cbk cache */
> +	cbk_cache_invalidate(node, tree);
> +
> +	/* update c_count of parent */
> +	if (znode_parent(node) != NULL) {
> +		assert("nikita-472", znode_parent(node)->c_count > 0);
> +		/* father, onto your hands I forward my spirit... */
> +		znode_parent(node)->c_count --;
> +		node->in_parent.node = NULL;
> +	} else {
> +		/* orphaned znode?! Root? */

Drop the useless else branch.

> --- /dev/null	2003-09-23 21:59:22.000000000 +0400
> +++ linux-2.6.11-vs/fs/reiser4/debug.c	2005-06-03 17:49:38.293184063 +0400
> @@ -0,0 +1,447 @@
> +/* Copyright 2001, 2002, 2003 by Hans Reiser, licensing governed by
> + * reiser4/README */
> +
> +/* Debugging facilities. */
> +
> +/*
> + * This file contains generic debugging functions used by reiser4. Roughly
> + * following:
> + *
> + *     panicking: reiser4_do_panic(), reiser4_print_prefix().
> + *
> + *     locking: schedulable(), lock_counters(), print_lock_counters(),
> + *     no_counters_are_held(), commit_check_locks()
> + *
> + *     {debug,trace,log}_flags: reiser4_are_all_debugged(),
> + *     reiser4_is_debugged(), get_current_trace_flags(),
> + *     get_current_log_flags().
> + *
> + *     kmalloc/kfree leak detection: reiser4_kmalloc(), reiser4_kfree(),
> + *     reiser4_kfree_in_sb().

Please don't do this! We've had enough trouble trying to get the
current subsystem specific allocators out, so do not introduce a new one. If
you need memory leak detection, make it generic and submit that. Reiser4, like
all other subsystems, should use kmalloc() and friends directly.

> --- /dev/null	2003-09-23 21:59:22.000000000 +0400
> +++ linux-2.6.11-vs/fs/reiser4/debug.h	2005-06-03 17:49:38.297184283 +0400
> +
> +/*
> + * Error code tracing facility. (Idea is borrowed from XFS code.)

Could this be turned into a generic facility?

> + *
> + * Suppose some strange and/or unexpected code is returned from some function
> + * (for example, write(2) returns -EEXIST). It is possible to place a
> + * breakpoint in the reiser4_write(), but it is too late here. How to find out
--
> +#define RETERR(code) 				\
> +({						\
> +	typeof(code) __code;			\
> +						\
> +	__code = (code);			\
> +	return_err(__code, __FILE__, __LINE__);	\
> +	__code;					\
> +})

> +#define reiser4_internal

Please drop the above useless #define.

> --- /dev/null	2003-09-23 21:59:22.000000000 +0400
> +++ linux-2.6.11-vs/fs/reiser4/emergency_flush.c	2005-06-03 17:51:59.000905353 +0400
> @@ -0,0 +1,913 @@
> +/* Copyright 2002, 2003 by Hans Reiser, licensing governed by reiser4/README */
> +
> +/* This file exists only until VM gets fixed to reserve pages properly, which
> + * might or might not be very political. */

Please fix vm instead of working around it in fs.

> --- /dev/null	2003-09-23 21:59:22.000000000 +0400
> +++ linux-2.6.11-vs/fs/reiser4/init_super.c	2005-06-03 17:51:27.959201950 +0400
> +
> +#define _INIT_PARAM_LIST (struct super_block * s, reiser4_context * ctx, void * data, int silent)
> +#define _DONE_PARAM_LIST (struct super_block * s)
> +
> +#define _INIT_(subsys) static int _init_##subsys _INIT_PARAM_LIST
> +#define _DONE_(subsys) static void _done_##subsys _DONE_PARAM_LIST

Please remove this macro obfuscation.

> +
> +_DONE_EMPTY(exit_context)
> +
> +struct reiser4_subsys {
> +	int  (*init) _INIT_PARAM_LIST;
> +	void (*done) _DONE_PARAM_LIST;
> +};
> +
> +#define _SUBSYS(subsys) {.init = &_init_##subsys, .done = &_done_##subsys}
> +static struct reiser4_subsys subsys_array[] = {
> +	_SUBSYS(mount_flags_check),
> +	_SUBSYS(sinfo),
> +	_SUBSYS(context),
> +	_SUBSYS(parse_options),
> +	_SUBSYS(object_ops),
> +	_SUBSYS(read_super),
> +	_SUBSYS(tree0),
> +	_SUBSYS(txnmgr),
> +	_SUBSYS(ktxnmgrd_context),
> +	_SUBSYS(ktxnmgrd),
> +	_SUBSYS(entd),
> +	_SUBSYS(formatted_fake),
> +	_SUBSYS(disk_format),
> +	_SUBSYS(sb_counters),
> +	_SUBSYS(d_cursor),
> +	_SUBSYS(fs_root),
> +	_SUBSYS(safelink),
> +	_SUBSYS(exit_context)
> +};

The above is overkill and silly. parse_options and read_super, for
example, are _not_ a subsystem inits but regular fs ops. Please consider
dropping this altogether but at least trim it down to something sane.

> --- /dev/null	2003-09-23 21:59:22.000000000 +0400
> +++ linux-2.6.11-vs/fs/reiser4/page_cache.c	2005-06-03 17:51:59.003905518 +0400
> +/* one-time initialization of fake inodes handling functions. */
> +reiser4_internal int
> +init_fakes()
> +{
> +	return 0;
> +}

Please drop this empty function.

Re: -mm -> 2.6.13 merge status

[Hans Reiser]

Pekka Enberg wrote:

>Hi Hans,
>
>On 6/22/05, Hans Reiser <reiser@namesys.com> wrote:
>  
>
>>I would in particular love to have you Andi Kleen do a full review of V4
>>if you could be that generous with your time, as I liked much of the
>>advice you gave us on V3.
>>    
>>
>
>Well, I am not Andi Kleen and this is not even in the ballpark of a full
>review but here goes...
>  
>
thanks kindly for your time, your comments were appreciated

>P.S. Would it be possible to have a version without the plugin stuff
>submitted (and perhaps file as directory)?
>
No, I am sorry.  It is like asking for ext2 without directories.

> It would make much more
>sense for the rest of us to review reiser4 without the most controversial
>bits and get the bits that people agree on merged.
>
>                                     Pekka
>
>  
>
>>--- /dev/null	2003-09-23 21:59:22.000000000 +0400
>>+++ linux-2.6.11-vs/fs/reiser4/pool.c	2005-06-03 17:49:38.668204642 +0400
>>+/* initialise new pool */
>>+reiser4_internal void
>>+reiser4_init_pool(reiser4_pool * pool /* pool to initialise */ ,
>>+		  size_t obj_size /* size of objects in @pool */ ,
>>+		  int num_of_objs /* number of preallocated objects */ ,
>>+		  char *data /* area for preallocated objects */ )
>>+{
>>+	reiser4_pool_header *h;
>>+	int i;
>>+
>>+	assert("nikita-955", pool != NULL);
>>    
>>
>
>These assertion codes are meaningless to the rest of us so please drop
>them.
>  
>
I think you don't appreciate the role of assertions in making code
easier to audit and debug.

>  
>
>>--- /dev/null	2003-09-23 21:59:22.000000000 +0400
>>+++ linux-2.6.11-vs/fs/reiser4/type_safe_hash.h	2005-06-03 17:49:38.751209197 +0400
>>@@ -0,0 +1,320 @@
>>+/* Copyright 2001, 2002, 2003 by Hans Reiser, licensing governed by
>>+ * reiser4/README */
>>+
>>+/* A hash table class that uses hash chains (singly-linked) and is
>>+   parametrized to provide type safety.  */
>>    
>>
>
>This belongs to include/linux, not reiser4.
>  
>
Too much politics are involved in modifying other peoples directories,
or I'd agree with you.  The first person outside the reiser4 project to
use it should move it into a different place.

>  
>
>>--- /dev/null	2003-09-23 21:59:22.000000000 +0400
>>+++ linux-2.6.11-vs/fs/reiser4/type_safe_list.h	2005-06-03 17:49:38.755209417 +0400
>>@@ -0,0 +1,436 @@
>>+/* Copyright 2001, 2002, 2003 by Hans Reiser, licensing governed by reiser4/README */
>>+
>>+#ifndef __REISER4_TYPE_SAFE_LIST_H__
>>+#define __REISER4_TYPE_SAFE_LIST_H__
>>+
>>+#include "debug.h"
>>+/* A circular doubly linked list that differs from the previous
>>+   <linux/list.h> implementation because it is parametrized to provide
>>+   type safety.  This data structure is also useful as a queue or stack.
>>    
>>
>
>This belongs to include/linux, not reiser4.
>  
>
Yes, but see above about first person outside reiser4 project should.....

>  
>
>>--- /dev/null	2003-09-23 21:59:22.000000000 +0400
>>+++ linux-2.6.11-vs/fs/reiser4/vfs_ops.c	2005-06-03 17:51:28.110210237 +0400
>>+/* ->get_sb() method of file_system operations. */
>>+static struct super_block *
>>+reiser4_get_sb(struct file_system_type *fs_type	/* file
>>+						 * system
>>+						 * type */ ,
>>+	       int flags /* flags */ ,
>>+	       const char *dev_name /* device name */ ,
>>+	       void *data /* mount options */ )
>>    
>>
>
>Please drop the useless parameter comments.
>  
>
vs, figure out who added the flags and device name comments and ask them
to prepare a patch.  If nobody admits to the shameful act,;-) have
Edward fix it.

>  
>
>>+/*
>>+ * Initialization stages for reiser4.
>>+ *
>>+ * These enumerate various things that have to be done during reiser4
>>+ * startup. Initialization code (init_reiser4()) keeps track of what stage was
>>+ * reached, so that proper undo can be done if error occurs during
>>+ * initialization.
>>+ */
>>+typedef enum {
>>+	INIT_NONE,               /* nothing is initialized yet */
>>+	INIT_INODECACHE,         /* inode cache created */
>>+	INIT_CONTEXT_MGR,        /* list of active contexts created */
>>+	INIT_ZNODES,             /* znode slab created */
>>+	INIT_PLUGINS,            /* plugins initialized */
>>+	INIT_PLUGIN_SET,         /* psets initialized */
>>+	INIT_TXN,                /* transaction manager initialized */
>>+	INIT_FAKES,              /* fake inode initialized */
>>+	INIT_JNODES,             /* jnode slab initialized */
>>+	INIT_EFLUSH,             /* emergency flush initialized */
>>+	INIT_FQS,                /* flush queues initialized */
>>+	INIT_DENTRY_FSDATA,      /* dentry_fsdata slab initialized */
>>+	INIT_FILE_FSDATA,        /* file_fsdata slab initialized */
>>+	INIT_D_CURSOR,           /* d_cursor suport initialized */
>>+	INIT_FS_REGISTERED,      /* reiser4 file system type registered */
>>+} reiser4_init_stage;
>>    
>>
>
>Please use regular gotos instead. This is a silly hack especially since you
>don't have release function for all of the stages.
>  
>
I'll let vs comment.

>  
>
>>+reiser4_internal void reiser4_handle_error(void)
>>+{
>>+	struct super_block *sb = reiser4_get_current_sb();
>>+
>>+	if ( !sb )
>>+		return;
>>+	reiser4_status_write(REISER4_STATUS_DAMAGED, 0, "Filesystem error occured");
>>+	switch ( get_super_private(sb)->onerror ) {
>>+	case 0:
>>+		reiser4_panic("foobar-42", "Filesystem error occured\n");
>>+	case 1:
>>+		if ( sb->s_flags & MS_RDONLY )
>>+			return;
>>+		sb->s_flags |= MS_RDONLY;
>>+		break;
>>+	case 2:
>>+		machine_restart(NULL);
>>    
>>
>
>Probably not something you should do at fs level.
>  
>
Not sure why you say that.

vs, given the existence of case 0, why do we need to have case 2?

>  
>
>>+/* free this znode */
>>+reiser4_internal void
>>+zfree(znode * node /* znode to free */ )
>>+{
>>+	assert("nikita-465", node != NULL);
>>+	assert("nikita-2120", znode_page(node) == NULL);
>>+	assert("nikita-2301", owners_list_empty(&node->lock.owners));
>>+	assert("nikita-2302", requestors_list_empty(&node->lock.requestors));
>>+	assert("nikita-2663", capture_list_is_clean(ZJNODE(node)) && NODE_LIST(ZJNODE(node)) == NOT_CAPTURED);
>>+	assert("nikita-2773", !JF_ISSET(ZJNODE(node), JNODE_EFLUSH));
>>+	assert("nikita-3220", list_empty(&ZJNODE(node)->jnodes));
>>+	assert("nikita-3293", !znode_is_right_connected(node));
>>+	assert("nikita-3294", !znode_is_left_connected(node));
>>+	assert("nikita-3295", node->left == NULL);
>>+	assert("nikita-3296", node->right == NULL);
>>    
>>
>
>Are all these still required? Seems bit too defensive for the kernel.
>  
>
Hmm, someday must put you in a room with DARPA guys, and let you get us
another round of funding by trying to convince them that our code is too
defensive.;-)

This is not too defensive.  Nikita did well here.  The culture of code
auditors is very different from most programmers.  Like most
specialists, they have wisdom to offer those who listen to them.  In
essence, they teach that every function should have a specification of
every possible restriction on allowed input that can be imagined and is
correct as a restriction.  Code auditors love assertions like this.  I
look at my understanding of this before DARPA, and I wince.  DARPA
patiently taught me much in this area as I listened to security talks at
our meetings, and I thank them for it. 

Large scale projects like reiser4 are crippled by debugging costs. 
Anything that reduces debugging time is worth so much.....

>  
>
>>+
>>+
>>+	/* not yet phash_jnode_destroy(ZJNODE(node)); */
>>+
>>+	/* poison memory. */
>>+	ON_DEBUG(memset(node, 0xde, sizeof *node));
>>    
>>
>
>Poisoning belongs to slab, not fs.
>  
>
vs, please comment.

>  
>
>>+/* allocate fresh znode */
>>+reiser4_internal znode *
>>+zalloc(int gfp_flag /* allocation flag */ )
>>+{
>>+	znode *node;
>>+
>>+	node = kmem_cache_alloc(znode_slab, gfp_flag);
>>+	return node;
>>+}
>>    
>>
>
>Please drop this useless wrapper.
>  
>
Thanks.  vs, I think he is right here.... I see zalloc used in only two
places.....  Or is the desire to ease future porting work?

>  
>
>>+reiser4_internal void
>>+znode_remove(znode * node /* znode to remove */ , reiser4_tree * tree)
>>+{
>>+	assert("nikita-2108", node != NULL);
>>+	assert("nikita-470", node->c_count == 0);
>>+	assert("zam-879", rw_tree_is_write_locked(tree));
>>+
>>+	/* remove reference to this znode from cbk cache */
>>+	cbk_cache_invalidate(node, tree);
>>+
>>+	/* update c_count of parent */
>>+	if (znode_parent(node) != NULL) {
>>+		assert("nikita-472", znode_parent(node)->c_count > 0);
>>+		/* father, onto your hands I forward my spirit... */
>>+		znode_parent(node)->c_count --;
>>+		node->in_parent.node = NULL;
>>+	} else {
>>+		/* orphaned znode?! Root? */
>>    
>>
>
>Drop the useless else branch.
>  
>
>>--- /dev/null	2003-09-23 21:59:22.000000000 +0400
>>+++ linux-2.6.11-vs/fs/reiser4/debug.c	2005-06-03 17:49:38.293184063 +0400
>>@@ -0,0 +1,447 @@
>>+/* Copyright 2001, 2002, 2003 by Hans Reiser, licensing governed by
>>+ * reiser4/README */
>>+
>>+/* Debugging facilities. */
>>+
>>+/*
>>+ * This file contains generic debugging functions used by reiser4. Roughly
>>+ * following:
>>+ *
>>+ *     panicking: reiser4_do_panic(), reiser4_print_prefix().
>>+ *
>>+ *     locking: schedulable(), lock_counters(), print_lock_counters(),
>>+ *     no_counters_are_held(), commit_check_locks()
>>+ *
>>+ *     {debug,trace,log}_flags: reiser4_are_all_debugged(),
>>+ *     reiser4_is_debugged(), get_current_trace_flags(),
>>+ *     get_current_log_flags().
>>+ *
>>+ *     kmalloc/kfree leak detection: reiser4_kmalloc(), reiser4_kfree(),
>>+ *     reiser4_kfree_in_sb().
>>    
>>
>
>Please don't do this! We've had enough trouble trying to get the
>current subsystem specific allocators out, so do not introduce a new one. If
>you need memory leak detection, make it generic and submit that. Reiser4, like
>all other subsystems, should use kmalloc() and friends directly.
>  
>
I will let vs comment.

>  
>
>>--- /dev/null	2003-09-23 21:59:22.000000000 +0400
>>+++ linux-2.6.11-vs/fs/reiser4/debug.h	2005-06-03 17:49:38.297184283 +0400
>>+
>>+/*
>>+ * Error code tracing facility. (Idea is borrowed from XFS code.)
>>    
>>
>
>Could this be turned into a generic facility?
>  
>
Probably it already is one.  Getting it used as such sounds like a lot
of political work though.  Maybe the first person outside the reiser4
project to want to use it should move the code into a different location.

>  
>
>>+ *
>>+ * Suppose some strange and/or unexpected code is returned from some function
>>+ * (for example, write(2) returns -EEXIST). It is possible to place a
>>+ * breakpoint in the reiser4_write(), but it is too late here. How to find out
>>    
>>
>--
>  
>
>>+#define RETERR(code) 				\
>>+({						\
>>+	typeof(code) __code;			\
>>+						\
>>+	__code = (code);			\
>>+	return_err(__code, __FILE__, __LINE__);	\
>>+	__code;					\
>>+})
>>    
>>
>
>  
>
>>+#define reiser4_internal
>>    
>>
>
>Please drop the above useless #define.
>
>  
>
>>--- /dev/null	2003-09-23 21:59:22.000000000 +0400
>>+++ linux-2.6.11-vs/fs/reiser4/emergency_flush.c	2005-06-03 17:51:59.000905353 +0400
>>@@ -0,0 +1,913 @@
>>+/* Copyright 2002, 2003 by Hans Reiser, licensing governed by reiser4/README */
>>+
>>+/* This file exists only until VM gets fixed to reserve pages properly, which
>>+ * might or might not be very political. */
>>    
>>
>
>Please fix vm instead of working around it in fs.
>  
>
actually I want to see emergency flush die very very much.   As for
fixing vm, easier said than done, politically.

>  
>
>>--- /dev/null	2003-09-23 21:59:22.000000000 +0400
>>+++ linux-2.6.11-vs/fs/reiser4/init_super.c	2005-06-03 17:51:27.959201950 +0400
>>+
>>+#define _INIT_PARAM_LIST (struct super_block * s, reiser4_context * ctx, void * data, int silent)
>>+#define _DONE_PARAM_LIST (struct super_block * s)
>>+
>>+#define _INIT_(subsys) static int _init_##subsys _INIT_PARAM_LIST
>>+#define _DONE_(subsys) static void _done_##subsys _DONE_PARAM_LIST
>>    
>>
>
>Please remove this macro obfuscation.
>  
>
vs, I think he is right, do you?

>  
>
>>+
>>+_DONE_EMPTY(exit_context)
>>+
>>+struct reiser4_subsys {
>>+	int  (*init) _INIT_PARAM_LIST;
>>+	void (*done) _DONE_PARAM_LIST;
>>+};
>>+
>>+#define _SUBSYS(subsys) {.init = &_init_##subsys, .done = &_done_##subsys}
>>+static struct reiser4_subsys subsys_array[] = {
>>+	_SUBSYS(mount_flags_check),
>>+	_SUBSYS(sinfo),
>>+	_SUBSYS(context),
>>+	_SUBSYS(parse_options),
>>+	_SUBSYS(object_ops),
>>+	_SUBSYS(read_super),
>>+	_SUBSYS(tree0),
>>+	_SUBSYS(txnmgr),
>>+	_SUBSYS(ktxnmgrd_context),
>>+	_SUBSYS(ktxnmgrd),
>>+	_SUBSYS(entd),
>>+	_SUBSYS(formatted_fake),
>>+	_SUBSYS(disk_format),
>>+	_SUBSYS(sb_counters),
>>+	_SUBSYS(d_cursor),
>>+	_SUBSYS(fs_root),
>>+	_SUBSYS(safelink),
>>+	_SUBSYS(exit_context)
>>+};
>>    
>>
>
>The above is overkill and silly. parse_options and read_super, for
>example, are _not_ a subsystem inits but regular fs ops. Please consider
>dropping this altogether but at least trim it down to something sane.
>  
>
vs please comment.

>  
>
>>--- /dev/null	2003-09-23 21:59:22.000000000 +0400
>>+++ linux-2.6.11-vs/fs/reiser4/page_cache.c	2005-06-03 17:51:59.003905518 +0400
>>+/* one-time initialization of fake inodes handling functions. */
>>+reiser4_internal int
>>+init_fakes()
>>+{
>>+	return 0;
>>+}
>>    
>>
>
>Please drop this empty function.
>
>
>  
>
sure.

Re: -mm -> 2.6.13 merge status

[Pekka J Enberg]

Hi Hans, 

On Thu, 2005-06-23 at 00:42 -0700, Hans Reiser wrote:
> > These assertion codes are meaningless to the rest of us so please drop
> > them. 
> 
> I think you don't appreciate the role of assertions in making code
> easier to audit and debug.

I did not say you should drop the assertions. I referred to the
"nikita-955" part which is redundant and pointless. Using
__FILE__:__LINE__ (or BUG_ON even) will give you enough information to
identify where the error occured. 

On Thu, 2005-06-23 at 00:42 -0700, Hans Reiser wrote:
> > This belongs to include/linux, not reiser4. 
> 
> Too much politics are involved in modifying other peoples directories,
> or I'd agree with you.  The first person outside the reiser4 project to
> use it should move it into a different place.

What politics? Please do hide generic code in your fs. How should
someone outside reiser4 know you have type-safe hash tables and linked
lists in there? Why should they care? It is obvious that you did not
think <linux/list.h> and <linux/jhash.h> were sufficient so please fix
those instead and use them. 

> >>+reiser4_internal void reiser4_handle_error(void)
> >>+{
> >>+   struct super_block *sb = reiser4_get_current_sb();
> >>+
> >>+   if ( !sb )
> >>+           return;
> >>+   reiser4_status_write(REISER4_STATUS_DAMAGED, 0, "Filesystem error occured");
> >>+   switch ( get_super_private(sb)->onerror ) {
> >>+   case 0:
> >>+           reiser4_panic("foobar-42", "Filesystem error occured\n");
> >>+   case 1:
> >>+           if ( sb->s_flags & MS_RDONLY )
> >>+                   return;
> >>+           sb->s_flags |= MS_RDONLY;
> >>+           break;
> >>+   case 2:
> >>+           machine_restart(NULL);
> >>    
> >>
> >
> > Probably not something you should do at fs level. 
> 
> Not sure why you say that.

Because Reiser4 hitting an error condition and restarting the machine
silently is silly. Just do panic() there. 

> This is not too defensive.  Nikita did well here.  The culture of code
> auditors is very different from most programmers.  Like most
> specialists, they have wisdom to offer those who listen to them.  In
> essence, they teach that every function should have a specification of
> every possible restriction on allowed input that can be imagined and is
> correct as a restriction.  Code auditors love assertions like this.  I
> look at my understanding of this before DARPA, and I wince.  DARPA
> patiently taught me much in this area as I listened to security talks at
> our meetings, and I thank them for it. 

Hans, I am aware of techniques such as design-by-contract but it is not
the kernel coding style. That's because it makes the code harder to read
and refactor. 

> Large scale projects like reiser4 are crippled by debugging costs. 
> Anything that reduces debugging time is worth so much.....

Then start writing automated unit tests. 

> >>+/* allocate fresh znode */
> >>+reiser4_internal znode *
> >>+zalloc(int gfp_flag /* allocation flag */ )
> >>+{
> >>+   znode *node;
> >>+
> >>+   node = kmem_cache_alloc(znode_slab, gfp_flag);
> >>+   return node;
> >>+}
> >>    
> >>
> >
> >Please drop this useless wrapper.
> >  
> >
> Thanks.  vs, I think he is right here.... I see zalloc used in only two
> places.....  Or is the desire to ease future porting work?

Then add it later. It is a useless wrapper now. 

> >>--- /dev/null       2003-09-23 21:59:22.000000000 +0400
> >>+++ linux-2.6.11-vs/fs/reiser4/debug.h      2005-06-03 17:49:38.297184283 +0400
> >>+
> >>+/*
> >>+ * Error code tracing facility. (Idea is borrowed from XFS code.)
> >>    
> >>
> >
> >Could this be turned into a generic facility? 
> 
> 
> Probably it already is one.  Getting it used as such sounds like a lot
> of political work though.  Maybe the first person outside the reiser4
> project to want to use it should move the code into a different location.

What political work? Just whoop up a patch to introduce it as a generic
facility and let others review it. There are plenty of janitors that are
willing to convert the existing code. Please note that you're now 
duplicating code from XFS (even if it is just an idea you borrowed). 

This stuff is fairly simple: if the technique you're using is good, it
should probably be generic; if it isn't, you shouldn't be using it
either. Please don't let the pissing contest between you and hch create
more work for the rest of us. 

> actually I want to see emergency flush die very very much.   As for
> fixing vm, easier said than done, politically.

As you might have noticed, I don't care for politics. Just post the patch
to fix the vm for review. 

                       Pekka 

Re: -mm -> 2.6.13 merge status

[Hans Reiser]

Pekka J Enberg wrote:

> Hi Hans,
> On Thu, 2005-06-23 at 00:42 -0700, Hans Reiser wrote:
>
>> > These assertion codes are meaningless to the rest of us so please drop
>> > them.
>> I think you don't appreciate the role of assertions in making code
>> easier to audit and debug.
>
>
> I did not say you should drop the assertions. I referred to the
> "nikita-955" part which is redundant and pointless. Using
> __FILE__:__LINE__ (or BUG_ON even) will give you enough information to
> identify where the error occured.

but then it does not tell me who I assign the bug to.

>
> Because Reiser4 hitting an error condition and restarting the machine
> silently is silly. Just do panic() there.

Well, it seems we agreed and did not realize it.  Oh well.  The silent
restart seems like a silly option to have available.  If someone can
think of a case where it is useful, let me know, otherwise vs please
remove it.

Re: -mm -> 2.6.13 merge status

[Vladimir Saveliev]

Hello

On Thu, 2005-06-23 at 11:42, Hans Reiser wrote:
> Pekka Enberg wrote:
> 
> >
> >>--- /dev/null	2003-09-23 21:59:22.000000000 +0400
> >>+++ linux-2.6.11-vs/fs/reiser4/pool.c	2005-06-03 17:49:38.668204642 +0400
> >>+/* initialise new pool */
> >>+reiser4_internal void
> >>+reiser4_init_pool(reiser4_pool * pool /* pool to initialise */ ,
> >>+		  size_t obj_size /* size of objects in @pool */ ,
> >>+		  int num_of_objs /* number of preallocated objects */ ,
> >>+		  char *data /* area for preallocated objects */ )
> >>+{
> >>+	reiser4_pool_header *h;
> >>+	int i;
> >>+
> >>+	assert("nikita-955", pool != NULL);
> >>    
> >>
> >
> >These assertion codes are meaningless to the rest of us so please drop
> >them.
> > 

Pekka, am I correct that you object aginst assertion ids like "joe-700"?

> >>--- /dev/null	2003-09-23 21:59:22.000000000 +0400
> >>+++ linux-2.6.11-vs/fs/reiser4/type_safe_hash.h	2005-06-03 17:49:38.751209197 +0400
> >>@@ -0,0 +1,320 @@
> >>+/* Copyright 2001, 2002, 2003 by Hans Reiser, licensing governed by
> >>+ * reiser4/README */
> >>+
> >>+/* A hash table class that uses hash chains (singly-linked) and is
> >>+   parametrized to provide type safety.  */
> >
> >This belongs to include/linux, not reiser4.

ok.

> >>--- /dev/null	2003-09-23 21:59:22.000000000 +0400
> >>+++ linux-2.6.11-vs/fs/reiser4/type_safe_list.h	2005-06-03 17:49:38.755209417 +0400
> >>@@ -0,0 +1,436 @@
> >>+/* Copyright 2001, 2002, 2003 by Hans Reiser, licensing governed by reiser4/README */
> >>+
> >>+#ifndef __REISER4_TYPE_SAFE_LIST_H__
> >>+#define __REISER4_TYPE_SAFE_LIST_H__
> >>+
> >>+#include "debug.h"
> >>+/* A circular doubly linked list that differs from the previous
> >>+   <linux/list.h> implementation because it is parametrized to provide
> >>+   type safety.  This data structure is also useful as a queue or stack.
> >
> >This belongs to include/linux, not reiser4.
> >

ok

> >>+/*
> >>+ * Initialization stages for reiser4.
> >>+ *
> >>+ * These enumerate various things that have to be done during reiser4
> >>+ * startup. Initialization code (init_reiser4()) keeps track of what stage was
> >>+ * reached, so that proper undo can be done if error occurs during
> >>+ * initialization.
> >>+ */
> >>+typedef enum {
> >>+	INIT_NONE,               /* nothing is initialized yet */
> >>+	INIT_INODECACHE,         /* inode cache created */
> >>+	INIT_CONTEXT_MGR,        /* list of active contexts created */
> >>+	INIT_ZNODES,             /* znode slab created */
> >>+	INIT_PLUGINS,            /* plugins initialized */
> >>+	INIT_PLUGIN_SET,         /* psets initialized */
> >>+	INIT_TXN,                /* transaction manager initialized */
> >>+	INIT_FAKES,              /* fake inode initialized */
> >>+	INIT_JNODES,             /* jnode slab initialized */
> >>+	INIT_EFLUSH,             /* emergency flush initialized */
> >>+	INIT_FQS,                /* flush queues initialized */
> >>+	INIT_DENTRY_FSDATA,      /* dentry_fsdata slab initialized */
> >>+	INIT_FILE_FSDATA,        /* file_fsdata slab initialized */
> >>+	INIT_D_CURSOR,           /* d_cursor suport initialized */
> >>+	INIT_FS_REGISTERED,      /* reiser4 file system type registered */
> >>+} reiser4_init_stage;
> >>    
> >>
> >
> >Please use regular gotos instead. This is a silly hack especially since you
> >don't have release function for all of the stages.
> >  
> >
> I'll let vs comment.
> 

IMHO, it is convinient. But lets change it as requested.

> >  
> >
> >>+reiser4_internal void reiser4_handle_error(void)
> >>+{
> >>+	struct super_block *sb = reiser4_get_current_sb();
> >>+
> >>+	if ( !sb )
> >>+		return;
> >>+	reiser4_status_write(REISER4_STATUS_DAMAGED, 0, "Filesystem error occured");
> >>+	switch ( get_super_private(sb)->onerror ) {
> >>+	case 0:
> >>+		reiser4_panic("foobar-42", "Filesystem error occured\n");
> >>+	case 1:
> >>+		if ( sb->s_flags & MS_RDONLY )
> >>+			return;
> >>+		sb->s_flags |= MS_RDONLY;
> >>+		break;
> >>+	case 2:
> >>+		machine_restart(NULL);
> >>    
> >>
> >
> >Probably not something you should do at fs level.
> >  
ok

> >>+
> >>+	/* not yet phash_jnode_destroy(ZJNODE(node)); */
> >>+
> >>+	/* poison memory. */
> >>+	ON_DEBUG(memset(node, 0xde, sizeof *node));
> >>    
> >>
> >
> >Poisoning belongs to slab, not fs.
> >  

ok

> >  
> >
> >>+/* allocate fresh znode */
> >>+reiser4_internal znode *
> >>+zalloc(int gfp_flag /* allocation flag */ )
> >>+{
> >>+	znode *node;
> >>+
> >>+	node = kmem_cache_alloc(znode_slab, gfp_flag);
> >>+	return node;
> >>+}
> >>    
> >>
> >
> >Please drop this useless wrapper.
> >  

ok

> >  
> >
> >>+reiser4_internal void
> >>+znode_remove(znode * node /* znode to remove */ , reiser4_tree * tree)
> >>+{
> >>+	assert("nikita-2108", node != NULL);
> >>+	assert("nikita-470", node->c_count == 0);
> >>+	assert("zam-879", rw_tree_is_write_locked(tree));
> >>+
> >>+	/* remove reference to this znode from cbk cache */
> >>+	cbk_cache_invalidate(node, tree);
> >>+
> >>+	/* update c_count of parent */
> >>+	if (znode_parent(node) != NULL) {
> >>+		assert("nikita-472", znode_parent(node)->c_count > 0);
> >>+		/* father, onto your hands I forward my spirit... */
> >>+		znode_parent(node)->c_count --;
> >>+		node->in_parent.node = NULL;
> >>+	} else {
> >>+		/* orphaned znode?! Root? */
> >>    
> >>
> >
> >Drop the useless else branch.
> >  
> >
> >>--- /dev/null	2003-09-23 21:59:22.000000000 +0400
> >>+++ linux-2.6.11-vs/fs/reiser4/debug.c	2005-06-03 17:49:38.293184063 +0400
> >>@@ -0,0 +1,447 @@
> >>+/* Copyright 2001, 2002, 2003 by Hans Reiser, licensing governed by
> >>+ * reiser4/README */
> >>+
> >>+/* Debugging facilities. */
> >>+
> >>+/*
> >>+ * This file contains generic debugging functions used by reiser4. Roughly
> >>+ * following:
> >>+ *
> >>+ *     panicking: reiser4_do_panic(), reiser4_print_prefix().
> >>+ *
> >>+ *     locking: schedulable(), lock_counters(), print_lock_counters(),
> >>+ *     no_counters_are_held(), commit_check_locks()
> >>+ *
> >>+ *     {debug,trace,log}_flags: reiser4_are_all_debugged(),
> >>+ *     reiser4_is_debugged(), get_current_trace_flags(),
> >>+ *     get_current_log_flags().
> >>+ *
> >>+ *     kmalloc/kfree leak detection: reiser4_kmalloc(), reiser4_kfree(),
> >>+ *     reiser4_kfree_in_sb().
> >>    
> >>
> >
> >Please don't do this! We've had enough trouble trying to get the
> >current subsystem specific allocators out, so do not introduce a new one. If
> >you need memory leak detection, make it generic and submit that. Reiser4, like
> >all other subsystems, should use kmalloc() and friends directly.
> >  
> >
> I will let vs comment.
> 
I agree with Pekka.

> >  
> >
> >>--- /dev/null	2003-09-23 21:59:22.000000000 +0400
> >>+++ linux-2.6.11-vs/fs/reiser4/debug.h	2005-06-03 17:49:38.297184283 +0400
> >>+
> >>+/*
> >>+ * Error code tracing facility. (Idea is borrowed from XFS code.)
> >>    
> >>
> >
> >Could this be turned into a generic facility?
> >  

I do not think that it will ever become accepted.
To get use of that task_t would have to be extended with fields to store
error code, file name and line in it, and several return addresses.
Moreover lines like 
return -ENOENT;
would have to be replaced with:
return RETERR(-ENOENT);

This is debugging feature, we should probably move it to our internal
debugging patch.

> >
> >  
> >
> >>+#define reiser4_internal
> >>    
> >>
> >
> >Please drop the above useless #define.
> >

ok

> >
> >>--- /dev/null	2003-09-23 21:59:22.000000000 +0400
> >>+++ linux-2.6.11-vs/fs/reiser4/init_super.c	2005-06-03 17:51:27.959201950 +0400
> >>+
> >>+#define _INIT_PARAM_LIST (struct super_block * s, reiser4_context * ctx, void * data, int silent)
> >>+#define _DONE_PARAM_LIST (struct super_block * s)
> >>+
> >>+#define _INIT_(subsys) static int _init_##subsys _INIT_PARAM_LIST
> >>+#define _DONE_(subsys) static void _done_##subsys _DONE_PARAM_LIST
> >>    
> >>
> >
> >Please remove this macro obfuscation.
> >  
> >
> vs, I think he is right, do you?
> 
> >  
> >
> >>+
> >>+_DONE_EMPTY(exit_context)
> >>+
> >>+struct reiser4_subsys {
> >>+	int  (*init) _INIT_PARAM_LIST;
> >>+	void (*done) _DONE_PARAM_LIST;
> >>+};
> >>+
> >>+#define _SUBSYS(subsys) {.init = &_init_##subsys, .done = &_done_##subsys}
> >>+static struct reiser4_subsys subsys_array[] = {
> >>+	_SUBSYS(mount_flags_check),
> >>+	_SUBSYS(sinfo),
> >>+	_SUBSYS(context),
> >>+	_SUBSYS(parse_options),
> >>+	_SUBSYS(object_ops),
> >>+	_SUBSYS(read_super),
> >>+	_SUBSYS(tree0),
> >>+	_SUBSYS(txnmgr),
> >>+	_SUBSYS(ktxnmgrd_context),
> >>+	_SUBSYS(ktxnmgrd),
> >>+	_SUBSYS(entd),
> >>+	_SUBSYS(formatted_fake),
> >>+	_SUBSYS(disk_format),
> >>+	_SUBSYS(sb_counters),
> >>+	_SUBSYS(d_cursor),
> >>+	_SUBSYS(fs_root),
> >>+	_SUBSYS(safelink),
> >>+	_SUBSYS(exit_context)
> >>+};
> >>    
> >>
> >
> >The above is overkill and silly. parse_options and read_super, for
> >example, are _not_ a subsystem inits but regular fs ops. Please consider
> >dropping this altogether but at least trim it down to something sane.
> > 

Pekka, would you prefer something like:

reiser4_fill_super()
{
    if (init_a() == 0) {
	if (init_b() == 0) {
	    if (init_c() == 0) {
		if (init_last() == 0)
		    return 0;
		else {
		    done_c();
		    done_b();
		    done_a();
		}
	    } else {
		done_b();
		done_a();
	    }
	} else {
	    done_a();
	}
    }
}

With these macros we have reiser4_fill_super to look like the below, and
it remains unchanged when something new is added for initilizing on
filesystem mounting.

reiser4_fill_super()
{
	for (i = 0; i < REISER4_NR_SUBSYS; i++) {
		ret = subsys_array[i].init(s, &ctx, data, silent);
		if (ret) {
			done_super(s, i - 1);
			return ret;
		}
	}
}

Re: -mm -> 2.6.13 merge status

[Olivier Galibert]

On Thu, Jun 23, 2005 at 08:15:03PM +0400, Vladimir Saveliev wrote:
> Pekka, would you prefer something like:
> 
> reiser4_fill_super()
> {
>     if (init_a() == 0) {
> 	if (init_b() == 0) {
> 	    if (init_c() == 0) {
> 		if (init_last() == 0)
> 		    return 0;
> 		else {
> 		    done_c();
> 		    done_b();
> 		    done_a();
> 		}
> 	    } else {
> 		done_b();
> 		done_a();
> 	    }
> 	} else {
> 	    done_a();
> 	}
>     }
> }

No, I think he means the traditional:

reiser4_fill_super()
{
   if (init_a())
     goto failed_a;
   if (init_b())
     goto failed_b;
   if (init_c())
     goto failed_c;
   if (init_last())
     goto failed_last;
   return 0;

 failed_last:
   done_c();
 failed_c:
   done_b();
 failed_b:
   done_a();
 failed_a:
   return 1;
}

Re: -mm -> 2.6.13 merge status

[Ross Biro]

On 6/23/05, Olivier Galibert <galibert@pobox.com> wrote:

> No, I think he means the traditional:
> 
> reiser4_fill_super()
> {
>    if (init_a())
>      goto failed_a;
.
.
.

IMO this works very well when the initialization always completes or
fails totally in a single routine.  When your init routine can leave
something partially inited, then putting all of the cleanup code in a
single function and using the enums eliminates duplicate code and
makes things easier to read.  (it's a state machine like many device
drivers and network stacks).

Also, perhaps a compromise on the asserts at the beggining of
functions.  If they are moved into a header file, say
resier4_contracts.h and replaced with a single macro, you get most of
the benefits and elminate most of the clutter.  If properly done,
there may even be some advantages gained by auto generating the
conttact.h file(s) from some sort of formal spec or design doc.

    Ross

Re: -mm -> 2.6.13 merge status

[Hans Reiser]

Vladimir Saveliev wrote:

>  
>
>>>>+/*
>>>>+ * Initialization stages for reiser4.
>>>>+ *
>>>>+ * These enumerate various things that have to be done during reiser4
>>>>+ * startup. Initialization code (init_reiser4()) keeps track of what stage was
>>>>+ * reached, so that proper undo can be done if error occurs during
>>>>+ * initialization.
>>>>+ */
>>>>+typedef enum {
>>>>+	INIT_NONE,               /* nothing is initialized yet */
>>>>+	INIT_INODECACHE,         /* inode cache created */
>>>>+	INIT_CONTEXT_MGR,        /* list of active contexts created */
>>>>+	INIT_ZNODES,             /* znode slab created */
>>>>+	INIT_PLUGINS,            /* plugins initialized */
>>>>+	INIT_PLUGIN_SET,         /* psets initialized */
>>>>+	INIT_TXN,                /* transaction manager initialized */
>>>>+	INIT_FAKES,              /* fake inode initialized */
>>>>+	INIT_JNODES,             /* jnode slab initialized */
>>>>+	INIT_EFLUSH,             /* emergency flush initialized */
>>>>+	INIT_FQS,                /* flush queues initialized */
>>>>+	INIT_DENTRY_FSDATA,      /* dentry_fsdata slab initialized */
>>>>+	INIT_FILE_FSDATA,        /* file_fsdata slab initialized */
>>>>+	INIT_D_CURSOR,           /* d_cursor suport initialized */
>>>>+	INIT_FS_REGISTERED,      /* reiser4 file system type registered */
>>>>+} reiser4_init_stage;
>>>>   
>>>>
>>>>        
>>>>
>>>Please use regular gotos instead. This is a silly hack especially since you
>>>don't have release function for all of the stages.
>>> 
>>>
>>>      
>>>
>>I'll let vs comment.
>>
>>    
>>
>
>IMHO, it is convinient. But lets change it as requested.
>  
>
No, if you like it, say so and it stays.

>>>>+ *
>>>>+ *     kmalloc/kfree leak detection: reiser4_kmalloc(), reiser4_kfree(),
>>>>+ *     reiser4_kfree_in_sb().
>>>>   
>>>>
>>>>        
>>>>
>>>Please don't do this! We've had enough trouble trying to get the
>>>current subsystem specific allocators out, so do not introduce a new one. If
>>>you need memory leak detection, make it generic and submit that. Reiser4, like
>>>all other subsystems, should use kmalloc() and friends directly.
>>> 
>>>
>>>      
>>>
>>I will let vs comment.
>>
>>    
>>
>I agree with Pekka.
>  
>
Ok, can you make it generic easily?

>  
>
>>> 
>>>
>>>      
>>>
>>>>--- /dev/null	2003-09-23 21:59:22.000000000 +0400
>>>>+++ linux-2.6.11-vs/fs/reiser4/debug.h	2005-06-03 17:49:38.297184283 +0400
>>>>+
>>>>+/*
>>>>+ * Error code tracing facility. (Idea is borrowed from XFS code.)
>>>>   
>>>>
>>>>        
>>>>
>>>Could this be turned into a generic facility?
>>> 
>>>      
>>>
>
>I do not think that it will ever become accepted.
>To get use of that task_t would have to be extended with fields to store
>error code, file name and line in it, and several return addresses.
>Moreover lines like 
>return -ENOENT;
>would have to be replaced with:
>return RETERR(-ENOENT);
>
>This is debugging feature, we should probably move it to our internal
>debugging patch.
>
>  
>
>>> 
>>>
>>>      
>>>
>>>>+#define reiser4_internal
>>>>   
>>>>
>>>>        
>>>>
>>>Please drop the above useless #define.
>>>
>>>      
>>>
>
>ok
>
>  
>
>>>>--- /dev/null	2003-09-23 21:59:22.000000000 +0400
>>>>+++ linux-2.6.11-vs/fs/reiser4/init_super.c	2005-06-03 17:51:27.959201950 +0400
>>>>+
>>>>+#define _INIT_PARAM_LIST (struct super_block * s, reiser4_context * ctx, void * data, int silent)
>>>>+#define _DONE_PARAM_LIST (struct super_block * s)
>>>>+
>>>>+#define _INIT_(subsys) static int _init_##subsys _INIT_PARAM_LIST
>>>>+#define _DONE_(subsys) static void _done_##subsys _DONE_PARAM_LIST
>>>>   
>>>>
>>>>        
>>>>
>>>Please remove this macro obfuscation.
>>> 
>>>
>>>      
>>>
>>vs, I think he is right, do you?
>>
>>    
>>
>>> 
>>>
>>>      
>>>
>>>>+
>>>>+_DONE_EMPTY(exit_context)
>>>>+
>>>>+struct reiser4_subsys {
>>>>+	int  (*init) _INIT_PARAM_LIST;
>>>>+	void (*done) _DONE_PARAM_LIST;
>>>>+};
>>>>+
>>>>+#define _SUBSYS(subsys) {.init = &_init_##subsys, .done = &_done_##subsys}
>>>>+static struct reiser4_subsys subsys_array[] = {
>>>>+	_SUBSYS(mount_flags_check),
>>>>+	_SUBSYS(sinfo),
>>>>+	_SUBSYS(context),
>>>>+	_SUBSYS(parse_options),
>>>>+	_SUBSYS(object_ops),
>>>>+	_SUBSYS(read_super),
>>>>+	_SUBSYS(tree0),
>>>>+	_SUBSYS(txnmgr),
>>>>+	_SUBSYS(ktxnmgrd_context),
>>>>+	_SUBSYS(ktxnmgrd),
>>>>+	_SUBSYS(entd),
>>>>+	_SUBSYS(formatted_fake),
>>>>+	_SUBSYS(disk_format),
>>>>+	_SUBSYS(sb_counters),
>>>>+	_SUBSYS(d_cursor),
>>>>+	_SUBSYS(fs_root),
>>>>+	_SUBSYS(safelink),
>>>>+	_SUBSYS(exit_context)
>>>>+};
>>>>   
>>>>
>>>>        
>>>>
>>>The above is overkill and silly. parse_options and read_super, for
>>>example, are _not_ a subsystem inits but regular fs ops. Please consider
>>>dropping this altogether but at least trim it down to something sane.
>>>
>>>      
>>>
>
>Pekka, would you prefer something like:
>
>reiser4_fill_super()
>{
>    if (init_a() == 0) {
>	if (init_b() == 0) {
>	    if (init_c() == 0) {
>		if (init_last() == 0)
>		    return 0;
>		else {
>		    done_c();
>		    done_b();
>		    done_a();
>		}
>	    } else {
>		done_b();
>		done_a();
>	    }
>	} else {
>	    done_a();
>	}
>    }
>}
>  
>
I think the above is easier to read than the below.  Macros can obscure
sometimes, and one of our weaknesses is a tendency to use macros in such
a way that it frustrates meta-. use in emacs.   Nikita did however
mention that there was something that could understand macros when
constructing tags files, but I forgot what that was.

>With these macros we have reiser4_fill_super to look like the below, and
>it remains unchanged when something new is added for initilizing on
>filesystem mounting.
>
>reiser4_fill_super()
>{
>	for (i = 0; i < REISER4_NR_SUBSYS; i++) {
>		ret = subsys_array[i].init(s, &ctx, data, silent);
>		if (ret) {
>			done_super(s, i - 1);
>			return ret;
>		}
>	}
>}
>
>
>
>
>  
>

Re: -mm -> 2.6.13 merge status

[Nikita Danilov]

Hans Reiser writes:

[...]

 > I think the above is easier to read than the below.  Macros can obscure
 > sometimes, and one of our weaknesses is a tendency to use macros in such
 > a way that it frustrates meta-. use in emacs.   Nikita did however
 > mention that there was something that could understand macros when
 > constructing tags files, but I forgot what that was.

xref.el (http://xref-tech.com/xrefactory/main.html). With it one can
type current->[TAB] and it shows fields of struct task_struct in the
emacs completion buffer.

Nikita.

Re: -mm -> 2.6.13 merge status

[Jeff Mahoney]

Pekka Enberg wrote:
>>--- /dev/null	2003-09-23 21:59:22.000000000 +0400
>>+++ linux-2.6.11-vs/fs/reiser4/pool.c	2005-06-03 17:49:38.668204642 +0400
>>+/* initialise new pool */
>>+reiser4_internal void
>>+reiser4_init_pool(reiser4_pool * pool /* pool to initialise */ ,
>>+		  size_t obj_size /* size of objects in @pool */ ,
>>+		  int num_of_objs /* number of preallocated objects */ ,
>>+		  char *data /* area for preallocated objects */ )
>>+{
>>+	reiser4_pool_header *h;
>>+	int i;
>>+
>>+	assert("nikita-955", pool != NULL);
> 
> These assertion codes are meaningless to the rest of us so please drop
> them.

As someone who spends time debugging reiser3 issues, I've grown
accustomed to the named assertions. They make discussing a particular
assertion much more natural in conversation than file:line. It also
makes difficult to reproduce assertions more trackable over time. The
assertion number never changes, but the line number can with even the
most trivial of patches.

That said, one of my gripes with the named assertions in reiser3 (and
reiser4 now) is that the development staff changes over time. There are
many named assertions in reiser3 that refer to developers no longer
employed by Hans. The quoted one is a perfect example.

Hans, would it be acceptable to you to keep only numbered assertions and
 keep a code responsbility list internal to namesys? It would serve a
dual purpose of keeping the idea of named assertions, but also remove a
huge number of strings that bloat the implementation.

-Jeff

-- 
Jeff Mahoney
SuSE Labs

Re: -mm -> 2.6.13 merge status

[Andrew Morton]

Jeff Mahoney <jeffm@suse.de> wrote:
>
> >>+	assert("nikita-955", pool != NULL);
>  > 
>  > These assertion codes are meaningless to the rest of us so please drop
>  > them.
> 
>  As someone who spends time debugging reiser3 issues, I've grown
>  accustomed to the named assertions. They make discussing a particular
>  assertion much more natural in conversation than file:line.

__FUNCTION__?

Re: -mm -> 2.6.13 merge status

[Jeff Mahoney]

Andrew Morton wrote:
> Jeff Mahoney <jeffm@suse.de> wrote:
>>>>+	assert("nikita-955", pool != NULL);
>> > 
>> > These assertion codes are meaningless to the rest of us so please drop
>> > them.
>>
>> As someone who spends time debugging reiser3 issues, I've grown
>> accustomed to the named assertions. They make discussing a particular
>> assertion much more natural in conversation than file:line.
> 
> __FUNCTION__?

__FUNCTION__ gives additional context, sure, but it doesn't address one
 of the main advantages of numbered assertions: the ability to track the
same assertion across different versions without having to verify that
it is indeed the same assertion on every one. The line number can still
change very easily, and if there are several assertions in a row, it's
not immediately obvious (at a glance) that it is the same assertion.
Reiser[34] warnings use the same mechanism.

I do agree that some pain comes with it, you can end up with assertion
numbers that are all over the place or you start by spreading them out
in a manner we all thought we left behind when we moved beyond BASIC.
I'm not totally committed to it, I just wanted to address the assumption
that numbered assertions were worthless to the rest of the world.

However, I do want to take a moment to address what I think is a bigger
issue in the code. Perhaps it's not enough to be a barrier to inclusion,
but since I'm going through the reiser3 code and addressing these now, I
thought I'd mention them.

All the assertions (a quick grep through the code shows something like
3800 of them) ultimately result in a reiser4_panic, which BUG()'s. Are
*all* of these assertions really worth taking the system down? I think a
reiser4_error function that can abort just that filesystem and recover
somewhat gracefully would be a bit more in order.

-Jeff

-- 
Jeff Mahoney
SuSE Labs

Re: -mm -> 2.6.13 merge status

[Hans Reiser]

Jeff Mahoney wrote:

>All the assertions (a quick grep through the code shows something like
>3800 of them) ultimately result in a reiser4_panic, which BUG()'s. Are
>*all* of these assertions really worth taking the system down? I think a
>reiser4_error function that can abort just that filesystem and recover
>somewhat gracefully would be a bit more in order.
>  
>
A good point.  Thanks for it.

Re: -mm -> 2.6.13 merge status

[Jens Axboe]

On Thu, Jun 23 2005, Andrew Morton wrote:
> Jeff Mahoney <jeffm@suse.de> wrote:
> >
> > >>+	assert("nikita-955", pool != NULL);
> >  > 
> >  > These assertion codes are meaningless to the rest of us so please drop
> >  > them.
> > 
> >  As someone who spends time debugging reiser3 issues, I've grown
> >  accustomed to the named assertions. They make discussing a particular
> >  assertion much more natural in conversation than file:line.
> 
> __FUNCTION__?

Doesn't help a lot. I've also been annoyed several times in the past
when having to lookup a BUG() for a kernel source I don't exactly have
at hand right then and there. If you have constructs ala

function()
{
        if (cond_a)
                BUG();
        if (cond_b)
                BUG();
        if (cond_c)
                BUG();

        do_stuff...
}

then it's impossible to know which one it is without the identical
source at hand.

That said, I don't like the reiser name-number style. If you must do
something like this, mark responsibility by using a named identifier
covering the layer in question instead.

        assert("trace_hash-89", is_hashed(foo) != 0);

or whatever.

-- 
Jens Axboe

Re: -mm -> 2.6.13 merge status

[Pekka Enberg]

Hi,

On Thu, 2005-06-23 at 21:32 +0200, Jens Axboe wrote:
> then it's impossible to know which one it is without the identical
> source at hand.

In which case, debugging is risky IMO (the source code could have
changed a lot).

On Thu, 2005-06-23 at 21:32 +0200, Jens Axboe wrote:
> That said, I don't like the reiser name-number style. If you must do
> something like this, mark responsibility by using a named identifier
> covering the layer in question instead.
> 
>         assert("trace_hash-89", is_hashed(foo) != 0);

A human readable message would be nicer. For example, "foo was hashed".

		Pekka

Re: -mm -> 2.6.13 merge status

[Hans Reiser]

Pekka Enberg wrote:

>
>
>On Thu, 2005-06-23 at 21:32 +0200, Jens Axboe wrote:
>  
>
>>That said, I don't like the reiser name-number style. If you must do
>>something like this, mark responsibility by using a named identifier
>>covering the layer in question instead.
>>
>>        assert("trace_hash-89", is_hashed(foo) != 0);
>>    
>>
Lots of people like corporate anonymity.  Some don't.  I don't.  I like
knowing who wrote what.  It helps me know who to pay how much.  It helps
me know who to forward the bug report to.   Losing your anonymity
exposes you, mostly for better since more communication is on balance a
good thing, but the fear is there for some.  I don't think we can agree
on this, it is an issue of the soul. 

Re: -mm -> 2.6.13 merge status

[Theodore Ts'o]

On Sat, Jun 25, 2005 at 12:23:41PM -0700, Hans Reiser wrote:
> >>
> >>        assert("trace_hash-89", is_hashed(foo) != 0);
> >>
> Lots of people like corporate anonymity.  Some don't.  I don't.  I like
> knowing who wrote what.  It helps me know who to pay how much.  It helps
> me know who to forward the bug report to.   Losing your anonymity
> exposes you, mostly for better since more communication is on balance a
> good thing, but the fear is there for some.  I don't think we can agree
> on this, it is an issue of the soul. 

Fallacy.

The assert doesn't tell you who is at fault; it tells you who placed
the assert which triggered; it could have triggered due to bugs caused
by anyone, including the propietary binary-only module from Nvidia
which the user loaded into his system....

						- Ted

Re: -mm -> 2.6.13 merge status

[Hans Reiser]

Theodore Ts'o wrote:

>On Sat, Jun 25, 2005 at 12:23:41PM -0700, Hans Reiser wrote:
>  
>
>>>>       assert("trace_hash-89", is_hashed(foo) != 0);
>>>>
>>>>        
>>>>
>>Lots of people like corporate anonymity.  Some don't.  I don't.  I like
>>knowing who wrote what.  It helps me know who to pay how much.  It helps
>>me know who to forward the bug report to.   Losing your anonymity
>>exposes you, mostly for better since more communication is on balance a
>>good thing, but the fear is there for some.  I don't think we can agree
>>on this, it is an issue of the soul. 
>>    
>>
>
>Fallacy.
>
>The assert doesn't tell you who is at fault; it tells you who placed
>the assert which triggered; it could have triggered due to bugs caused
>by anyone, including the propietary binary-only module from Nvidia
>which the user loaded into his system....
>
>						- Ted
>
>
>  
>
If you read the thread again carefully, you will see that I already said
that it doesn't tell you who is at fault for the bug. Furthermore, I
said that the basis of the resistance of some developers to the use of
this is that they are not fully convinced that others understand that it
identifies only the assertion writer not the bug writer. As the boss of
the guys writing these assertions, I see no reason to indulge baseless
fears. When guys become experienced members of our team, they lose this
fear. Sending the bug report to the assertion writer often works nicely
as a first step, in my project, in my experience, in the cases where I
don't know anything about the likely implications of the assertion myself.

Re: -mm -> 2.6.13 merge status

[Hubert Chan]

On Sat, 25 Jun 2005 12:23:41 -0700, Hans Reiser <reiser@namesys.com> said:

>>> assert("trace_hash-89", is_hashed(foo) != 0);

> Lots of people like corporate anonymity.  Some don't.  I don't.  I
> like knowing who wrote what. ...

For what it's worth (I know: not much), I like the named asserts in
Reiser4/Reiserfs.  Although I haven't been bitten by any BUGs yet (maybe
I'm just lucky), whenever I see these on the mailing list, it gives the
impression that the users are interacting more directly with the
developers, and it helps us to get to know the developers better.

If people really want more standard-looking identifiers, I think Namesys
should keep the names and make a hybrid identifier, like
"nikita-123(<file>:<line>)"

-- 
Hubert Chan <hubert@uhoreg.ca> - http://www.uhoreg.ca/
PGP/GnuPG key: 1024D/124B61FA
Fingerprint: 96C5 012F 5F74 A5F7 1FF7  5291 AF29 C719 124B 61FA
Key available at wwwkeys.pgp.net.   Encrypted e-mail preferred.

Re: -mm -> 2.6.13 merge status

[Nikita Danilov]

Hubert Chan <hubert@uhoreg.ca> writes:

> On Sat, 25 Jun 2005 12:23:41 -0700, Hans Reiser <reiser@namesys.com> said:
>
>>>> assert("trace_hash-89", is_hashed(foo) != 0);
>
>> Lots of people like corporate anonymity.  Some don't.  I don't.  I
>> like knowing who wrote what. ...
>
> For what it's worth (I know: not much), I like the named asserts in
> Reiser4/Reiserfs.  Although I haven't been bitten by any BUGs yet (maybe
> I'm just lucky), whenever I see these on the mailing list, it gives the
> impression that the users are interacting more directly with the
> developers, and it helps us to get to know the developers better.
>
> If people really want more standard-looking identifiers, I think Namesys
> should keep the names and make a hybrid identifier, like
> "nikita-123(<file>:<line>)"

This already happens: together with <uid>-<serial>, reiser4 outputs
__FILE__, __LINE__, __FUNCTION__, and a bunch of other stuff:

----------------------------------------------------------------------
reiser4[xine(11262)]: txn_end (fs/reiser4/txnmgr.c:504)[nominaodiosasunt-2967]:
code: -2 at fs/reiser4/search.c:1285
reiser4 panicked cowardly: assertion failed: lock_stack_isclean(get_current_lock_stack())
----------------------------------------------------------------------

Nikita.

Re: -mm -> 2.6.13 merge status

[Jens Axboe]

On Sat, Jun 25 2005, Pekka Enberg wrote:
> Hi,
> 
> On Thu, 2005-06-23 at 21:32 +0200, Jens Axboe wrote:
> > then it's impossible to know which one it is without the identical
> > source at hand.
> 
> In which case, debugging is risky IMO (the source code could have
> changed a lot).

That's not an argument, there are plenty of cases where knowing which
BUG() triggered provides ample clue to at least start thinking about
possible issues.

> On Thu, 2005-06-23 at 21:32 +0200, Jens Axboe wrote:
> > That said, I don't like the reiser name-number style. If you must do
> > something like this, mark responsibility by using a named identifier
> > covering the layer in question instead.
> > 
> >         assert("trace_hash-89", is_hashed(foo) != 0);
> 
> A human readable message would be nicer. For example, "foo was hashed".

Indeed.

-- 
Jens Axboe

Re: -mm -> 2.6.13 merge status

[Andi Kleen]

> > On Thu, 2005-06-23 at 21:32 +0200, Jens Axboe wrote:
> > > That said, I don't like the reiser name-number style. If you must do
> > > something like this, mark responsibility by using a named identifier
> > > covering the layer in question instead.
> > > 
> > >         assert("trace_hash-89", is_hashed(foo) != 0);
> > 
> > A human readable message would be nicer. For example, "foo was hashed".
> 
> Indeed.

You can just dump the expression (with #argument). That is what
traditional userspace assert did forever.

It won't help for BUG_ON(a || b || c || d || e) but these
are bad style anyways and should be avoided.

-Andi

Re: -mm -> 2.6.13 merge status

[Pekka J Enberg]

On Mon, 2005-06-27 at 09:28 +0200, Andi Kleen wrote:
> You can just dump the expression (with #argument). That is what
> traditional userspace assert did forever.
> 
> It won't help for BUG_ON(a || b || c || d || e) but these
> are bad style anyways and should be avoided.

Sounds good to me. Jens, would this work for you?

			Pekka

Show BUG_ON expression when assertion fails.

Signed-off-by: Pekka Enberg <penberg@cs.helsinki.fi>
---

 bug.h |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletion(-)

Index: 2.6/include/asm-generic/bug.h
===================================================================
--- 2.6.orig/include/asm-generic/bug.h
+++ 2.6/include/asm-generic/bug.h
@@ -13,7 +13,12 @@
 #endif
 
 #ifndef HAVE_ARCH_BUG_ON
-#define BUG_ON(condition) do { if (unlikely((condition)!=0)) BUG(); } while(0)
+#define BUG_ON(condition) do { \
+	if (unlikely((condition) != 0)) { \
+		printk("kernel BUG '%s' at %s:%d!\n", #condition, __FILE__, __LINE__); \
+		panic("BUG!"); \
+	} \
+} while(0)
 #endif
 
 #ifndef HAVE_ARCH_WARN_ON

Re: -mm -> 2.6.13 merge status

[Jörn Engel]

On Mon, 27 June 2005 10:49:19 +0300, Pekka J Enberg wrote:
>  
>  #ifndef HAVE_ARCH_BUG_ON
> -#define BUG_ON(condition) do { if (unlikely((condition)!=0)) BUG(); } while(0)
> +#define BUG_ON(condition) do { \
> +	if (unlikely((condition) != 0)) { \
> +		printk("kernel BUG '%s' at %s:%d!\n", #condition, __FILE__, __LINE__); \
> +		panic("BUG!"); \
> +	} \
> +} while(0)
>  #endif

o How about those architectures, where BUG() and panic() are not the
  same thing?
o Embedded people might prefer not to have the additional string
  constants in the kernel.  Some config option would ease their wrath.
  And no, not all embedded people #define BUG() to nothing.

Jörn

-- 
Happiness isn't having what you want, it's wanting what you have.
-- unknown

Re: -mm -> 2.6.13 merge status

[Andi Kleen]

On Mon, Jun 27, 2005 at 10:49:19AM +0300, Pekka J Enberg wrote:
> On Mon, 2005-06-27 at 09:28 +0200, Andi Kleen wrote:
> > You can just dump the expression (with #argument). That is what
> > traditional userspace assert did forever.
> > 
> > It won't help for BUG_ON(a || b || c || d || e) but these
> > are bad style anyways and should be avoided.
> 
> Sounds good to me. Jens, would this work for you?

It won't work for me because it'll bloat the kernel .text
considerable. There is a reason why BUG is implemented
like it is. Compare it.

-Andi

[PATCH] verbose BUG_ON reporting

[Pekka J Enberg]

On Mon, 27 Jun 2005, Andi Kleen wrote:
> It won't work for me because it'll bloat the kernel .text
> considerable. There is a reason why BUG is implemented
> like it is. Compare it.

The assertion codes bloat the kernel all the same. So how about this 
instead?

This patch adds a CONFIG_DEBUG_BUG_ON_VERBOSE that makes BUG_ON report the
evaluated expression in human readable form when the assertion fails.

The size of vmlinux with allyesconfig increases about 100k when the config
option is enabled:

    text    data     bss      dec filename
19333351 6699691 1845604 27878646 vmlinux-2.6.12-git8
19434139 6699691 1845604 27979434 vmlinux-2.6.12-git8-verbose-bug_on

Signed-off-by: Pekka Enberg <penberg@cs.helsinki.fi>
---

 include/asm-generic/bug.h |   11 ++++++++++-
 lib/Kconfig.debug         |    7 +++++++
 2 files changed, 17 insertions(+), 1 deletion(-)

Index: 2.6/include/asm-generic/bug.h
===================================================================
--- 2.6.orig/include/asm-generic/bug.h
+++ 2.6/include/asm-generic/bug.h
@@ -12,8 +12,17 @@
 } while (0)
 #endif
 
+#ifdef CONFIG_DEBUG_BUG_ON_VERBOSE
+#define FAIL_BUG_ON(expr_str) do { \
+	printk("BUG_ON(%s) failed.\n", expr_str); \
+	BUG(); \
+} while (0)
+#else
+#define FAIL_BUG_ON(expr_str) BUG()
+#endif
+
 #ifndef HAVE_ARCH_BUG_ON
-#define BUG_ON(condition) do { if (unlikely((condition)!=0)) BUG(); } while(0)
+#define BUG_ON(condition) do { if (unlikely((condition)!=0)) FAIL_BUG_ON(#condition); } while(0)
 #endif
 
 #ifndef HAVE_ARCH_WARN_ON
Index: 2.6/lib/Kconfig.debug
===================================================================
--- 2.6.orig/lib/Kconfig.debug
+++ 2.6/lib/Kconfig.debug
@@ -116,6 +116,13 @@ config DEBUG_BUGVERBOSE
 	  of the BUG call as well as the EIP and oops trace.  This aids
 	  debugging but costs about 70-100K of memory.
 
+config DEBUG_BUG_ON_VERBOSE
+	bool "Verbose BUG_ON() reporting"
+	depends on DEBUG_KERNEL && BUG
+	default false
+	help
+	  Say Y here to make BUG_ON() panics output the evaluated expression.
+
 config DEBUG_INFO
 	bool "Compile the kernel with debug info"
 	depends on DEBUG_KERNEL

Re: [PATCH] verbose BUG_ON reporting

[Andi Kleen]

On Mon, Jun 27, 2005 at 03:27:50PM +0300, Pekka J Enberg wrote:
> On Mon, 27 Jun 2005, Andi Kleen wrote:
> > It won't work for me because it'll bloat the kernel .text
> > considerable. There is a reason why BUG is implemented
> > like it is. Compare it.
> 
> The assertion codes bloat the kernel all the same. So how about this 
> instead?

It's still useless - it is too bloated to turn on by default
and then if you need it you still won't have it.  And when you
explicitely turn it on then you likely don't need it because
you control the source.

-Andi

Re: verbose BUG_ON reporting

[Pekka J Enberg]

Andi Kleen writes:
> It's still useless - it is too bloated to turn on by default
> and then if you need it you still won't have it.  And when you
> explicitely turn it on then you likely don't need it because
> you control the source.

Hmm. Would a BUG_ON_WITH_TEXT be a better solution? The home-grown assert 
macro in reiser4 can't be a long-term solution if people really want this 
kind of functionality. Btw, the bloat argument applies to assertion codes 
too. 

                   Pekka 

Re: [PATCH] verbose BUG_ON reporting

[Jörn Engel]

On Mon, 27 June 2005 15:27:50 +0300, Pekka J Enberg wrote:
> On Mon, 27 Jun 2005, Andi Kleen wrote:
> > It won't work for me because it'll bloat the kernel .text
> > considerable. There is a reason why BUG is implemented
> > like it is. Compare it.
> 
> The assertion codes bloat the kernel all the same. So how about this 
> instead?

Removes my objections.  I still wouldn't want to enable the config
option, but maybe others do.

Jörn

-- 
A surrounded army must be given a way out.
-- Sun Tzu

Re: -mm -> 2.6.13 merge status

[Hans Reiser]

Jeff Mahoney wrote:

>Pekka Enberg wrote:
>  
>
>>>--- /dev/null	2003-09-23 21:59:22.000000000 +0400
>>>+++ linux-2.6.11-vs/fs/reiser4/pool.c	2005-06-03 17:49:38.668204642 +0400
>>>+/* initialise new pool */
>>>+reiser4_internal void
>>>+reiser4_init_pool(reiser4_pool * pool /* pool to initialise */ ,
>>>+		  size_t obj_size /* size of objects in @pool */ ,
>>>+		  int num_of_objs /* number of preallocated objects */ ,
>>>+		  char *data /* area for preallocated objects */ )
>>>+{
>>>+	reiser4_pool_header *h;
>>>+	int i;
>>>+
>>>+	assert("nikita-955", pool != NULL);
>>>      
>>>
>>These assertion codes are meaningless to the rest of us so please drop
>>them.
>>    
>>
>
>As someone who spends time debugging reiser3 issues, I've grown
>accustomed to the named assertions. They make discussing a particular
>assertion much more natural in conversation than file:line. It also
>makes difficult to reproduce assertions more trackable over time. The
>assertion number never changes, but the line number can with even the
>most trivial of patches.
>
>That said, one of my gripes with the named assertions in reiser3 (and
>reiser4 now) is that the development staff changes over time. There are
>many named assertions in reiser3 that refer to developers no longer
>employed by Hans. The quoted one is a perfect example.
>  
>
Yes, but when I get stuck I still send him an email and he still sends
me an answer.  He is a nice guy even if he can't stand working for me....

>Hans, would it be acceptable to you to keep only numbered assertions and
> keep a code responsbility list internal to namesys?
>
No effort to document who is the current maintainer of a section of our
code (and thus presumed to be able to figure something nonobvious about
it out) has ever worked as well as these named assertions. 

Efforts to put at the top of files who worked on what part of it always
get miserably out of date, and people are always too shy to update them
for valid social reasons.

Internal lists are not the open source way.

The reason some developers hate these named assertions is because they
are afraid that it makes them famous for their bugs, when actually it
does not.  Assertions hit are not equal to bugs authored, and users
understand that better than those developers think.  Writing an
assertion means you can understand a bug, not that you created it.  The
real effect of your name being on many assertions is that people know
you contributed a lot.

It is not necessary for Namesys to be an opaque corporation with no
faces on its surface.  My name is on the filesystem, every bit of credit
I can give to the others I owe them many times over. 

Re: -mm -> 2.6.13 merge status

[Hubert Chan]

On Thu, 23 Jun 2005 14:37:05 -0400, Jeff Mahoney <jeffm@suse.de> said:

> As someone who spends time debugging reiser3 issues, I've grown
> accustomed to the named assertions. They make discussing a particular
> assertion much more natural in conversation than file:line. It also
> makes difficult to reproduce assertions more trackable over time. The
> assertion number never changes, but the line number can with even the
> most trivial of patches.

How about something of the form "nikita-955(file:line)"?  Or the
reverse: "file:line(nikita-955)".  Would that keep everyone happy?

-- 
Hubert Chan <hubert@uhoreg.ca> - http://www.uhoreg.ca/
PGP/GnuPG key: 1024D/124B61FA
Fingerprint: 96C5 012F 5F74 A5F7 1FF7  5291 AF29 C719 124B 61FA
Key available at wwwkeys.pgp.net.   Encrypted e-mail preferred.

Re: -mm -> 2.6.13 merge status

[Christian Trefzer]

[-- Attachment #1: Type: text/plain, Size: 1160 bytes --]

Hubert Chan schrieb:
> How about something of the form "nikita-955(file:line)"?  Or the
> reverse: "file:line(nikita-955)".  Would that keep everyone happy?
> 
Damn, I was wondering how long it would take until someone would come up 
with a compromise solution ; ) Compromises everywhere will lead to 
nowhere, I've learned that the hard way. But this is really not a major 
issue, so let's not make a showstopper out of this one and the likes.

For what I know about the whole inclusion discussion until now, there's 
been a whole lot of flamewar chickenshit so far. Considering that I have 
no FS developing abilities whatsoever, I'm pretty pissed at people who 
do know better in their field and should know better than waste their 
time on discussions other than constructive ones.

Get the personal bullshit out of the way, everyone, please! Get in touch 
and work out your differences in a productive manner. If every 
interesting yet at first intrusive extension to the kernel causes as 
much kindergarten as this one, where will we end up? Stagnation sucks, 
yet good progress is sometimes slow-paced...

Peace, everyone!
Chris
(hardcore, not hippie)


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 894 bytes --]

Re: -mm -> 2.6.13 merge status

[Hans Reiser]

Christian Trefzer wrote:

> Hubert Chan schrieb:
>
>> How about something of the form "nikita-955(file:line)"?  Or the
>> reverse: "file:line(nikita-955)".  Would that keep everyone happy?
>>
Makes me happy.....

Re: -mm -> 2.6.13 merge status

[Christian Trefzer]

[-- Attachment #1: Type: text/plain, Size: 400 bytes --]

Hans Reiser schrieb:
> Christian Trefzer wrote:
> 
> 
>>Hubert Chan schrieb:
>>
>>
>>>How about something of the form "nikita-955(file:line)"?  Or the
>>>reverse: "file:line(nikita-955)".  Would that keep everyone happy?
>>>
> 
> Makes me happy.....
> 

Nice, how about the others?

Hey, if we need some objective and neutral mediators on lkml, I'd be 
glad to give my reading frenzies a meaning : )

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 894 bytes --]

Re: reiser4 plugins

[Andi Kleen]

First Hans let me remind you that this discussion is not XFS vs
reiser4. Christoph does a lot of reviewing and your child definitely
is in serious need of that to be mergeable. I'm sure Christoph is able
to review inpartially even when he is involved with other FS.

Jeff Garzik <jgarzik@pobox.com> writes:
> 
> You're basically implementing another VFS layer inside of reiser4,
> which is a big layering violation.
> 
> This sort of feature should -not- be done at the low-level filesystem level.
> 
> What happens if people decide plugins are a good idea, and they want
> them in ext3?  We need massive surgery to extract the guts from
> reiser4.

We already kind of have them, there are toolkits to do stackable FS with
the existing VFS.

However I suspect Hans is unwilling to redo his file system in this
point. While it looks quite unnecessary there might be other
areas which deserve more attention. In general all the code
needs review, even if it is not as controversal as the reinvented VFS.

-Andi

Re: reiser4 plugins

[Hans Reiser]

Andi Kleen wrote:

> Christoph does a lot of reviewing 
>
and he is notorious for making needed linux contributors go away and not
come back, and I won't say which famous person on this mailing list told
me that....

>and your child definitely
>is in serious need of that to be mergeable. I'm sure Christoph is able
>to review inpartially even when he is involved with other FS.
>  
>
As impartial as a puppy on PCP....

Christoph is aggressive about things he does not take the time to
understand or ask about first.  I hate that.   I wish he would go away
please.  He is not exactly an Ousterhout, Rob Pike, Granger, Mazieres,
Frans Kaashoek, etc.,  in his accomplishments, so why is he reviewing
other people's filesystems?  Reviews are great, how about finding
persons who have created filesystem innovations (and thus are less
likely to reject innovations without understanding them) to do them? 

How about review by benchmark instead?

It works, it runs faster than the competition, users like it, we
addressed the core kernel patch complaints, it should go in and receive
the exposure that will result in lots of useful improvements and
suggestions.   It seems like we are getting an unusual review process. 

I used a bunch of algorithms for which there was a consensus among those
consulted that the algorithms were a bad idea, only the fact that I paid
for the code to be written and required that it be done my way (ignoring
the "you just use me as a typewriter" remarks as best I could)  caused
the algorithms to get implemented at all, and the benchmarks then proved
the algorithms were a good idea.  V3 performance sucks in major part
because I listened to the consensus when I knew better.   I don't really
care for consensus on my FS anymore.   If you guys want to understand
what I am doing I am happy to talk about it extensively, but please
don't require that I groupthink.  I frankly think that with my
benchmarks, I should be allowed to tinker on my own. 

Hans The Mad

Re: reiser4 plugins

[Kyle Moffett]

On Jun 21, 2005, at 22:47:13, Hans Reiser wrote:
> Andi Kleen wrote:
>> and your child definitely
>> is in serious need of that to be mergeable. I'm sure Christoph is  
>> able
>> to review inpartially even when he is involved with other FS.
> As impartial as a puppy on PCP....

So where else are you planning to get a competent reviewer who is fluent
in the internals of filesystems?  Good reviewers don't grow on trees,  
and
in order to be able to understand filesystem issues, one must generally
have worked with them before...  Besides, what good is insulting others
going to do?

> Christoph is aggressive about things he does not take the time to
> understand or ask about first.
[rant snipped]

 From my objective re-reading of his posts, I can see that he is  
critical
of things that are difficult to understand not just to be critical, but
to provoke additional thought over those portions of the code.  In many
cases this leads to better abstractions and simpler code than otherwise.

> How about review by benchmark instead?

/dev/sda is a great filesystem with awesome benchmarks, assuming one  
only
needs to store a single file.  Besides, benchmarks aren't the only thing
important about code.  If the interface consists of:

   void clear_current_filename(void);
   void add_char_to_current_filename(char x);
   void read_bytes_from_current_file(char *byte, unsigned long size);
   void write_bytes_to_current_file(const char *byte, unsigned long  
size);

then this is clearly not a good API, regardless of how well or poorly it
may perform.

> It works, it runs faster than the competition, users like it, we
> addressed the core kernel patch complaints, it should go in and  
> receive
> the exposure that will result in lots of useful improvements and
> suggestions.   It seems like we are getting an unusual review process.

If you look over other patches in -mm, you will see that your review
process is not unusual, especially given the number of concerns that  
other
developers have raised over Reiser4.

[irrelevant algorithm rant snipped]

> If you guys want to understand
> what I am doing I am happy to talk about it extensively, but please
> don't require that I groupthink.  I frankly think that with my
> benchmarks, I should be allowed to tinker on my own.

Yes, you can tinker on your own all you want.  Another project that has
taken that route is GrSecurity, which was alive and well last I checked.

If you don't like others criticisms, take up your marbles and go home,
just don't expect them to accept your work when you've not fixed it to
community standards.

Cheers,
Kyle Moffett

--
Somone asked my why I work on this free (http://www.fsf.org/philosophy/)
software stuff and not get a real job. Charles Shultz had the best  
answer:

"Why do musicians compose symphonies and poets write poems? They do it
because life wouldn't have any meaning for them if they didn't.  
That's why
I draw cartoons. It's my life."
-- Charles Shultz

Re: reiser4 plugins

[Nikita Danilov]

Hans Reiser writes:
 > Andi Kleen wrote:
 > 
 > > Christoph does a lot of reviewing 
 > >
 > and he is notorious for making needed linux contributors go away and not
 > come back, and I won't say which famous person on this mailing list told
 > me that....
 > 
 > >and your child definitely
 > >is in serious need of that to be mergeable. I'm sure Christoph is able
 > >to review inpartially even when he is involved with other FS.
 > >  
 > >
 > As impartial as a puppy on PCP....
 > 
 > Christoph is aggressive about things he does not take the time to
 > understand or ask about first.  I hate that.   I wish he would go away
 > please.  He is not exactly an Ousterhout, Rob Pike, Granger, Mazieres,
 > Frans Kaashoek, etc.,  in his accomplishments, so why is he reviewing
 > other people's filesystems?  Reviews are great, how about finding
 > persons who have created filesystem innovations (and thus are less
 > likely to reject innovations without understanding them) to do them? 

Well, because of his classy hair-style of course.

Seriously, Linux is not managed by a committee. There is nobody to
appoint Official File System Reviewers of Her Majesty. Everything here
(including your credentials as a file system designer) is
self-proclaimed.

 > 
 > How about review by benchmark instead?

[...]

 >                                   I frankly think that with my
 > benchmarks, I should be allowed to tinker on my own. 

I am afraid it will sound picky, but 10 month ago you said you are
planning to replace benchmarks on the namesys.com with fairer ones:

http://marc.theaimsgroup.com/?l=reiserfs&m=109368686019301&w=2

Last time I checked, http://namesys.com/benchmarks.html still features
only mongo runs with overwrite/modify phases off and with all operations
done in readdir order (most favorable mode for reiser4).

 >
 > Hans The Mad
 > 

Nikita.

RE: -mm -> 2.6.13 merge status

[Ian Pratt]

 
> > Xen is making similar noises w.r.t. using kexec for 
> flexible bootloader.
> 
> Oh cool, then we should look at what they're doing instead of 
> reinventing the wheel. Any pointer we can follow, or person 
> we would contact?

Mark.Williamson@cl.cam.ac.uk

Best,
Ian

Re: -mm -> 2.6.13 merge status

[Mark Williamson]

> > > Xen is making similar noises w.r.t. using kexec for
> >
> > flexible bootloader.
> >
> > Oh cool, then we should look at what they're doing instead of
> > reinventing the wheel. Any pointer we can follow, or person
> > we would contact?

Right now, that would be me (hello!).

All going well - I should have Xen guests kexec-ing properly in the next day 
or so.  All the machinery is in place, so it's just a question of doing some 
plumbing.  nb. this is a separate issue to kexecing the whole host, which 
I'll probably look at later.

Carsten: can you tell me what you were planning for your bootloader?  Also, if 
you could point me at any docs regarding your current / proposed boot 
sequence that'd be really interesting.

Regarding our kexec-based bootloader:
* We call running virtual machines "domains".
* Under Xen, guests get built using a kernel specified in domain 0's (the 
"host") filesystem.  That implies that the user of the guest domain can't 
choose their kernel.
* To fix this we now have a bootloader than runs in domain 0, which can poke 
around in the guest's filesystem, find a menu.lst / grub.conf, then load the 
appropriate kernel.
* This provides the functionality the user wants but implies that programs in 
dom0 have to access the guest filesystem, which could be malicious.  We'd 
rather not have programs in dom0 trusting guests.
* The proposed solution is to initially run a "bootloader kernel", with a 
bootloader app on an initrd.  This will run in the guest itself, so all the 
filesystem accesses occur in an unprivileged virtual machine.
* The bootloader will cause a kexec to the new kernel.
* This fixes the isolation problem and immediately allows us to support all 
the random filesystems Linux supports ;-)

My current plan is that the bootloader app will act much like Grub and use 
Grub's config files.  It'll also be possible to use something more 
heavyweight, such as XenoBoot 
(http://www.cl.cam.ac.uk/Research/SRG/netos/xeno/xenoboot/).  It's possible 
that XenoBoot has some code we could use in the bootloader - I haven't 
looked.

Feel free to mail me offline.  If our goals are compatible, it'd be good to 
work together on this.

Cheers,
Mark

Re: -mm -> 2.6.13 merge status

[Bodo Eggert]

CC Linus, Jesper Juhl (who's currently doing some cleanups)

Andrew Morton <akpm@osdl.org> wrote:

> *general sigh*.  I wish people would absorb CodingStyle.  It's not hard,
> and fixing the style post-facto creates a real mess.  I now have a great
> string of kexec patches followed by a "kexec-code-cleanup.patch" which
> totally buggers up the patch sequencing and really needs to be split into
> 18 parts and sprinkled back over the entire series.

I scripted an automatic whitespace cleanup, which resuled in a fat
patchbomb (about 18 MB, split into > 3600 files (because that way, some
patches are going to apply cleanly)). Obviously applying that would increase
the patch size for the next version by 100%, so that won't be the way to go.
(If you still want to look, see
 http://7eggert.dyndns.org/l/patches/trailing-ws/)

Therefore I suggest that I will
 - make a script that will take a patch, apply it and cleanup the patched
   files as far as a simple script can do the job, so each patched file
   will be ws-clean and the amount of patches will still stay low.
 - a second script that will do some cleanup while the resulting patch is
   less than an annoying amount of KB, so you can cleanup some files that
   wouldn't get patched otherwise.
-- 
Ich danke GMX dafür, die Verwendung meiner Adressen mittels per SPF
verbreiteten Lügen zu sabotieren.

-mm -> 2.6.13 merge status

[Andrew Morton]

This summarises my current thinking on various patches which are presently
in -mm.  I cover large things and small-but-controversial things.  Anything
which isn't covered here (and that's a lot of material) is probably a "will
merge", unless it obviously isn't.

(If you reply to this email it would be a good idea to alter the Subject:
to reflect which feature you are discussing)



git-ocfs

    The OCFS2 filesystem.  OK by me, although I'm not sure it's had enough
    review.

sparsemem

    OK by me for a merge.  Need to poke arch maintainers first, check that
    they've looked at it sufficiently closely.

vm-early-zone-reclaim

    Needs some convincing benchmark numbers to back it up.  Otherwise OK.

avoiding-mmap-fragmentation

    Tricky.  Addresses vm area fragmentation issues due to recent
    optimisations to the free-area lookup code.  Will merge.

periodically-drain-non-local-pagesets

    Will merge

pcibus_to_node and users

    Will merge

CONFIG_HZ for x86 and ia64: changes default HZ to 250, make HZ Kconfigurable.

    Will merge (will switch default to 1000 Hz later if that seems necessary)

dmi-*.patch

    Will merge.  I have a comment "The below break x440".  Maybe it got
    fixed.  We'll doubtless hear if not.

xen-*.patch

    These are little cleanups and abstractions which make a Xen merge
    easier.  May as well merge them.

CPU hotplug for x86 and x86_64

    Not really useful on current hardware, but these provide
    infrastructure which some power management patches need, and it seems
    sensible to make the reference architecture support hotplug.  Will merge.

swsusp-on-SMP

    Will merge.

cfq version 3

    Not sure.  Jens seems to be setting up a few git trees.  On hold.

RCUification of the key management code

    Don't know - dhowells seemed diffident last time we discussed this.

timers-fixes-improvements.patch

    SMP speedups for the core timer code.  It was bumpy, but this seems
    stable now.  Will merge.

kprobes-*

    Will merge

rapidio-*

    Will merge.

namespace*.patch

    Awaiting viro ack.

xtensa architecture

    Is xtensa now, or will it be in the future a sufficiently popular
    architecture to justify the cost of having this code in the tree?

    Heaven knows.  Will merge.

dlm-*.patch: Red Hat distributed lock manager

    Hard.  Right now it seems that no in-kernel projects will use this and
    only one out-of-kernel project will use it.  Shelve the problem until
    after Kernel Summit, where some light may be shed.

    Opinions are sought...

connector.patch

    Nice idea IMO, but there are still questions around the
    implementation.  More dialogue needed ;)

connector-add-a-fork-connector.patch

    OK, but needs connector.

inotify

    There are still concerns about the userspace API and internal
    implementation details.  More slogging needed.

pcmcia-*.patch

    Makes the pcmcia layer generate hotplug events and deprecates cardmgr.
    Will merge.

NUMA-aware slab allocator

    Seems stable now, but it needs some ifdef reduction work before
    merging, please.

CPU scheduler

    Will merge some of these patches.  We're still discussing which ones.

perfctr

    Not yet, but getting closer.  The PPC64 guys still need to sort out a
    few interface issues with Mikael.  We might be able to fit this into
    2.6.13 if people get a move on.

cachefs

    This is a ton of code which knows rather a lot about pagecache
    internals.  It allows the AFS client to cache file contents on a local
    blockdev.

    I don't think it's a justified addition for only AFS and I'd prefer to
    see it proven for NFS as well.

    Issues around add-page-becoming-writable-notification.patch need to
    be resolved.

cachefs-for-nfs

    A recent addition.  Needs review from NFS developers and considerably
    more testing.

    These things aren't looking likely for 2.6.13.

kexec and kdump

    I guess we should merge these.

    I'm still concerned that the various device shutdown problems will
    mean that the success rate for crashing kernels is not high enough for
    kdump to be considered a success.  In which case in six months time we'll
    hear rumours about vendors shipping wholly different crashdump
    implementations, which would be quite bad.

    But I think this has gone as far as it can go in -mm, so it's a bit of
    a punt.

reiser4

    Merge it, I guess.

    The patches still contain all the reiser4-specific namespace
    enhancements, only it is disabled, so it is effectively dead code.  Maybe
    we should ask that it actually be removed?

v9fs

    I'm not sure that this has a sufficiently high
    usefulness-to-maintenance-cost ratio.

fuse

    This is useful, but there are, AFAIK, two issues:

    - We're still deadlocked over some permission-checking hacks in there

    - It has an NFS server implementation which only works if the
      to-be-served file happens to be in dcache.

      It has been said that a userspace NFS server can be used to get
      full NFS server functionality with FUSE.  I think the half-assed kernel
      implementation should be done away with.

execute-in-place

    Will merge.  Have the embedded guys commented on the usefulness of
    this for execute-out-of-ROM?

Re: -mm -> 2.6.13 merge status

[Andrey Panin]

[-- Attachment #1: Type: text/plain, Size: 6529 bytes --]

On 171, 06 20, 2005 at 11:54:58 -0700, Andrew Morton wrote:
> 
> This summarises my current thinking on various patches which are presently
> in -mm.  I cover large things and small-but-controversial things.  Anything
> which isn't covered here (and that's a lot of material) is probably a "will
> merge", unless it obviously isn't.
> 
> (If you reply to this email it would be a good idea to alter the Subject:
> to reflect which feature you are discussing)
> 
> 
> 
> git-ocfs
> 
>     The OCFS2 filesystem.  OK by me, although I'm not sure it's had enough
>     review.
> 
> sparsemem
> 
>     OK by me for a merge.  Need to poke arch maintainers first, check that
>     they've looked at it sufficiently closely.
> 
> vm-early-zone-reclaim
> 
>     Needs some convincing benchmark numbers to back it up.  Otherwise OK.
> 
> avoiding-mmap-fragmentation
> 
>     Tricky.  Addresses vm area fragmentation issues due to recent
>     optimisations to the free-area lookup code.  Will merge.
> 
> periodically-drain-non-local-pagesets
> 
>     Will merge
> 
> pcibus_to_node and users
> 
>     Will merge
> 
> CONFIG_HZ for x86 and ia64: changes default HZ to 250, make HZ Kconfigurable.
> 
>     Will merge (will switch default to 1000 Hz later if that seems necessary)
> 
> dmi-*.patch
> 
>     Will merge.  I have a comment "The below break x440".  Maybe it got
>     fixed.  We'll doubtless hear if not.

Fixed, patch merged in -mm as dmi-move-acpi-sleep-quirk-fix.patch

http://marc.theaimsgroup.com/?l=linux-kernel&m=111829134708641&w=2
http://marc.theaimsgroup.com/?l=linux-kernel&m=111832375203467&w=2

> xen-*.patch
> 
>     These are little cleanups and abstractions which make a Xen merge
>     easier.  May as well merge them.
> 
> CPU hotplug for x86 and x86_64
> 
>     Not really useful on current hardware, but these provide
>     infrastructure which some power management patches need, and it seems
>     sensible to make the reference architecture support hotplug.  Will merge.
> 
> swsusp-on-SMP
> 
>     Will merge.
> 
> cfq version 3
> 
>     Not sure.  Jens seems to be setting up a few git trees.  On hold.
> 
> RCUification of the key management code
> 
>     Don't know - dhowells seemed diffident last time we discussed this.
> 
> timers-fixes-improvements.patch
> 
>     SMP speedups for the core timer code.  It was bumpy, but this seems
>     stable now.  Will merge.
> 
> kprobes-*
> 
>     Will merge
> 
> rapidio-*
> 
>     Will merge.
> 
> namespace*.patch
> 
>     Awaiting viro ack.
> 
> xtensa architecture
> 
>     Is xtensa now, or will it be in the future a sufficiently popular
>     architecture to justify the cost of having this code in the tree?
> 
>     Heaven knows.  Will merge.
> 
> dlm-*.patch: Red Hat distributed lock manager
> 
>     Hard.  Right now it seems that no in-kernel projects will use this and
>     only one out-of-kernel project will use it.  Shelve the problem until
>     after Kernel Summit, where some light may be shed.
> 
>     Opinions are sought...
> 
> connector.patch
> 
>     Nice idea IMO, but there are still questions around the
>     implementation.  More dialogue needed ;)
> 
> connector-add-a-fork-connector.patch
> 
>     OK, but needs connector.
> 
> inotify
> 
>     There are still concerns about the userspace API and internal
>     implementation details.  More slogging needed.
> 
> pcmcia-*.patch
> 
>     Makes the pcmcia layer generate hotplug events and deprecates cardmgr.
>     Will merge.
> 
> NUMA-aware slab allocator
> 
>     Seems stable now, but it needs some ifdef reduction work before
>     merging, please.
> 
> CPU scheduler
> 
>     Will merge some of these patches.  We're still discussing which ones.
> 
> perfctr
> 
>     Not yet, but getting closer.  The PPC64 guys still need to sort out a
>     few interface issues with Mikael.  We might be able to fit this into
>     2.6.13 if people get a move on.
> 
> cachefs
> 
>     This is a ton of code which knows rather a lot about pagecache
>     internals.  It allows the AFS client to cache file contents on a local
>     blockdev.
> 
>     I don't think it's a justified addition for only AFS and I'd prefer to
>     see it proven for NFS as well.
> 
>     Issues around add-page-becoming-writable-notification.patch need to
>     be resolved.
> 
> cachefs-for-nfs
> 
>     A recent addition.  Needs review from NFS developers and considerably
>     more testing.
> 
>     These things aren't looking likely for 2.6.13.
> 
> kexec and kdump
> 
>     I guess we should merge these.
> 
>     I'm still concerned that the various device shutdown problems will
>     mean that the success rate for crashing kernels is not high enough for
>     kdump to be considered a success.  In which case in six months time we'll
>     hear rumours about vendors shipping wholly different crashdump
>     implementations, which would be quite bad.
> 
>     But I think this has gone as far as it can go in -mm, so it's a bit of
>     a punt.
> 
> reiser4
> 
>     Merge it, I guess.
> 
>     The patches still contain all the reiser4-specific namespace
>     enhancements, only it is disabled, so it is effectively dead code.  Maybe
>     we should ask that it actually be removed?
> 
> v9fs
> 
>     I'm not sure that this has a sufficiently high
>     usefulness-to-maintenance-cost ratio.
> 
> fuse
> 
>     This is useful, but there are, AFAIK, two issues:
> 
>     - We're still deadlocked over some permission-checking hacks in there
> 
>     - It has an NFS server implementation which only works if the
>       to-be-served file happens to be in dcache.
> 
>       It has been said that a userspace NFS server can be used to get
>       full NFS server functionality with FUSE.  I think the half-assed kernel
>       implementation should be done away with.
> 
> execute-in-place
> 
>     Will merge.  Have the embedded guys commented on the usefulness of
>     this for execute-out-of-ROM?
> 
> 
> 
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

-- 
Andrey Panin		| Linux and UNIX system administrator
pazke@donpac.ru		| PGP key: wwwkeys.pgp.net

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: -mm -> 2.6.13 merge status

[Alan Cox]

On Maw, 2005-06-21 at 07:54, Andrew Morton wrote:
> CONFIG_HZ for x86 and ia64: changes default HZ to 250, make HZ Kconfigurable.
>     Will merge (will switch default to 1000 Hz later if that seems necessary)

This has been in Fedora for a while. DaveJ can probably give you more
info. From own testing 100Hz is how far down you want to go to avoid
random clock slew on laptops and to see power improvements.

Re: -mm -> 2.6.13 merge status

[Arjan van de Ven]

[-- Attachment #1: Type: text/plain, Size: 613 bytes --]

On Tue, 2005-06-21 at 13:35 +0100, Alan Cox wrote:
> On Maw, 2005-06-21 at 07:54, Andrew Morton wrote:
> > CONFIG_HZ for x86 and ia64: changes default HZ to 250, make HZ Kconfigurable.
> >     Will merge (will switch default to 1000 Hz later if that seems necessary)
> 
> This has been in Fedora for a while. DaveJ can probably give you more
> info. From own testing 100Hz is how far down you want to go to avoid
> random clock slew on laptops and to see power improvements.

actually 250Hz is a not so fun value. 300 is a lot nicer (multiple of
both 50Hz and 60Hz and thus covers most TV standards)


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: -mm -> 2.6.13 merge status

[Erik Slagter]

On Tue, 2005-06-21 at 09:07 -0400, Arjan van de Ven wrote:
> On Tue, 2005-06-21 at 13:35 +0100, Alan Cox wrote:
> > On Maw, 2005-06-21 at 07:54, Andrew Morton wrote:
> > > CONFIG_HZ for x86 and ia64: changes default HZ to 250, make HZ Kconfigurable.
> > >     Will merge (will switch default to 1000 Hz later if that seems necessary)
> > 
> > This has been in Fedora for a while. DaveJ can probably give you more
> > info. From own testing 100Hz is how far down you want to go to avoid
> > random clock slew on laptops and to see power improvements.
> 
> actually 250Hz is a not so fun value. 300 is a lot nicer (multiple of
> both 50Hz and 60Hz and thus covers most TV standards)

Sorry, the ITU-R "M" standard specifies 30000/1001 frames (60000/1001
fields) per second, that's close to 30/60 but not the same. Now please
please don't make use run HZ = (60000/1001 * 5) or similar ;-)

BTW it seems to me that power management using C2/C3 states will work
much more efficiently with a lower HZ because the chipset/processor will
be a larger percentage of the time in c2/c3 mode, right?

Re: -mm -> 2.6.13 merge status

[Carsten Otte]

On 6/21/05, Andrew Morton <akpm@osdl.org> wrote:
> execute-in-place
> 
>     Will merge.  Have the embedded guys commented on the usefulness of
>     this for execute-out-of-ROM?
Allright. Going to push our test-team to run their tests on the xip
code that is in -mm.

Re: -mm -> 2.6.13 merge status

[Jörn Engel]

On Mon, 20 June 2005 23:54:58 -0700, Andrew Morton wrote:
> 
> execute-in-place
> 
>     Will merge.  Have the embedded guys commented on the usefulness of
>     this for execute-out-of-ROM?

It looks fairly useful, but XIP for NOR flashes still needs additional
work.  No objections from my side.

Jörn

-- 
Optimizations always bust things, because all optimizations are, in
the long haul, a form of cheating, and cheaters eventually get caught.
-- Larry Wall 

Re: -mm -> 2.6.13 merge status

[Martin Hicks]

On Mon, Jun 20, 2005 at 11:54:58PM -0700, Andrew Morton wrote:
> 
> vm-early-zone-reclaim
> 
>     Needs some convincing benchmark numbers to back it up.  Otherwise OK.

The only benchmarks I have for this were included in my last mail to
linux-mm:

http://marc.theaimsgroup.com/?l=linux-mm&m=111763597218177&w=2

Are they convincing?  Well, the patch doesn't seem to make the memory
thrashing case much worse ("make -j" kernbench run) which is a good
thing since the VM is trying to reclaim much earlier.

In the same e-mail I mention that there is a fairly good performance
gain in the optimal case, where processes are tied to a single node and
the node's memory is filled with page cache.  With zone reclaim turned
on the "make -j8" kernel build runs in 700 seconds;  735 seconds with
no reclaim.

mh

-- 
Martin Hicks                Wild Open Source Inc.
mort@wildopensource.com     613-266-2296

Re: -mm -> 2.6.13 merge status

[Andrew Morton]

Martin Hicks <mort@wildopensource.com> wrote:
>
> On Mon, Jun 20, 2005 at 11:54:58PM -0700, Andrew Morton wrote:
>  > 
>  > vm-early-zone-reclaim
>  > 
>  >     Needs some convincing benchmark numbers to back it up.  Otherwise OK.
> 
>  The only benchmarks I have for this were included in my last mail to
>  linux-mm:
> 
>  http://marc.theaimsgroup.com/?l=linux-mm&m=111763597218177&w=2
> 
>  Are they convincing?  Well, the patch doesn't seem to make the memory
>  thrashing case much worse ("make -j" kernbench run) which is a good
>  thing since the VM is trying to reclaim much earlier.
> 
>  In the same e-mail I mention that there is a fairly good performance
>  gain in the optimal case, where processes are tied to a single node and
>  the node's memory is filled with page cache.  With zone reclaim turned
>  on the "make -j8" kernel build runs in 700 seconds;  735 seconds with
>  no reclaim.

Ah, OK, I failed to capture that info.  (I always have to move the info in
the [patch 0/n] email into the first real patch, and this time I didn't)

Thanks.

Re: -mm -> 2.6.13 merge status

[Martin Hicks]

On Tue, Jun 21, 2005 at 12:54:57PM -0700, Andrew Morton wrote:
> 
> Ah, OK, I failed to capture that info.  (I always have to move the info in
> the [patch 0/n] email into the first real patch, and this time I didn't)

Oops.  I'll try to remember to stick the benchmark info into one of the
real patches next time.

mh

-- 
Martin Hicks                Wild Open Source Inc.
mort@wildopensource.com     613-266-2296

Re: -mm -> 2.6.13 merge status

[Jeff Garzik]

Andrew Morton wrote:
> git-ocfs
> 
>     The OCFS2 filesystem.  OK by me, although I'm not sure it's had enough
>     review.

Every time I come up with a complaint about ocfs2, the Oracle guys 
manage to shoot me down.  I think it's OK to merge.


> sparsemem
> 
>     OK by me for a merge.  Need to poke arch maintainers first, check that
>     they've looked at it sufficiently closely.

seems sane, though there are some whitespace niggles that should be 
cleaned up


> rapidio-*
> 
>     Will merge.

send through netdev, as is proper


> dlm-*.patch: Red Hat distributed lock manager
> 
>     Hard.  Right now it seems that no in-kernel projects will use this and
>     only one out-of-kernel project will use it.  Shelve the problem until
>     after Kernel Summit, where some light may be shed.
> 
>     Opinions are sought...

I hate to say it, since its my own employer, but I agree:  wait for 
in-kernel users, at the very least.


> connector.patch
> 
>     Nice idea IMO, but there are still questions around the
>     implementation.  More dialogue needed ;)
> 
> connector-add-a-fork-connector.patch
> 
>     OK, but needs connector.

I don't like connector


> inotify
> 
>     There are still concerns about the userspace API and internal
>     implementation details.  More slogging needed.

We should ask hpa what he needs for kernel.org.  Ideally kernel.org 
probably wants <something> that facilitates listening to <something> for 
a list of files being changed.  That would greatly speed up the robots, 
and possibly rsync-like activities too.


> pcmcia-*.patch
> 
>     Makes the pcmcia layer generate hotplug events and deprecates cardmgr.
>     Will merge.

Testing?  The goal behind the patch is certainly good, but I worry about 
exposure.


> cachefs
> 
>     This is a ton of code which knows rather a lot about pagecache
>     internals.  It allows the AFS client to cache file contents on a local
>     blockdev.
> 
>     I don't think it's a justified addition for only AFS and I'd prefer to
>     see it proven for NFS as well.
> 
>     Issues around add-page-becoming-writable-notification.patch need to
>     be resolved.
> 
> cachefs-for-nfs
> 
>     A recent addition.  Needs review from NFS developers and considerably
>     more testing.
> 
>     These things aren't looking likely for 2.6.13.

If I could vote more than once, I would!  I really like cachefs, and 
have been pushing for its inclusion for a while.


> kexec and kdump
> 
>     I guess we should merge these.
> 
>     I'm still concerned that the various device shutdown problems will
>     mean that the success rate for crashing kernels is not high enough for
>     kdump to be considered a success.  In which case in six months time we'll
>     hear rumours about vendors shipping wholly different crashdump
>     implementations, which would be quite bad.
> 
>     But I think this has gone as far as it can go in -mm, so it's a bit of
>     a punt.

I'm not particularly pleased with these, and indeed vendors ARE shipping 
other crashdump methods.


> reiser4
> 
>     Merge it, I guess.
> 
>     The patches still contain all the reiser4-specific namespace
>     enhancements, only it is disabled, so it is effectively dead code.  Maybe
>     we should ask that it actually be removed?

The plugin stuff is crap.  This is not a filesystem but a filesystem + 
new layer.  IMO considered in that light, it duplicates functionality 
elsewhere.


> v9fs
> 
>     I'm not sure that this has a sufficiently high
>     usefulness-to-maintenance-cost ratio.

agreed (though I think 9P is neat)


>       It has been said that a userspace NFS server can be used to get
>       full NFS server functionality with FUSE.  I think the half-assed kernel
>       implementation should be done away with.

"It has been said" -- its true.  A userspace NFS server can do 100% of 
userspace FS functionality.

	Jeff

Re: -mm -> 2.6.13 merge status

[Robert Love]

On Tue, 2005-06-21 at 11:26 -0400, Jeff Garzik wrote:

> > inotify
> > 
> >     There are still concerns about the userspace API and internal
> >     implementation details.  More slogging needed.
> 
> We should ask hpa what he needs for kernel.org.  Ideally kernel.org 
> probably wants <something> that facilitates listening to <something> for 
> a list of files being changed.  That would greatly speed up the robots, 
> and possibly rsync-like activities too.

I've talked to some people who've hooked inotify into rsync
successfully.  Cool hack.

	Robert Love

Re: -mm -> 2.6.13 merge status

[Christoph Lameter]

On Tue, 21 Jun 2005, Robert Love wrote:

> > We should ask hpa what he needs for kernel.org.  Ideally kernel.org 
> > probably wants <something> that facilitates listening to <something> for 
> > a list of files being changed.  That would greatly speed up the robots, 
> > and possibly rsync-like activities too.
> 
> I've talked to some people who've hooked inotify into rsync
> successfully.  Cool hack.

I noticed that select() is not working on real files. Could inotify 
be used to fix select()?

Re: -mm -> 2.6.13 merge status

[Robert Love]

On Tue, 2005-06-21 at 12:22 -0700, Christoph Lameter wrote:

> I noticed that select() is not working on real files. Could inotify 
> be used to fix select()?

Select the system call?  It should work fine.   ;-)

Who is confused?

	Robert Love

Re: -mm -> 2.6.13 merge status

[Christoph Lameter]

On Tue, 21 Jun 2005, Robert Love wrote:

> On Tue, 2005-06-21 at 12:22 -0700, Christoph Lameter wrote:
> 
> > I noticed that select() is not working on real files. Could inotify 
> > be used to fix select()?
> 
> Select the system call?  It should work fine.   ;-)

Hmmm. I just wrote an app that uses select to do essentially a "tail" 
waiting for new content in a log file. The file descriptors for real disk 
files are always ready even if there is no content available for the 
application.

The file is positioned at the end of the file after open via lseek.
select tells me that data is available but the read() returns zero bytes.

The current fix on the app level is to checking if useful work was 
done as a result of "READY" file descriptors. If the read() operations
do not return any data then the app will simply sleep for a couple of 
seconds. So the app degenerates to a kind of poll mode if disk files are 
used.

Re: -mm -> 2.6.13 merge status

[Zan Lynx]

[-- Attachment #1: Type: text/plain, Size: 631 bytes --]

On Tue, 2005-06-21 at 15:38 -0400, Robert Love wrote:
> On Tue, 2005-06-21 at 12:22 -0700, Christoph Lameter wrote:
> 
> > I noticed that select() is not working on real files. Could inotify 
> > be used to fix select()?
> 
> Select the system call?  It should work fine.   ;-)
> 
> Who is confused?
> 
> 	Robert Love

Sounds interesting.  tail -f could use it.  Instead of sleep 1, seek to
current position, read to eof; just select() for read on the file and
sleep in select() until someone else writes to that file.  

I've never tried doing that.  It might work, for all I know.
-- 
Zan Lynx <zlynx@acm.org>

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: -mm -> 2.6.13 merge status

[Christoph Lameter]

On Tue, 21 Jun 2005, Zan Lynx wrote:

> I've never tried doing that.  It might work, for all I know.

I was told that Linux cannot do this. It always returns the filehandles as 
ready for disk files.

Re: -mm -> 2.6.13 merge status

[Robert Love]

On Tue, 2005-06-21 at 13:06 -0700, Christoph Lameter wrote:
> On Tue, 21 Jun 2005, Zan Lynx wrote:
> 
> > I've never tried doing that.  It might work, for all I know.
> 
> I was told that Linux cannot do this. It always returns the filehandles as 
> ready for disk files.

Inotify would definitely work.

	Robert Love

Re: -mm -> 2.6.13 merge status

[Christoph Lameter]

On Tue, 21 Jun 2005, Robert Love wrote:

> > I was told that Linux cannot do this. It always returns the filehandles as 
> > ready for disk files.
> 
> Inotify would definitely work.

Well we could use it in kernel to make select() work correctly. For disk 
files set up a notification for write and then only return from select if 
new data is available.

Re: -mm -> 2.6.13 merge status

[Zan Lynx]

[-- Attachment #1: Type: text/plain, Size: 511 bytes --]

On Tue, 2005-06-21 at 13:10 -0700, Christoph Lameter wrote:
> On Tue, 21 Jun 2005, Robert Love wrote:
> 
> > > I was told that Linux cannot do this. It always returns the filehandles as 
> > > ready for disk files.
> > 
> > Inotify would definitely work.
> 
> Well we could use it in kernel to make select() work correctly. For disk 
> files set up a notification for write and then only return from select if 
> new data is available.

You could do it inside glibc.
-- 
Zan Lynx <zlynx@acm.org>

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: -mm -> 2.6.13 merge status

[Matthias Urlichs]

Hi, Christoph Lameter wrote:

> Well we could use it in kernel to make select() work correctly.

select() already works correctly. It answers the "will I not block if I
call read()/write() on this" question, and since you never block on files
(assuming infinite disk speed ;-) select() will always return True on it.

You can't change this, it's in POSIX.

... or maybe I misunderstood your comment.

-- 
Matthias Urlichs   |   {M:U} IT Design @ m-u-it.de   |  smurf@smurf.noris.de
Disclaimer: The quote was selected randomly. Really. | http://smurf.noris.de
 - -
"I don't really miss God
 but i sure miss Santa Claus!"
      [Courtney Love]

Re: -mm -> 2.6.13 merge status

[Alan Cox]

On Maw, 2005-06-21 at 21:06, Christoph Lameter wrote:
> On Tue, 21 Jun 2005, Zan Lynx wrote:
> 
> > I've never tried doing that.  It might work, for all I know.
> 
> I was told that Linux cannot do this. It always returns the filehandles as 
> ready for disk files.

That is because disk files are always ready - select/poll are for waits
for data (or space) to become available not for events in the sense of
inotify.
That said there *is* scope in the poll() API [but not select()] to add a
new kind of poll notification type.

Re: -mm -> 2.6.13 merge status

[Stephen Hemminger]

On Tue, 21 Jun 2005 14:02:09 -0600
Zan Lynx <zlynx@acm.org> wrote:

> On Tue, 2005-06-21 at 15:38 -0400, Robert Love wrote:
> > On Tue, 2005-06-21 at 12:22 -0700, Christoph Lameter wrote:
> > 
> > > I noticed that select() is not working on real files. Could inotify 
> > > be used to fix select()?
> > 
> > Select the system call?  It should work fine.   ;-)
> > 
> > Who is confused?
> > 
> > 	Robert Love
> 
> Sounds interesting.  tail -f could use it.  Instead of sleep 1, seek to
> current position, read to eof; just select() for read on the file and
> sleep in select() until someone else writes to that file.  
> 
> I've never tried doing that.  It might work, for all I know.
> -- 
> Zan Lynx <zlynx@acm.org>


Posix requires select() of regular files always return true:
	http://www.opengroup.org/onlinepubs/009695399/functions/select.html

	File descriptors associated with regular files shall always select true for ready to read, 
	ready to write, and error conditions.

-- 
Stephen Hemminger	<shemminger@osdl.org>

Re: -mm -> 2.6.13 merge status

[Jeff Garzik]

Christoph Lameter wrote:
> On Tue, 21 Jun 2005, Robert Love wrote:
> 
> 
>>>We should ask hpa what he needs for kernel.org.  Ideally kernel.org 
>>>probably wants <something> that facilitates listening to <something> for 
>>>a list of files being changed.  That would greatly speed up the robots, 
>>>and possibly rsync-like activities too.
>>
>>I've talked to some people who've hooked inotify into rsync
>>successfully.  Cool hack.
> 
> 
> I noticed that select() is not working on real files. Could inotify 
> be used to fix select()?

Non-blocking file I/O is an open issue.

AIO is probably a better path.

	Jeff

Re: -mm -> 2.6.13 merge status

[Christoph Lameter]

On Tue, 21 Jun 2005, Jeff Garzik wrote:

> Non-blocking file I/O is an open issue.
> 
> AIO is probably a better path.

AIO is requiring you to poll and check if I/O is complete. select() does 
not require any polling and just needs to be made to work the way it was 
intended to.

Re: -mm -> 2.6.13 merge status

[Jeff Garzik]

Christoph Lameter wrote:
> On Tue, 21 Jun 2005, Jeff Garzik wrote:
> 
> 
>>Non-blocking file I/O is an open issue.
>>
>>AIO is probably a better path.
> 
> 
> AIO is requiring you to poll and check if I/O is complete. select() does 

Incorrect.  The entire point of AIO is that its an async callback 
system, when the I/O is complete...  just like the kernel's internal I/O 
request queue system.

	Jeff

Re: -mm -> 2.6.13 merge status

[Christoph Lameter]

On Tue, 21 Jun 2005, Jeff Garzik wrote:

> > AIO is requiring you to poll and check if I/O is complete. select() does 
> 
> Incorrect.  The entire point of AIO is that its an async callback system, when
> the I/O is complete...  just like the kernel's internal I/O request queue
> system.

Hmmm.. Okay it may work like dnotify. You get some signal and 
then its up to you to figure out what was going on. Traditionally select() 
does that all for you and tells you which stream got input.

Re: -mm -> 2.6.13 merge status

[Matt Porter]

On Tue, Jun 21, 2005 at 11:26:44AM -0400, Jeff Garzik wrote:
> Andrew Morton wrote:
> > rapidio-*
> > 
> >     Will merge.
> 
> send through netdev, as is proper

rapidio-support-net-driver.patch is the only netdev portion.

-Matt

Re: -mm -> 2.6.13 merge status

[randy_dunlap]

On Tue, 21 Jun 2005 11:26:44 -0400 Jeff Garzik wrote:

| Andrew Morton wrote:
| 
| > connector.patch
| > 
| >     Nice idea IMO, but there are still questions around the
| >     implementation.  More dialogue needed ;)
| > 
| > connector-add-a-fork-connector.patch
| > 
| >     OK, but needs connector.
| 
| I don't like connector

can you be more specific, like you did with reiser4?


| > kexec and kdump
| > 
| >     I guess we should merge these.
| > 
| >     I'm still concerned that the various device shutdown problems will
| >     mean that the success rate for crashing kernels is not high enough for
| >     kdump to be considered a success.  In which case in six months time we'll
| >     hear rumours about vendors shipping wholly different crashdump
| >     implementations, which would be quite bad.
| > 
| >     But I think this has gone as far as it can go in -mm, so it's a bit of
| >     a punt.
| 
| I'm not particularly pleased with these, and indeed vendors ARE shipping 
| other crashdump methods.

any specifics on the "not particularly pleased" part?

| > reiser4
| > 
| >     Merge it, I guess.
| > 
| >     The patches still contain all the reiser4-specific namespace
| >     enhancements, only it is disabled, so it is effectively dead code.  Maybe
| >     we should ask that it actually be removed?
| 
| The plugin stuff is crap.  This is not a filesystem but a filesystem + 
| new layer.  IMO considered in that light, it duplicates functionality 
| elsewhere.

I don't think that r4 is just a filesystem either, but you know more
about that than I do.


thanks,
---
~Randy

Re: -mm -> 2.6.13 merge status

[Hans Reiser]

Jeff Garzik wrote:

>
>
>> reiser4
>>
>>    
>
>
> The plugin stuff is crap.  This is not a filesystem but a filesystem +
> new layer.  IMO considered in that light, it duplicates functionality
> elsewhere.


What functionality where?  Please remember that this is per file, per
item, per node, per attribute, per disk format, per bitmap, per super
block, etc.,  abstracting, not per filesystem abstracting.

Plugins allow a number of things:

1) They allow us to never pay the cost to change the disk format again,
no matter how much we add in future years.  This really matters: the
prohibitive cost of disk format changes are the number one impediment to
filesystem improvements, and the primary reason why most filesystems
ossify after time has past.

2) They allow us to easily structure code for reuse.   If we want to
create a new kind of file that is like some other kind of file except
for one thing, we just write the one thing, and then easily reuse all
the other code, and create a new plugin id. 

The use of plugins forced all the programmers to think about reusability
at every layer of design.  V3 of reiserfs is way too hard to work on and
modify.  If you ask one of the team to code something for V3 instead of
V4, they quietly groan at the thought.  It is just so much easier to do
in V4.

When I asked one of our team to completely change the key assignment
algorithm for V4 (which controls what things get packed near what in the
tree), he complained that it would take 6 weeks to do it.  Under V3 it
would have taken 3 months.  It took him 3 days, and now to change it
again would take him 3 hours I think.  Oh, by the way, the change
boosted performance dramatically.

If you take the time to become familiar with coding within our plugin
layer, I think you will find yourself wanting the same to exist for
other filesystems.  Of course, no other filesystem needs to be impacted
by our plugin layer, and there is no way reiser4 could easily be
rewritten to exist without it (it would be like requiring that the
kernel not use function calls and only use gotos). 

Reiser4's plugin layer has as its explicit objective making it possible
for the weekend hacker to add something useful to reiser4 and send it in
for inclusion.  We want to democratize filesystem innovation so that
random bright guys who usually work on something other than filesystems
can act on their bright ideas without spending 3 years in the filesystem
field to do it.  I believe that most great filesystem innovations are
envisioned by persons not working on filesystems, and go nowhere because
of the especially high cost of entry into our field.

I am not as bright as other filesystem developers, and so we must tinker
with three ideas and keep one of them.  The speed of tinkering is
crucial to us, and the plugin layer increases that speed several fold.

Please reconsider your view. 

Re: -mm -> 2.6.13 merge status

[Christoph Hellwig]

Hans, we had this discussion before.  And the consensus was pretty simple:
the disk structure plugins are your business and fine to keep.  The
higher-level pluging that just add another useless abstraction below
file_operation/inode_operation/etc.  are not.  keep the former and kill
the latter and you're one step further.

Re: -mm -> 2.6.13 merge status

[Andrew Morton]

Jeff Garzik <jgarzik@pobox.com> wrote:
>
> > sparsemem
> > 
> >     OK by me for a merge.  Need to poke arch maintainers first, check that
> >     they've looked at it sufficiently closely.
> 
> seems sane, though there are some whitespace niggles that should be 
> cleaned up
> 

There are?  I thought I fixed most of them.

*general sigh*.  I wish people would absorb CodingStyle.  It's not hard,
and fixing the style post-facto creates a real mess.  I now have a great
string of kexec patches followed by a "kexec-code-cleanup.patch" which
totally buggers up the patch sequencing and really needs to be split into
18 parts and sprinkled back over the entire series.

> > rapidio-*
> > 
> >     Will merge.
> 
> send through netdev, as is proper
> 

OK.  But then the master version vanishes into the jgarzik git forest and I
won't know how to get it ;)

> > connector.patch
> > 
> >     Nice idea IMO, but there are still questions around the
> >     implementation.  More dialogue needed ;)
> > 
> > connector-add-a-fork-connector.patch
> > 
> >     OK, but needs connector.
> 
> I don't like connector
> 

How come?

> 
> > pcmcia-*.patch
> > 
> >     Makes the pcmcia layer generate hotplug events and deprecates cardmgr.
> >     Will merge.
> 
> Testing?  The goal behind the patch is certainly good, but I worry about 
> exposure.
> 

Yes, there will be a few problems I guess.  But people are testing it - we
know, because we've had lots of bug reports which were actually due to
greg-pci breakage...

> 
> > cachefs
> > 
> >     This is a ton of code which knows rather a lot about pagecache
> >     internals.  It allows the AFS client to cache file contents on a local
> >     blockdev.
> > 
> >     I don't think it's a justified addition for only AFS and I'd prefer to
> >     see it proven for NFS as well.
> > 
> >     Issues around add-page-becoming-writable-notification.patch need to
> >     be resolved.
> > 
> > cachefs-for-nfs
> > 
> >     A recent addition.  Needs review from NFS developers and considerably
> >     more testing.
> > 
> >     These things aren't looking likely for 2.6.13.
> 
> If I could vote more than once, I would!  I really like cachefs, and 
> have been pushing for its inclusion for a while.
> 

You've been using it?

> > kexec and kdump
> > 
> >     I guess we should merge these.
> > 
> >     I'm still concerned that the various device shutdown problems will
> >     mean that the success rate for crashing kernels is not high enough for
> >     kdump to be considered a success.  In which case in six months time we'll
> >     hear rumours about vendors shipping wholly different crashdump
> >     implementations, which would be quite bad.
> > 
> >     But I think this has gone as far as it can go in -mm, so it's a bit of
> >     a punt.
> 
> I'm not particularly pleased with these,

How come?

> and indeed vendors ARE shipping 
> other crashdump methods.

Which ones?

> 
> > reiser4
> > 
> >     Merge it, I guess.
> > 
> >     The patches still contain all the reiser4-specific namespace
> >     enhancements, only it is disabled, so it is effectively dead code.  Maybe
> >     we should ask that it actually be removed?
> 
> The plugin stuff is crap.  This is not a filesystem but a filesystem + 
> new layer.  IMO considered in that light, it duplicates functionality 
> elsewhere.
> 

hm.

Re: -mm -> 2.6.13 merge status

[Gerrit Huizenga]

On Tue, 21 Jun 2005 13:22:04 PDT, Andrew Morton wrote:
> Jeff Garzik <jgarzik@pobox.com> wrote:
> > > kexec and kdump
> > > 
> > >     I guess we should merge these.
> > > 
> > >     I'm still concerned that the various device shutdown problems will
> > >     mean that the success rate for crashing kernels is not high enough for
> > >     kdump to be considered a success.  In which case in six months time we'll
> > >     hear rumours about vendors shipping wholly different crashdump
> > >     implementations, which would be quite bad.
> > > 
> > >     But I think this has gone as far as it can go in -mm, so it's a bit of
> > >     a punt.
> > 
> > I'm not particularly pleased with these,
> 
> How come?
> 
> > and indeed vendors ARE shipping 
> > other crashdump methods.
> 
> Which ones?

And which ones that __WORK__?  We have a few customers out there from
both distros and all the crash dump methods that I've seen fail either
ALWAYS or ALMOST ALWAYS on customer sites.  And yes, we hear about them
and I believe that our partners understand the pain that this causes
us and our customers.

Kexec/kdump has a chance of working reliably.  The others are complete
crap.

gerrit

Re: -mm -> 2.6.13 merge status

[Andrew Morton]

Gerrit Huizenga <gh@us.ibm.com> wrote:
>
> Kexec/kdump has a chance of working reliably.

IOW: Kexec/kdump has a chance of not working reliably.

Worried.

Re: -mm -> 2.6.13 merge status

[Gerrit Huizenga]

On Tue, 21 Jun 2005 14:04:41 PDT, Andrew Morton wrote:
> Gerrit Huizenga <gh@us.ibm.com> wrote:
> >
> > Kexec/kdump has a chance of working reliably.
> 
> IOW: Kexec/kdump has a chance of not working reliably.
> 
> Worried.

No worries.  Machine locks up hard, hardware failures, etc., there
is a possibility that nothing but a hard reset can unlock a machine.
But that is rare and outside the scope of the simple locking problems
that today prevent crash dumps.  There are still some rough edges in
PCI shutdown code, reinitialization, etc. that will need to be shaken
out over time with more experience, but those at least can be addressed
in the fundamental architecture of kexec/kdump.

About the only possible solution that *might* be fail proof (and even
that case I doubt) would be an external dump program under control
of the firmware (assuming the firmware can still run) which does a
copy of memory off to some device without any assistance from the
kernel.

Kexec/kdump needs much wider exposure at this point and there will
a few bumps along the way.  They should be isolated to cases where
the machine is already crashing and the only thing that doesn't work
is the crash dump/restart.  And those we will fix like any other bugs.

gerrit

Re: -mm -> 2.6.13 merge status

[Arjan van de Ven]

[-- Attachment #1: Type: text/plain, Size: 852 bytes --]

On Tue, 2005-06-21 at 14:04 -0700, Andrew Morton wrote:
> Gerrit Huizenga <gh@us.ibm.com> wrote:
> >
> > Kexec/kdump has a chance of working reliably.
> 
> IOW: Kexec/kdump has a chance of not working reliably.
> 
> Worried.

it's about rates... you can hose your system so bad that nothing can fix
it.

the "distro" stuff probably succeeds 10% of the time in non-artificial
cases, the kexec one has the potential at least to go 90% (so yes I
admit that I pull these numbers out of my backside but I'm with Gerrit
on this one). The stuff the distros ship is also not so nice in that you
can't dump to LVM or SAN or ... etc; kexec gets all that right. It's
also far more an all-or-nothing thing; if you make the transition you
know you're going to get a good dump; most of the other dumps die
halfway in the process at some point.


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: -mm -> 2.6.13 merge status

[Martin J. Bligh]

--Andrew Morton <akpm@osdl.org> wrote (on Tuesday, June 21, 2005 14:04:41 -0700):

> Gerrit Huizenga <gh@us.ibm.com> wrote:
>> 
>> Kexec/kdump has a chance of working reliably.
> 
> IOW: Kexec/kdump has a chance of not working reliably.
> 
> Worried.

Personally I'm more concerned about the design issues - I can't see how
any of the other options are sustainable / workable. Things that require
maintaining their own driver base are just insane. Things that dump from
the panicing kernel are just broken. People want to be able to dump to 
disk / network / flash-ram card / god-knows-what, so we need something
that's flexible.

I don't think kdump is perfect and bug-free yet, but at least it has a 
design that looks like it'll be workable and sustainable through the future. 
Plus it's a small patch on top of kexec, which is useful in it's own right
(for fast reboot, etc) so we get to reuse a lot of code.

We could go into how crashdump itself is important (eg. first time failure 
capture is critical for customers, less downtime, I can ship you better 
data on bugs I find in test, etc, etc) but I kind of assumed most people
were convinced of that by now. Even Linus seemed to think kdump was the
sensible way forward (at KS last year), and he seems to be one of the 
most ardent sceptics of crashdump I've ever met ;-)

M.

Re: -mm -> 2.6.13 merge status

[Carsten Otte]

On 6/21/05, Andrew Morton <akpm@osdl.org> wrote:
> > and indeed vendors ARE shipping
> > other crashdump methods.
> 
> Which ones?
For 390, we ship standalone bootable crashdump tools with both sles
and rhel. As for kexec, I'd like to see a kexec based 390 bootloader
in the future which would be more flexible then our current one. So
I'd like to vote for merging kexec/kdump.

Re: -mm -> 2.6.13 merge status

[Chris Wright]

* Carsten Otte (cotte.de@gmail.com) wrote:
> On 6/21/05, Andrew Morton <akpm@osdl.org> wrote:
> > > and indeed vendors ARE shipping
> > > other crashdump methods.
> > 
> > Which ones?
> For 390, we ship standalone bootable crashdump tools with both sles
> and rhel. As for kexec, I'd like to see a kexec based 390 bootloader
> in the future which would be more flexible then our current one. So
> I'd like to vote for merging kexec/kdump.

Xen is making similar noises w.r.t. using kexec for flexible bootloader.

thanks,
-chris

Re: -mm -> 2.6.13 merge status

[Carsten Otte]

On 6/23/05, Chris Wright <chrisw@osdl.org> wrote:
> * Carsten Otte (cotte.de@gmail.com) wrote:
> > For 390, we ship standalone bootable crashdump tools with both sles
> > and rhel. As for kexec, I'd like to see a kexec based 390 bootloader
> > in the future which would be more flexible then our current one. So
> > I'd like to vote for merging kexec/kdump.
> 
> Xen is making similar noises w.r.t. using kexec for flexible bootloader.

Oh cool, then we should look at what they're doing instead of reinventing
the wheel. Any pointer we can follow, or person we would contact?

Re: -mm -> 2.6.13 merge status

[Eric W. Biederman]

Andrew Morton <akpm@osdl.org> writes:

> Jeff Garzik <jgarzik@pobox.com> wrote:
> >
> > > sparsemem
> > > 
> > >     OK by me for a merge.  Need to poke arch maintainers first, check that
> > >     they've looked at it sufficiently closely.
> > 
> > seems sane, though there are some whitespace niggles that should be 
> > cleaned up
> > 
> 
> There are?  I thought I fixed most of them.
> 
> *general sigh*.  I wish people would absorb CodingStyle.  It's not hard,
> and fixing the style post-facto creates a real mess.  I now have a great
> string of kexec patches followed by a "kexec-code-cleanup.patch" which
> totally buggers up the patch sequencing and really needs to be split into
> 18 parts and sprinkled back over the entire series.

It looks like I have another hole in my schedule where I can put some
work into kexec so I will see what I can do.

If you want people to absorb CodingStyle it needs to be more explicit.
Of the things that patch fixes you almost have to read between
the lines of CodingStyle to see.  If there is anything backing
it up at all.  Until the problems were pointed out to me I simply
could not see them, and reading CodingStyle was not enlightening.
I point this out not to complain but more so people know which
part of the process needs fixing.

> > > kexec and kdump
> > > 
> > >     I guess we should merge these.
> > > 
> > >     I'm still concerned that the various device shutdown problems will
> > >     mean that the success rate for crashing kernels is not high enough for
> > > kdump to be considered a success.  In which case in six months time we'll
> 
> > >     hear rumours about vendors shipping wholly different crashdump
> > >     implementations, which would be quite bad.
> > > 
> > >     But I think this has gone as far as it can go in -mm, so it's a bit of
> > >     a punt.
> > 
> > I'm not particularly pleased with these,
> 
> How come?

Please tell.

With respect to users of crashdumps there are at least two groups
converging on kexec based crashdumps.  Is there active development
on any of the rest of the tools.

On to the practical response.  The kexec has effectively zero
impact on the kernel, except when it is invoked, and the
code is small.  Kexec is also useful for a lot more than 
just crash dumps.  It happens that crashdumps seem to be the only
case where the other alternatives are noticeably less sane.

There is also another important piece about kexec based crashdumps
that is not usually envisioned.  The kexec based solution is much more
flexible.  For example on a cluster the worst case scenario for
a network based crashdump is all 1000+ nodes will output a crashdump
simultaneously.  Poor crashdump server.  Where with the kexec based
approach it is possible to have the machines send an SNMP alert
and then you can come in and have the machine dump only when you are
ready.  Or you might even start a gdb-stubs server and interact
with a live core dump. :)  And all of this happens to fall out
naturally from using a kernel after the crash.

There are a few associated bug fixes that are problematic but I think
they are fixable.   For the crashdump case the work really is going
into getting hardening linux so it can run on imperfectly behaving hardware.
I.e.  things that are bugs in any event but that using the kernel to
take a crashdump exacerbates.

Andrew the good news is unless something comes up I will have time
starting about Monday to help with the 2.6.13 merge.  It looks like
the first thing I should do is split up the indent patch so it pairs
well with the rest.  And then I have a few pending patches for the user
space and I think I can fix a number of the device_shutdown problems,
for at least the normal kexec path.

Eric

Re: -mm -> 2.6.13 merge status

[Andrew Morton]

ebiederm@xmission.com (Eric W. Biederman) wrote:
>
> Andrew the good news is unless something comes up I will have time
> starting about Monday to help with the 2.6.13 merge.  It looks like
> the first thing I should do is split up the indent patch so it pairs
> well with the rest.  And then I have a few pending patches for the user
> space and I think I can fix a number of the device_shutdown problems,
> for at least the normal kexec path.

I'd be inclined to leave it as-is, really.  I'm not sure that it's worth
the effort+risk of significantly refactoring the patches.

I've cut it down from 58 patches to 45 and will take another pass at it in
the next day or two, but it's looking like 40-odd patches is the right
number.

Re: -mm -> 2.6.13 merge status

[Eric W. Biederman]

Andrew Morton <akpm@osdl.org> writes:

> ebiederm@xmission.com (Eric W. Biederman) wrote:
> >
> > Andrew the good news is unless something comes up I will have time
> > starting about Monday to help with the 2.6.13 merge.  It looks like
> > the first thing I should do is split up the indent patch so it pairs
> > well with the rest.  And then I have a few pending patches for the user
> > space and I think I can fix a number of the device_shutdown problems,
> > for at least the normal kexec path.
> 
> I'd be inclined to leave it as-is, really.  I'm not sure that it's worth
> the effort+risk of significantly refactoring the patches.
> 
> I've cut it down from 58 patches to 45 and will take another pass at it in
> the next day or two, but it's looking like 40-odd patches is the right
> number.

Ok.  Then I won't put a priority on that piece.

I will still look at cleaning up the sematics of the reboot
path so fewer things break.

Eric

Re: -mm -> 2.6.13 merge status

[Paul Jackson]

> I wish people would absorb CodingStyle.

Some people just can't see it, Andrew.  Just like some people
are tone deaf, some people don't notice minor variations in
code spacing and layout, unless pointed out in tedious detail.

Not that I disagree with you ... ;).

-- 
                  I won't rest till it's the best ...
                  Programmer, Linux Scalability
                  Paul Jackson <pj@sgi.com> 1.925.600.0401

Re: -mm -> 2.6.13 merge status

[randy_dunlap]

On Thu, 23 Jun 2005 21:06:38 -0700 Paul Jackson wrote:

| > I wish people would absorb CodingStyle.
| 
| Some people just can't see it, Andrew.  Just like some people
| are tone deaf, some people don't notice minor variations in
| code spacing and layout, unless pointed out in tedious detail.
| 
| Not that I disagree with you ... ;).

I also agree.

The problem (for me at least) is that bad coding style needs
to be fixed before I can do a functional code review, so it
slows down the review cycle quite a bit.  Further, it's mostly
well-known what the requirements are, so there aren't very
many good excuses not to follow CodingStyle...

---
~Randy

Re: -mm -> 2.6.13 merge status

[Lee Revell]

On Mon, 2005-06-20 at 23:54 -0700, Andrew Morton wrote:
> CONFIG_HZ for x86 and ia64: changes default HZ to 250, make HZ
> Kconfigurable.
> 
>     Will merge (will switch default to 1000 Hz later if that seems
> necessary) 

Are you serious?  You're changing the *default* HZ in a stable kernel
series?!?

This is a big regression, it degrades the resolution of system calls.

Lee

Re: -mm -> 2.6.13 merge status

[Nish Aravamudan]

On 6/21/05, Lee Revell <rlrevell@joe-job.com> wrote:
> On Mon, 2005-06-20 at 23:54 -0700, Andrew Morton wrote:
> > CONFIG_HZ for x86 and ia64: changes default HZ to 250, make HZ
> > Kconfigurable.
> >
> >     Will merge (will switch default to 1000 Hz later if that seems
> > necessary)
> 
> Are you serious?  You're changing the *default* HZ in a stable kernel
> series?!?
> 
> This is a big regression, it degrades the resolution of system calls.

Not that my opinion should sway anybody else, but I really would
prefer more of the in-kernel sleep callers were converted to use
human-time units (and thus were verified to be correct) so that
switching HZ will result in the *same* latencies. How much of moving
to lower HZ values is due to the fact that everything is request 10ms
for 1 jiffy of sleep instead of 1 ms? It's hard to tell if the gain is
there or from the lower frequency of interrupts.

I've sent out a lot of patches in this direction (using msleep() and
msleep_interruptible() and my soft-timer rework on top of John
Stultz's timeofday rework converts the entire soft-timer subsystem to
use human-time instead of jiffies as it's unit of expiration), but
there is still *a lot* of work left to do :( I will keep sending
patches, but am being pulled in other directions currently.

Just my $.02.

Thanks,
Nish

Re: -mm -> 2.6.13 merge status

[Nish Aravamudan]

On 6/21/05, Nish Aravamudan <nish.aravamudan@gmail.com> wrote:
> On 6/21/05, Lee Revell <rlrevell@joe-job.com> wrote:
> > On Mon, 2005-06-20 at 23:54 -0700, Andrew Morton wrote:
> > > CONFIG_HZ for x86 and ia64: changes default HZ to 250, make HZ
> > > Kconfigurable.
> > >
> > >     Will merge (will switch default to 1000 Hz later if that seems
> > > necessary)
> >
> > Are you serious?  You're changing the *default* HZ in a stable kernel
> > series?!?
> >
> > This is a big regression, it degrades the resolution of system calls.
> 
> Not that my opinion should sway anybody else, but I really would
> prefer more of the in-kernel sleep callers were converted to use
> human-time units (and thus were verified to be correct) so that
> switching HZ will result in the *same* latencies. How much of moving
> to lower HZ values is due to the fact that everything is request 10ms
> for 1 jiffy of sleep instead of 1 ms? It's hard to tell if the gain is
> there or from the lower frequency of interrupts.

After some further consideration, I don't think that my patches would
be at all changed by adjusting HZ's default value. I just want to make
sure maintainers are still responsive to appropriate patches to split
time-based delays from tick-based delays. So, CONFIG_HZ is ok by me,
but I consider it a band-aid.

Thanks,
Nish

Re: -mm -> 2.6.13 merge status

[Andrew Morton]

Lee Revell <rlrevell@joe-job.com> wrote:
>
> On Mon, 2005-06-20 at 23:54 -0700, Andrew Morton wrote:
> > CONFIG_HZ for x86 and ia64: changes default HZ to 250, make HZ
> > Kconfigurable.
> > 
> >     Will merge (will switch default to 1000 Hz later if that seems
> > necessary) 
> 
> Are you serious?  You're changing the *default* HZ in a stable kernel
> series?!?
> 
> This is a big regression, it degrades the resolution of system calls.
> 

Well we'll see what happens.  As I said, if it's determined to be a real
problem we'll put the default back to 1000Hz prior to 2.6.13 release.

Re: -mm -> 2.6.13 merge status

[Lee Revell]

On Tue, 2005-06-21 at 13:32 -0700, Andrew Morton wrote:
> Lee Revell <rlrevell@joe-job.com> wrote:
> >
> > On Mon, 2005-06-20 at 23:54 -0700, Andrew Morton wrote:
> > > CONFIG_HZ for x86 and ia64: changes default HZ to 250, make HZ
> > > Kconfigurable.
> > > 
> > >     Will merge (will switch default to 1000 Hz later if that seems
> > > necessary) 
> > 
> > Are you serious?  You're changing the *default* HZ in a stable kernel
> > series?!?
> > 
> > This is a big regression, it degrades the resolution of system calls.
> > 
> 
> Well we'll see what happens.  As I said, if it's determined to be a real
> problem we'll put the default back to 1000Hz prior to 2.6.13 release.
> 

I just think it's silly to merge CONFIG_HZ this late in the game, when
dynamic tick and high res timers are right around the corner.  Seems
like more trouble than it's worth.

Lee

Re: -mm -> 2.6.13 merge status

[Christoph Hellwig]

> git-ocfs
> 
>     The OCFS2 filesystem.  OK by me, although I'm not sure it's had enough
>     review.

I'll try to take a look next week.  A major blocker is that it's not
endian-clean yet.  Even if other review items where perfect that's something
preventing it from going to mainline completely.

Re: -mm -> 2.6.13 merge status

[Christoph Hellwig]

On Mon, Jun 20, 2005 at 11:54:58PM -0700, Andrew Morton wrote:
> reiser4

So looking over the code a little for the plugin debate I found that a
reason patch introduces a ->put_inode method in reiser4.  We plan to
kill ->put_inode because it's the wrong abstraction and almost impossible
to use non-racy (reiser4 usage is racy).

I had a discussion with someone from namesys how to solve this correctly,
but I don't remember the details of either the solution or problem anymore.
Unless someone else does let's describe the problem again so we can find
a proper fix.

Re: -mm -> 2.6.13 merge status

[Vladimir Saveliev]

Hello

On Mon, 2005-06-27 at 13:06, Christoph Hellwig wrote:
> On Mon, Jun 20, 2005 at 11:54:58PM -0700, Andrew Morton wrote:
> > reiser4
> 
> So looking over the code a little for the plugin debate I found that a
> reason patch introduces a ->put_inode method in reiser4.  We plan to
> kill ->put_inode because it's the wrong abstraction and almost impossible
> to use non-racy (reiser4 usage is racy).

Sorry, would you please explain what is wrong in having the below

if (inode->i_nlink != 0 || atomic_read(&inode->i_count) > 1)

in reiser4_put_inode.


> I had a discussion with someone from namesys how to solve this correctly,

We were trying to avoid need to have reiser4_drop_inode because you said
drop_inode is a hack for hugetlbfs.

The problem is that on file removal reiser4 wants to do
truncate_inode_pages in reiser4_delete_inode. We used reiser4_drop_inode
for that. As long as drop_inode was about to die, we decided to do file
predeletion in reiser4_put_inode when inode is about to get into
iput_final with inode->i_nlink == 0.

It is a pity that put_inode is also wrong abstraction.

> but I don't remember the details of either the solution or problem anymore.

You said:
--------
So what you want is actually to move the  truncate_inode_pages out of
generic_delete_inode and into ->delete_inode?


Looking at the code another strategt makes more sense:

 - move the truncate_inode_pages at the beginning of clear_inode.
   All callers but one already do it just before that call, but
   the one that doesn't will require a full audit of all ->delete_inode
   instances
 - make the first half of clear_inode into a helper (__clear_inode or
   whatever), and make ->clear_inode responsible for calling it as first
   thing for a normal fs or call it in clear_inode if ->clear_inode
doesn't
   exist.  that way we can also move the invalidate_inode_buffers out
there
   easily later for filesystems that don't use buffer_heads at all.

p.s. please try to keep -fsdevel Cc'ed on the mail related to core
changes
-------

I hoped that we can solve the problem internally in reiser4. If
put_inode is about to be removed we will have to do ssomething like
that.


> Unless someone else does let's describe the problem again so we can find
> a proper fix.
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

Re: -mm -> 2.6.13 merge status

[Christoph Hellwig]

On Mon, Jun 27, 2005 at 06:25:43PM +0400, Vladimir Saveliev wrote:
> Sorry, would you please explain what is wrong in having the below
> 
> if (inode->i_nlink != 0 || atomic_read(&inode->i_count) > 1)
> 
> in reiser4_put_inode.

Between that atomic_read(&inode->i_count) and the
atomic_dec_and_lock(&inode->i_count, &inode_lock)) in iput someone could
have grabbed a reference.

> The problem is that on file removal reiser4 wants to do
> truncate_inode_pages in reiser4_delete_inode. We used reiser4_drop_inode
> for that. As long as drop_inode was about to die, we decided to do file

drop_inode is not going to die, we need it to support filesystems that
want to call generic_delete_inode even for a non-null i_nlink.  What's
hopefully going to die is the last instance of it that isn't either
generic_drop_inode or generic_delete_inode.

> You said:
> --------
> So what you want is actually to move the  truncate_inode_pages out of
> generic_delete_inode and into ->delete_inode?
> 
> 
> Looking at the code another strategt makes more sense:
> 
>  - move the truncate_inode_pages at the beginning of clear_inode.
>    All callers but one already do it just before that call, but
>    the one that doesn't will require a full audit of all ->delete_inode
>    instances
>  - make the first half of clear_inode into a helper (__clear_inode or
>    whatever), and make ->clear_inode responsible for calling it as first
>    thing for a normal fs or call it in clear_inode if ->clear_inode
> doesn't
>    exist.  that way we can also move the invalidate_inode_buffers out
> there
>    easily later for filesystems that don't use buffer_heads at all.
> 
> p.s. please try to keep -fsdevel Cc'ed on the mail related to core
> changes
> -------
> 
> I hoped that we can solve the problem internally in reiser4. If
> put_inode is about to be removed we will have to do ssomething like
> that.

In fact I know from some cluster filesystem folks that have a similar
problems as yours.  So getting the truncate_inode_pages under control
of the filesystems sounds like a very good choice.

Re: -mm -> 2.6.13 merge status

[Joel Becker]

On Mon, Jun 27, 2005 at 08:26:51PM +0100, Christoph Hellwig wrote:
> drop_inode is not going to die, we need it to support filesystems that
> want to call generic_delete_inode even for a non-null i_nlink.  What's
> hopefully going to die is the last instance of it that isn't either
> generic_drop_inode or generic_delete_inode.

	OCFS2 uses drop_inode as well, as it must handle last-close when
another node did the unlink.  It fixes up i_nlink in that case, then
calls generic_drop_inode().
	If there's a more elegant solution, we're all ears.

Joel

-- 

"When choosing between two evils, I always like to try the one
 I've never tried before."
        - Mae West

Joel Becker
Senior Member of Technical Staff
Oracle
E-mail: joel.becker@oracle.com
Phone: (650) 506-8127

Re: -mm -> 2.6.13 merge status

[Christoph Hellwig]

On Mon, Jun 27, 2005 at 12:44:02PM -0700, Joel Becker wrote:
> On Mon, Jun 27, 2005 at 08:26:51PM +0100, Christoph Hellwig wrote:
> > drop_inode is not going to die, we need it to support filesystems that
> > want to call generic_delete_inode even for a non-null i_nlink.  What's
> > hopefully going to die is the last instance of it that isn't either
> > generic_drop_inode or generic_delete_inode.
> 
> 	OCFS2 uses drop_inode as well, as it must handle last-close when
> another node did the unlink.  It fixes up i_nlink in that case, then
> calls generic_drop_inode().
> 	If there's a more elegant solution, we're all ears.

I think this still qualifies as calling generic_delete_inode because it's
a trivial wrapper.  Manipulating i_nlink seems rather odd to me, I'd
say you should rather call into generic_delete_inode directly if
OCFS2_INODE_MAYBE_ORPHANED is set (that's what generic_drop_inode will
do for i_nlink == 0 anyway).

In fact given every cluster and possibly many network filesystems will
need this it might make sense to take the OCFS2_INODE_MAYBE_ORPHANED into
the VFS, i.e. make it an i_state flag (after fixing can_unuse to not do
something totally stupid with i_state)

Re: -mm -> 2.6.13 merge status

[Christoph Hellwig]

> reiser4

sparse isn't to happy about this:

hch@macfly:/work/linux-2.6.12$ make C=1 SUBDIRS=fs/reiser4/ >/dev/null 2>err.log && wc -l err.log
2286 err.log

The log is at http://verein.lst.de/~hch/linux-2.6.12-mm2-fs-reiser4-errors.log

Re: -mm -> 2.6.13 merge status

[Hans Reiser]

Christoph Hellwig wrote:

>>reiser4
>>    
>>
>
>sparse isn't to happy about this:
>
>hch@macfly:/work/linux-2.6.12$ make C=1 SUBDIRS=fs/reiser4/ >/dev/null 2>err.log && wc -l err.log
>2286 err.log
>
>The log is at http://verein.lst.de/~hch/linux-2.6.12-mm2-fs-reiser4-errors.log
>
>
>  
>
Thanks for telling us about sparse, we will work on fixing these. 
Vitaly, can you do this?

Hans

Re: -mm -> 2.6.13 merge status

[Vitaly Fertman]

On Tuesday 28 June 2005 00:37, Hans Reiser wrote:
> Christoph Hellwig wrote:
> 
> >>reiser4
> >>    
> >>
> >
> >sparse isn't to happy about this:
> >
> >hch@macfly:/work/linux-2.6.12$ make C=1 SUBDIRS=fs/reiser4/ >/dev/null 2>err.log && wc -l err.log
> >2286 err.log
> >
> >The log is at http://verein.lst.de/~hch/linux-2.6.12-mm2-fs-reiser4-errors.log
> >
> >
> >  
> >
> Thanks for telling us about sparse, we will work on fixing these. 
> Vitaly, can you do this?

fixed, will be included into the next mm kernel.

-- 
Thanks,
Vitaly Fertman

Re: -mm -> 2.6.13 merge status

[Christoph Hellwig]

> v9fs
> 
>     I'm not sure that this has a sufficiently high
>     usefulness-to-maintenance-cost ratio.

Personally I think this is very useful to have.  It provides a portable
way to have simple userland or remote filesystems, and it's been around
for a long time in others OSes.

That beeing said there's a few issues with the code still I'd like to
see fixed:

  - there's three sparse warnings still.  Two of them are easily fixed
    by moving externs to headers, one doesn't look fixable until we get
    a sane in-kernel api for socket operations
  - some dentry handling looks rather odd.  Why are you for example
    calling d_drop in v9fs_vfs_symlink, v9fs_vfs_mknod and v9fs_vfs_link?
    Shouldn't all these call d_instantatiate to actually reuse the
    dentry as in v9fs_vfs_create?  Also what's the issue with
    v9fs_fid_insert?  It would seem better and more logical to me to
    always set d_fsdata in create/mknod/symlink/open before hashing it
    and then beeing able to rely on it beeing non-NULL.
  - buf_check_sizep, buf_check_size and buf_check_sizev should be made
    inlines, and lose the implict return.  Please don't hide such
    things in macros
  - please avoid using hlist_for_each, usually hlist_for_each_entry is
    a much better choice
  - dito for list_for_each_safe vs list_for_each_entry_safe
  - can you please check whether lib/idr.c fullfills your needs so we
    can get rid of idpool.c?
  - v9fs_inode2v9ses has lots of useless checks, inode->i_sb can never
    be NULL, and inode->i_sb->s_fs_info can't be either once set in
    fill_inode, which is before the first inode on the filesystem is
    created.  Also the argument is never NULL.  Because of that you
    can also kill all the return value checks in the callers.
  - do you really need to keep v9fs_dentry_delete just for the dprintk?
  - no need to check for a NULL file in v9fs_dir_readdir, the VFS gurantees
    it's not.  And if it was you'd better be off panic because something
    is enormously fscked.
  - Dito for v9fs_file_open
  - And the inode in v9fs_file_lock
  - And dir, file, file->d_inode, sb, v9ses in v9fs_remove.
  - And dir, sb and v9ses in v9fs_vfs_lookup
  - And dir, sb and v9ses in v9fs_vfs_symlink
  - And dir, sb and v9ses in v9fs_vfs_link
  - And dir, sb and v9ses in v9fs_vfs_mknod
  - copy_from_user returns the bytes actually copied in the failure case,
    but you should return -EFAULT instead of that number in v9fs_file_write
  - No need to implement v9fs_file_mmap, do_mmap_pgoff makes sure to error
    out if it's not present (and actually returns the correct errno)
  - I think it's pretty similar for all these checks for fid (=private_data)
    checks.  You always set them in open, so they can't be NULL
  - kfree can be called with a NULL argument just fine, you can remove
    lots of ifs for that. You also often set pointers to NULL just before
    freeing a structure - that's pretty useless as slab debugging will
    catch bugs with stary references very well, and overwrites these NULLs
    ASAP.
  - The call to ->put_inode in the error case of v9fs_get_inode is very
    wrong.  You'd actually panic if you ever hit this as v9fs doesn't
    implement a ->put_inode :-)
  - All the ISDIR checks in v9fs_remove can go, VFS makes sure to only
    call ->remove and ->rmdir on directories, and only the right one
    for each kind of child.
  - Please try to use generic_readlink instead of your own
    v9fs_vfs_readlink, as you're implemting ->follow_link and ->put_link
    that should just work
  - the last error case in v9fs_get_sb needs a dput on ->s_root

Also did you look into the VFS/NFS lookup intent bits to solve your
atomic create and open issue?