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
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, 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 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 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
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(:)"
This already happens: together with -, 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
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
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
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 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
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(:)"
--
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
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
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
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 Thu, 23 Jun 2005 14:37:05 -0400, Jeff Mahoney <[EMAIL PROTECTED]> 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 <[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
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
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
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 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
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__?
__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
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
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
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
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()
>{
>
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
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
> >>+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", n
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
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 and 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.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?
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
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
>>+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 s
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
> +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
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
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: 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
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
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 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 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 > > > >
