In 2.5.8-pre1 include/linux/nbd.h a struct member name was changed
from 'queue_lock' to 'tx_lock', with the comment "nbd compile fix"
from Dave Jones.
In fact, since nbd.c still reference 'queue_lock' I suspect that
the actual modifications to nbd.c were lost somewhere in etherspace
between Dave and Linus.
Either provide the right fix for nbd.c or apply the attached patch,
which reverts the patch to nbd.h.
In other words, the attached patch provides a "nbd compile fix" by
reverting the previous "nbd compile fix" :-)
Stelian.
===== include/linux/nbd.h 1.7 vs edited =====
--- 1.7/include/linux/nbd.h Sun Mar 31 15:43:18 2002
+++ edited/include/linux/nbd.h Thu Apr 4 13:53:15 2002
@@ -70,7 +70,7 @@
struct file * file; /* If == NULL, device is not ready, yet */
int magic; /* FIXME: not if debugging is off */
struct list_head queue_head; /* Requests are added here... */
- struct semaphore tx_lock;
+ struct semaphore queue_lock;
};
#endif
--
Stelian Pop <[email protected]>
Alcove - http://www.alcove.com
On Thu, Apr 04, 2002 at 03:58:26PM +0200, Stelian Pop wrote:
> In fact, since nbd.c still reference 'queue_lock' I suspect that
> the actual modifications to nbd.c were lost somewhere in etherspace
> between Dave and Linus.
Correct, there's a missing part, that came from 2.4
> Either provide the right fix for nbd.c or apply the attached patch,
> which reverts the patch to nbd.h.
2.4 simply does a s/queue_lock/tx_lock/ on drivers/block/nbd.c
I'll push that to Linus later today
--
| Dave Jones. http://www.codemonkey.org.uk
| SuSE Labs
Hi,
>
> On Thu, Apr 04, 2002 at 03:58:26PM +0200, Stelian Pop wrote:
> > In fact, since nbd.c still reference 'queue_lock' I suspect that
> > the actual modifications to nbd.c were lost somewhere in etherspace
> > between Dave and Linus.
>
> Correct, there's a missing part, that came from 2.4
>
Which I wrote and submitted with Pavel's approval a little while ago.
> > Either provide the right fix for nbd.c or apply the attached patch,
> > which reverts the patch to nbd.h.
>
> 2.4 simply does a s/queue_lock/tx_lock/ on drivers/block/nbd.c
> I'll push that to Linus later today
>
Not quite. They cover different things. The queue_lock originally covered the
queue and the request sending function. There was an obscure deadlock which
could occur in this case hence the split to a spin lock to cover the queue
and a semaphore to cover only the request sending function (hence tx_lock
rather than queue lock).
I've got a 2.5 version of that patch on my patches page at the moment, but
due to the block layer changes (if I've understood them correctly) the
fix should be done in a slightly different way. The reason that I've not
submitted the patch for 2.5 is that it doesn't yet work and I've not had
a chance to investigate properly yet (it hangs on writes sometimes). I'm
sure its probably something silly that I've done but I just don't see it
at the moment. Any hints or clues are welcome :-)
Steve.
On Thu, Apr 04, 2002 at 02:50:25PM +0100, Steven Whitehouse wrote:
> > 2.4 simply does a s/queue_lock/tx_lock/ on drivers/block/nbd.c
> > I'll push that to Linus later today
> Not quite. They cover different things. The queue_lock originally covered the
> queue and the request sending function. There was an obscure deadlock which
> could occur in this case hence the split to a spin lock to cover the queue
> and a semaphore to cover only the request sending function (hence tx_lock
> rather than queue lock).
*nod* I wussed out and just took the easy bits when I forward ported
those changes from 2.4
http://www.codemonkey.org.uk/linux-2.5_drivers_block_nbd.c.diff
I dropped the actual fix because it was incompatible with the bio
changes iirc.
> I've got a 2.5 version of that patch on my patches page at the moment, but
> due to the block layer changes (if I've understood them correctly) the
> fix should be done in a slightly different way. The reason that I've not
> submitted the patch for 2.5 is that it doesn't yet work and I've not had
> a chance to investigate properly yet (it hangs on writes sometimes). I'm
> sure its probably something silly that I've done but I just don't see it
> at the moment. Any hints or clues are welcome :-)
URL ?
--
| Dave Jones. http://www.codemonkey.org.uk
| SuSE Labs
Hi,
>
> On Thu, Apr 04, 2002 at 02:50:25PM +0100, Steven Whitehouse wrote:
> > > 2.4 simply does a s/queue_lock/tx_lock/ on drivers/block/nbd.c
> > > I'll push that to Linus later today
> > Not quite. They cover different things. The queue_lock originally covered the
> > queue and the request sending function. There was an obscure deadlock which
> > could occur in this case hence the split to a spin lock to cover the queue
> > and a semaphore to cover only the request sending function (hence tx_lock
> > rather than queue lock).
>
> *nod* I wussed out and just took the easy bits when I forward ported
> those changes from 2.4
> http://www.codemonkey.org.uk/linux-2.5_drivers_block_nbd.c.diff
>
> I dropped the actual fix because it was incompatible with the bio
> changes iirc.
>
That was my conclusion too. Since its now possible to mark requests
REQ_STARTED my current 2.5 patch does that (and leaves requests on the
request queue) rather than maintaining a separate internal queue like the
2.4 version does, but its fairly similar other than that.
> > I've got a 2.5 version of that patch on my patches page at the moment, but
> > due to the block layer changes (if I've understood them correctly) the
> > fix should be done in a slightly different way. The reason that I've not
> > submitted the patch for 2.5 is that it doesn't yet work and I've not had
> > a chance to investigate properly yet (it hangs on writes sometimes). I'm
> > sure its probably something silly that I've done but I just don't see it
> > at the moment. Any hints or clues are welcome :-)
>
> URL ?
>
Sorry, I should have mentioned that earlier:
http://www.chygwyn.com/~steve/kpatch/nbd-2.5.7-deadlock.diff
Steve.