Re: -mm - 2.6.13 merge status
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
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
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 [EMAIL PROTECTED]
---
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
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
Re: -mm - 2.6.13 merge status
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
Hubert Chan [EMAIL PROTECTED] writes: On Sat, 25 Jun 2005 12:23:41 -0700, Hans Reiser [EMAIL PROTECTED] 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
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
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
On Sat, 25 Jun 2005 12:23:41 -0700, Hans Reiser [EMAIL PROTECTED] 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 [EMAIL PROTECTED] - 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
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) signature.asc Description: OpenPGP digital signature
Re: -mm - 2.6.13 merge status
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
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 : ) signature.asc Description: OpenPGP digital signature
Re: -mm - 2.6.13 merge status
Hi Hans,
On 6/22/05, Hans Reiser [EMAIL PROTECTED] 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.0 +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.0 +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.0 +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.0 +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
Re: -mm - 2.6.13 merge status
Pekka Enberg wrote:
Hi Hans,
On 6/22/05, Hans Reiser [EMAIL PROTECTED] 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.0 +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.0 +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.0 +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.0 +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.
Re: -mm - 2.6.13 merge status
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
Hello
On Thu, 2005-06-23 at 11:42, Hans Reiser wrote:
Pekka Enberg wrote:
--- /dev/null 2003-09-23 21:59:22.0 +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.0 +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.0 +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... */
+
Re: -mm - 2.6.13 merge status
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
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.0 +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.0 +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
Re: -mm - 2.6.13 merge status
Pekka Enberg wrote:
--- /dev/null 2003-09-23 21:59:22.0 +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
Jeff Mahoney [EMAIL PROTECTED] 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 wrote:
Pekka Enberg wrote:
--- /dev/null2003-09-23 21:59:22.0 +0400
+++ linux-2.6.11-vs/fs/reiser4/pool.c2005-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
On Thu, Jun 23 2005, Andrew Morton wrote:
Jeff Mahoney [EMAIL PROTECTED] 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
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
On 6/23/05, Olivier Galibert [EMAIL PROTECTED] 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 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 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
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
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
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
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
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
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
