Sorry to repeat .. I didn't see this go out on the list and I haven't
had any reply. So let's ask again. Is this a new coding error in ll_rw_blk?
-----------------
The following has been lost from __make_request() in ll_rw_blk.c since
2.4.2 (incl):
out:
- if (!q->plugged)
- (q->request_fn)(q);
if (freereq)
The result is that a block device that doesn't do plugging doesn't
work.
If it has called blk_queue_pluggable() to register a no-op plug_fn,
then q->plugged will never be set (it's the duty of the plug_fn), and
the devices registered request function will never be called.
This behaviour is distinct from 2.4.0, where registering a no-op
plug_fn made things work fine.
Is this a coding oversight?
Peter ([email protected])
On Thu, Apr 19 2001, Peter T. Breuer wrote:
> Sorry to repeat .. I didn't see this go out on the list and I haven't
> had any reply. So let's ask again. Is this a new coding error in ll_rw_blk?
>
> -----------------
>
> The following has been lost from __make_request() in ll_rw_blk.c since
> 2.4.2 (incl):
>
> out:
> - if (!q->plugged)
> - (q->request_fn)(q);
> if (freereq)
>
> The result is that a block device that doesn't do plugging doesn't
> work.
>
> If it has called blk_queue_pluggable() to register a no-op plug_fn,
> then q->plugged will never be set (it's the duty of the plug_fn), and
> the devices registered request function will never be called.
>
> This behaviour is distinct from 2.4.0, where registering a no-op
> plug_fn made things work fine.
>
> Is this a coding oversight?
Check the archives, I replied to this days ago. But since I'm taking the
subject up anyway, let me expand on it a bit further.
Not using plugging is gone, blk_queue_pluggable has been removed from
the current 2.4.4-pre series if you check that. The main reason for
doing this, is that there are generally no reasons for _not_ using
plugging in the 2.4 series kernels. In 2.2 and previous, not using the
builtin plugging was generally done to disable request merging. In 2.4,
the queues have good control over what happens there with the
back/front/request merging functions -- so drivers can just use that.
Besides, the above hunk was removed because it is wrong. For devices
using plugging, we would re-call the request_fn while the device was
already active and serving requests. Not only is this a performance hit
we don't need to take, it also gave problems on some drivers.
--
Jens Axboe
OK .. thanks Jens. Sorry about the repeat .. my nameserver lost its fix
on the root servers thanks to some hurried upgrades, and sendmail
started quietly bouncing mail for "not having" a dns entry, and you
know about deja. Probably the list dropped me for the bounces.
Those are my excuses. Apologies again.
"Jens Axboe wrote:"
> > The result is that a block device that doesn't do plugging doesn't
> > work.
> >
> > If it has called blk_queue_pluggable() to register a no-op plug_fn,
> > then q->plugged will never be set (it's the duty of the plug_fn), and
> > the devices registered request function will never be called.
> >
> > This behaviour is distinct from 2.4.0, where registering a no-op
> > plug_fn made things work fine.
>
> Check the archives, I replied to this days ago. But since I'm taking the
> subject up anyway, let me expand on it a bit further.
Yes, please.
> Not using plugging is gone, blk_queue_pluggable has been removed from
> the current 2.4.4-pre series if you check that. The main reason for
> doing this, is that there are generally no reasons for _not_ using
> plugging in the 2.4 series kernels. In 2.2 and previous, not using the
I agree.
> builtin plugging was generally done to disable request merging. In 2.4,
> the queues have good control over what happens there with the
> back/front/request merging functions -- so drivers can just use that.
They can indeed. And I agree, it had become necessary to both enable
plugging and to enable request merging separately (via control over
these two sets of things), in order to get request merging. That was
silly.
> Besides, the above hunk was removed because it is wrong. For devices
> using plugging, we would re-call the request_fn while the device was
> already active and serving requests. Not only is this a performance hit
Not sure about that ...
> we don't need to take, it also gave problems on some drivers.
Well, I know scsi used to be treating the first element while still on
the queue, but presumably you are not referring to that.
So the consensus is that I should enable plugging while the plugging
function is still here and do nothing when it goes? I must say I don't
think it should really "go", since that means I have to add a no-op
macro to replace it, and I don't like #ifdefs.
BTW, I don't need request merging (and therefore don't need plugging)
because requests eventually go out over the net. Nevertheless, I have
always been interested in seeing the difference it could cause. I
have never seen the measurements reflect the gain that I would have
expected from request merging, although I would have naively expected
some (in MY case).
Thanks again.
Peter
On Thu, Apr 19 2001, Peter T. Breuer wrote:
> > Besides, the above hunk was removed because it is wrong. For devices
> > using plugging, we would re-call the request_fn while the device was
> > already active and serving requests. Not only is this a performance hit
>
> Not sure about that ...
It _is_ wrong. The code was correct for devices not using plugging, they
want request_fn to be called on each request add. However, for a
plugging driver in a !q->plugged state it was wrong.
> > we don't need to take, it also gave problems on some drivers.
>
> Well, I know scsi used to be treating the first element while still on
> the queue, but presumably you are not referring to that.
Not so, IDE does this. And btw, this is still assumed the default
behaviour unless explicitly disabled, for data protection reasons. SCSI
always peals the request off the queue before starting processing.
> So the consensus is that I should enable plugging while the plugging
> function is still here and do nothing when it goes? I must say I don't
> think it should really "go", since that means I have to add a no-op
> macro to replace it, and I don't like #ifdefs.
The moral would be that you should never do anything. You didn't enable
plugging with blk_queue_pluggable, only disabled it by using a noop
plug.
> BTW, I don't need request merging (and therefore don't need plugging)
> because requests eventually go out over the net. Nevertheless, I have
> always been interested in seeing the difference it could cause. I
Plugging will really not hurt you. If you really don't want plugging and
don't care for merging, then using a request_fn is the wrong approach
anyway. In that case, you simply want to use a make_request_fn style
request handling.
--
Jens Axboe
"A month of sundays ago Jens Axboe wrote:"
> On Thu, Apr 19 2001, Peter T. Breuer wrote:
> > So the consensus is that I should enable plugging while the plugging
> > function is still here and do nothing when it goes? I must say I don't
> > think it should really "go", since that means I have to add a no-op
> > macro to replace it, and I don't like #ifdefs.
>
> The moral would be that you should never do anything. You didn't enable
> plugging with blk_queue_pluggable, only disabled it by using a noop
> plug.
I was thinking about what has to be done to allow my code to compile in
older kernels. I _believe_ (I may be mistaken) that I had to _explicitly_
disable plugging at some stage. Probably in 2.2. and possibly in 2.4.0.
On that basis, I do need a plug_fn and a blk_queue_pluggable for
compilation against those kernels, and these should both be macro'ed to
oblivion in the newest kernels. No?
Apologies for the continued vagueness. There are a lot of states to
consider: two machine states (plugged/not plugged) and several code
states (whatever had to be done when to cause what).
Peter
On Thu, Apr 19 2001, Peter T. Breuer wrote:
> "A month of sundays ago Jens Axboe wrote:"
> > On Thu, Apr 19 2001, Peter T. Breuer wrote:
> > > So the consensus is that I should enable plugging while the plugging
> > > function is still here and do nothing when it goes? I must say I don't
> > > think it should really "go", since that means I have to add a no-op
> > > macro to replace it, and I don't like #ifdefs.
> >
> > The moral would be that you should never do anything. You didn't enable
> > plugging with blk_queue_pluggable, only disabled it by using a noop
> > plug.
>
> I was thinking about what has to be done to allow my code to compile in
> older kernels. I _believe_ (I may be mistaken) that I had to _explicitly_
> disable plugging at some stage. Probably in 2.2. and possibly in 2.4.0.
>
> On that basis, I do need a plug_fn and a blk_queue_pluggable for
> compilation against those kernels, and these should both be macro'ed to
> oblivion in the newest kernels. No?
Examine _why_ you don't want plugging. In 2.2, you would have to edit
the kernel manually to disable it for your device. For 2.4, as long as
there has been blk_queue_pluggable, there has also been the
disable-merge function mentioned. Why are you disabling plugging??
--
Jens Axboe
"Jens Axboe wrote:"
> Examine _why_ you don't want plugging. In 2.2, you would have to edit
> the kernel manually to disable it for your device.
True. Except that I borrowed a major which already got that special
treatment.
> For 2.4, as long as
> there has been blk_queue_pluggable, there has also been the
> disable-merge function mentioned. Why are you disabling plugging??
Fundamentally, to disable merging, as you suggest. I had merging
working fine in 2.0.*. Then I never could figure out what had to be
done in 2.2.*, so I disabled it. In 2.4, things work nicely - I don't
have to do anything and it all happens magically.
Nevertheless, I am left with baggage that I have to maintain -
certainly the driver has to work in 2.2 as well as in 2.4. Removing
the blah_plugging function now in 2.4 after having started off 2.4
with it around gives me one more #ifdef kernel_version in my code.
I don't think that's good for my code, and in general I don't think one
should remove this function half way through a stable series. Leave it
there, mark it as deprecated in big letters, and make it do nothing,
but leave it there, no?
Peter
On Thu, Apr 19 2001, Peter T. Breuer wrote:
> "Jens Axboe wrote:"
> > Examine _why_ you don't want plugging. In 2.2, you would have to edit
> > the kernel manually to disable it for your device.
>
> True. Except that I borrowed a major which already got that special
> treatment.
Ok
> > For 2.4, as long as
> > there has been blk_queue_pluggable, there has also been the
> > disable-merge function mentioned. Why are you disabling plugging??
>
> Fundamentally, to disable merging, as you suggest. I had merging
> working fine in 2.0.*. Then I never could figure out what had to be
> done in 2.2.*, so I disabled it. In 2.4, things work nicely - I don't
> have to do anything and it all happens magically.
Great
> Nevertheless, I am left with baggage that I have to maintain -
> certainly the driver has to work in 2.2 as well as in 2.4. Removing
> the blah_plugging function now in 2.4 after having started off 2.4
> with it around gives me one more #ifdef kernel_version in my code.
On the contrary, you are now given an exceptional opportunity to clean
up your code and get rid of blk_queue_pluggable and your noop plugging
function.
> I don't think that's good for my code, and in general I don't think one
> should remove this function half way through a stable series. Leave it
> there, mark it as deprecated in big letters, and make it do nothing,
> but leave it there, no?
Because most people are using it for the wrong reason anyway, so I'd
consider it a not-so-subtle hint. There should be no need for extra
ifdef's, on the contrary.
--
Jens Axboe
OK - agreed. But while I have your attention...
"Jens Axboe wrote:"
> On the contrary, you are now given an exceptional opportunity to clean
> up your code and get rid of blk_queue_pluggable and your noop plugging
> function.
In summary: blk_queue_pluggable can be removed for all driver codes
aimed at all 2.4.* kernels, because the intended effect can be obtained
through merge_reqeusts function controls.
My unease derives, I think, from the fact that I have occasionally used
plugging for other purposes. Namely for throttling the device. These
uses have always been experimental and uniformly unsuccessful, because
throttling that way backs up the VFS with dirty buffers and provokes
precisely the deadlock against VFS that I was trying to avoid. So ..
... how can I tell when VFS is nearly full? In those circumstances I
want to sync every _other_ device, thus giving me enough buffers at
least to flush something to the net with, thus freeing a request of
mine, plus its buffers.
Peter
On Thu, Apr 19 2001, Peter T. Breuer wrote:
> OK - agreed. But while I have your attention...
>
> "Jens Axboe wrote:"
> > On the contrary, you are now given an exceptional opportunity to clean
> > up your code and get rid of blk_queue_pluggable and your noop plugging
> > function.
>
> In summary: blk_queue_pluggable can be removed for all driver codes
> aimed at all 2.4.* kernels, because the intended effect can be obtained
> through merge_reqeusts function controls.
Yes
> My unease derives, I think, from the fact that I have occasionally used
> plugging for other purposes. Namely for throttling the device. These
> uses have always been experimental and uniformly unsuccessful, because
> throttling that way backs up the VFS with dirty buffers and provokes
> precisely the deadlock against VFS that I was trying to avoid. So ..
>
> ... how can I tell when VFS is nearly full? In those circumstances I
> want to sync every _other_ device, thus giving me enough buffers at
> least to flush something to the net with, thus freeing a request of
> mine, plus its buffers.
You can't, there's currently no way of doing what you suggest. The block
layer will throttle locked buffers for you. Besides, this would be the
very wrong place to do it. If you reject or throttle requests, you are
effectively throttling stuff that is already locked down and cannot be
touched.
--
Jens Axboe
"Jens Axboe wrote:"
> [ptb wrote]
> > through merge_reqeusts function controls.
> > My unease derives, I think, from the fact that I have occasionally used
> > plugging for other purposes. Namely for throttling the device. These
> > uses have always been experimental and uniformly unsuccessful, because
> > throttling that way backs up the VFS with dirty buffers and provokes
> > precisely the deadlock against VFS that I was trying to avoid. So ..
> >
> > ... how can I tell when VFS is nearly full? In those circumstances I
> > want to sync every _other_ device, thus giving me enough buffers at
> > least to flush something to the net with, thus freeing a request of
> > mine, plus its buffers.
>
> You can't, there's currently no way of doing what you suggest. The block
> layer will throttle locked buffers for you. Besides, this would be the
> very wrong place to do it. If you reject or throttle requests, you are
> effectively throttling stuff that is already locked down and cannot be
> touched.
Oh, I agree. Exactly. It's pointless to throttle via requests/plugging.
However, it appears possible to deadlock if there is no way to detect
generic VFS fullness. Let me explain the setting: part of the driver is
in user space, and it sends requests over the net. It holds onto the
requests and their buffers while the user space process does the
networking (over ssl, sometimes) until an ack comes back. There is an
obvious deadlock against VFS if the remote is on localhost (it has to
write the received requests to disk before acking, and it needs buffers
to do so ..), but there have been persistent sporadic reports that the
driver can occasionally deadlock even against a true remote host.
No such reports arise when I have forced a sync on the sender every
thousand requests or so, and people with problems report that mounting
an FS sync on the device solves their problem completely. This is
empirical evidence, but it suggests that there is a problem to be
avoided here. Thus I am keen to be able to detect how many buffers
there are that could be liberated by fsyncing _other_ devices, and
doing it.
I've never seen a way.
Peter