MIME-Version: 1.0
In-Reply-To: <87a980getx.fsf@nemi.mork.no>
References: <1406053161-5048-1-git-send-email-xerofoify@gmail.com>
	<8738dsi7l7.fsf@nemi.mork.no>
	<CAPDOMVjwzfmHAKGBENfpxN2cswckq2T=QbQa56k3w0X+cDuJKg@mail.gmail.com>
	<87a980getx.fsf@nemi.mork.no>
Date: Wed, 23 Jul 2014 15:13:24 -0400
Message-ID: <CAPDOMVjB3vLwfDAkSGpLMtth-TpQwuPgBRKKc9wmxHqFcH69uQ@mail.gmail.com>
Subject: Re: [PATCH] drbd: Remove fix me statements
From: Nick Krause <xerofoify@gmail.com>
To: =?UTF-8?Q?Bj=C3=B8rn_Mork?= <bjorn@mork.no>
Cc: drbd-dev@lists.linbit.com, drbd-user@lists.linbit.com,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8BIT
Sender: linux-kernel-owner@vger.kernel.org

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/