[4/9] btrfs: rework btrfs_space_info_add_old_bytes
diff mbox series

Message ID 20190822191102.13732-5-josef@toxicpanda.com
State New
Headers show
Series
  • Rework reserve ticket handling
Related show

Commit Message

Josef Bacik Aug. 22, 2019, 7:10 p.m. UTC
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.

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@toxicpanda.com>
---
 fs/btrfs/space-info.c | 43 ++++++++++++++++---------------------------
 1 file changed, 16 insertions(+), 27 deletions(-)

Comments

Nikolay Borisov Aug. 23, 2019, 7:48 a.m. UTC | #1
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@toxicpanda.com>
> ---
>  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);
>  }
>  
>
David Sterba Aug. 23, 2019, 12:30 p.m. UTC | #2
On Fri, Aug 23, 2019 at 10:48:35AM +0300, Nikolay Borisov wrote:
> 
> 
> 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:

I got lost there too. It's hard too keep track of what changes,
something a bit more strucutred could help understanding it.

Also the subject is too generic, I'd suggest "update overcommit logic
when refilling tickets" or something like that. Using 'rework' or
'revamp' in the subject applies to very few patches.

Patch
diff mbox series

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);
 }