Re: [Nbd] [PATCH 2/6] block: loop: don't hold lo_ctl_mutex in lo_open
- To: Jens Axboe <axboe@...161...>, Linux Kernel Mailing List <linux-kernel@...25...>, Christoph Hellwig <hch@...1270...>, Tejun Heo <tj@...1285...>
- Cc: "nbd-general@lists.sourceforge.net" <nbd-general@lists.sourceforge.net>, linux-s390@...25..., Ming Lei <ming.lei@...1301...>, Stefan Haberland <stefan.haberland@...1296...>, Sebastian Ott <sebott@...1294...>, Jarod Wilson <jarod@...696...>, Fabian Frederick <fabf@...1980...>, Alexander Viro <viro@...1300...>, David Herrmann <dh.herrmann@...17...>, Andrew Morton <akpm@...133...>
- Subject: Re: [Nbd] [PATCH 2/6] block: loop: don't hold lo_ctl_mutex in lo_open
- From: Ming Lei <ming.lei@...1301...>
- Date: Mon, 6 Apr 2015 00:28:22 +0800
- Message-id: <CACVXFVMFxAdG3BNtO82uhv8RTYErk5c9ts4+QVpbne+4WHnVqA@...18...>
- In-reply-to: <1428218688-4092-3-git-send-email-ming.lei@...1301...>
- References: <1428218688-4092-1-git-send-email-ming.lei@...1301...> <1428218688-4092-3-git-send-email-ming.lei@...1301...>
On Sun, Apr 5, 2015 at 3:24 PM, Ming Lei <ming.lei@...1301...> 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
Reply to: