On 8/1/24 2:42 PM, Shin'ichiro Kawasaki wrote:
> Commit 804e498e0496 ("sd: convert to the atomic queue limits API")
> introduced pairs of function calls to queue_limits_start_update() and
> queue_limits_commit_update(). These two functions lock and unlock
> q->limits_lock. In sd_revalidate_disk(), sd_read_cpr() is called after
> queue_limits_start_update() call and before
> queue_limits_commit_update() call. sd_read_cpr() locks q->sysfs_dir_lock
> and &q->sysfs_lock. Then new lock dependencies were created between
> q->limits_lock, q->sysfs_dir_lock and q->sysfs_lock, as follows:
>
> sd_revalidate_disk
> queue_limits_start_update
> mutex_lock(&q->limits_lock)
> sd_read_cpr
> disk_set_independent_access_ranges
> mutex_lock(&q->sysfs_dir_lock)
> mutex_lock(&q->sysfs_lock)
> mutex_unlock(&q->sysfs_lock)
> mutex_unlock(&q->sysfs_dir_lock)
> queue_limits_commit_update
> mutex_unlock(&q->limits_lock)
>
> However, the three locks already had reversed dependencies in other
> places. Then the new dependencies triggered the lockdep WARN "possible
> circular locking dependency detected" [1]. This WARN was observed by
> running the blktests test case srp/002.
>
> To avoid the WARN, move the sd_read_cpr() call in sd_revalidate_disk()
> after the queue_limits_commit_update() call. In other words, move the
> sd_read_cpr() call out of the q->limits_lock region.
>
> [1] https://lore.kernel.org/linux-scsi/vlmv53ni3ltwxplig5qnw4xsl2h6ccxijfbqzekx76vxoim5a5@dekv7q3es3tx/
>
> Fixes: 804e498e0496 ("sd: convert to the atomic queue limits API")
> Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@xxxxxxx>
Given that sd_read_cpr() does not change any limit, looks good to me.
Reviewed-by: Damien Le Moal <dlemoal@xxxxxxxxxx>
--
Damien Le Moal
Western Digital Research
