Re: [PATCH] btrfs: introduce BTRFS_RESERVE_FLUSH_EMERGENCY

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

On 2022/9/12 19:31, Qu Wenruo wrote:
>
>
> On 2022/9/12 19:21, Filipe Manana wrote:
>> On Mon, Sep 12, 2022 at 12:02 PM Qu Wenruo <quwenruo.btrfs@gmx.com>
>> wrote:
>>>
>>>
>>>
>>> On 2022/9/12 18:42, Filipe Manana wrote:
>>>> On Mon, Sep 12, 2022 at 11:26 AM Qu Wenruo <quwenruo.btrfs@gmx.com>
>>>> wrote:
>>>>>
>>>>>
>>>>>
>>>>> On 2022/9/12 17:59, Filipe Manana wrote:
>>>>>> On Fri, Sep 09, 2022 at 09:35:01AM -0400, Josef Bacik wrote:
>>>>>>> Inside of FB, as well as some user reports, we've had a consistent
>>>>>>> problem of occasional ENOSPC transaction aborts.  Inside FB we were
>>>>>>> seeing ~100-200 ENOSPC aborts per day in the fleet, which is a
>>>>>>> really
>>>>>>> low occurrence rate given the size of our fleet, but it's not
>>>>>>> nothing.
>>>>>>>
>>>>>>> There are two causes of this particular problem.
>>>>>>>
>>>>>>> First is delayed allocation.  The reservation system for delalloc
>>>>>>> assumes that contiguous dirty ranges will result in 1 file extent
>>>>>>> item.
>>>>>>> However if there is memory pressure that results in fragmented
>>>>>>> writeout,
>>>>>>> or there is fragmentation in the block groups, this won't
>>>>>>> necessarily be
>>>>>>> true.  Consider the case where we do a single 256MiB write to a
>>>>>>> file and
>>>>>>> then close it.  We will have 1 reservation for the inode update, the
>>>>>>> reservations for the checksum updates, and 1 reservation for the
>>>>>>> file
>>>>>>> extent item.  At some point later we decide to write this entire
>>>>>>> range
>>>>>>> out, but we're so fragmented that we break this into 100
>>>>>>> different file
>>>>>>> extents.  Since we've already closed the file and are no longer
>>>>>>> writing
>>>>>>> to it there's nothing to trigger a refill of the delalloc block
>>>>>>> rsv to
>>>>>>> satisfy the 99 new file extent reservations we need.  At this
>>>>>>> point we
>>>>>>> exhaust our delalloc reservation, and we begin to steal from the
>>>>>>> global
>>>>>>> reserve.  If you have enough of these cases going in parallel you
>>>>>>> can
>>>>>>> easily exhaust the global reserve, get an ENOSPC at
>>>>>>> btrfs_alloc_tree_block() time, and then abort the transaction.
>>>>>>
>>>>>> There's also another problem I pointed out in the past regarding
>>>>>> delalloc
>>>>>> reservations. The thing is that we assume for each ordered extent,
>>>>>> the new
>>>>>> file extent item will require changing only 1 leaf in the
>>>>>> subvolume tree.
>>>>>>
>>>>>> If our new extent has a size of 128M and currently for that range we
>>>>>> have 32768 extents each with a size of 4K, then we need to touch 157
>>>>>> leaves in order to drop those file extent items before inserting
>>>>>> the new
>>>>>> one at ordered extent completion. And our reservation that
>>>>>> happened at
>>>>>> buffered write time only accounted for 1 leaf/path for file extent
>>>>>> items
>>>>>> (plus 1 for the inode item). This is assuming the default leaf
>>>>>> size of 16K,
>>>>>> where we can have at most 208 file extent items per leaf.
>>>>>>
>>>>>> If we have just a single ordered extent triggering this case we
>>>>>> probably
>>>>>> won't reach -ENOSPC and a transaction abort, as we end up
>>>>>> consuming from
>>>>>> the global reserve and that may be able to satisfy our space needs.
>>>>>> However with multiple ordered extents hitting such cases of
>>>>>> insufficient
>>>>>> reserved space, and other tasks doing other things and consuming
>>>>>> from the
>>>>>> global reserve, then the chances of hitting -ENOSPC at
>>>>>> btrfs_finish_ordered_io()
>>>>>> become very high, leading to a transaction abort there.
>>>>>
>>>>> In fact, I'm also considering these problems, and for the highly
>>>>> fragmented free space case (and other cases like delayed refs
>>>>> generated
>>>>> delayed refs case), can we let btrfs itself to learn from last
>>>>> (several)
>>>>> delalloc (delayed refs) runs to generate an overall idea on how
>>>>> fragmented the free space is.
>>>>
>>>> That won't help for the case I described, because it has nothing to
>>>> do with
>>>> fragmentation.
>>>
>>> Isn't it?
>>
>> It isn't.
>>
>>>
>>> We have a 128MiB range dirtied, and we only reserve enough metadata for
>>> one ordered extent since at write/dirty time they are continous.
>>>
>>> But then we only have a bunch of 4K fragmented free extents, every time
>>> btrfs_reserve_extent() can only return a 4K extent for data.
>>>
>>> Aren't the two cases the same?
>>
>> Nop. That's the case Josef mentioned.
>>
>> In what I mentioned we have 1 ordered extent, say 128M for example.
>> So there were no issues with free space fragmentation.
>>
>> What I was saying, and I believe it was explicit enough, is that for
>> that file
>> range we have 32K 4K file extent items, so at finish ordered io, we
>> have to
>> drop all those file extent items before inserting the new one (the
>> 128M one).
>> And 32K spans much more than 1 leaf (path) - 157/158 leaves in case of
>> a 16K leaf/node size.
>
> OK, I see the problem now, it's in the drop time, not the reserve time,
> which I completely ignored previously...

But wait for a min, although this is indeed a problem for our current
btrfs_drop_extents(), it's less serious than the one mentioned by Josef
at reserve time.

The net result (for that subvolume tree at least) is, we should have
much less tree blocks needed for the subvolume.

Thus if we enhance btrfs_drop_extents() to locate the first leave and
the last leave, we can drop all the file extents extents between the two
leaves, without always doing the COW for 100+ leaves.

Although the delayed refs will be another huge problem...
We can only hope all these 32K extents are near each other, or we're
doomed again...

Thanks,
Qu
>
> Thanks,
> Qu
>
>>
>> See the problem?
>> At dealloc time we assume that to insert/replace the new extent item,
>> we will touch
>> only 1 leaf - only one path.
>>
>> This of course is not a problem if we are writing into a previously
>> unallocated file
>> range.
>>
>>>
>>> Thanks,
>>> Qu
>>>
>>>>
>>>> There's also other case where more than 1 ordered extent is created,
>>>> but
>>>> at reservation time we think it's only 1 ordered extent - compression,
>>>> it will split
>>>> into many 128K ordered extents. Again, this one has nothing do with
>>>> fragmentation.
>>>>
>>>>>
>>>>> Then round (down) that value for write/dirty time delalloc
>>>>> reservation?
>>>>>
>>>>> The same can go to delayed refs reservation too.
>>>>>
>>>>> If by somehow, the delayed refs are generating more than expected
>>>>> delayed refs, we increase the pre-reserved space accordingly.
>>>>>
>>>>>
>>>>>
>>>>> Another less dynamic method (but still the same idea) is to
>>>>> introduce a
>>>>> doomsday mode.
>>>>>
>>>>> In that mode, every time we need to reserve space for delalloc, we
>>>>> consider the extent can only be at most 4K, double the pre-reserve for
>>>>> delayed refs etc.
>>>>>
>>>>> But I'm afraid this can only reduce, but not really eliminate the
>>>>> ENOSPC
>>>>> at critical path.
>>>>>
>>>>>>
>>>>>>>
>>>>>>> The other case is the delayed refs reserve.  The delayed refs
>>>>>>> reserve
>>>>>>> updates its size based on outstanding delayed refs and dirty block
>>>>>>> groups.  However we only refill this block reserve when returning
>>>>>>> excess reservations and when we call
>>>>>>> btrfs_start_transaction(root, X).
>>>>>>> We will reserve 2*X credits at transaction start time, and fill in X
>>>>>>> into the delayed refs reserve to make sure it stays topped off.
>>>>>>> Generally this works well, but clearly has downsides.  If we do a
>>>>>>> particularly delayed ref heavy operation we may never catch up in
>>>>>>> our
>>>>>>> reservations.  Additionally running delayed refs generates more
>>>>>>> delayed
>>>>>>> refs, and at that point we may be committing the transaction and
>>>>>>> have no
>>>>>>> way to trigger a refill of our delayed refs rsv.  Then a similar
>>>>>>> thing
>>>>>>> occurs with the delalloc reserve.
>>>>>>>
>>>>>>> Generally speaking we well over-reserve in all of our block
>>>>>>> rsvs.  If we
>>>>>>> reserve 1 credit we're usually reserving around 264k of space,
>>>>>>> but we'll
>>>>>>> often not use any of that reservation, or use a few blocks of that
>>>>>>> reservation.  We can be reasonably sure that as long as you were
>>>>>>> able to
>>>>>>> reserve space up front for your operation you'll be able to find
>>>>>>> space
>>>>>>> on disk for that reservation.
>>>>>>
>>>>>> That's another elephant in the room. We assume that if a task
>>>>>> reserves
>>>>>> space, it will be able to allocate that space later.
>>>>>>
>>>>>> There are several things that can happen which will result in not
>>>>>> being
>>>>>> able to allocate space we reserved before:
>>>>>>
>>>>>> 1) Discard/fitrim - it removes a free space entry, does the
>>>>>> discard, and
>>>>>>       after that it adds back the free space entry. If all the
>>>>>> available space
>>>>>>       is in such an entry being discarded, the task fails to
>>>>>> allocate space;
>>>>>>
>>>>>> 2) fsync - it joins a transaction, doesn't reserve space and
>>>>>> starts allocating
>>>>>>       space for tree blocks, without ever reserving space (because
>>>>>> we want it
>>>>>>       to be fast and for most cases we don't know in advance, or
>>>>>> can estimate,
>>>>>>       how many tree blocks we will need to allocate). So it can
>>>>>> steal space that
>>>>>>       was reserved by some other task;
>>>>>>
>>>>>> 3) Scrub - scrub temporarily turns a block group into RO mode - if
>>>>>> all the
>>>>>>       available space was in that block group, than when the task
>>>>>> tries to
>>>>>>       allocate it will fail because the block group is now RO;
>>>>>
>>>>> Yes, that's one concern of the existing per-dev scrub.
>>>>> If we have 10 devices, all single profiles, then we may mark 10 block
>>>>> groups RO at once, hugely increase the chance of ENOSPC.
>>>>>
>>>>>>
>>>>>> 4) During space reservation we only check if free space counters.
>>>>>> There
>>>>>>       may be block groups with plenty of free space but their
>>>>>> profile is not
>>>>>>       compatible, so when trying to allocate an extent we are
>>>>>> forced to allocate
>>>>>>       a new block group with the necessary profile, which will
>>>>>> fail if there's
>>>>>>       not enough unallocated space.
>>>>>>       This mostly affects degraded mode only (hopefully).
>>>>>
>>>>> This is still pretty common, for RAID1 usage especially one device is
>>>>> just slightly (like 10MiB) larger than the other.
>>>>>
>>>>> The metadata over-reserve still believe we can allocate new
>>>>> metadata bg,
>>>>> but nope, and hit a ENOSPC at critical path.
>>>>>
>>>>> IIRC I had some very old patches for the problem, but haven't updated
>>>>> for a long time.
>>>>>
>>>>> Thanks,
>>>>> Qu
>>>>>
>>>>>>
>>>>>> I documented these at btrfs_chunk_alloc() sometime ago, but there
>>>>>> are a few
>>>>>> more similar cases.
>>>>>>
>>>>>>>
>>>>>>> So introduce a new flushing state,
>>>>>>> BTRFS_RESERVE_FLUSH_EMERGENCY.  This
>>>>>>> gets used in the case that we've exhausted our reserve and the
>>>>>>> global
>>>>>>> reserve.  It simply forces a reservation if we have enough actual
>>>>>>> space
>>>>>>> on disk to make the reservation, which is almost always the
>>>>>>> case.  This
>>>>>>> keeps us from hitting ENOSPC aborts in these odd occurrences
>>>>>>> where we've
>>>>>>> not kept up with the delayed work.
>>>>>>>
>>>>>>> Fixing this in a complete way is going to be relatively
>>>>>>> complicated and
>>>>>>> time consuming.  This patch is what I discussed with Filipe
>>>>>>> earlier this
>>>>>>> year, and what I put into our kernels inside FB.  With this patch
>>>>>>> we're
>>>>>>> down to 1-2 ENOSPC aborts per week, which is a significant
>>>>>>> reduction.
>>>>>>> This is a decent stop gap until we can work out a more wholistic
>>>>>>> solution to these two corner cases.
>>>>>>
>>>>>> Well, it's a lot more than 2 corner cases :)
>>>>>>
>>>>>> The change looks fine to me, it's simple and it should help reduce
>>>>>> the
>>>>>> frequency of several ENOSPC cases. So,
>>>>>>
>>>>>> Reviewed-by: Filipe Manana <fdmanana@suse.com>
>>>>>>
>>>>>> Thanks.
>>>>>>
>>>>>>>
>>>>>>> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
>>>>>>> ---
>>>>>>>     fs/btrfs/block-rsv.c  | 12 ++++++++++++
>>>>>>>     fs/btrfs/ctree.h      | 18 ++++++++++++++++++
>>>>>>>     fs/btrfs/space-info.c | 27 ++++++++++++++++++++++++++-
>>>>>>>     3 files changed, 56 insertions(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/fs/btrfs/block-rsv.c b/fs/btrfs/block-rsv.c
>>>>>>> index ec96285357e0..89e3e7d1bff6 100644
>>>>>>> --- a/fs/btrfs/block-rsv.c
>>>>>>> +++ b/fs/btrfs/block-rsv.c
>>>>>>> @@ -552,5 +552,17 @@ struct btrfs_block_rsv
>>>>>>> *btrfs_use_block_rsv(struct btrfs_trans_handle *trans,
>>>>>>>                if (!ret)
>>>>>>>                        return global_rsv;
>>>>>>>        }
>>>>>>> +
>>>>>>> +    /*
>>>>>>> +     * All hope is lost, but of course our reservations are overly
>>>>>>> +     * pessimistic, so instead of possibly having an ENOSPC
>>>>>>> abort here, try
>>>>>>> +     * one last time to force a reservation if there's enough
>>>>>>> actual space
>>>>>>> +     * on disk to make the reservation.
>>>>>>> +     */
>>>>>>> +    ret = btrfs_reserve_metadata_bytes(fs_info, block_rsv,
>>>>>>> blocksize,
>>>>>>> +
>>>>>>> BTRFS_RESERVE_FLUSH_EMERGENCY);
>>>>>>> +    if (!ret)
>>>>>>> +            return block_rsv;
>>>>>>> +
>>>>>>>        return ERR_PTR(ret);
>>>>>>>     }
>>>>>>> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
>>>>>>> index 0108585d838d..f221b3cb718d 100644
>>>>>>> --- a/fs/btrfs/ctree.h
>>>>>>> +++ b/fs/btrfs/ctree.h
>>>>>>> @@ -2986,6 +2986,24 @@ enum btrfs_reserve_flush_enum {
>>>>>>>         * Can be interrupted by a fatal signal.
>>>>>>>         */
>>>>>>>        BTRFS_RESERVE_FLUSH_ALL_STEAL,
>>>>>>> +
>>>>>>> +    /*
>>>>>>> +     * This is for btrfs_use_block_rsv only.  We have exhausted
>>>>>>> our block
>>>>>>> +     * rsv and our global block rsv.  This can happen for things
>>>>>>> like
>>>>>>> +     * delalloc where we are overwriting a lot of extents with a
>>>>>>> single
>>>>>>> +     * extent and didn't reserve enough space.  Alternatively it
>>>>>>> can happen
>>>>>>> +     * with delalloc where we reserve 1 extents worth for a
>>>>>>> large extent but
>>>>>>> +     * fragmentation leads to multiple extents being created.
>>>>>>> This will
>>>>>>> +     * give us the reservation in the case of
>>>>>>> +     *
>>>>>>> +     * if (num_bytes < (space_info->total_bytes -
>>>>>>> +     *                  btrfs_space_info_used(space_info, false))
>>>>>>> +     *
>>>>>>> +     * Which ignores bytes_may_use.  This is potentially
>>>>>>> dangerous, but our
>>>>>>> +     * reservation system is generally pessimistic so is able to
>>>>>>> absorb this
>>>>>>> +     * style of mistake.
>>>>>>> +     */
>>>>>>> +    BTRFS_RESERVE_FLUSH_EMERGENCY,
>>>>>>>     };
>>>>>>>
>>>>>>>     enum btrfs_flush_state {
>>>>>>> diff --git a/fs/btrfs/space-info.c b/fs/btrfs/space-info.c
>>>>>>> index 2e06b7c422c7..a0abc6dd01c2 100644
>>>>>>> --- a/fs/btrfs/space-info.c
>>>>>>> +++ b/fs/btrfs/space-info.c
>>>>>>> @@ -1583,6 +1583,16 @@ static inline bool can_steal(enum
>>>>>>> btrfs_reserve_flush_enum flush)
>>>>>>>                flush == BTRFS_RESERVE_FLUSH_EVICT);
>>>>>>>     }
>>>>>>>
>>>>>>> +/*
>>>>>>> + * NO_FLUSH and FLUSH_EMERGENCY don't want to create a ticket,
>>>>>>> they just want to
>>>>>>> + * fail as quickly as possible.
>>>>>>> + */
>>>>>>> +static inline bool can_ticket(enum btrfs_reserve_flush_enum flush)
>>>>>>> +{
>>>>>>> +    return (flush != BTRFS_RESERVE_NO_FLUSH &&
>>>>>>> +            flush != BTRFS_RESERVE_FLUSH_EMERGENCY);
>>>>>>> +}
>>>>>>> +
>>>>>>>     /**
>>>>>>>      * Try to reserve bytes from the block_rsv's space
>>>>>>>      *
>>>>>>> @@ -1644,6 +1654,21 @@ static int __reserve_bytes(struct
>>>>>>> btrfs_fs_info *fs_info,
>>>>>>>                ret = 0;
>>>>>>>        }
>>>>>>>
>>>>>>> +    /*
>>>>>>> +     * Things are dire, we need to make a reservation so we
>>>>>>> don't abort.  We
>>>>>>> +     * will let this reservation go through as long as we have
>>>>>>> actual space
>>>>>>> +     * left to allocate for the block.
>>>>>>> +     */
>>>>>>> +    if (ret && unlikely(flush == BTRFS_RESERVE_FLUSH_EMERGENCY)) {
>>>>>>
>>>>>> Does the unlikely() really makes any difference in this context?
>>>>>>
>>>>>>> +            used = btrfs_space_info_used(space_info, false);
>>>>>>> +            if (used + orig_bytes <=
>>>>>>> +                writable_total_bytes(fs_info, space_info)) {
>>>>>>> +
>>>>>>> btrfs_space_info_update_bytes_may_use(fs_info, space_info,
>>>>>>> +
>>>>>>> orig_bytes);
>>>>>>> +                    ret = 0;
>>>>>>> +            }
>>>>>>> +    }
>>>>>>> +
>>>>>>>        /*
>>>>>>>         * If we couldn't make a reservation then setup our
>>>>>>> reservation ticket
>>>>>>>         * and kick the async worker if it's not already running.
>>>>>>> @@ -1651,7 +1676,7 @@ static int __reserve_bytes(struct
>>>>>>> btrfs_fs_info *fs_info,
>>>>>>>         * If we are a priority flusher then we just need to add
>>>>>>> our ticket to
>>>>>>>         * the list and we will do our own flushing further down.
>>>>>>>         */
>>>>>>> -    if (ret && flush != BTRFS_RESERVE_NO_FLUSH) {
>>>>>>> +    if (ret && can_ticket(flush)) {
>>>>>>>                ticket.bytes = orig_bytes;
>>>>>>>                ticket.error = 0;
>>>>>>>                space_info->reclaim_size += ticket.bytes;
>>>>>>> --
>>>>>>> 2.26.3
>>>>>>>