[..] > >>>> @@ -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?
