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(-)