Re: [PATCH] block: model freeze & enter queue as rwsem for supporting lockdep

[Christoph Hellwig <hch@lst.de>]

On Fri, Oct 18, 2024 at 09:57:12AM -0700, Bart Van Assche wrote:
> Thank you for having reported this issue and for having proposed a
> patch. I think the following is missing from the patch description:
> (a) An analysis of which code causes the inconsistent nested lock order.
> (b) A discussion of potential alternatives.
>
> It seems unavoidable to me that some code freezes request queue(s)
> before the limits are updated. Additionally, there is code that freezes
> queues first (sd_revalidate_disk()), executes commands and next updates
> limits. Hence the inconsistent order of freezing queues and obtaining
> limits_lock.
>
> The alternative (entirely untested) solution below has the following
> advantages:
> * No additional information has to be provided to lockdep about the
>   locking order.
> * No new flags are required (SKIP_FREEZE_LOCKDEP).
> * No exceptions are necessary for blk_queue_exit() nor for the NVMe
>   driver.

As mentioned by Ming fixing the lockdep reports is a bit different
from adding it first.  But I'll chime in for your proposal to fix
the report anyway.

I think requiring the queue to be frozen before we start the queue
limits update is reasonable in general, but a major pain for
sd_revalidate_disk, and unfortunately I don't think your proposed patch
works there as it doesn't protect against concurrent updates from
say sysfs, and also doesn't scale to adding new limits.

One option to deal with this would be to add an update sequence to
the queue_limits.  sd_revalidate_disk would sample it where it
currently starts the update, then drop the lock and query the
device, then takes the limits lock again, checks for the sequence.
If it matches it commits the update, else it retries.