* [PATCH] drbd: Remove fix me statements @ 2014-07-22 18:19 Nicholas Krause 2014-07-23 12:27 ` Bjørn Mork 0 siblings, 1 reply; 5+ messages in thread From: Nicholas Krause @ 2014-07-22 18:19 UTC (permalink / raw) To: drbd-dev; +Cc: drbd-user, linux-kernel This removes no longer needed fix me statements as the spinlocks are needed to protect against other users of the list accessing it when items on it are being moved off it. Signed-off-by: Nicholas Krause <xerofoify@gmail.com> --- drivers/block/drbd/drbd_worker.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/block/drbd/drbd_worker.c b/drivers/block/drbd/drbd_worker.c index d8f57b6..8793a32 100644 --- a/drivers/block/drbd/drbd_worker.c +++ b/drivers/block/drbd/drbd_worker.c @@ -1847,12 +1847,12 @@ static void wait_for_work(struct drbd_connection *connection, struct list_head * int send_barrier; prepare_to_wait(&connection->sender_work.q_wait, &wait, TASK_INTERRUPTIBLE); spin_lock_irq(&connection->resource->req_lock); - spin_lock(&connection->sender_work.q_lock); /* FIXME get rid of this one? */ + spin_lock(&connection->sender_work.q_lock); /* dequeue single item only, * we still use drbd_queue_work_front() in some places */ if (!list_empty(&connection->sender_work.q)) list_move(connection->sender_work.q.next, work_list); - spin_unlock(&connection->sender_work.q_lock); /* FIXME get rid of this one? */ + spin_unlock(&connection->sender_work.q_lock); if (!list_empty(work_list) || signal_pending(current)) { spin_unlock_irq(&connection->resource->req_lock); break; -- 1.9.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] drbd: Remove fix me statements 2014-07-22 18:19 [PATCH] drbd: Remove fix me statements Nicholas Krause @ 2014-07-23 12:27 ` Bjørn Mork 2014-07-23 15:45 ` Nick Krause 0 siblings, 1 reply; 5+ messages in thread From: Bjørn Mork @ 2014-07-23 12:27 UTC (permalink / raw) To: Nicholas Krause; +Cc: drbd-dev, drbd-user, linux-kernel Nicholas Krause <xerofoify@gmail.com> writes: > This removes no longer needed fix me statements If you claim that they no longer are needed then I expect you to explain what's changed since they were added. > as the spinlocks > are needed to protect against other users of the list accessing > it when items on it are being moved off it. Yes, that's most likely why the locking is there. The FIXMEs on the other hand, are probably there to tell you that it would be nice to get rid of the double locking. I don't see that changing. Bjørn ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drbd: Remove fix me statements 2014-07-23 12:27 ` Bjørn Mork @ 2014-07-23 15:45 ` Nick Krause 2014-07-23 17:33 ` Bjørn Mork 0 siblings, 1 reply; 5+ messages in thread From: Nick Krause @ 2014-07-23 15:45 UTC (permalink / raw) To: Bjørn Mork; +Cc: drbd-dev, drbd-user, linux-kernel On Wed, Jul 23, 2014 at 8:27 AM, Bjørn Mork <bjorn@mork.no> wrote: > Nicholas Krause <xerofoify@gmail.com> writes: > >> This removes no longer needed fix me statements > > If you claim that they no longer are needed then I expect you to explain > what's changed since they were added. > >> as the spinlocks >> are needed to protect against other users of the list accessing >> it when items on it are being moved off it. > > Yes, that's most likely why the locking is there. > > The FIXMEs on the other hand, are probably there to tell you that it > would be nice to get rid of the double locking. I don't see that > changing. > > > Bjørn > 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. Cheers Nick ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drbd: Remove fix me statements 2014-07-23 15:45 ` Nick Krause @ 2014-07-23 17:33 ` Bjørn Mork 2014-07-23 19:13 ` Nick Krause 0 siblings, 1 reply; 5+ messages in thread From: Bjørn Mork @ 2014-07-23 17:33 UTC (permalink / raw) To: Nick Krause; +Cc: drbd-dev, drbd-user, linux-kernel 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) ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drbd: Remove fix me statements 2014-07-23 17:33 ` Bjørn Mork @ 2014-07-23 19:13 ` Nick Krause 0 siblings, 0 replies; 5+ messages in thread From: Nick Krause @ 2014-07-23 19:13 UTC (permalink / raw) To: Bjørn Mork; +Cc: drbd-dev, drbd-user, linux-kernel 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2014-07-23 19:13 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-07-22 18:19 [PATCH] drbd: Remove fix me statements Nicholas Krause 2014-07-23 12:27 ` Bjørn Mork 2014-07-23 15:45 ` Nick Krause 2014-07-23 17:33 ` Bjørn Mork 2014-07-23 19:13 ` Nick Krause
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.