swap: don't do discard if no discard option added

Even don't add discard option, swapon will do discard, this sounds buggy, especially when discard is slow or buggy.

---
 mm/swapfile.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: linux/mm/swapfile.c
===================================================================
@@ -2105,7 +2105,7 @@ SYSCALL_DEFINE2(swapon, const char __use
 			p->flags |= SWP_SOLIDSTATE;
 			p->cluster_next = 1 + (random32() % p->highest_bit);
 		}
-		if (discard_swap(p) == 0 && (swap_flags & SWAP_FLAG_DISCARD))
+		if ((swap_flags & SWAP_FLAG_DISCARD) && discard_swap(p) == 0)
 			p->flags |= SWP_DISCARDABLE;
 	}
 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Hello,

just tested this patch and this solves my problem where I have very long boot times and even some timeout problems.

Regards,
Holger

That changelog is pretty hard to understand. I rewrote it as below.

From: Shaohua Li <[email protected]>
Subject: swap: don't do discard if no discard option added

When swapon() was not passed the SWAP_FLAG_DISCARD option, sys_swapon() will still perform a discard operation. This can cause problems if discard is slow or buggy.

Reverse the order of the check so that a discard operation is performed only if the sys_swapon() caller is attempting to enable discard.

---

 mm/swapfile.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff -puN mm/swapfile.c~swap-dont-do-discard-if-no-discard-option-added mm/swapfile.c
@@ -2105,7 +2105,7 @@ SYSCALL_DEFINE2(swapon, const char __use
 			p->flags |= SWP_SOLIDSTATE;
 			p->cluster_next = 1 + (random32() % p->highest_bit);
 		}
-		if (discard_swap(p) == 0 && (swap_flags & SWAP_FLAG_DISCARD))
+		if ((swap_flags & SWAP_FLAG_DISCARD) && discard_swap(p) == 0)
 			p->flags |= SWP_DISCARDABLE;
 	}
 

_

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

It's not a bug in swapon, it's an intentional feature, made explicit in commit 339944663273 "swap: discard while swapping only if SWAP_FLAG_DISCARD" and in the swapon(2) manpage. We were also careful in wording the swapon(8) manpage and the comment on SWAP_FLAG_DISCARD in swap.h - too lawyerly ;-?

It appears to be a bug in the Vertex 2: I did receive one other such report on a Vertex 2 fourteen months ago, and in the absence of further reports, we decided to consider that user's drive defective. I wonder if Holger's drive is defective, or if it's true of all Vertex 2s, or if it depends on the firmware revision, and a later revision fixes it.

If the latter (if there is a firmware revision which fixes it), then I think it's clear that SWAP_FLAG_DISCARD should continue to behave as it does at present, with discard at swapon independent of it.

Holger, do you have the latest firmware on this drive? Have any other Vertex 2 users observed this behaviour?

I've seen no such problem with the original OCZ Vertex, nor with their Vertex 3, nor with the Intel drives I've tried (and you report no problem with FusionIO's, though no advantage either).

But if there's no good firmware for the Vertex 2, I'm not so sure what to do: two reports in fourteen months, on a superseded drive - is that strong enough to disable a feature which appeared to offer some advantage on others?

Is there a lower level at which we could blacklist the Vertex 2 to disable driver support for its discard?

Hugh

2012/3/21 Hugh Dickins <[email protected]>:

Holger uses raid for swap. We currently didn't do discard request merge as low SCSI driver doesn't allow. So for the raid0 case, we will split big discard request to chunk size, which is 512k. This will increase discard request number. This can be fixed later. But on the other hand, if user doesn't explictly enable discard, why enable it? Like fs, we didn't do runtime discard and only run trim occasionally since discard is slow.

Thanks,
Shaohua

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

This. Neither the swapon manpage nor the SWAP_FLAG_DISCARD comment nor .c code comments nor the 339944663 changelog explain why we do a single discard at swapon() time and then never again.

It sure *looks* like a bug. If it isn't then some explanation is sorely needed.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

The 339944663 changelog does explain why it's done at swapon(): "While discard at swapon still shows as slightly beneficial" - surely I don't need to justify doing something beneficial?

Discard is a way of informing the drive that we're no longer interested in the old contents: if the drive can make use of it, that's useful information when adding a swap area - as it is when doing mkfs.

And the 339944663 changelog explains why we're making the running discard conditional on a flag: "discarding 1MB swap cluster when allocating is now disadvantageous" - for more detail see changelog below.

I don't understand what's so wrong with doing something once, at the time that it is cheap and effective. Doing it repeatedly turned out to be costly, as explained in the changelog.

But now we do find that "cheap" inappropriate on the Vertex2.

I can certainly see that a longer name than SWAP_FLAG_DISCARD would have prevented this confusion - perhaps, SWAP_FLAG_DISCARD_CLUSTER_AFTER_USE, but that is spelt out in the comment at its definition.

commit 3399446632739fcd05fd8b272b476a69c6e6d14a

Author: Hugh Dickins <[email protected]>
Date:   Thu Sep 9 16:38:11 2010 -0700

swap: discard while swapping only if SWAP_FLAG_DISCARD

Tests with recent firmware on Intel X25-M 80GB and OCZ Vertex 60GB SSDs show a shift since I last tested in December: in part because of firmware updates, in part because of the necessary move from barriers to awaiting completion at the block layer. While discard at swapon still shows as slightly beneficial on both, discarding 1MB swap cluster when allocating is now disadvanteous: adds 25% overhead on Intel, adds 230% on OCZ (YMMV).

Surrender: discard as presently implemented is more hindrance than help for swap; but might prove useful on other devices, or with improvements. So continue to do the discard at swapon, but make discard while swapping conditional on a SWAP_FLAG_DISCARD to sys_swapon() (which has been using only the lower 16 bits of int flags).

We can add a --discard or -d to swapon(8), and a "discard" to swap in /etc/fstab: matching the mount option for btrfs, ext4, fat, gfs2, nilfs2.

index bf4eb62..7cdd633 100644
@@ -19,6 +19,7 @@ struct bio;
 #define SWAP_FLAG_PREFER	0x8000	/* set if swap priority specified */
 #define SWAP_FLAG_PRIO_MASK	0x7fff
 #define SWAP_FLAG_PRIO_SHIFT	0
+#define SWAP_FLAG_DISCARD	0x10000 /* discard swap cluster after use */
 
 static inline int current_is_kswapd(void)
 {

@@ -142,7 +143,7 @@ struct swap_extent {
 enum {
 	SWP_USED	= (1 << 0),	/* is slot in swap_info[] used? */
 	SWP_WRITEOK	= (1 << 1),	/* ok to write to this swap?	*/
-	SWP_DISCARDABLE = (1 << 2),	/* blkdev supports discard */
+	SWP_DISCARDABLE = (1 << 2),	/* swapon+blkdev support discard */
 	SWP_DISCARDING	= (1 << 3),	/* now discarding a free cluster */
 	SWP_SOLIDSTATE	= (1 << 4),	/* blkdev seeks are cheap */
 	SWP_CONTINUED	= (1 << 5),	/* swap_map has count continuation */

index 1894dea..7c703ff 100644
@@ -2047,7 +2047,7 @@ SYSCALL_DEFINE2(swapon, const char __user *, specialfile, int, swap_flags)
 			p->flags |= SWP_SOLIDSTATE;
 			p->cluster_next = 1 + (random32() % p->highest_bit);
 		}
-		if (discard_swap(p) == 0)
+		if (discard_swap(p) == 0 && (swap_flags & SWAP_FLAG_DISCARD))
 			p->flags |= SWP_DISCARDABLE;
 	}
 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Are you sure that's a significant factor? I can certainly imagine it magnifying the issue, but only Vertex2 has been reported as a problem.

Historic really: swap discard went in early, when it sounded like just the right thing for swap, and we imagined that vendors would implement it sensibly if they implemented it at all.

There appeared to be no need for such a flag at that time. But once different firmwares appeared, and also block developers switched discard over from using a barrier to waiting for completion, the use of discard when allocating fresh clusters became often much slower: so we added the flag for that case.

The use of discard at swapon time still appeared to be a worthwhile win, not needing any flag: swapon is indeed occasional. But now the Vertex2 has shown just how unbearable that can be.

On the Vertex2 - or do you know of others which pose this problem?

Hugh

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

I have three of those drives put together via MD to a raid 0 and I do not think they are defective, since they worked (without discard) so far. Firmware is also the new-es it's 1.35, just checked with OCZ website.

Thank you for the pointer with the firmware, I have posted a support question at OCZ.

Yes, it has the latest firmware.

No, I agree that one should not disable a feature that is useful to so many, for the reasons you mention. However, it would be good if there is some way to disable this, other then having to always patch the kernel.

Regards,
Holger

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Thanks very much for checking and reporting back.

Great: please let me know if they have anything of interest to add.

I've now ordered a Vertex2 myself, to see if it sheds any light on what goes on. Probably not; but my Vertex1 has recently started giving errors, so I might as well use a 2 to replace it. By the time I came to investigate last year's report, the Vertex3 was imminent, so I held on for that; but it turned out not to share the problem.

I should not exaggerate the effectiveness of the swapon at discard, on any drive I tried: "useful to so many" is probably going too far. I've called the effect "slight": when I've time I shall measure again just what that amounts to, and whether it's worth the effort of doing anything cleverer than Shaohua's patch - quite possibly not.

Hugh

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>