On Wed, Jul 17, 2013 at 03:09:13PM -0700, Andrew Morton wrote:
> On Tue, 16 Jul 2013 04:43:41 +0800 Shaohua Li <shli@xxxxxxxxxx> wrote:
>
> > swap can do cluster discard for SSD, which is good, but there are some problems
> > here:
> > 1. swap do the discard just before page reclaim gets a swap entry and writes
> > the disk sectors. This is useless for high end SSD, because an overwrite to a
> > sector implies a discard to original sector too. A discard + overwrite ==
> > overwrite.
> > 2. the purpose of doing discard is to improve SSD firmware garbage collection.
> > Idealy we should send discard as early as possible, so firmware can do
> > something smart. Sending discard just after swap entry is freed is considered
> > early compared to sending discard before write. Of course, if workload is
> > already bound to gc speed, sending discard earlier or later doesn't make
> > difference.
> > 3. block discard is a sync API, which will delay scan_swap_map() significantly.
> > 4. Write and discard command can be executed parallel in PCIe SSD. Making
> > swap discard async can make execution more efficiently.
> >
> > This patch makes swap discard async and move discard to where swap entry is
> > freed. Discard and write have no dependence now, so above issues can be avoided.
> > Idealy we should do discard for any freed sectors, but some SSD discard is very
> > slow. This patch still does discard for a whole cluster.
> >
> > My test does a several round of 'mmap, write, unmap', which will trigger a lot
> > of swap discard. In a fusionio card, with this patch, the test runtime is
> > reduced to 18% of the time without it, so around 5.5x faster.
> >
> > ...
> >
> > +static void swap_do_scheduled_discard(struct swap_info_struct *si)
> > +{
> > + struct swap_cluster_info *info;
> > + unsigned int idx;
> > +
> > + info = si->cluster_info;
> > +
> > + while (!cluster_is_null(&si->discard_cluster_head)) {
> > + idx = cluster_next(&si->discard_cluster_head);
> > +
> > + cluster_set_next_flag(&si->discard_cluster_head,
> > + cluster_next(&info[idx]), 0);
> > + if (cluster_next(&si->discard_cluster_tail) == idx) {
> > + cluster_set_null(&si->discard_cluster_head);
> > + cluster_set_null(&si->discard_cluster_tail);
> > + }
> > + spin_unlock(&si->lock);
> > +
> > + discard_swap_cluster(si, idx * SWAPFILE_CLUSTER,
> > + SWAPFILE_CLUSTER);
> > +
> > + spin_lock(&si->lock);
> > + cluster_set_flag(&info[idx], CLUSTER_FLAG_FREE);
>
> Wait. How can we do this? We dropped the spinlock, so `idx' is now
> invalid.
idx is the current cluster (an index of array) we are discarding, it's not
invalid after we drop lock.
> > + if (cluster_is_null(&si->free_cluster_head)) {
> > + cluster_set_next_flag(&si->free_cluster_head,
> > + idx, 0);
> > + cluster_set_next_flag(&si->free_cluster_tail,
> > + idx, 0);
> > + } else {
> > + unsigned int next;
> > +
> > + next = cluster_next(&si->free_cluster_tail);
> > + cluster_set_next(&info[next], idx);
> > + cluster_set_next_flag(&si->free_cluster_tail,
> > + idx, 0);
>
> ditto.
>
> > + }
> > + memset(si->swap_map + idx * SWAPFILE_CLUSTER,
> > + 0, SWAPFILE_CLUSTER);
>
> again.
>
> > + }
> > +}
> > +
> >
> > ...
> >
> > @@ -331,19 +414,6 @@ static unsigned long scan_swap_map(struc
> > si->cluster_nr = SWAPFILE_CLUSTER - 1;
> > goto checks;
> > }
> > - if (si->flags & SWP_PAGE_DISCARD) {
> > - /*
> > - * Start range check on racing allocations, in case
> > - * they overlap the cluster we eventually decide on
> > - * (we scan without swap_lock to allow preemption).
> > - * It's hardly conceivable that cluster_nr could be
> > - * wrapped during our scan, but don't depend on it.
> > - */
> > - if (si->lowest_alloc)
> > - goto checks;
> > - si->lowest_alloc = si->max;
> > - si->highest_alloc = 0;
> > - }
> > check_cluster:
> > if (!cluster_is_null(&si->free_cluster_head)) {
> > offset = cluster_next(&si->free_cluster_head) *
> > @@ -351,15 +421,22 @@ check_cluster:
> > last_in_cluster = offset + SWAPFILE_CLUSTER - 1;
> > si->cluster_next = offset;
> > si->cluster_nr = SWAPFILE_CLUSTER - 1;
> > - found_free_cluster = 1;
> > goto checks;
> > } else if (si->cluster_info) {
> > /*
> > + * we don't have free cluster but have some clusters in
> > + * discarding, do discard now and reclaim them
> > + */
> > + if (!cluster_is_null(&si->discard_cluster_head)) {
> > + swap_do_scheduled_discard(si);
> > + goto check_cluster;
>
> Again, swap_do_scheduled_discard() might have dropped the lock. The
> state which scan_swap_map() has copied in from the swap_info_struct is
> now invalidated. `scan_base' and `offset' might have changed.
> si->cluster_nr may have changed.
it doesn't matter actually. With it, we might scan to different position, but
it doesn't break anthing. But I agree this is confusion. I'll move the goto
above.
Thanks,
Shaohua
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>