Re: [PATCH] drbd: Remove fix me statements

[Nick Krause <xerofoify@gmail.com>]

On Wed, Jul 23, 2014 at 1:33 PM, Bjørn Mork <bjorn@mork.no> wrote:
> Nick Krause <xerofoify@gmail.com> writes:
>
>> Bjorn,
>> Can we remove the double locking  as you are stating or do  we still need it
>> to protect against the list being accessed  as the list seems to be moving
>> to a spinlock protected list.
>
> I wouldn't know.
>
> The only thing I know is that the original author of those lines, who we
> must assume has thorough knowlegde of this code, did not know how to fix
> that in a simple and straight forward way.
>
> From this we can deduce that there is more to it than just changing a
> couple of lines. If you don't alrady know this code in and out, then you
> would have to start by analyzing the current locking model and
> understanding that.  Then, assuming the current double locking is in
> fact necessary, you would need to redesign it so that you can make one
> of the locks go away.  Then you need to implement your new design.  Then
> test it _thoroughly_ to eliminate all the small bugs. Everyone adds bugs
> when writing non-trivial code.  (You seem to think that you can delegate
> all the bug squashing to others simply because you don't own the
> hardware.  That is not so.  If you don't have access to hardware for
> testing, then you should not add any bugs.  Yes, this implies that you
> cannot write non-trivial code for hardware you don't have). Then you must
> verify that the result is at least as efficient as the old code was. Or
> there would be no point, would there?
>
> When all this is done, and the testing shows it is a success, *then* you
> can remove the FIXME comment with a nice commit message explaining the
> new locking model and why it now is safe to drop one of the locks.
>
> There is a fat chance that this just isn't worth all the work.  Which is
> most likely why the FIXME was stuck there in the first place.
>
> You should understand that noone will add a FIXME for anything trivial.
> And if an author who knows the code well finds something non-trivial,
> then you should definitely not touch it without investing enough time to
> have a similar understanding of the code.
>
> Note again that I am writing all this as purely generic comments.  I
> don't know anything at all about the code in question, and I wouldn't
> dare touching it without spending a lot of time understanding it first.
>
> As Steven said: find an area to focus on.  Spend some time understanding
> a small part of the kernel instead of jumping all around.
>
> And: Being able to test code yourself is absolutely necessary in the
> beginning.  But you don't necessarily have to run out and buy some odd
> new hardware for that. I'm pretty sure many drivers and other parts of
> the kernel is in use on the hardware you already have at hand :-)
> Choose among those parts for your learning experience.
>
> Your USB hcd patch is a nice example of code that you most likely can
> test yourself.  And the pacth was fine too, except for the lack of a
> proper commit message explaining why it was OK.  But most of us will
> just look at the "Acked-by: Alan Stern" line and figure that the change
> definitely must be fine :-)
>
>
> Bjørn (who also has sumitted his share of buggy patches, creating
> unnecessary work for innoncent maintainers in the past.  Sorry about
> that Greg, Oliver, Alan, David, Mauro and all the others... I'm afraid I
> cannot even guarantee that it won't happen again, but I do try my best)


Bjorn ,
Thanks for the reply and the advice seems this is more work then
I am time for now.
Cheers Nick