Re: [PATCH v3 1/5] btrfs: scrub: Introduce full stripe lock for RAID56

[David Sterba <dsterba@suse.cz>]

On Thu, Mar 30, 2017 at 09:03:21AM +0800, Qu Wenruo wrote:
> >> +static int lock_full_stripe(struct btrfs_fs_info *fs_info, u64 bytenr)
> >> +{
> >> +	struct btrfs_block_group_cache *bg_cache;
> >> +	struct btrfs_full_stripe_locks_tree *locks_root;
> >> +	struct full_stripe_lock *existing;
> >> +	u64 fstripe_start;
> >> +	int ret = 0;
> >> +
> >> +	bg_cache = btrfs_lookup_block_group(fs_info, bytenr);
> >> +	if (!bg_cache)
> >> +		return -ENOENT;
> >> +
> >
> > When starting to scrub a chunk, we've already increased a ref for block group,
> > could you please put a ASSERT to catch it?
> 
> Personally I prefer WARN_ON() than ASSERT().
> 
> ASSERT() always panic the modules and forces us to reset the system. 
> Wiping out any possibility to check the system.

I think the sematnics of WARN_ON and ASSERT are different, so it should
be decided case by case which one to use. Assert is good for 'never
happens' or catch errors at development time (wrong API use, invariant
condition that must always match).

Also the asserts are gone if the config option is unset, while WARN_ON
will stay in some form (verbose or not). Both are suitable for catching
problems, but the warning is for less critical errors so we want to know
when it happens but still can continue.

The above case looks like a candidate for ASSERT as the refcounts must
be correct, continuing with the warning could lead to other unspecified
problems.