[RFC]swap: don't do discard if no discard option added

[RFC]swap: don't do discard if no discard option added

[Shaohua Li]

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

Reported-by: Holger Kiehl <Holger.Kiehl@dwd.de>
Signed-off-by: Shaohua Li <shli@fusionio.com>
---
  mm/swapfile.c |    2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

Index: linux/mm/swapfile.c
===================================================================
--- linux.orig/mm/swapfile.c	2012-03-20 20:11:59.222767526 +0800
+++ linux/mm/swapfile.c	2012-03-20 20:13:25.362767387 +0800
@@ -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 majordomo@kvack.org.  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:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC]swap: don't do discard if no discard option added

[Holger Kiehl]

Hello,

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

Regards,
Holger


On Tue, 20 Mar 2012, Shaohua Li wrote:

>
> Even don't add discard option, swapon will do discard, this sounds buggy,
> especially when discard is slow or buggy.
>
> Reported-by: Holger Kiehl <Holger.Kiehl@dwd.de>
> Signed-off-by: Shaohua Li <shli@fusionio.com>
> ---
>  mm/swapfile.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> Index: linux/mm/swapfile.c
> ===================================================================
> --- linux.orig/mm/swapfile.c	2012-03-20 20:11:59.222767526 +0800
> +++ linux/mm/swapfile.c	2012-03-20 20:13:25.362767387 +0800
> @@ -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 majordomo@kvack.org.  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:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC]swap: don't do discard if no discard option added

[Andrew Morton]

On Tue, 20 Mar 2012 20:34:38 +0800
Shaohua Li <shli@kernel.org> wrote:

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

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


From: Shaohua Li <shli@kernel.org>
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.

Signed-off-by: Shaohua Li <shli@fusionio.com>
Reported-by: Holger Kiehl <Holger.Kiehl@dwd.de>
Tested-by: Holger Kiehl <Holger.Kiehl@dwd.de>
Cc: Hugh Dickins <hughd@google.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 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
--- a/mm/swapfile.c~swap-dont-do-discard-if-no-discard-option-added
+++ a/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 majordomo@kvack.org.  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:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC]swap: don't do discard if no discard option added

[Hugh Dickins]

On Tue, 20 Mar 2012, Shaohua Li wrote:
> 
> Even don't add discard option, swapon will do discard, this sounds buggy,
> especially when discard is slow or buggy.

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

> 
> Reported-by: Holger Kiehl <Holger.Kiehl@dwd.de>
> Signed-off-by: Shaohua Li <shli@fusionio.com>
> ---
>  mm/swapfile.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Index: linux/mm/swapfile.c
> ===================================================================
> --- linux.orig/mm/swapfile.c	2012-03-20 20:11:59.222767526 +0800
> +++ linux/mm/swapfile.c	2012-03-20 20:13:25.362767387 +0800
> @@ -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 majordomo@kvack.org.  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:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC]swap: don't do discard if no discard option added

[Shaohua Li]

2012/3/21 Hugh Dickins <hughd@google.com>:
> On Tue, 20 Mar 2012, Shaohua Li wrote:
>>
>> Even don't add discard option, swapon will do discard, this sounds buggy,
>> especially when discard is slow or buggy.
>
> 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?
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 majordomo@kvack.org.  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:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC]swap: don't do discard if no discard option added

[Andrew Morton]

On Wed, 21 Mar 2012 12:31:47 +0800 Shaohua Li <shli@kernel.org> wrote:

>  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.

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 majordomo@kvack.org.  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:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC]swap: don't do discard if no discard option added

[Holger Kiehl]

On Tue, 20 Mar 2012, Hugh Dickins wrote:

> On Tue, 20 Mar 2012, Shaohua Li wrote:
>>
>> Even don't add discard option, swapon will do discard, this sounds buggy,
>> especially when discard is slow or buggy.
>
> 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.
>
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.

> 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?
>
Yes, it has the latest firmware.

> 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?
>
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 majordomo@kvack.org.  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:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC]swap: don't do discard if no discard option added

[Hugh Dickins]

On Wed, 21 Mar 2012, Shaohua Li wrote:
> 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.

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.

> 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

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.

> since discard is slow.

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 majordomo@kvack.org.  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:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC]swap: don't do discard if no discard option added

[Hugh Dickins]

On Tue, 20 Mar 2012, Andrew Morton wrote:
> On Wed, 21 Mar 2012 12:31:47 +0800 Shaohua Li <shli@kernel.org> wrote:
> 
> >  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.
> 
> 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

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.

> and then never again.

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.

> 
> It sure *looks* like a bug.

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.

> If it isn't then some explanation is sorely needed.

commit 3399446632739fcd05fd8b272b476a69c6e6d14a
Author: Hugh Dickins <hughd@google.com>
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.
    
    Signed-off-by: Hugh Dickins <hughd@google.com>
    Cc: Christoph Hellwig <hch@lst.de>
    Cc: Nigel Cunningham <nigel@tuxonice.net>
    Cc: Tejun Heo <tj@kernel.org>
    Cc: Jens Axboe <jaxboe@fusionio.com>
    Cc: James Bottomley <James.Bottomley@hansenpartnership.com>
    Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
    Cc: <stable@kernel.org>
    Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
    Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

diff --git a/include/linux/swap.h b/include/linux/swap.h
index bf4eb62..7cdd633 100644
--- a/include/linux/swap.h
+++ b/include/linux/swap.h
@@ -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 */
diff --git a/mm/swapfile.c b/mm/swapfile.c
index 1894dea..7c703ff 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -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 majordomo@kvack.org.  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:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC]swap: don't do discard if no discard option added

[Hugh Dickins]

On Wed, 21 Mar 2012, Holger Kiehl wrote:
> On Tue, 20 Mar 2012, Hugh Dickins wrote:
> > 
> > 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.
> > 
> 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.

Thanks very much for checking and reporting back.

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

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.

> > 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?
> > 
> 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.

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 majordomo@kvack.org.  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:"dont@kvack.org"> email@kvack.org </a>