From: David Sterba <dsterba@suse.cz>
To: Qu Wenruo <wqu@suse.com>
Cc: linux-btrfs@vger.kernel.org, Nikolay Borisov <nborisov@suse.com>
Subject: Re: [PATCH] btrfs: qgroup: Don't hold qgroup_ioctl_lock in btrfs_qgroup_inherit()
Date: Wed, 12 Jun 2019 15:53:24 +0200 [thread overview]
Message-ID: <20190612135324.GJ3563@twin.jikos.cz> (raw)
In-Reply-To: <20190612075745.25024-1-wqu@suse.com>
On Wed, Jun 12, 2019 at 03:57:45PM +0800, Qu Wenruo wrote:
> [BUG]
> Lockdep will report the following circular locking dependency:
>
> WARNING: possible circular locking dependency detected
> 5.2.0-rc2-custom #24 Tainted: G O
> ------------------------------------------------------
> btrfs/8631 is trying to acquire lock:
> 000000002536438c (&fs_info->qgroup_ioctl_lock#2){+.+.}, at: btrfs_qgroup_inherit+0x40/0x620 [btrfs]
>
> but task is already holding lock:
> 000000003d52cc23 (&fs_info->tree_log_mutex){+.+.}, at: create_pending_snapshot+0x8b6/0xe60 [btrfs]
>
> which lock already depends on the new lock.
>
> the existing dependency chain (in reverse order) is:
>
> -> #2 (&fs_info->tree_log_mutex){+.+.}:
> __mutex_lock+0x76/0x940
> mutex_lock_nested+0x1b/0x20
> btrfs_commit_transaction+0x475/0xa00 [btrfs]
> btrfs_commit_super+0x71/0x80 [btrfs]
> close_ctree+0x2bd/0x320 [btrfs]
> btrfs_put_super+0x15/0x20 [btrfs]
> generic_shutdown_super+0x72/0x110
> kill_anon_super+0x18/0x30
> btrfs_kill_super+0x16/0xa0 [btrfs]
> deactivate_locked_super+0x3a/0x80
> deactivate_super+0x51/0x60
> cleanup_mnt+0x3f/0x80
> __cleanup_mnt+0x12/0x20
> task_work_run+0x94/0xb0
> exit_to_usermode_loop+0xd8/0xe0
> do_syscall_64+0x210/0x240
> entry_SYSCALL_64_after_hwframe+0x49/0xbe
>
> -> #1 (&fs_info->reloc_mutex){+.+.}:
> __mutex_lock+0x76/0x940
> mutex_lock_nested+0x1b/0x20
> btrfs_commit_transaction+0x40d/0xa00 [btrfs]
> btrfs_quota_enable+0x2da/0x730 [btrfs]
> btrfs_ioctl+0x2691/0x2b40 [btrfs]
> do_vfs_ioctl+0xa9/0x6d0
> ksys_ioctl+0x67/0x90
> __x64_sys_ioctl+0x1a/0x20
> do_syscall_64+0x65/0x240
> entry_SYSCALL_64_after_hwframe+0x49/0xbe
>
> -> #0 (&fs_info->qgroup_ioctl_lock#2){+.+.}:
> lock_acquire+0xa7/0x190
> __mutex_lock+0x76/0x940
> mutex_lock_nested+0x1b/0x20
> btrfs_qgroup_inherit+0x40/0x620 [btrfs]
> create_pending_snapshot+0x9d7/0xe60 [btrfs]
> create_pending_snapshots+0x94/0xb0 [btrfs]
> btrfs_commit_transaction+0x415/0xa00 [btrfs]
> btrfs_mksubvol+0x496/0x4e0 [btrfs]
> btrfs_ioctl_snap_create_transid+0x174/0x180 [btrfs]
> btrfs_ioctl_snap_create_v2+0x11c/0x180 [btrfs]
> btrfs_ioctl+0xa90/0x2b40 [btrfs]
> do_vfs_ioctl+0xa9/0x6d0
> ksys_ioctl+0x67/0x90
> __x64_sys_ioctl+0x1a/0x20
> do_syscall_64+0x65/0x240
> entry_SYSCALL_64_after_hwframe+0x49/0xbe
>
> other info that might help us debug this:
>
> Chain exists of:
> &fs_info->qgroup_ioctl_lock#2 --> &fs_info->reloc_mutex --> &fs_info->tree_log_mutex
>
> Possible unsafe locking scenario:
>
> CPU0 CPU1
> ---- ----
> lock(&fs_info->tree_log_mutex);
> lock(&fs_info->reloc_mutex);
> lock(&fs_info->tree_log_mutex);
> lock(&fs_info->qgroup_ioctl_lock#2);
>
> *** DEADLOCK ***
>
> 6 locks held by btrfs/8631:
> #0: 00000000ed8f23f6 (sb_writers#12){.+.+}, at: mnt_want_write_file+0x28/0x60
> #1: 000000009fb1597a (&type->i_mutex_dir_key#10/1){+.+.}, at: btrfs_mksubvol+0x70/0x4e0 [btrfs]
> #2: 0000000088c5ad88 (&fs_info->subvol_sem){++++}, at: btrfs_mksubvol+0x128/0x4e0 [btrfs]
> #3: 000000009606fc3e (sb_internal#2){.+.+}, at: start_transaction+0x37a/0x520 [btrfs]
> #4: 00000000f82bbdf5 (&fs_info->reloc_mutex){+.+.}, at: btrfs_commit_transaction+0x40d/0xa00 [btrfs]
> #5: 000000003d52cc23 (&fs_info->tree_log_mutex){+.+.}, at: create_pending_snapshot+0x8b6/0xe60 [btrfs]
>
> [CAUSE]
> Due to the delayed subvolume creation, we need to call
> btrfs_qgroup_inherit() inside commit transaction code, with a lot of
> other mutex hold.
> This hell of lock chain can lead to above problem.
>
> [FIX]
> On the other hand, we don't really need to hold qgroup_ioctl_lock if
> we're in the context of create_pending_snapshot().
> As in that context, we're the only one being able to modify qgroup.
>
> All other qgroup functions which needs qgroup_ioctl_lock are either
> holding a transaction handle, or will start a new transaction:
> Functions will start a new transaction():
> * btrfs_quota_enable()
> * btrfs_quota_disable()
> Functions hold a transaction handler:
> * btrfs_add_qgroup_relation()
> * btrfs_del_qgroup_relation()
> * btrfs_create_qgroup()
> * btrfs_remove_qgroup()
> * btrfs_limit_qgroup()
> * btrfs_qgroup_inherit() call inside create_subvol()
>
> So we have a higher level protection provided by transaction, thus we
> don't need to always hold qgroup_ioctl_lock in btrfs_qgroup_inherit().
>
> Only the btrfs_qgroup_inherit() call in create_subvol() needs to hold
> qgroup_ioctl_lock, while the btrfs_qgroup_inherit() call in
> create_pending_snapshot() is already protected by transaction.
>
> So the fix is to manually hold qgroup_ioctl_lock inside create_subvol()
> while skip the lock inside create_pending_snapshot.
Would it be possible to add that as a run-time assertion? Eg. check the
state of the transaction if it's inside commit, and if not then check
the locks?
next prev parent reply other threads:[~2019-06-12 13:52 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-06-12 7:57 [PATCH] btrfs: qgroup: Don't hold qgroup_ioctl_lock in btrfs_qgroup_inherit() Qu Wenruo
2019-06-12 13:53 ` David Sterba [this message]
2019-06-12 14:05 ` Qu Wenruo
2019-06-12 14:12 ` David Sterba
2019-06-13 9:30 Qu Wenruo
2019-06-13 16:03 ` David Sterba
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190612135324.GJ3563@twin.jikos.cz \
--to=dsterba@suse.cz \
--cc=linux-btrfs@vger.kernel.org \
--cc=nborisov@suse.com \
--cc=wqu@suse.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).