Re: reiser4 merging action list

Re: reiser4 merging action list

[Andi Kleen]

Hans Reiser <reiser@namesys.com> writes:

>    * metafiles should be disabled until we can present code that works
> right.  Half the list thinks we cannot solve the cycles problem ever. 
> Disable metafiles and postpone problem until working code, or the
> failure to produce it, makes it possible to do more than rant at each
> other.  This is currently already done in the -mm patches, but is
> mentioned lest someone think it forgotten.
> 
>    * update the locking documentation
> 
> Probably I forget something.

These are all big picture issues, but I think some low level attention to
the individual code is still needed.

Some stuff that stood out from a very quick look:

I would like for the custom spin lock debugging (spin_macros.h) and
profiling code to be removed (prof.[ch], spinprof.[ch]). Such code shouldn't 
be in specific subsystems. 

The division functions in lib.h are useless IMHO, both callers seem
to use divide by a power of two. And gcc supports shift in 64bit
fine in the kernel. Can you remove that please? 

statcnt.h: This is completely useless because you don't align
the individual fields for cache lines - so you will still
have false sharing everywhere. Also using NR_CPUS is nasty
because it can be very big - num_possible_cpus() is better. 
It should use the new dynamic per cpu allocator.

Best you just remove it for now and use atomic_t and readd properly
when you do real SMP tuning with measurements.

debug.[ch]: A lot of these functions like "schedulable" are name space
space polluting. 
reiser4_kmalloc() such wrappers are deprecated. Please remove.
xmemset et.al should be replaced with the normal functions everywhere

Best would be probably to remove most of these files for submission.

What is reiser4_internal? Can't you just use static like
everybody else?

status_flags.c: Please remove that CONFIG_FRAME_POINTER code.
In general i think it would be better if you removed that 
"private mini crashdumping".

Is there any reason you can't just use wait queues like everybody
else instead of these reimplemented condition variables in kcond.[ch]?

In general it would be good if someone experienced not from the reiser team
would read the whole source and looks for obvious problems
(I didn't, just mentioning stuff I from a quick look at some support
files) 

-Andi

Re: reiser4 merging action list

[Vladimir Saveliev]

Hello

On Tue, 2005-06-28 at 06:58, Andi Kleen wrote:
> Hans Reiser <reiser@namesys.com> writes:
> 
> >    * metafiles should be disabled until we can present code that works
> > right.  Half the list thinks we cannot solve the cycles problem ever. 
> > Disable metafiles and postpone problem until working code, or the
> > failure to produce it, makes it possible to do more than rant at each
> > other.  This is currently already done in the -mm patches, but is
> > mentioned lest someone think it forgotten.
> > 
> >    * update the locking documentation
> > 
> > Probably I forget something.
> 
> These are all big picture issues, but I think some low level attention to
> the individual code is still needed.
> 
> Some stuff that stood out from a very quick look:
> 
> I would like for the custom spin lock debugging (spin_macros.h) and
> profiling code to be removed (prof.[ch], spinprof.[ch]). Such code shouldn't 
> be in specific subsystems. 
> 
sorry, Andi, I guess you are looking at something old. Reiser4 does not
have neither prof.[ch], nor spinprof.[ch] and we removed already some
debugging code from spin_macros.h.

> The division functions in lib.h are useless IMHO, both callers seem
> to use divide by a power of two. And gcc supports shift in 64bit
> fine in the kernel. Can you remove that please? 
> 
ok

> statcnt.h: This is completely useless because you don't align
> the individual fields for cache lines - so you will still
> have false sharing everywhere. Also using NR_CPUS is nasty
> because it can be very big - num_possible_cpus() is better. 
> It should use the new dynamic per cpu allocator.
> 
statcnt.h is already removed.

> Best you just remove it for now and use atomic_t and readd properly
> when you do real SMP tuning with measurements.
> 
> debug.[ch]: A lot of these functions like "schedulable" are name space
> space polluting. 
> reiser4_kmalloc() such wrappers are deprecated. Please remove.
ok
> xmemset et.al should be replaced with the normal functions everywhere
> 
done.

> Best would be probably to remove most of these files for submission.
> 
> What is reiser4_internal? Can't you just use static like
> everybody else?

ok

> status_flags.c: Please remove that CONFIG_FRAME_POINTER code.
> In general i think it would be better if you removed that 
> "private mini crashdumping".

ok

> 
> Is there any reason you can't just use wait queues like everybody
> else instead of these reimplemented condition variables in kcond.[ch]?
> 
I will investigate that

> In general it would be good if someone experienced not from the reiser team
> would read the whole source and looks for obvious problems
> (I didn't, just mentioning stuff I from a quick look at some support
> files) 
> 
> -Andi
> 
> 

Re: reiser4 merging action list

[Andi Kleen]

On Tue, Jun 28, 2005 at 12:37:11PM +0400, Vladimir Saveliev wrote:
> have neither prof.[ch], nor spinprof.[ch] and we removed already some
> debugging code from spin_macros.h.

Yes, i was looking at some older tree with reiser4. Sorry, just
ignore what is already done.

But still spin_macros.h should be completely removed imho. Such
custom lock wrappers are strongly discouraged because it 
makes it hard for others to read your code.


> > statcnt.h: This is completely useless because you don't align
> > the individual fields for cache lines - so you will still
> > have false sharing everywhere. Also using NR_CPUS is nasty
> > because it can be very big - num_possible_cpus() is better. 
> > It should use the new dynamic per cpu allocator.
> > 
> statcnt.h is already removed.

Great.

-Andi

Re: reiser4 merging action list

[Artem B. Bityuckiy]

Andi Kleen wrote:
> Yes, i was looking at some older tree with reiser4. Sorry, just
> ignore what is already done.
> 
> But still spin_macros.h should be completely removed imho. Such
> custom lock wrappers are strongly discouraged because it 
> makes it hard for others to read your code.
>
I may comfirm that this makes Reiser4 very difficult to investigate.
Ctags doesn't work with that too..

-- 
Best Regards,
Artem B. Bityuckiy,
St.-Petersburg, Russia.

Re: reiser4 merging action list

[Hans Reiser]

Pekka Enberg wrote:

>Andrew Morton wrote:
>  
>
>>>There's also the custom list, hash and debug code.  We should either
>>>
>>>a) remove them or
>>>
>>>b) generify them and submit as standalone works or
>>>
>>>c) justify them as custom-to-reiser4 and leave them as-is.
>>>      
>>>
>
>On 6/29/05, Hans Reiser <reiser@namesys.com> wrote:
>  
>
>>either b) or c) is ok with me for the list code.  The debug code should
>>be c) I think.
>>
>>Probably vs can offer a more detailed and accurate opinion,
>>    
>>
>
>I completely agree that the current state of the generic hashing
>facilities is somewhat poor but I fail to see why you can't use
><linux/list.h>.
>  
>
I'll let vs and maybe nikita comment.

>As for the debugging code, I would love to see that turned into
>something generic (every subsystem has their own now) but it is
>definitely not something that should stop you from merging.
>
>                                Pekka
>
>
>  
>
If I encourage you to make a patch, is that ok of me? 

Re: reiser4 merging action list

[Pekka Enberg]

Andrew Morton wrote:
> > There's also the custom list, hash and debug code.  We should either
> >
> > a) remove them or
> >
> > b) generify them and submit as standalone works or
> >
> > c) justify them as custom-to-reiser4 and leave them as-is.

On 6/29/05, Hans Reiser <reiser@namesys.com> wrote:
> either b) or c) is ok with me for the list code.  The debug code should
> be c) I think.
> 
> Probably vs can offer a more detailed and accurate opinion,

I completely agree that the current state of the generic hashing
facilities is somewhat poor but I fail to see why you can't use
<linux/list.h>.

As for the debugging code, I would love to see that turned into
something generic (every subsystem has their own now) but it is
definitely not something that should stop you from merging.

                                Pekka

Re: reiser4 merging action list

[Hans Reiser]

Andrew Morton wrote:

>Hans Reiser <reiser@namesys.com> wrote:
>  
>
>>Andrew asked me to put together a list of things that need to be done
>>before merging:
>>    
>>
>
>Thanks.
>
>As I said to Hans, if we can get a list of bullet-point actions nailed down
>and agreed to then we have an uncontroversial path to happiness and a
>merge.  Let's get down and concentrate on technical specifics.
>
>Hans, please maintain this list and republish it as we work through things.
>
>  
>
>>    * VFS will dispatch directly to the method of the plugin for the
>>*_operations methods.  This requires duplicating to all plugin methods
>>the common code currently used by all reiser4 plugins for a given
>>method.  It has the desirable side effect of making the methods more
>>fully self-contained, which is somethng I had wanted two years ago and
>>was a little sad to not get, and the cost of duplicating some code. 
>>Since not all plugin methods are *_operations, it means we have two
>>structures with duplicated data, and duplicate data that must be in sync
>>at all times is classical badness in programming technique (see Codd and
>>normalization).                                 vs owns this task
>>
>>    * review all sparse complaints, and revise as appropriate. 
>>
>>    * panic and code beauty: everyone agrees that having function, file,
>>and line added to reiser4_panic output hurts nothing (I hope).  Everyone
>>agrees that restarting the machine without an error message seems like a
>>useless option to allow.   Much else was argued, not sure if anything
>>was a consensus view.  Various detail improvements were suggested by
>>Pecca, and I agreed with half of them.
>>
>>
>>   * metafiles should be disabled until we can present code that works
>>right.  Half the list thinks we cannot solve the cycles problem ever. 
>>Disable metafiles and postpone problem until working code, or the
>>failure to produce it, makes it possible to do more than rant at each
>>other.  This is currently already done in the -mm patches, but is
>>mentioned lest someone think it forgotten.
>>
>>   * update the locking documentation
>>
>>    
>>
>
>There's also the custom list, hash and debug code.  We should either
>
>a) remove them or
>
>b) generify them and submit as standalone works or
>
>c) justify them as custom-to-reiser4 and leave them as-is.
>
>
>
>
>  
>
either b) or c) is ok with me for the list code.  The debug code should
be c) I think.

Probably vs can offer a more detailed and accurate opinion,

Hans

Re: reiser4 merging action list

[Adrian Bunk]

On Mon, Jun 27, 2005 at 04:00:01PM -0700, Hans Reiser wrote:
> 
> Andrew asked me to put together a list of things that need to be done
> before merging:
>...
> Probably I forget something.


  * remove the dependency on !4KSTACKS


> Best,
> 
> Hans

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: reiser4 merging action list

[Christoph Hellwig]

On Mon, Jun 27, 2005 at 04:00:01PM -0700, Hans Reiser wrote:
> 
> Andrew asked me to put together a list of things that need to be done
> before merging:

...

> Probably I forget something.

I've started to do a very basic look over the tree and there's a few
more things that spring to mind:

 - cpp abuse.  There's quite a lot of really odd macros - the typesafe_list,
   typesafe_hash stuff is mentioned already, but there's really horrible
   stuff like _INIT_ and _DONE_ bits in init_super.c, and the wrappers for
   plugin method invocations.
 - endianess handling.  The d* types are a lovely attempt to make sure
   you're not missing endianess conversions.  We have a more general
   way to ensure that now using sparse, that's the __le* / __be* types.
   Try running sparse -Wbitwise to find places you'll need that annotations
   (after the normal sparse warnings are fixed, else you'll have a hard
   time seeing them I guess) - after that the single element struct thing
   can go away.  Also you're defining a CPU_IN_DISK_ORDER macro that's
   never used..
 - lease use kthread_* instead of kernel_thread() + lots of opencoding

Re: reiser4 merging action list

[Andrew Morton]

Hans Reiser <reiser@namesys.com> wrote:
>
> 
> Andrew asked me to put together a list of things that need to be done
> before merging:

Thanks.

As I said to Hans, if we can get a list of bullet-point actions nailed down
and agreed to then we have an uncontroversial path to happiness and a
merge.  Let's get down and concentrate on technical specifics.

Hans, please maintain this list and republish it as we work through things.

>     * VFS will dispatch directly to the method of the plugin for the
> *_operations methods.  This requires duplicating to all plugin methods
> the common code currently used by all reiser4 plugins for a given
> method.  It has the desirable side effect of making the methods more
> fully self-contained, which is somethng I had wanted two years ago and
> was a little sad to not get, and the cost of duplicating some code. 
> Since not all plugin methods are *_operations, it means we have two
> structures with duplicated data, and duplicate data that must be in sync
> at all times is classical badness in programming technique (see Codd and
> normalization).                                 vs owns this task
> 
>     * review all sparse complaints, and revise as appropriate. 
> 
>     * panic and code beauty: everyone agrees that having function, file,
> and line added to reiser4_panic output hurts nothing (I hope).  Everyone
> agrees that restarting the machine without an error message seems like a
> useless option to allow.   Much else was argued, not sure if anything
> was a consensus view.  Various detail improvements were suggested by
> Pecca, and I agreed with half of them.
> 
> 
>    * metafiles should be disabled until we can present code that works
> right.  Half the list thinks we cannot solve the cycles problem ever. 
> Disable metafiles and postpone problem until working code, or the
> failure to produce it, makes it possible to do more than rant at each
> other.  This is currently already done in the -mm patches, but is
> mentioned lest someone think it forgotten.
> 
>    * update the locking documentation
> 

There's also the custom list, hash and debug code.  We should either

a) remove them or

b) generify them and submit as standalone works or

c) justify them as custom-to-reiser4 and leave them as-is.

Re: reiser4 merging action list

[Hans Reiser]

Andrew asked me to put together a list of things that need to be done
before merging:

    * VFS will dispatch directly to the method of the plugin for the
*_operations methods.  This requires duplicating to all plugin methods
the common code currently used by all reiser4 plugins for a given
method.  It has the desirable side effect of making the methods more
fully self-contained, which is somethng I had wanted two years ago and
was a little sad to not get, and the cost of duplicating some code. 
Since not all plugin methods are *_operations, it means we have two
structures with duplicated data, and duplicate data that must be in sync
at all times is classical badness in programming technique (see Codd and
normalization).                                 vs owns this task

    * review all sparse complaints, and revise as appropriate. 

    * panic and code beauty: everyone agrees that having function, file,
and line added to reiser4_panic output hurts nothing (I hope).  Everyone
agrees that restarting the machine without an error message seems like a
useless option to allow.   Much else was argued, not sure if anything
was a consensus view.  Various detail improvements were suggested by
Pecca, and I agreed with half of them.


   * metafiles should be disabled until we can present code that works
right.  Half the list thinks we cannot solve the cycles problem ever. 
Disable metafiles and postpone problem until working code, or the
failure to produce it, makes it possible to do more than rant at each
other.  This is currently already done in the -mm patches, but is
mentioned lest someone think it forgotten.

   * update the locking documentation

Probably I forget something.

Best,

Hans