| Message ID | 20200810154242.782802-15-josef@toxicpanda.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Convert to an rwsem for our tree locking | expand |
On Mon, Aug 10, 2020 at 11:42:39AM -0400, Josef Bacik wrote: > --- a/fs/btrfs/locking.h > +++ b/fs/btrfs/locking.h > @@ -16,6 +16,11 @@ > #define BTRFS_WRITE_LOCK_BLOCKING 3 > #define BTRFS_READ_LOCK_BLOCKING 4 > > +/* > + * We are limited in number of subclasses by MAX_LOCKDEP_SUBCLASSES, which at > + * the time of this patch is 8, which is how many we're using here. Keep this > + * in mind if you decide you want to add another subclass. > + */ > enum btrfs_lock_nesting { > BTRFS_NESTING_NORMAL = 0, > > @@ -55,6 +60,15 @@ enum btrfs_lock_nesting { > * handle this case where we need to allocate a new split block. > */ > BTRFS_NESTING_SPLIT, > + > + /* > + * When promoting a new block to a root we need to have a special > + * subclass so we don't confuse lockdep, as it will appear that we are > + * locking a higher level node before a lower level one. Copying also > + * has this problem as it appears we're locking the same block again > + * when we make a snapshot of an existing root. > + */ > + BTRFS_NESTING_NEW_ROOT, > }; To prevent accidental addition (and not reading the comment about the maximum count), I suggest to use the static_assert with one extra definition like --- a/fs/btrfs/locking.h +++ b/fs/btrfs/locking.h @@ -14,11 +14,6 @@ #define BTRFS_WRITE_LOCK 1 #define BTRFS_READ_LOCK 2 -/* - * We are limited in number of subclasses by MAX_LOCKDEP_SUBCLASSES, which at - * the time of this patch is 8, which is how many we're using here. Keep this - * in mind if you decide you want to add another subclass. - */ enum btrfs_lock_nesting { BTRFS_NESTING_NORMAL = 0, @@ -67,8 +62,19 @@ enum btrfs_lock_nesting { * when we make a snapshot of an existing root. */ BTRFS_NESTING_NEW_ROOT, + + /* + * We are limited in number of subclasses by MAX_LOCKDEP_SUBCLASSES, + * which at the time of this patch is 8, which is how many we're using + * here. Keep this in mind if you decide you want to add another + * subclass. + */ + BTRFS_NESTING_MAX }; +static_assert(BTRFS_NESTING_MAX <= MAX_LOCKDEP_SUBCLASSES, + "too many lock subclasses defined"); + struct btrfs_path; void __btrfs_tree_lock(struct extent_buffer *eb, enum btrfs_lock_nesting nest); ---
diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c index 82dac6510a86..b0d6d86f449d 100644 --- a/fs/btrfs/ctree.c +++ b/fs/btrfs/ctree.c @@ -198,7 +198,8 @@ int btrfs_copy_root(struct btrfs_trans_handle *trans, btrfs_node_key(buf, &disk_key, 0); cow = btrfs_alloc_tree_block(trans, root, 0, new_root_objectid, - &disk_key, level, buf->start, 0, BTRFS_NESTING_NORMAL); + &disk_key, level, buf->start, 0, + BTRFS_NESTING_NEW_ROOT); if (IS_ERR(cow)) return PTR_ERR(cow); @@ -3343,7 +3344,7 @@ static noinline int insert_new_root(struct btrfs_trans_handle *trans, c = alloc_tree_block_no_bg_flush(trans, root, 0, &lower_key, level, root->node->start, 0, - BTRFS_NESTING_NORMAL); + BTRFS_NESTING_NEW_ROOT); if (IS_ERR(c)) return PTR_ERR(c); diff --git a/fs/btrfs/locking.h b/fs/btrfs/locking.h index 4f5586fed25a..c845a0c07a1c 100644 --- a/fs/btrfs/locking.h +++ b/fs/btrfs/locking.h @@ -16,6 +16,11 @@ #define BTRFS_WRITE_LOCK_BLOCKING 3 #define BTRFS_READ_LOCK_BLOCKING 4 +/* + * We are limited in number of subclasses by MAX_LOCKDEP_SUBCLASSES, which at + * the time of this patch is 8, which is how many we're using here. Keep this + * in mind if you decide you want to add another subclass. + */ enum btrfs_lock_nesting { BTRFS_NESTING_NORMAL = 0, @@ -55,6 +60,15 @@ enum btrfs_lock_nesting { * handle this case where we need to allocate a new split block. */ BTRFS_NESTING_SPLIT, + + /* + * When promoting a new block to a root we need to have a special + * subclass so we don't confuse lockdep, as it will appear that we are + * locking a higher level node before a lower level one. Copying also + * has this problem as it appears we're locking the same block again + * when we make a snapshot of an existing root. + */ + BTRFS_NESTING_NEW_ROOT, }; struct btrfs_path;
The way we add new roots is confusing from a locking perspective for lockdep. We generally have the rule that we lock things in order from highest level to lowest, but in the case of adding a new level to the tree we actually allocate a new block for the root, which makes the locking go in reverse. A similar issue exists for snapshotting, we cow the original root for the root of a new tree, however they're at the same level. Address this by using BTRFS_NESTING_NEW_ROOT for these operations. Signed-off-by: Josef Bacik <josef@toxicpanda.com> --- fs/btrfs/ctree.c | 5 +++-- fs/btrfs/locking.h | 14 ++++++++++++++ 2 files changed, 17 insertions(+), 2 deletions(-)