Message ID | 1524823126-23414-1-git-send-email-ethanwu@synology.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On 27.04.2018 12:58, ethanwu wrote: > In preivous patch: > Btrfs: kill trans in run_delalloc_nocow and btrfs_cross_ref_exist > We avoid starting btrfs transaction and get this information from > fs_info->running_transaction directly. > > When accessing running_transaction in check_delayed_ref, there's a > chance that current transaction will be freed by commit transaction > after the NULL pointer check of running_transaction is passed. > > After looking all the other places using fs_info->running_transaction, > they are either protected by trans_lock or holding the transactions. > > Fix this by using trans_lock and incrementing the use_count. Yep, looking at how ->running_transaction is set/cleared, holding the trans_lock is correct here. However there is one point which you need to fix. > > Signed-off-by: ethanwu <ethanwu@synology.com> > --- > fs/btrfs/extent-tree.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c > index c1618ab..c8fd37b 100644 > --- a/fs/btrfs/extent-tree.c > +++ b/fs/btrfs/extent-tree.c > @@ -3149,7 +3149,11 @@ static noinline int check_delayed_ref(struct btrfs_root *root, > struct rb_node *node; > int ret = 0; > > + spin_lock(&root->fs_info->trans_lock); > cur_trans = root->fs_info->running_transaction; > + if (cur_trans) > + atomic_inc(&cur_trans->use_count); The ->use_count is using refcount_inc now, not atomic_inc. Please base all further upstream patches on upstream code > + spin_unlock(&root->fs_info->trans_lock); > if (!cur_trans) > return 0; > > @@ -3158,6 +3162,7 @@ static noinline int check_delayed_ref(struct btrfs_root *root, > head = btrfs_find_delayed_ref_head(delayed_refs, bytenr); > if (!head) { > spin_unlock(&delayed_refs->lock); > + btrfs_put_transaction(cur_trans); > return 0; > } > > @@ -3174,6 +3179,7 @@ static noinline int check_delayed_ref(struct btrfs_root *root, > mutex_lock(&head->mutex); > mutex_unlock(&head->mutex); > btrfs_put_delayed_ref_head(head); > + btrfs_put_transaction(cur_trans); > return -EAGAIN; > } > spin_unlock(&delayed_refs->lock); > @@ -3206,6 +3212,7 @@ static noinline int check_delayed_ref(struct btrfs_root *root, > } > spin_unlock(&head->lock); > mutex_unlock(&head->mutex); > + btrfs_put_transaction(cur_trans); > return ret; > } > > -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi ethanwu, Thank you for the patch! Yet something to improve: [auto build test ERROR on v4.17-rc2] [also build test ERROR on next-20180426] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/ethanwu/btrfs-Take-trans-lock-before-access-running-trans-in-check_delayed_ref/20180428-223414 config: i386-randconfig-x014-201816 (attached as .config) compiler: gcc-7 (Debian 7.3.0-16) 7.3.0 reproduce: # save the attached .config to linux build tree make ARCH=i386 All errors (new ones prefixed by >>): fs/btrfs/extent-tree.c: In function 'check_delayed_ref': >> fs/btrfs/extent-tree.c:3148:14: error: passing argument 1 of 'atomic_inc' from incompatible pointer type [-Werror=incompatible-pointer-types] atomic_inc(&cur_trans->use_count); ^ In file included from arch/x86/include/asm/atomic.h:283:0, from include/linux/atomic.h:5, from include/linux/rcupdate.h:38, from include/linux/rculist.h:11, from include/linux/pid.h:5, from include/linux/sched.h:14, from fs/btrfs/extent-tree.c:6: include/asm-generic/atomic-instrumented.h:100:29: note: expected 'atomic_t * {aka struct <anonymous> *}' but argument is of type 'refcount_t * {aka struct refcount_struct *}' static __always_inline void atomic_inc(atomic_t *v) ^~~~~~~~~~ cc1: some warnings being treated as errors vim +/atomic_inc +3148 fs/btrfs/extent-tree.c 3132 3133 static noinline int check_delayed_ref(struct btrfs_root *root, 3134 struct btrfs_path *path, 3135 u64 objectid, u64 offset, u64 bytenr) 3136 { 3137 struct btrfs_delayed_ref_head *head; 3138 struct btrfs_delayed_ref_node *ref; 3139 struct btrfs_delayed_data_ref *data_ref; 3140 struct btrfs_delayed_ref_root *delayed_refs; 3141 struct btrfs_transaction *cur_trans; 3142 struct rb_node *node; 3143 int ret = 0; 3144 3145 spin_lock(&root->fs_info->trans_lock); 3146 cur_trans = root->fs_info->running_transaction; 3147 if (cur_trans) > 3148 atomic_inc(&cur_trans->use_count); 3149 spin_unlock(&root->fs_info->trans_lock); 3150 if (!cur_trans) 3151 return 0; 3152 3153 delayed_refs = &cur_trans->delayed_refs; 3154 spin_lock(&delayed_refs->lock); 3155 head = btrfs_find_delayed_ref_head(delayed_refs, bytenr); 3156 if (!head) { 3157 spin_unlock(&delayed_refs->lock); 3158 btrfs_put_transaction(cur_trans); 3159 return 0; 3160 } 3161 3162 if (!mutex_trylock(&head->mutex)) { 3163 refcount_inc(&head->refs); 3164 spin_unlock(&delayed_refs->lock); 3165 3166 btrfs_release_path(path); 3167 3168 /* 3169 * Mutex was contended, block until it's released and let 3170 * caller try again 3171 */ 3172 mutex_lock(&head->mutex); 3173 mutex_unlock(&head->mutex); 3174 btrfs_put_delayed_ref_head(head); 3175 btrfs_put_transaction(cur_trans); 3176 return -EAGAIN; 3177 } 3178 spin_unlock(&delayed_refs->lock); 3179 3180 spin_lock(&head->lock); 3181 /* 3182 * XXX: We should replace this with a proper search function in the 3183 * future. 3184 */ 3185 for (node = rb_first(&head->ref_tree); node; node = rb_next(node)) { 3186 ref = rb_entry(node, struct btrfs_delayed_ref_node, ref_node); 3187 /* If it's a shared ref we know a cross reference exists */ 3188 if (ref->type != BTRFS_EXTENT_DATA_REF_KEY) { 3189 ret = 1; 3190 break; 3191 } 3192 3193 data_ref = btrfs_delayed_node_to_data_ref(ref); 3194 3195 /* 3196 * If our ref doesn't match the one we're currently looking at 3197 * then we have a cross reference. 3198 */ 3199 if (data_ref->root != root->root_key.objectid || 3200 data_ref->objectid != objectid || 3201 data_ref->offset != offset) { 3202 ret = 1; 3203 break; 3204 } 3205 } 3206 spin_unlock(&head->lock); 3207 mutex_unlock(&head->mutex); 3208 btrfs_put_transaction(cur_trans); 3209 return ret; 3210 } 3211 --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi ethanwu, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on v4.17-rc2] [also build test WARNING on next-20180426] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/ethanwu/btrfs-Take-trans-lock-before-access-running-trans-in-check_delayed_ref/20180428-223414 config: i386-randconfig-a0-201816 (attached as .config) compiler: gcc-4.9 (Debian 4.9.4-2) 4.9.4 reproduce: # save the attached .config to linux build tree make ARCH=i386 All warnings (new ones prefixed by >>): fs/btrfs/extent-tree.c: In function 'check_delayed_ref': >> fs/btrfs/extent-tree.c:3148:14: warning: passing argument 1 of 'atomic_inc' from incompatible pointer type atomic_inc(&cur_trans->use_count); ^ In file included from arch/x86/include/asm/atomic.h:283:0, from include/linux/atomic.h:5, from include/linux/rcupdate.h:38, from include/linux/rculist.h:11, from include/linux/pid.h:5, from include/linux/sched.h:14, from fs/btrfs/extent-tree.c:6: include/asm-generic/atomic-instrumented.h:100:29: note: expected 'struct atomic_t *' but argument is of type 'struct refcount_t *' static __always_inline void atomic_inc(atomic_t *v) ^ vim +/atomic_inc +3148 fs/btrfs/extent-tree.c 3132 3133 static noinline int check_delayed_ref(struct btrfs_root *root, 3134 struct btrfs_path *path, 3135 u64 objectid, u64 offset, u64 bytenr) 3136 { 3137 struct btrfs_delayed_ref_head *head; 3138 struct btrfs_delayed_ref_node *ref; 3139 struct btrfs_delayed_data_ref *data_ref; 3140 struct btrfs_delayed_ref_root *delayed_refs; 3141 struct btrfs_transaction *cur_trans; 3142 struct rb_node *node; 3143 int ret = 0; 3144 3145 spin_lock(&root->fs_info->trans_lock); 3146 cur_trans = root->fs_info->running_transaction; 3147 if (cur_trans) > 3148 atomic_inc(&cur_trans->use_count); 3149 spin_unlock(&root->fs_info->trans_lock); 3150 if (!cur_trans) 3151 return 0; 3152 3153 delayed_refs = &cur_trans->delayed_refs; 3154 spin_lock(&delayed_refs->lock); 3155 head = btrfs_find_delayed_ref_head(delayed_refs, bytenr); 3156 if (!head) { 3157 spin_unlock(&delayed_refs->lock); 3158 btrfs_put_transaction(cur_trans); 3159 return 0; 3160 } 3161 3162 if (!mutex_trylock(&head->mutex)) { 3163 refcount_inc(&head->refs); 3164 spin_unlock(&delayed_refs->lock); 3165 3166 btrfs_release_path(path); 3167 3168 /* 3169 * Mutex was contended, block until it's released and let 3170 * caller try again 3171 */ 3172 mutex_lock(&head->mutex); 3173 mutex_unlock(&head->mutex); 3174 btrfs_put_delayed_ref_head(head); 3175 btrfs_put_transaction(cur_trans); 3176 return -EAGAIN; 3177 } 3178 spin_unlock(&delayed_refs->lock); 3179 3180 spin_lock(&head->lock); 3181 /* 3182 * XXX: We should replace this with a proper search function in the 3183 * future. 3184 */ 3185 for (node = rb_first(&head->ref_tree); node; node = rb_next(node)) { 3186 ref = rb_entry(node, struct btrfs_delayed_ref_node, ref_node); 3187 /* If it's a shared ref we know a cross reference exists */ 3188 if (ref->type != BTRFS_EXTENT_DATA_REF_KEY) { 3189 ret = 1; 3190 break; 3191 } 3192 3193 data_ref = btrfs_delayed_node_to_data_ref(ref); 3194 3195 /* 3196 * If our ref doesn't match the one we're currently looking at 3197 * then we have a cross reference. 3198 */ 3199 if (data_ref->root != root->root_key.objectid || 3200 data_ref->objectid != objectid || 3201 data_ref->offset != offset) { 3202 ret = 1; 3203 break; 3204 } 3205 } 3206 spin_unlock(&head->lock); 3207 mutex_unlock(&head->mutex); 3208 btrfs_put_transaction(cur_trans); 3209 return ret; 3210 } 3211 --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi ethanwu, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on v4.17-rc2] [also build test WARNING on next-20180426] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/ethanwu/btrfs-Take-trans-lock-before-access-running-trans-in-check_delayed_ref/20180428-223414 reproduce: # apt-get install sparse make ARCH=x86_64 allmodconfig make C=1 CF=-D__CHECK_ENDIAN__ sparse warnings: (new ones prefixed by >>) fs/btrfs/extent-tree.c:278:39: sparse: expression using sizeof(void) fs/btrfs/extent-tree.c:278:39: sparse: expression using sizeof(void) fs/btrfs/extent-tree.c:401:16: sparse: expression using sizeof(void) >> fs/btrfs/extent-tree.c:3148:29: sparse: incorrect type in argument 1 (different base types) @@ expected struct atomic_t [usertype] *v @@ got ct atomic_t [usertype] *v @@ fs/btrfs/extent-tree.c:3148:29: expected struct atomic_t [usertype] *v fs/btrfs/extent-tree.c:3148:29: got struct refcount_struct *<noident> fs/btrfs/extent-tree.c:4502:26: sparse: expression using sizeof(void) fs/btrfs/extent-tree.c:4850:31: sparse: expression using sizeof(void) fs/btrfs/extent-tree.c:4850:31: sparse: expression using sizeof(void) fs/btrfs/extent-tree.c:5043:22: sparse: expression using sizeof(void) fs/btrfs/extent-tree.c:5060:22: sparse: expression using sizeof(void) fs/btrfs/extent-tree.c:5060:22: sparse: expression using sizeof(void) fs/btrfs/extent-tree.c:5043:22: sparse: expression using sizeof(void) fs/btrfs/extent-tree.c:5060:22: sparse: expression using sizeof(void) fs/btrfs/extent-tree.c:5060:22: sparse: expression using sizeof(void) fs/btrfs/extent-tree.c:5043:22: sparse: expression using sizeof(void) fs/btrfs/extent-tree.c:5060:22: sparse: expression using sizeof(void) fs/btrfs/extent-tree.c:5060:22: sparse: expression using sizeof(void) fs/btrfs/extent-tree.c:5043:22: sparse: expression using sizeof(void) fs/btrfs/extent-tree.c:5060:22: sparse: expression using sizeof(void) fs/btrfs/extent-tree.c:5060:22: sparse: expression using sizeof(void) fs/btrfs/extent-tree.c:5612:48: sparse: expression using sizeof(void) fs/btrfs/extent-tree.c:5612:48: sparse: expression using sizeof(void) fs/btrfs/extent-tree.c:5861:21: sparse: expression using sizeof(void) fs/btrfs/extent-tree.c:5866:27: sparse: expression using sizeof(void) fs/btrfs/extent-tree.c:5872:37: sparse: expression using sizeof(void) fs/btrfs/extent-tree.c:5872:37: sparse: expression using sizeof(void) fs/btrfs/extent-tree.c:6308:29: sparse: expression using sizeof(void) fs/btrfs/extent-tree.c:6308:29: sparse: expression using sizeof(void) fs/btrfs/extent-tree.c:6737:23: sparse: expression using sizeof(void) fs/btrfs/extent-tree.c:6737:23: sparse: expression using sizeof(void) fs/btrfs/extent-tree.c:6740:31: sparse: expression using sizeof(void) fs/btrfs/extent-tree.c:6740:31: sparse: expression using sizeof(void) fs/btrfs/extent-tree.c:6782:42: sparse: expression using sizeof(void) fs/btrfs/extent-tree.c:6782:42: sparse: expression using sizeof(void) fs/btrfs/extent-tree.c:7551:24: sparse: expression using sizeof(void) fs/btrfs/extent-tree.c:7551:24: sparse: expression using sizeof(void) fs/btrfs/extent-tree.c:7552:24: sparse: expression using sizeof(void) fs/btrfs/extent-tree.c:7552:24: sparse: expression using sizeof(void) fs/btrfs/extent-tree.c:7716:43: sparse: expression using sizeof(void) fs/btrfs/extent-tree.c:7716:43: sparse: expression using sizeof(void) fs/btrfs/extent-tree.c:8064:37: sparse: expression using sizeof(void) fs/btrfs/extent-tree.c:8064:37: sparse: expression using sizeof(void) fs/btrfs/extent-tree.c:8067:37: sparse: expression using sizeof(void) fs/btrfs/extent-tree.c:8067:37: sparse: expression using sizeof(void) fs/btrfs/extent-tree.c:8586:35: sparse: expression using sizeof(void) fs/btrfs/extent-tree.c:8589:35: sparse: expression using sizeof(void) fs/btrfs/extent-tree.c:8589:35: sparse: expression using sizeof(void) fs/btrfs/extent-tree.c:11035:25: sparse: expression using sizeof(void) fs/btrfs/extent-tree.c:11035:25: sparse: expression using sizeof(void) fs/btrfs/extent-tree.c:11036:23: sparse: expression using sizeof(void) fs/btrfs/extent-tree.c:11036:23: sparse: expression using sizeof(void) fs/btrfs/extent-tree.c:2558:20: sparse: context imbalance in 'cleanup_extent_op' - unexpected unlock fs/btrfs/extent-tree.c:2590:28: sparse: context imbalance in 'cleanup_ref_head' - unexpected unlock fs/btrfs/extent-tree.c:2702:26: sparse: context imbalance in '__btrfs_run_delayed_refs' - different lock contexts for basic block fs/btrfs/extent-tree.c:7418:39: sparse: context imbalance in 'btrfs_lock_cluster' - wrong count at exit fs/btrfs/extent-tree.c:7695:44: sparse: context imbalance in 'find_free_extent' - unexpected unlock fs/btrfs/extent-tree.c:9830:9: sparse: context imbalance in 'btrfs_put_block_group_cache' - wrong count at exit fs/btrfs/extent-tree.c: In function 'check_delayed_ref': fs/btrfs/extent-tree.c:3148:14: error: passing argument 1 of 'atomic_inc' from incompatible pointer type [-Werror=incompatible-pointer-types] atomic_inc(&cur_trans->use_count); ^ In file included from arch/x86/include/asm/atomic.h:283:0, from include/linux/atomic.h:5, from include/linux/rcupdate.h:38, from include/linux/rculist.h:11, from include/linux/pid.h:5, from include/linux/sched.h:14, from fs/btrfs/extent-tree.c:6: include/asm-generic/atomic-instrumented.h:100:29: note: expected 'atomic_t * {aka struct <anonymous> *}' but argument is of type 'refcount_t * {aka struct refcount_struct *}' static __always_inline void atomic_inc(atomic_t *v) ^~~~~~~~~~ cc1: some warnings being treated as errors vim +3148 fs/btrfs/extent-tree.c 3132 3133 static noinline int check_delayed_ref(struct btrfs_root *root, 3134 struct btrfs_path *path, 3135 u64 objectid, u64 offset, u64 bytenr) 3136 { 3137 struct btrfs_delayed_ref_head *head; 3138 struct btrfs_delayed_ref_node *ref; 3139 struct btrfs_delayed_data_ref *data_ref; 3140 struct btrfs_delayed_ref_root *delayed_refs; 3141 struct btrfs_transaction *cur_trans; 3142 struct rb_node *node; 3143 int ret = 0; 3144 3145 spin_lock(&root->fs_info->trans_lock); 3146 cur_trans = root->fs_info->running_transaction; 3147 if (cur_trans) > 3148 atomic_inc(&cur_trans->use_count); 3149 spin_unlock(&root->fs_info->trans_lock); 3150 if (!cur_trans) 3151 return 0; 3152 3153 delayed_refs = &cur_trans->delayed_refs; 3154 spin_lock(&delayed_refs->lock); 3155 head = btrfs_find_delayed_ref_head(delayed_refs, bytenr); 3156 if (!head) { 3157 spin_unlock(&delayed_refs->lock); 3158 btrfs_put_transaction(cur_trans); 3159 return 0; 3160 } 3161 3162 if (!mutex_trylock(&head->mutex)) { 3163 refcount_inc(&head->refs); 3164 spin_unlock(&delayed_refs->lock); 3165 3166 btrfs_release_path(path); 3167 3168 /* 3169 * Mutex was contended, block until it's released and let 3170 * caller try again 3171 */ 3172 mutex_lock(&head->mutex); 3173 mutex_unlock(&head->mutex); 3174 btrfs_put_delayed_ref_head(head); 3175 btrfs_put_transaction(cur_trans); 3176 return -EAGAIN; 3177 } 3178 spin_unlock(&delayed_refs->lock); 3179 3180 spin_lock(&head->lock); 3181 /* 3182 * XXX: We should replace this with a proper search function in the 3183 * future. 3184 */ 3185 for (node = rb_first(&head->ref_tree); node; node = rb_next(node)) { 3186 ref = rb_entry(node, struct btrfs_delayed_ref_node, ref_node); 3187 /* If it's a shared ref we know a cross reference exists */ 3188 if (ref->type != BTRFS_EXTENT_DATA_REF_KEY) { 3189 ret = 1; 3190 break; 3191 } 3192 3193 data_ref = btrfs_delayed_node_to_data_ref(ref); 3194 3195 /* 3196 * If our ref doesn't match the one we're currently looking at 3197 * then we have a cross reference. 3198 */ 3199 if (data_ref->root != root->root_key.objectid || 3200 data_ref->objectid != objectid || 3201 data_ref->offset != offset) { 3202 ret = 1; 3203 break; 3204 } 3205 } 3206 spin_unlock(&head->lock); 3207 mutex_unlock(&head->mutex); 3208 btrfs_put_transaction(cur_trans); 3209 return ret; 3210 } 3211 --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c index c1618ab..c8fd37b 100644 --- a/fs/btrfs/extent-tree.c +++ b/fs/btrfs/extent-tree.c @@ -3149,7 +3149,11 @@ static noinline int check_delayed_ref(struct btrfs_root *root, struct rb_node *node; int ret = 0; + spin_lock(&root->fs_info->trans_lock); cur_trans = root->fs_info->running_transaction; + if (cur_trans) + atomic_inc(&cur_trans->use_count); + spin_unlock(&root->fs_info->trans_lock); if (!cur_trans) return 0; @@ -3158,6 +3162,7 @@ static noinline int check_delayed_ref(struct btrfs_root *root, head = btrfs_find_delayed_ref_head(delayed_refs, bytenr); if (!head) { spin_unlock(&delayed_refs->lock); + btrfs_put_transaction(cur_trans); return 0; } @@ -3174,6 +3179,7 @@ static noinline int check_delayed_ref(struct btrfs_root *root, mutex_lock(&head->mutex); mutex_unlock(&head->mutex); btrfs_put_delayed_ref_head(head); + btrfs_put_transaction(cur_trans); return -EAGAIN; } spin_unlock(&delayed_refs->lock); @@ -3206,6 +3212,7 @@ static noinline int check_delayed_ref(struct btrfs_root *root, } spin_unlock(&head->lock); mutex_unlock(&head->mutex); + btrfs_put_transaction(cur_trans); return ret; }
In preivous patch: Btrfs: kill trans in run_delalloc_nocow and btrfs_cross_ref_exist We avoid starting btrfs transaction and get this information from fs_info->running_transaction directly. When accessing running_transaction in check_delayed_ref, there's a chance that current transaction will be freed by commit transaction after the NULL pointer check of running_transaction is passed. After looking all the other places using fs_info->running_transaction, they are either protected by trans_lock or holding the transactions. Fix this by using trans_lock and incrementing the use_count. Signed-off-by: ethanwu <ethanwu@synology.com> --- fs/btrfs/extent-tree.c | 7 +++++++ 1 file changed, 7 insertions(+)