Re: [PATCH 4/8] btrfs: cleanup btrfs_discard_update_discardable usage

[Filipe Manana <fdmanana@gmail.com>]

On Wed, Nov 4, 2020 at 6:21 PM Josef Bacik <josef@toxicpanda.com> wrote:
>
> On 11/4/20 10:54 AM, Filipe Manana wrote:
> > On Fri, Oct 23, 2020 at 5:12 PM Josef Bacik <josef@toxicpanda.com> wrote:
> >>
> >> This passes in the block_group and the free_space_ctl, but we can get
> >> this from the block group itself.  Part of this is because we call it
> >> from __load_free_space_cache, which can be called for the inode cache as
> >> well.  Move that call into the block group specific load section, wrap
> >> it in the right lock that we need, and fix up the arguments to only take
> >> the block group.  Add a lockdep_assert as well for good measure to make
> >> sure we don't mess up the locking again.
> >
> > So this is actually 2 different things in one patch:
> >
> > 1) A cleanup to remove an unnecessary argument to
> > btrfs_discard_update_discardable();
> >
> > 2) A bug because btrfs_discard_update_discardable() is not being
> > called with the lock ->tree_lock held in one specific context.
>
> Yeah but the specific context is on load, so we won't have concurrent modifiers
> to the tree until _after_ the cache is successfully loaded.  Of course this
> patchset changes that so it's important now, but prior to this we didn't
> necessarily need the lock, so it's not really a bug fix, just an adjustment.
>
> However I'm always happy to inflate my patch counts, makes me look good at
> performance review time ;).  I'm happy to respin with it broken out.  Thanks,

Then make it 3! One more just to add the assertion!

I'm fine with it as it is, maybe just add an explicit note that we
can't have concurrent access in the load case, so it's not fixing any
bug, but just prevention.

Reviewed-by: Filipe Manana <fdmanana@suse.com>

Thanks.

>
> Josef



-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”