Re: [PATCH 1/2] block: avoid to hold q->limits_lock across APIs for atomic update queue limits

[Christoph Hellwig <hch@lst.de>]

On Wed, Dec 18, 2024 at 06:57:45AM -0800, Damien Le Moal wrote:
> > Yeah agreed but I see sd_revalidate_disk() is probably the only exception 
> > which allocates the blk-mq request. Can't we fix it? 
> 
> If we change where limits_lock is taken now, we will again introduce races
> between user config and discovery/revalidation, which is what
> queue_limits_start_update() and queue_limits_commit_update() intended to fix in
> the first place.
> 
> So changing sd_revalidate_disk() is not the right approach.

Well, sd_revalidate_disk is a bit special in that it needs a command
on the same queue to query the information.  So it needs to be able
to issue commands without the queue frozen.  Freezing the queue inside
the limits lock support that, sd just can't use the convenience helpers
that lock and freeze.

> This is overly complicated ... As I suggested, I think that a simpler approach
> is to call blk_mq_freeze_queue() and blk_mq_unfreeze_queue() inside
> queue_limits_commit_update(). Doing so, no driver should need to directly call
> freeze/unfreeze. But that would be a cleanup. Let's first fix the few instances
> that have the update/freeze order wrong. As mentioned, the pattern simply needs

Yes, the queue only needs to be frozen for the actual update,
which would remove the need for the locking.  The big question for both
variants is if we can get rid of all the callers that have the queue
already frozen and then start an update.