Re: [PATCH 1/4] btrfs: make BTRFS_RESERVE_FLUSH_EVICT use the global rsv stealing code
From: Nikolay Borisov <hidden>
Date: 2021-11-04 12:52:17
On 28.10.21 г. 22:50, Josef Bacik wrote:
I forgot to convert this over when I introduced the global reserve stealing code to the space flushing code. Evict was simply trying to make its reservation and then if it failed it would steal from the global rsv, which is racey because it's outside of the normal ticketing code. Fix this by setting ticket->steal if we are BTRFS_RESERVE_FLUSH_EVICT, and then make the priority flushing path do the steal for us. Signed-off-by: Josef Bacik <josef@toxicpanda.com> --- fs/btrfs/inode.c | 15 ++++++--------- fs/btrfs/space-info.c | 24 +++++++++++++++++++++++- 2 files changed, 29 insertions(+), 10 deletions(-)
<snip>
quoted hunk ↗ jump to hunk
diff --git a/fs/btrfs/space-info.c b/fs/btrfs/space-info.c index 48d77f360a24..c548d34aed28 100644 --- a/fs/btrfs/space-info.c +++ b/fs/btrfs/space-info.c@@ -1380,6 +1380,22 @@ static int handle_reserve_ticket(struct btrfs_fs_info *fs_info, spin_lock(&space_info->lock); ret = ticket->error; + + /* + * If we can steal from the block rsv and we don't have an error and we + * didn't make our reservation then go ahead and try to steal our + * reservation. + */ + if (ticket->steal && !ret && ticket->bytes) { + /* + * If we succeed we need to run btrfs_try_granting_tickets() for + * the same reason as described below. + */ + if (steal_from_global_rsv(fs_info, space_info, ticket)) + btrfs_try_granting_tickets(fs_info, space_info); + }
Instead of having this code here, wouldn't it be better if it's moved in priority_reclaim_metadata_space since this behavior is really part of the flushing behavior, similarly to how maybe_fail_all_tickets is called from the ordinary reclaim path? Actually reading the comment about calling btrfs_try_granting_tickets makes me think that the !list_empty() condition should also be moved in priority_reclaim_metadata_space as those behaviors only pertain to priority tickets, since ordinary ticket will either have been satisfied or failed due to maybe_fail_all_tickets always removing ordinary tickets from the ticket list hence the list_empty() will always be true for them. If I have to be even more nit-picky the priority flushing path is priority_reclaim_metadata_space and not handle_reserve_ticket itself. So the code is slightly contradicting the last sentence in the changelog :)
quoted hunk ↗ jump to hunk
+ + if (ticket->bytes || ticket->error) { /* * We were a priority ticket, so we need to delete ourselves@@ -1438,6 +1454,12 @@ static inline void maybe_clamp_preempt(struct btrfs_fs_info *fs_info, space_info->clamp = min(space_info->clamp + 1, 8); } +static inline bool is_steal_flush_state(enum btrfs_reserve_flush_enum flush)
nint: A better name is should_stable or can_steal
+{
+ return (flush == BTRFS_RESERVE_FLUSH_ALL_STEAL ||
+ flush == BTRFS_RESERVE_FLUSH_EVICT);
+}<snip>