Re: [PATCH v3 1/2] btrfs: fix deadlock between chunk allocation and chunk btree modifications

[David Sterba <dsterba@suse.cz>]

On Wed, Oct 13, 2021 at 03:21:47PM +0100, Filipe Manana wrote:
> > > @@ -3724,19 +3718,13 @@ void check_system_chunk(struct btrfs_trans_handle *trans, u64 type)
> > >       left = info->total_bytes - btrfs_space_info_used(info, true);
> > >       spin_unlock(&info->lock);
> > >
> > > -     num_devs = get_profile_num_devs(fs_info, type);
> > > -
> > > -     /* num_devs device items to update and 1 chunk item to add or remove */
> > > -     thresh = btrfs_calc_metadata_size(fs_info, num_devs) +
> > > -             btrfs_calc_insert_metadata_size(fs_info, 1);
> > > -
> > > -     if (left < thresh && btrfs_test_opt(fs_info, ENOSPC_DEBUG)) {
> > > +     if (left < bytes && btrfs_test_opt(fs_info, ENOSPC_DEBUG)) {
> > >               btrfs_info(fs_info, "left=%llu, need=%llu, flags=%llu",
> > > -                        left, thresh, type);
> > > +                        left, bytes, type);
> > >               btrfs_dump_space_info(fs_info, info, 0, 0);
> > >       }
> >
> > This can be simplified to if (btrfs_test_opt(fs_info, ENOSPC_DEBUG))
> > and nested inside the next if (left < bytes). I checked
> > and even with the extra nesting the code doesn't break the 76 char limit.
> 
> This is a bug fix only, I'm not reformatting code blocks I'm not
> really changing.

I tend to agree to keep the fix minimal and do unrelated cleanups if it
happens in the scope of the fix. Backporing such patches is easier but I
understand the comment that sometimes it's worth to do the collateral
cleanups. No hard rules here.

> > > +/*
> > > + * Reserve space in the system space for allocating or removing a chunk.
> > > + * The caller must be holding fs_info->chunk_mutex.
> >
> > Better to use lockdep_assert_held.
> 
> reserve_chunk_space() does that, that's why I didn't add it here again.

I'm not sure what's the overhead of lockdep_assert_held but it could be
potentially a perf hit, where we would care even for a debugging build.
If the call chain is not spread over many functions/files I'd say it's
ok to do lockdep_assert only on the entry function and not each in the
call sub tree.