On 13/06/2024 20:26, Yosry Ahmed wrote:
[..]
@@ -498,6 +507,8 @@ static void swap_do_scheduled_discard(struct swap_info_struct *si)
__free_cluster(si, idx);
memset(si->swap_map + idx * SWAPFILE_CLUSTER,
0, SWAPFILE_CLUSTER);
+ for (i = 0; i < SWAPFILE_CLUSTER; i++)
+ clear_bit(idx * SWAPFILE_CLUSTER + i, si->zeromap);
Same here. I didn't look into the specific code paths, but shouldn't the
cluster be unused (and hence its zeromap bits already cleared?).
I think this one is needed (or atleast very good to have). There are 2
paths:
1) swap_cluster_schedule_discard (clears zeromap) -> swap_discard_work
-> swap_do_scheduled_discard (clears zeromap)
Path 1 doesnt need it as swap_cluster_schedule_discard already clears it.
2) scan_swap_map_slots -> scan_swap_map_try_ssd_cluster ->
swap_do_scheduled_discard (clears zeromap)
Path 2 might need it as zeromap isnt cleared earlier I believe
(eventhough I think it might already be 0).
Aren't the clusters in the discard list free by definition? It seems
like we add a cluster there from swap_cluster_schedule_discard(),
which we establish above that it gets called on a free cluster, right?
You mean for path 2? Its not from swap_cluster_schedule_discard. The
whole call path is
get_swap_pages -> scan_swap_map_slots -> scan_swap_map_try_ssd_cluster
-> swap_do_scheduled_discard. Nowhere up until swap_do_scheduled_discard
was the zeromap cleared, which is why I think we should add it here.
swap_do_scheduled_discard() iterates over clusters from
si->discard_clusters. Clusters are added to that list from
swap_cluster_schedule_discard().
IOW, swap_cluster_schedule_discard() schedules freed clusters to be
discarded, and swap_do_scheduled_discard() later does the actual
discarding, whether it's through si->discard_work scheduled by
swap_cluster_schedule_discard(), or when looking for a free cluster
through scan_swap_map_try_ssd_cluster().
Did I miss anything?
Ah ok, and the schedule_discard in free_cluster wont be called scheduled
before swap_range_free. Will only keep the one in swap_range_free. Thanks!
