On Wed 01-12-21 08:27:02, Christoph Hellwig wrote: > On Tue, Nov 30, 2021 at 06:26:13PM +0100, Jan Kara wrote: > > 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 <jack@xxxxxxxx> SUSE Labs, CR
