Re: [PATCH 2/6] block: loop: don't hold lo_ctl_mutex in lo_open

Ming Lei
2015-04-05T16:28:22Z

On Sun, Apr 5, 2015 at 3:24 PM, Ming Lei <ming.lei@canonical.com> wrote:
> The lo_ctl_mutex is held for running all ioctl handlers, and
> in some ioctl handlers, ioctl_by_bdev(BLKRRPART) is called for
> rereading partitions, which requires bd_mutex.
>
> So it is easy to cause failure because trylock(bd_mutex) may
> fail inside blkdev_reread_part(), and follows the lock context:
>
> blkid or other application:
> ->open()
> ->mutex_lock(bd_mutex)
> ->lo_open()
> ->mutex_lock(lo_ctl_mutex)
>
> losetup(set fd ioctl):
> ->mutex_lock(lo_ctl_mutex)
> ->ioctl_by_bdev(BLKRRPART)
> ->trylock(bd_mutex)
>
> This patch trys to eliminate the ABBA lock dependency by removing
> lo_ctl_mutext in lo_open() with the following approach:
>
> 1) introduce lo_open_mutex to protect lo_refcnt and avoid acquiring
> lo_ctl_mutex in lo_open():
> - for open vs. add/del loop, no any problem because of loop_index_mutex
> - lo_open_mutex is used for syncing open() and loop_clr_fd()
> - both open() and release() have been serialized by bd_mutex already
>
> 2) don't hold lo_ctl_mutex for decreasing/checking lo_refcnt in
> lo_release(), then lo_ctl_mutex is only required for the last release.

Another simpler way is to make lo_refcnt as atomic_t and remove
lo_ctrl_mutext in lo_open(), and freeze request queue during clearing
fd, and better to freeze queue during setting fd too, so will update in
v1 with this way.

Thanks,
Ming Lei