On 22.08.19 г. 22:10 ч., Josef Bacik wrote:
> If there are pending tickets and we are overcommitted we will simply
> return free'd reservations to space_info->bytes_may_use if we cannot
> overcommit any more. This is problematic because we assume any free
> space would have been added to the tickets, and so if we go from an
> overcommitted state to not overcommitted we could have plenty of space
> in our space_info but be unable to make progress on our tickets because
> we only refill tickets from previous reservations.
>
> Consider the following example. We were allowed to overcommit to
> space_info->total_bytes + 2mib. Now we've allocated all of our chunks
> and are no longer allowed to overcommit those extra 2mib. Assume there
> is a 3mib reservation we are now freeing. Because we can no longer
> overcommit we do not partially refill the ticket with the 3mib, instead
> we subtract it from space_info->bytes_may_use. Now the total reserved
> space is 1mib less than total_bytes, meaning we have 1mib of space we
> could reserve. Now assume that our ticket is 2mib, and we only have
> 1mib of space to reclaim, so we have a partial refilling to 1mib. We
> keep trying to flush and eventually give up and ENOSPC the ticket, when
> there was the remaining 1mib left in the space_info for usage.
The wording of this paragraph makes it a bit hard to understand. How
about something like:
Consider an example where a request is allowed to overcommit
space_info->total_bytes + 2mib. At this point it's no longer possible to
overcommit extra space. At the same time there is an existing 3mib
reservation which is being freed. Due to the existing check failing:
if (check_overcommit &&
!can_overcommit(fs_info, space_info, 0, flush, false))
btrfs_space_info_add_old_bytes won't partially refill tickets with those
3mib, instead it will subtract them from space_info->bytes_may_use. This
results in the total reserved space being 1mib below
space_info->total_bytes. <You need to expand on where the 2mib ticket
came - was it part of the original reservation that caused the
overcommit or is it a new reservation that comes while we are at 1mb
below space_info->total_bytes>
>
> Instead of doing this partial filling of tickets dance we need to simply
> add our space to the space_info, and then do the normal check to see if
> we can satisfy the whole reservation. If we can then we wake up the
> ticket and carry on. This solves the above problem and makes it much
> more straightforward to understand how the tickets are satisfied.
>
> Signed-off-by: Josef Bacik <josef@xxxxxxxxxxxxxx>
> ---
> fs/btrfs/space-info.c | 43 ++++++++++++++++---------------------------
> 1 file changed, 16 insertions(+), 27 deletions(-)
>
> diff --git a/fs/btrfs/space-info.c b/fs/btrfs/space-info.c
> index a0a36d5768e1..357fe7548e07 100644
> --- a/fs/btrfs/space-info.c
> +++ b/fs/btrfs/space-info.c
> @@ -233,52 +233,41 @@ void btrfs_space_info_add_old_bytes(struct btrfs_fs_info *fs_info,
> struct btrfs_space_info *space_info,
> u64 num_bytes)
> {
> - struct reserve_ticket *ticket;
> struct list_head *head;
> - u64 used;
> enum btrfs_reserve_flush_enum flush = BTRFS_RESERVE_NO_FLUSH;
> - bool check_overcommit = false;
>
> spin_lock(&space_info->lock);
> head = &space_info->priority_tickets;
> + btrfs_space_info_update_bytes_may_use(fs_info, space_info, -num_bytes);
>
> - /*
> - * If we are over our limit then we need to check and see if we can
> - * overcommit, and if we can't then we just need to free up our space
> - * and not satisfy any requests.
> - */
> - used = btrfs_space_info_used(space_info, true);
> - if (used - num_bytes >= space_info->total_bytes)
> - check_overcommit = true;
> again:
> - while (!list_empty(head) && num_bytes) {
> - ticket = list_first_entry(head, struct reserve_ticket,
> - list);
> - /*
> - * We use 0 bytes because this space is already reserved, so
> - * adding the ticket space would be a double count.
> - */
> - if (check_overcommit &&
> - !can_overcommit(fs_info, space_info, 0, flush, false))
> - break;
> - if (num_bytes >= ticket->bytes) {
> + while (!list_empty(head)) {
> + struct reserve_ticket *ticket;
> + u64 used = btrfs_space_info_used(space_info, true);
> +
> + ticket = list_first_entry(head, struct reserve_ticket, list);
> +
> + /* Check and see if our ticket can be satisified now. */
> + if ((used + ticket->bytes <= space_info->total_bytes) ||
> + can_overcommit(fs_info, space_info, ticket->bytes, flush,
> + false)) {
> + btrfs_space_info_update_bytes_may_use(fs_info,
> + space_info,
> + ticket->bytes);
> list_del_init(&ticket->list);
> - num_bytes -= ticket->bytes;
> ticket->bytes = 0;
> space_info->tickets_id++;
> wake_up(&ticket->wait);
> } else {
> - ticket->bytes -= num_bytes;
> - num_bytes = 0;
> + break;
> }
> }
>
> - if (num_bytes && head == &space_info->priority_tickets) {
> + if (head == &space_info->priority_tickets) {
> head = &space_info->tickets;
> flush = BTRFS_RESERVE_FLUSH_ALL;
> goto again;
> }
> - btrfs_space_info_update_bytes_may_use(fs_info, space_info, -num_bytes);
> spin_unlock(&space_info->lock);
> }
>
>
