Il giorno 15 mar 2017, alle ore 17:30, Jens Axboe [off-list ref] ha =

scritto:

=20
On 03/15/2017 09:47 AM, Jens Axboe wrote:

quoted

I think you understood me correctly. Currently I think the putting of
the io context is somewhat of a mess. You have seemingly random =

places

quoted

where you have to use special unlock functions, to ensure that you
notice that some caller deeper down has set ->ioc_to_put. I took a =

quick

quoted

look at it, and by far most of the cases can return an io_context to
free quite easily. You can mark these functions __must_check to =

ensure

quoted

that we don't drop an io_context, inadvertently. That's already a win
over the random ->ioc_to_put store. And you can then get rid of
bfq_unlock_put_ioc and it's irq variant as well.
=20
The places where you are already returning a value, like off dispatch
for instance, you can just pass in a pointer to an io_context =

pointer.

quoted

=20
If you get this right, it'll be a lot less fragile and hacky than =

your

quoted

current approach.

=20
Even just looking a little closer, you also find cases where you
potentially twice store ->ioc_to_put. That kind of mixup can't happen =

if

you return it properly.
=20
In __bfq_dispatch_request(), for instance. You call =

bfq_select_queue(),

and that in turn calls bfq_bfqq_expire(), which calls
__bfq_bfqq_expire() which can set ->ioc_to_put. But later on,
__bfq_dispatch_request() calls bfq_dispatch_rq_from_bfqq(), which in
turn calls bfq_bfqq_expire() that can also set ->ioc_to_put. There's =

no

"magic" bfq_unlock_and_put_ioc() in-between those. Maybe the former =

call

never sets ->ioc_to_put if it returns with bfqq =3D=3D NULL? Hard to =

tell.

=20
Or __bfq_insert_request(), it calls bfq_add_request(), which may set
->ioc_to_put through bfq_bfqq_handle_idle_busy_switch() ->
bfq_bfqq_expire(). And then from calling bfq_rq_enqueued() ->
bfq_bfqq_expire().
=20

I have checked that.  Basically, since a queue can't be expired twice,
then it should never happen that ioc_to_put is set twice before being
used.  Yet, I do agree that using a shared field and exploiting
collateral effects makes code very complex and fragile (maybe even
buggy if my speculative check is wrong).  Just, it has been the best
solution I found, to avoid deferred work as you asked.  In fact, I
still find quite heavy the alternative of passing a pointer to an ioc
forth and back across seven or eight nested functions.

There might be more, but I think the above is plenty of evidence that
the current ->ioc_to_put solution is a bad hack, fragile, and already
has bugs.
=20
How often do you expect this putting of the io_context to happen?

Unfortunately often, as it must be done also every time the in-service
queue is reset.  But, in this respect, are we sure that we do need to
grab a reference to the ioc when we set a queue in service (as done in
cfq, and copied into bfq)?  I mean, we have the hook exit_ioc for
controlling the disappearing of an ioc.  Am I missing something here
too?

Thanks,
Paolo

If
it's not a very frequent occurence, maybe using a deferred workqueue =

to

put it IS the right solution. As it currently stands, the code doesn't
really work, and it's fragile. It can't be cleaned up without
refactoring, since the call paths are all extremely intermingled.
=20
--=20
Jens Axboe
=20