Re: [PATCH 1/5] btrfs: fix qgroup reservation leak on failure to allocate ordered extent

[Boris Burkov <boris@bur.io>]

On Thu, May 08, 2025 at 02:40:22PM +0100, Filipe Manana wrote:
> On Wed, May 7, 2025 at 11:33 PM Boris Burkov <boris@bur.io> wrote:
> >
> > On Wed, May 07, 2025 at 06:23:13PM +0100, fdmanana@kernel.org wrote:
> > > From: Filipe Manana <fdmanana@suse.com>
> > >
> > > If we fail to allocate an ordered extent for a COW write we end up leaking
> > > a qgroup data reservation since we called btrfs_qgroup_release_data() but
> > > we didn't call btrfs_qgroup_free_refroot() (which would happen when
> > > running the respective data delayed ref created by ordered extent
> > > completion or when finishing the ordered extent in case an error happened).
> > >
> > > So make sure we call btrfs_qgroup_free_refroot() if we fail to allocate an
> > > ordered extent for a COW write.
> >
> > I haven't tried it myself yet, but I believe that this patch will double
> > free reservation from the qgroup when this case occurs.
> 
> Nop, see below.
> 
> >
> > Can you share the context where you saw this bug? Have you run fstests
> > with qgroups or squotas enabled? I think this should show pretty quickly
> > in generic/475 with qgroups on.
> 
> Yes, I have run fstests. I always do before sending a patch, no matter
> how simple or trivial it is (or seems to be).

For the record, I was not suggesting that you hadn't run fstests, I just
wasn't sure if you ran them with qgroups on for every test as well.

> 
> This isn't a scenario that can be triggered with fstests since there
> are no test cases that inject memory allocation failures on ordered
> extents or anything else.
> generic/475 simulates IO failures with dm error, so I don't see why
> you think that would be relevant when the problem here is on ordered
> extent allocation failure and not IO errors.
> 

Yes, I noticed that later, while discussing with Qu.

> >
> > Consider, for example, the following execution of the dio case:
> >
> > btrfs_dio_iomap_begin
> >   btrfs_check_data_free_space // reserves the data into `reserved`, sets dio_data->data_space_reserved
> >   btrfs_get_blocks_direct_write
> >     btrfs_create_dio_extent
> >       btrfs_alloc_ordered_extent
> >         alloc_ordered_extent // fails and frees refroot, reserved is "wrong" now.
> >       // error propagates up
> >     // error propagates up via PTR_ERR
> >
> > which brings us to the code:
> > if (ret < 0)
> >         goto unlock_err;
> > ...
> > unlock_err:
> > ...
> > if (dio_data->data_space_reserved) {
> >         btrfs_free_reserved_data_space()
> > }
> >
> > so the execution continues...
> >
> > btrfs_free_reserved_data_space
> >   btrfs_qgroup_free_data
> >     __btrfs_qgroup_release_data
> >       qgroup_free_reserved_data
> >         btrfs_qgroup_free_refroot
> >
> > This will result in a underflow of the reservation once everything
> > outstanding gets released.
> 
> No, it won't.
> 
> For a COW write, before we failed to allocate the ordered extent, at
> alloc_ordered_extent(), we called btrfs_qgroup_release_data().
> That function will find all subranges in the inode's iotree marked
> with EXTENT_QGROUP_RESERVED, clear that bit from them and sum their
> lengths into @qgroup_rsv (local variable from alloc_ordered_extent()).
> 
> So calling qgroup_free_reserved_data() in an error path such as that
> one will do nothing because it can't find any more ranges in the
> inode's iotree marked with EXTENT_QGROUP_RESERVED.
> 
> So we leak reserved space... from the moment we called
> btrfs_qgroup_release_data(), at alloc_ordered_extent(), we transferred
> how we track the reserved space - which was intended to be in the
> ordered extent and then when the ordered extent completes a delayed
> data ref is created and when that delayed ref is ran we release the
> space with btrfs_qgroup_free_refroot(). But since we failed to
> allocate the ordered extent and the reserved space is no longer
> tracked in the inode's iotree, we fail to release qgroup space.
> 
> Actually patch 3 in the patchset updates the comments at
> alloc_ordered_extent() with those details to make it clear.
> 
> Hope it's more clear now what's going on and how qgroup tracks reserved space.
> 
> Thanks.
> 
> 
> 
> >
> > Furthermore, raw calls to free_refroot in cases where we have a reserved
> > changeset make me worried, because they will run afoul of races with
> > multiple threads touching the various bits. I don't see the bugs here,
> > but the reservation lifetime is really tricky so I wouldn't be surprised
> > if something like that was wrong too.
> >
> > As of the last time I looked at this, I think cow_file_range handles
> > this correctly as well. Looking really quickly now, it looks like maybe
> > submit_one_async_extent() might not do a qgroup free, but I'm not sure
> > where the corresponding reservation is coming from.
> >
> > I think if you have indeed found a different codepath that makes a data
> > reservation but doesn't release the qgroup part when allocating the
> > ordered extent fails, then the fastest path to a fix is to do that at
> > the same level as where it calls btrfs_check_data_free_space or (however
> > it gets the reservation), as is currently done by the main
> > ordered_extent allocation paths. With async_extent, we might need to
> > plumb through the reserved extent changeset through the async chunk to
> > do it perfectly...
> >
> > Thanks,
> > Boris
> >
> > >
> > > Fixes: 7dbeaad0af7d ("btrfs: change timing for qgroup reserved space for ordered extents to fix reserved space leak")
> > > Signed-off-by: Filipe Manana <fdmanana@suse.com>
> > > ---
> > >  fs/btrfs/ordered-data.c | 12 +++++++++---
> > >  1 file changed, 9 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/fs/btrfs/ordered-data.c b/fs/btrfs/ordered-data.c
> > > index ae49f87b27e8..e44d3dd17caf 100644
> > > --- a/fs/btrfs/ordered-data.c
> > > +++ b/fs/btrfs/ordered-data.c
> > > @@ -153,9 +153,10 @@ static struct btrfs_ordered_extent *alloc_ordered_extent(
> > >       struct btrfs_ordered_extent *entry;
> > >       int ret;
> > >       u64 qgroup_rsv = 0;
> > > +     const bool is_nocow = (flags &
> > > +            ((1U << BTRFS_ORDERED_NOCOW) | (1U << BTRFS_ORDERED_PREALLOC)));
> > >
> > > -     if (flags &
> > > -         ((1U << BTRFS_ORDERED_NOCOW) | (1U << BTRFS_ORDERED_PREALLOC))) {
> > > +     if (is_nocow) {
> > >               /* For nocow write, we can release the qgroup rsv right now */
> > >               ret = btrfs_qgroup_free_data(inode, NULL, file_offset, num_bytes, &qgroup_rsv);
> > >               if (ret < 0)
> > > @@ -170,8 +171,13 @@ static struct btrfs_ordered_extent *alloc_ordered_extent(
> > >                       return ERR_PTR(ret);
> > >       }
> > >       entry = kmem_cache_zalloc(btrfs_ordered_extent_cache, GFP_NOFS);
> > > -     if (!entry)
> > > +     if (!entry) {
> > > +             if (!is_nocow)
> > > +                     btrfs_qgroup_free_refroot(inode->root->fs_info,
> > > +                                               btrfs_root_id(inode->root),
> > > +                                               qgroup_rsv, BTRFS_QGROUP_RSV_DATA);
> > >               return ERR_PTR(-ENOMEM);
> > > +     }
> > >
> > >       entry->file_offset = file_offset;
> > >       entry->num_bytes = num_bytes;
> > > --
> > > 2.47.2
> > >