On Wed, Apr 8, 2020 at 6:19 PM David Sterba <dsterba@xxxxxxx> wrote:
>
> On Tue, Apr 07, 2020 at 11:38:49AM +0100, fdmanana@xxxxxxxxxx wrote:
> > From: Filipe Manana <fdmanana@xxxxxxxx>
> >
> > Whenever we add a ticket to a space_info object we increment the object's
> > reclaim_size counter witht the ticket's bytes, and we decrement it with
> > the corresponding amount only when we are able to grant the requested
> > space to the ticket. When we are not able to grant the space to a ticket,
> > or when the ticket is removed due to a signal (e.g. an application has
> > received sigterm from the terminal) we never decrement the counter with
> > the corresponding bytes from the ticket. This leak can result in the
> > space reclaim code to later do much more work than necessary. So fix it
> > by decrementing the counter when those two cases happen as well.
> >
> > Fixes: db161806dc5615 ("btrfs: account ticket size at add/delete time")
> > Signed-off-by: Filipe Manana <fdmanana@xxxxxxxx>
>
> There's a minor conflict with Josef's patch "btrfs: run
> btrfs_try_granting_tickets if a priority ticket fails" so I'll apply
> yours to misc-5.7 as it's a regression fix.
>
> > @@ -1121,7 +1129,7 @@ static int handle_reserve_ticket(struct btrfs_fs_info *fs_info,
> > * either the async reclaim job deletes the ticket from the list
> > * or we delete it ourselves at wait_reserve_ticket().
> > */
> > - list_del_init(&ticket->list);
> > + remove_ticket(space_info, ticket);
> > if (!ret)
> > ret = -ENOSPC;
> > }
>
> The conflicting hunk is:
>
> --- a/fs/btrfs/space-info.c
> +++ b/fs/btrfs/space-info.c
> @@ -1156,11 +1156,17 @@ static int handle_reserve_ticket(struct btrfs_fs_info *fs_info,
> ret = ticket->error;
> if (ticket->bytes || ticket->error) {
> /*
> - * Need to delete here for priority tickets. For regular tickets
> - * either the async reclaim job deletes the ticket from the list
> - * or we delete it ourselves at wait_reserve_ticket().
> + * We were a priority ticket, so we need to delete ourselves
> + * from the list. Because we could have other priority tickets
> + * behind us that require less space, run
> + * btrfs_try_granting_tickets() to see if their reservations can
> + * now be made.
> */
> - list_del_init(&ticket->list);
> + if (!list_empty(&ticket->list)) {
> + list_del_init(&ticket->list);
> + btrfs_try_granting_tickets(fs_info, space_info);
> + }
> +
> if (!ret)
> ret = -ENOSPC;
> }
> ---
>
> so I assume the correct fixup is to replace list_del_init with
> remove_ticket.
Yes, correct.
Thanks.
