| Message ID | 409ff4f5a9365bec56c6a6dc77190b7a3b3645e6.1660938272.git.josef@toxicpanda.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: take the correct ctl lock when removing free space cache | expand |
On Fri, Aug 19, 2022 at 03:44:41PM -0400, Josef Bacik wrote: > This is a fixup for > > btrfs: call __btrfs_remove_free_space_cache_locked on cache load failure > > I was taking the wrong ctl lock, this can be folded into that patch. Folded and for-next updated.
Hi Josef,
I love your patch! Yet something to improve:
[auto build test ERROR on kdave/for-next]
[also build test ERROR on next-20220819]
[cannot apply to linus/master v6.0-rc1]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Josef-Bacik/btrfs-take-the-correct-ctl-lock-when-removing-free-space-cache/20220820-034858
base: https://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git for-next
config: i386-randconfig-a003 (https://download.01.org/0day-ci/archive/20220820/202208200644.E0g69cAc-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-5) 11.3.0
reproduce (this is a W=1 build):
# https://github.com/intel-lab-lkp/linux/commit/318faedfa41e18cc0da723a0405510d0c474d99e
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Josef-Bacik/btrfs-take-the-correct-ctl-lock-when-removing-free-space-cache/20220820-034858
git checkout 318faedfa41e18cc0da723a0405510d0c474d99e
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash fs/btrfs/
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
fs/btrfs/free-space-cache.c: In function 'load_free_space_cache':
>> fs/btrfs/free-space-cache.c:1038:35: error: invalid type argument of '->' (have 'struct btrfs_free_space_ctl')
1038 | spin_lock(&tmp_ctl->tree_lock);
| ^~
fs/btrfs/free-space-cache.c:1040:37: error: invalid type argument of '->' (have 'struct btrfs_free_space_ctl')
1040 | spin_unlock(&tmp_ctl->tree_lock);
| ^~
vim +1038 fs/btrfs/free-space-cache.c
942
943 int load_free_space_cache(struct btrfs_block_group *block_group)
944 {
945 struct btrfs_fs_info *fs_info = block_group->fs_info;
946 struct btrfs_free_space_ctl *ctl = block_group->free_space_ctl;
947 struct btrfs_free_space_ctl tmp_ctl = {};
948 struct inode *inode;
949 struct btrfs_path *path;
950 int ret = 0;
951 bool matched;
952 u64 used = block_group->used;
953
954 /*
955 * Because we could potentially discard our loaded free space, we want
956 * to load everything into a temporary structure first, and then if it's
957 * valid copy it all into the actual free space ctl.
958 */
959 btrfs_init_free_space_ctl(block_group, &tmp_ctl);
960
961 /*
962 * If this block group has been marked to be cleared for one reason or
963 * another then we can't trust the on disk cache, so just return.
964 */
965 spin_lock(&block_group->lock);
966 if (block_group->disk_cache_state != BTRFS_DC_WRITTEN) {
967 spin_unlock(&block_group->lock);
968 return 0;
969 }
970 spin_unlock(&block_group->lock);
971
972 path = btrfs_alloc_path();
973 if (!path)
974 return 0;
975 path->search_commit_root = 1;
976 path->skip_locking = 1;
977
978 /*
979 * We must pass a path with search_commit_root set to btrfs_iget in
980 * order to avoid a deadlock when allocating extents for the tree root.
981 *
982 * When we are COWing an extent buffer from the tree root, when looking
983 * for a free extent, at extent-tree.c:find_free_extent(), we can find
984 * block group without its free space cache loaded. When we find one
985 * we must load its space cache which requires reading its free space
986 * cache's inode item from the root tree. If this inode item is located
987 * in the same leaf that we started COWing before, then we end up in
988 * deadlock on the extent buffer (trying to read lock it when we
989 * previously write locked it).
990 *
991 * It's safe to read the inode item using the commit root because
992 * block groups, once loaded, stay in memory forever (until they are
993 * removed) as well as their space caches once loaded. New block groups
994 * once created get their ->cached field set to BTRFS_CACHE_FINISHED so
995 * we will never try to read their inode item while the fs is mounted.
996 */
997 inode = lookup_free_space_inode(block_group, path);
998 if (IS_ERR(inode)) {
999 btrfs_free_path(path);
1000 return 0;
1001 }
1002
1003 /* We may have converted the inode and made the cache invalid. */
1004 spin_lock(&block_group->lock);
1005 if (block_group->disk_cache_state != BTRFS_DC_WRITTEN) {
1006 spin_unlock(&block_group->lock);
1007 btrfs_free_path(path);
1008 goto out;
1009 }
1010 spin_unlock(&block_group->lock);
1011
1012 /*
1013 * Reinitialize the class of struct inode's mapping->invalidate_lock for
1014 * free space inodes to prevent false positives related to locks for normal
1015 * inodes.
1016 */
1017 lockdep_set_class(&(&inode->i_data)->invalidate_lock,
1018 &btrfs_free_space_inode_key);
1019
1020 ret = __load_free_space_cache(fs_info->tree_root, inode, &tmp_ctl,
1021 path, block_group->start);
1022 btrfs_free_path(path);
1023 if (ret <= 0)
1024 goto out;
1025
1026 matched = (tmp_ctl.free_space == (block_group->length - used -
1027 block_group->bytes_super));
1028
1029 if (matched) {
1030 ret = copy_free_space_cache(block_group, &tmp_ctl);
1031 /*
1032 * ret == 1 means we successfully loaded the free space cache,
1033 * so we need to re-set it here.
1034 */
1035 if (ret == 0)
1036 ret = 1;
1037 } else {
> 1038 spin_lock(&tmp_ctl->tree_lock);
1039 __btrfs_remove_free_space_cache(&tmp_ctl);
1040 spin_unlock(&tmp_ctl->tree_lock);
1041 btrfs_warn(fs_info,
1042 "block group %llu has wrong amount of free space",
1043 block_group->start);
1044 ret = -1;
1045 }
1046 out:
1047 if (ret < 0) {
1048 /* This cache is bogus, make sure it gets cleared */
1049 spin_lock(&block_group->lock);
1050 block_group->disk_cache_state = BTRFS_DC_CLEAR;
1051 spin_unlock(&block_group->lock);
1052 ret = 0;
1053
1054 btrfs_warn(fs_info,
1055 "failed to load free space cache for block group %llu, rebuilding it now",
1056 block_group->start);
1057 }
1058
1059 spin_lock(&ctl->tree_lock);
1060 btrfs_discard_update_discardable(block_group);
1061 spin_unlock(&ctl->tree_lock);
1062 iput(inode);
1063 return ret;
1064 }
1065
Hi Josef,
I love your patch! Yet something to improve:
[auto build test ERROR on kdave/for-next]
[also build test ERROR on next-20220819]
[cannot apply to linus/master v6.0-rc1]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Josef-Bacik/btrfs-take-the-correct-ctl-lock-when-removing-free-space-cache/20220820-034858
base: https://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git for-next
config: arm64-randconfig-r031-20220820 (https://download.01.org/0day-ci/archive/20220820/202208200922.2GjdXWTx-lkp@intel.com/config)
compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project 0ac597f3cacf60479ffd36b03766fa7462dabd78)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install arm64 cross compiling tool for clang build
# apt-get install binutils-aarch64-linux-gnu
# https://github.com/intel-lab-lkp/linux/commit/318faedfa41e18cc0da723a0405510d0c474d99e
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Josef-Bacik/btrfs-take-the-correct-ctl-lock-when-removing-free-space-cache/20220820-034858
git checkout 318faedfa41e18cc0da723a0405510d0c474d99e
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm64 SHELL=/bin/bash ./ fs/
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
>> fs/btrfs/free-space-cache.c:1038:21: error: member reference type 'struct btrfs_free_space_ctl' is not a pointer; did you mean to use '.'?
spin_lock(&tmp_ctl->tree_lock);
~~~~~~~^~
.
>> fs/btrfs/free-space-cache.c:1038:13: error: cannot take the address of an rvalue of type 'spinlock_t' (aka 'struct spinlock')
spin_lock(&tmp_ctl->tree_lock);
^~~~~~~~~~~~~~~~~~~
fs/btrfs/free-space-cache.c:1040:23: error: member reference type 'struct btrfs_free_space_ctl' is not a pointer; did you mean to use '.'?
spin_unlock(&tmp_ctl->tree_lock);
~~~~~~~^~
.
fs/btrfs/free-space-cache.c:1040:15: error: cannot take the address of an rvalue of type 'spinlock_t' (aka 'struct spinlock')
spin_unlock(&tmp_ctl->tree_lock);
^~~~~~~~~~~~~~~~~~~
4 errors generated.
vim +1038 fs/btrfs/free-space-cache.c
942
943 int load_free_space_cache(struct btrfs_block_group *block_group)
944 {
945 struct btrfs_fs_info *fs_info = block_group->fs_info;
946 struct btrfs_free_space_ctl *ctl = block_group->free_space_ctl;
947 struct btrfs_free_space_ctl tmp_ctl = {};
948 struct inode *inode;
949 struct btrfs_path *path;
950 int ret = 0;
951 bool matched;
952 u64 used = block_group->used;
953
954 /*
955 * Because we could potentially discard our loaded free space, we want
956 * to load everything into a temporary structure first, and then if it's
957 * valid copy it all into the actual free space ctl.
958 */
959 btrfs_init_free_space_ctl(block_group, &tmp_ctl);
960
961 /*
962 * If this block group has been marked to be cleared for one reason or
963 * another then we can't trust the on disk cache, so just return.
964 */
965 spin_lock(&block_group->lock);
966 if (block_group->disk_cache_state != BTRFS_DC_WRITTEN) {
967 spin_unlock(&block_group->lock);
968 return 0;
969 }
970 spin_unlock(&block_group->lock);
971
972 path = btrfs_alloc_path();
973 if (!path)
974 return 0;
975 path->search_commit_root = 1;
976 path->skip_locking = 1;
977
978 /*
979 * We must pass a path with search_commit_root set to btrfs_iget in
980 * order to avoid a deadlock when allocating extents for the tree root.
981 *
982 * When we are COWing an extent buffer from the tree root, when looking
983 * for a free extent, at extent-tree.c:find_free_extent(), we can find
984 * block group without its free space cache loaded. When we find one
985 * we must load its space cache which requires reading its free space
986 * cache's inode item from the root tree. If this inode item is located
987 * in the same leaf that we started COWing before, then we end up in
988 * deadlock on the extent buffer (trying to read lock it when we
989 * previously write locked it).
990 *
991 * It's safe to read the inode item using the commit root because
992 * block groups, once loaded, stay in memory forever (until they are
993 * removed) as well as their space caches once loaded. New block groups
994 * once created get their ->cached field set to BTRFS_CACHE_FINISHED so
995 * we will never try to read their inode item while the fs is mounted.
996 */
997 inode = lookup_free_space_inode(block_group, path);
998 if (IS_ERR(inode)) {
999 btrfs_free_path(path);
1000 return 0;
1001 }
1002
1003 /* We may have converted the inode and made the cache invalid. */
1004 spin_lock(&block_group->lock);
1005 if (block_group->disk_cache_state != BTRFS_DC_WRITTEN) {
1006 spin_unlock(&block_group->lock);
1007 btrfs_free_path(path);
1008 goto out;
1009 }
1010 spin_unlock(&block_group->lock);
1011
1012 /*
1013 * Reinitialize the class of struct inode's mapping->invalidate_lock for
1014 * free space inodes to prevent false positives related to locks for normal
1015 * inodes.
1016 */
1017 lockdep_set_class(&(&inode->i_data)->invalidate_lock,
1018 &btrfs_free_space_inode_key);
1019
1020 ret = __load_free_space_cache(fs_info->tree_root, inode, &tmp_ctl,
1021 path, block_group->start);
1022 btrfs_free_path(path);
1023 if (ret <= 0)
1024 goto out;
1025
1026 matched = (tmp_ctl.free_space == (block_group->length - used -
1027 block_group->bytes_super));
1028
1029 if (matched) {
1030 ret = copy_free_space_cache(block_group, &tmp_ctl);
1031 /*
1032 * ret == 1 means we successfully loaded the free space cache,
1033 * so we need to re-set it here.
1034 */
1035 if (ret == 0)
1036 ret = 1;
1037 } else {
> 1038 spin_lock(&tmp_ctl->tree_lock);
1039 __btrfs_remove_free_space_cache(&tmp_ctl);
1040 spin_unlock(&tmp_ctl->tree_lock);
1041 btrfs_warn(fs_info,
1042 "block group %llu has wrong amount of free space",
1043 block_group->start);
1044 ret = -1;
1045 }
1046 out:
1047 if (ret < 0) {
1048 /* This cache is bogus, make sure it gets cleared */
1049 spin_lock(&block_group->lock);
1050 block_group->disk_cache_state = BTRFS_DC_CLEAR;
1051 spin_unlock(&block_group->lock);
1052 ret = 0;
1053
1054 btrfs_warn(fs_info,
1055 "failed to load free space cache for block group %llu, rebuilding it now",
1056 block_group->start);
1057 }
1058
1059 spin_lock(&ctl->tree_lock);
1060 btrfs_discard_update_discardable(block_group);
1061 spin_unlock(&ctl->tree_lock);
1062 iput(inode);
1063 return ret;
1064 }
1065
diff --git a/fs/btrfs/free-space-cache.c b/fs/btrfs/free-space-cache.c index 6b70371d4918..157cd712a923 100644 --- a/fs/btrfs/free-space-cache.c +++ b/fs/btrfs/free-space-cache.c @@ -1035,9 +1035,9 @@ int load_free_space_cache(struct btrfs_block_group *block_group) if (ret == 0) ret = 1; } else { - spin_lock(&ctl->tree_lock); + spin_lock(&tmp_ctl->tree_lock); __btrfs_remove_free_space_cache(&tmp_ctl); - spin_unlock(&ctl->tree_lock); + spin_unlock(&tmp_ctl->tree_lock); btrfs_warn(fs_info, "block group %llu has wrong amount of free space", block_group->start);
This is a fixup for btrfs: call __btrfs_remove_free_space_cache_locked on cache load failure I was taking the wrong ctl lock, this can be folded into that patch. Signed-off-by: Josef Bacik <josef@toxicpanda.com> --- fs/btrfs/free-space-cache.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)