On Mon, Apr 03, 2017 at 10:13:46AM +0800, Qu Wenruo wrote: > Unlike mirror based profiles, RAID5/6 recovery needs to read out the > whole full stripe. > > And if we don't do proper protect, it can easily cause race condition. > > Introduce 2 new functions: lock_full_stripe() and unlock_full_stripe() > for RAID5/6. > Which stores a rb_tree of mutex for full stripes, so scrub callers can > use them to lock a full stripe to avoid race. > > Signed-off-by: Qu Wenruo <quwen...@cn.fujitsu.com> > Reviewed-by: Liu Bo <bo.li....@oracle.com>
Overall looks good to me now. A few comments below. > +/* > + * Unlock a full stripe. > + * NOTE: Caller must ensure it's the same context calling corresponding > + * lock_full_stripe(). > + * > + * Return 0 if we unlock full stripe without problem. > + * Return <0 for error > + */ > +static int unlock_full_stripe(struct btrfs_fs_info *fs_info, u64 bytenr, > + bool locked) > +{ > + struct btrfs_block_group_cache *bg_cache; > + struct btrfs_full_stripe_locks_tree *locks_root; > + struct full_stripe_lock *fstripe_lock; > + u64 fstripe_start; > + bool freeit = false; > + int ret = 0; > + > + /* If we didn't acquire full stripe lock, no need to continue */ > + if (!locked) > + return 0; > + > + bg_cache = btrfs_lookup_block_group(fs_info, bytenr); > + if (!bg_cache) { > + ASSERT(0); Well, ok. As mentioned in other mails, the asserts will need a systematic change all over the codebase. > + return -ENOENT; > + } > + if (!(bg_cache->flags & BTRFS_BLOCK_GROUP_RAID56_MASK)) > + goto out; > + > + locks_root = &bg_cache->full_stripe_locks_root; > + fstripe_start = get_full_stripe_logical(bg_cache, bytenr); > + if (ret < 0) > + goto out; This 'if' is a no-op, as ret is not modified since initialization. > + > + mutex_lock(&locks_root->lock); > + fstripe_lock = search_full_stripe_lock(locks_root, fstripe_start); > + /* Unpaired unlock_full_stripe() detected */ > + if (!fstripe_lock) { > + WARN_ON(1); > + ret = -ENOENT; > + mutex_unlock(&locks_root->lock); > + goto out; > + } > + > + if (fstripe_lock->refs == 0) { > + WARN_ON(1); > + btrfs_warn(fs_info, "full stripe lock at %llu refcount > underflow", > + fstripe_lock->logical); > + } else { > + fstripe_lock->refs--; > + } > + > + if (fstripe_lock->refs == 0) { > + rb_erase(&fstripe_lock->node, &locks_root->root); > + freeit = true; > + } > + mutex_unlock(&locks_root->lock); > + > + mutex_unlock(&fstripe_lock->mutex); > + if (freeit) > + kfree(fstripe_lock); > +out: > + btrfs_put_block_group(bg_cache); > + return ret; > +} > + > +/* > * used for workers that require transaction commits (i.e., for the > * NOCOW case) > */ -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html