Re: [PATCH 6/7] block: cleanup ioc_clear_queue

On Wed 01-12-21 08:27:02, Christoph Hellwig wrote:

On Tue, Nov 30, 2021 at 06:26:13PM +0100, Jan Kara wrote:

quoted

I'm not quite sure about dropping the rcu protection here. This function
generally runs without any protection so what guards us against icq being
freed just after we've got its pointer from the list?

How does the RCU protection scheme work for the icq lookups?
ioc_lookup_icq takes it and then drops it before getting any kind
of refcount, so this all looks weird.  But I guess you are right that
I should probably keep this cargo culted scheme unless I have an
actual plan on how this could work.

I agree the RCU there looks like a bit of cargo-cult and would need better
documentation if nothing else. But I think the logic behind RCU protection
inside __ioc_clear_queue() is that you can safely acquire ioc->lock and
check ICQ_DESTROYED flag - which should be set if ioc got already freed, if
not set, you hold the ioc->lock so you won the race to free the ioc.
For ioc_lookup_icq() I'm not sure what's going on there, there RCU looks
completely pointless.

While we're at it:  I don't see how put put_io_context could
be called under q->queue_lock and thus actually need the whole
workqueue scheme.

I don't see that either but I think in the past an equivalent of
blk_mq_free_request() could get called during request merging while holding
all the locks (I have just recently fixed a deadlock due to this in BFQ by
postponing freeing of merged requests to the caller) and
blk_mq_free_request() will call put_io_context(). So at this point I don't
think it is needed anymore.

Then again we really need to do an audit on queue_lock and split it into
actually documented locks now that the old request code is gone.

A worthy goal :)
								Honza
-- 
Jan Kara [off-list ref]
SUSE Labs, CR