[PATCH 00/02] swap: allowing a more flexible DISCARD policy V2

[PATCH 00/02] swap: allowing a more flexible DISCARD policy V2

[Rafael Aquini]

Considering the use cases where the swap device supports discard:
a) and can do it quickly;
b) but it's slow to do in small granularities (or concurrent with other
   I/O);
c) but the implementation is so horrendous that you don't even want to
   send one down;

And assuming that the sysadmin considers it useful to send the discards down
at all, we would (probably) want the following solutions:

  i. do the fine-grained discards for freed swap pages, if device is 
     capable of doing so optimally;
 ii. do single-time (batched) swap area discards, either at swapon 
     or via something like fstrim (not implemented yet);
iii. allow doing both single-time and fine-grained discards; or
 iv. turn it off completely (default behavior)


As implemented today, one can only enable/disable discards for swap, but 
one cannot select, for instance, solution (ii) on a swap device like (b)
even though the single-time discard is regarded to be interesting, or
necessary to the workload because it would imply (1), and the device
is not capable of performing it optimally.

This patchset addresses the scenario depicted above by introducing a
way to ensure the (probably) wanted solutions (i, ii, iii and iv) can
be flexibly flagged through swapon(8) to allow a sysadmin to select
the best suitable swap discard policy accordingly to system constraints.

Changeset from V1:
01 (kernel)       swap: discard while swapping only if SWAP_FLAG_DISCARD_PAGES
* ensure backwards compatibility with older swapon(8) releases;      (mkosaki)

02 (util-linux) swapon: allow a more flexible swap discard policy
* introduce discard policy selector as an optional argument of --discard option;
* rename user-visible discard policy names;           (mkosaki, karel, jmoyer) 

[PATCH 00/02] swap: allowing a more flexible DISCARD policy V2

[Rafael Aquini]

Considering the use cases where the swap device supports discard:
a) and can do it quickly;
b) but it's slow to do in small granularities (or concurrent with other
   I/O);
c) but the implementation is so horrendous that you don't even want to
   send one down;

And assuming that the sysadmin considers it useful to send the discards down
at all, we would (probably) want the following solutions:

  i. do the fine-grained discards for freed swap pages, if device is 
     capable of doing so optimally;
 ii. do single-time (batched) swap area discards, either at swapon 
     or via something like fstrim (not implemented yet);
iii. allow doing both single-time and fine-grained discards; or
 iv. turn it off completely (default behavior)


As implemented today, one can only enable/disable discards for swap, but 
one cannot select, for instance, solution (ii) on a swap device like (b)
even though the single-time discard is regarded to be interesting, or
necessary to the workload because it would imply (1), and the device
is not capable of performing it optimally.

This patchset addresses the scenario depicted above by introducing a
way to ensure the (probably) wanted solutions (i, ii, iii and iv) can
be flexibly flagged through swapon(8) to allow a sysadmin to select
the best suitable swap discard policy accordingly to system constraints.

Changeset from V1:
01 (kernel)       swap: discard while swapping only if SWAP_FLAG_DISCARD_PAGES
* ensure backwards compatibility with older swapon(8) releases;      (mkosaki)

02 (util-linux) swapon: allow a more flexible swap discard policy
* introduce discard policy selector as an optional argument of --discard option;
* rename user-visible discard policy names;           (mkosaki, karel, jmoyer) 

--
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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 01/02] swap: discard while swapping only if SWAP_FLAG_DISCARD_PAGES

[Rafael Aquini]

This patch introduces SWAP_FLAG_DISCARD_PAGES and SWAP_FLAG_DISCARD_ONCE
new flags to allow more flexibe swap discard policies being flagged through
swapon(8). The default behavior is to keep both single-time, or batched, area
discards (SWAP_FLAG_DISCARD_ONCE) and fine-grained discards for page-clusters
(SWAP_FLAG_DISCARD_PAGES) enabled, in order to keep consistentcy with older
kernel behavior, as well as maintain compatibility with older swapon(8).
However, through the new introduced flags the best suitable discard policy 
can be selected accordingly to any given swap device constraint.

Signed-off-by: Rafael Aquini <aquini@redhat.com>
---
 include/linux/swap.h | 13 +++++++++----
 mm/swapfile.c        | 55 +++++++++++++++++++++++++++++++++++++++++++++++-----
 2 files changed, 59 insertions(+), 9 deletions(-)

diff --git a/include/linux/swap.h b/include/linux/swap.h
index 1701ce4..33fa21f 100644
--- a/include/linux/swap.h
+++ b/include/linux/swap.h
@@ -19,10 +19,13 @@ 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 */
+#define SWAP_FLAG_DISCARD	0x10000 /* enable discard for swap */
+#define SWAP_FLAG_DISCARD_ONCE	0x20000 /* discard swap area at swapon-time */
+#define SWAP_FLAG_DISCARD_PAGES 0x40000 /* discard page-clusters after use */
 
 #define SWAP_FLAGS_VALID	(SWAP_FLAG_PRIO_MASK | SWAP_FLAG_PREFER | \
-				 SWAP_FLAG_DISCARD)
+				 SWAP_FLAG_DISCARD | SWAP_FLAG_DISCARD_ONCE | \
+				 SWAP_FLAG_DISCARD_PAGES)
 
 static inline int current_is_kswapd(void)
 {
@@ -146,14 +149,16 @@ 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),	/* swapon+blkdev support discard */
+	SWP_DISCARDABLE = (1 << 2),	/* 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 */
 	SWP_BLKDEV	= (1 << 6),	/* its a block device */
 	SWP_FILE	= (1 << 7),	/* set after swap_activate success */
+	SWP_AREA_DISCARD = (1 << 8),	/* single-time swap area discards */
+	SWP_PAGE_DISCARD = (1 << 9),	/* freed swap page-cluster discards */
 					/* add others here before... */
-	SWP_SCANNING	= (1 << 8),	/* refcount in scan_swap_map */
+	SWP_SCANNING	= (1 << 10),	/* refcount in scan_swap_map */
 };
 
 #define SWAP_CLUSTER_MAX 32UL
diff --git a/mm/swapfile.c b/mm/swapfile.c
index 6c340d9..719513d 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -212,7 +212,7 @@ static unsigned long scan_swap_map(struct swap_info_struct *si,
 			si->cluster_nr = SWAPFILE_CLUSTER - 1;
 			goto checks;
 		}
-		if (si->flags & SWP_DISCARDABLE) {
+		if (si->flags & SWP_PAGE_DISCARD) {
 			/*
 			 * Start range check on racing allocations, in case
 			 * they overlap the cluster we eventually decide on
@@ -322,7 +322,7 @@ checks:
 
 	if (si->lowest_alloc) {
 		/*
-		 * Only set when SWP_DISCARDABLE, and there's a scan
+		 * Only set when SWP_PAGE_DISCARD, and there's a scan
 		 * for a free cluster in progress or just completed.
 		 */
 		if (found_free_cluster) {
@@ -2016,6 +2016,20 @@ static int setup_swap_map_and_extents(struct swap_info_struct *p,
 	return nr_extents;
 }
 
+/*
+ * Helper to sys_swapon determining if a given swap
+ * backing device queue supports DISCARD operations.
+ */
+static bool swap_discardable(struct swap_info_struct *si)
+{
+	struct request_queue *q = bdev_get_queue(si->bdev);
+
+	if (!q || !blk_queue_discard(q))
+		return false;
+
+	return true;
+}
+
 SYSCALL_DEFINE2(swapon, const char __user *, specialfile, int, swap_flags)
 {
 	struct swap_info_struct *p;
@@ -2123,8 +2137,37 @@ SYSCALL_DEFINE2(swapon, const char __user *, specialfile, int, swap_flags)
 			p->flags |= SWP_SOLIDSTATE;
 			p->cluster_next = 1 + (prandom_u32() % p->highest_bit);
 		}
-		if ((swap_flags & SWAP_FLAG_DISCARD) && discard_swap(p) == 0)
-			p->flags |= SWP_DISCARDABLE;
+
+		if ((swap_flags & SWAP_FLAG_DISCARD) && swap_discardable(p)) {
+			/*
+			 * When discard is enabled for swap, with no particular
+			 * policy flagged, we set all swap discard flags here
+			 * in order to sustain backward compatibility with
+			 * older swapon(8) releases.
+			 */
+			p->flags |= (SWP_DISCARDABLE | SWP_AREA_DISCARD |
+				     SWP_PAGE_DISCARD);
+
+			/*
+			 * By flagging sys_swapon, a sysadmin can tell us to
+			 * either do sinle-time area discards only, or to just
+			 * perform discards for released swap page-clusters.
+			 * Now it's time to adjust the p->flags accordingly.
+			 */
+			if (swap_flags & SWAP_FLAG_DISCARD_ONCE)
+				p->flags &= ~SWP_PAGE_DISCARD;
+			else if (swap_flags & SWAP_FLAG_DISCARD_PAGES)
+				p->flags &= ~SWP_AREA_DISCARD;
+
+			/* issue a swapon-time discard if it's still required */
+			if (p->flags & SWP_AREA_DISCARD) {
+				int err = discard_swap(p);
+				if (unlikely(err))
+					printk(KERN_ERR
+					       "swapon: discard_swap(%p): %d\n",
+						p, err);
+			}
+		}
 	}
 
 	mutex_lock(&swapon_mutex);
@@ -2135,11 +2178,13 @@ SYSCALL_DEFINE2(swapon, const char __user *, specialfile, int, swap_flags)
 	enable_swap_info(p, prio, swap_map, frontswap_map);
 
 	printk(KERN_INFO "Adding %uk swap on %s.  "
-			"Priority:%d extents:%d across:%lluk %s%s%s\n",
+			"Priority:%d extents:%d across:%lluk %s%s%s%s%s\n",
 		p->pages<<(PAGE_SHIFT-10), name->name, p->prio,
 		nr_extents, (unsigned long long)span<<(PAGE_SHIFT-10),
 		(p->flags & SWP_SOLIDSTATE) ? "SS" : "",
 		(p->flags & SWP_DISCARDABLE) ? "D" : "",
+		(p->flags & SWP_AREA_DISCARD) ? "s" : "",
+		(p->flags & SWP_PAGE_DISCARD) ? "c" : "",
 		(frontswap_map) ? "FS" : "");
 
 	mutex_unlock(&swapon_mutex);
-- 
1.8.1.4

[PATCH 01/02] swap: discard while swapping only if SWAP_FLAG_DISCARD_PAGES

[Rafael Aquini]

This patch introduces SWAP_FLAG_DISCARD_PAGES and SWAP_FLAG_DISCARD_ONCE
new flags to allow more flexibe swap discard policies being flagged through
swapon(8). The default behavior is to keep both single-time, or batched, area
discards (SWAP_FLAG_DISCARD_ONCE) and fine-grained discards for page-clusters
(SWAP_FLAG_DISCARD_PAGES) enabled, in order to keep consistentcy with older
kernel behavior, as well as maintain compatibility with older swapon(8).
However, through the new introduced flags the best suitable discard policy 
can be selected accordingly to any given swap device constraint.

Signed-off-by: Rafael Aquini <aquini@redhat.com>
---
 include/linux/swap.h | 13 +++++++++----
 mm/swapfile.c        | 55 +++++++++++++++++++++++++++++++++++++++++++++++-----
 2 files changed, 59 insertions(+), 9 deletions(-)

diff --git a/include/linux/swap.h b/include/linux/swap.h
index 1701ce4..33fa21f 100644
--- a/include/linux/swap.h
+++ b/include/linux/swap.h
@@ -19,10 +19,13 @@ 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 */
+#define SWAP_FLAG_DISCARD	0x10000 /* enable discard for swap */
+#define SWAP_FLAG_DISCARD_ONCE	0x20000 /* discard swap area at swapon-time */
+#define SWAP_FLAG_DISCARD_PAGES 0x40000 /* discard page-clusters after use */
 
 #define SWAP_FLAGS_VALID	(SWAP_FLAG_PRIO_MASK | SWAP_FLAG_PREFER | \
-				 SWAP_FLAG_DISCARD)
+				 SWAP_FLAG_DISCARD | SWAP_FLAG_DISCARD_ONCE | \
+				 SWAP_FLAG_DISCARD_PAGES)
 
 static inline int current_is_kswapd(void)
 {
@@ -146,14 +149,16 @@ 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),	/* swapon+blkdev support discard */
+	SWP_DISCARDABLE = (1 << 2),	/* 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 */
 	SWP_BLKDEV	= (1 << 6),	/* its a block device */
 	SWP_FILE	= (1 << 7),	/* set after swap_activate success */
+	SWP_AREA_DISCARD = (1 << 8),	/* single-time swap area discards */
+	SWP_PAGE_DISCARD = (1 << 9),	/* freed swap page-cluster discards */
 					/* add others here before... */
-	SWP_SCANNING	= (1 << 8),	/* refcount in scan_swap_map */
+	SWP_SCANNING	= (1 << 10),	/* refcount in scan_swap_map */
 };
 
 #define SWAP_CLUSTER_MAX 32UL
diff --git a/mm/swapfile.c b/mm/swapfile.c
index 6c340d9..719513d 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -212,7 +212,7 @@ static unsigned long scan_swap_map(struct swap_info_struct *si,
 			si->cluster_nr = SWAPFILE_CLUSTER - 1;
 			goto checks;
 		}
-		if (si->flags & SWP_DISCARDABLE) {
+		if (si->flags & SWP_PAGE_DISCARD) {
 			/*
 			 * Start range check on racing allocations, in case
 			 * they overlap the cluster we eventually decide on
@@ -322,7 +322,7 @@ checks:
 
 	if (si->lowest_alloc) {
 		/*
-		 * Only set when SWP_DISCARDABLE, and there's a scan
+		 * Only set when SWP_PAGE_DISCARD, and there's a scan
 		 * for a free cluster in progress or just completed.
 		 */
 		if (found_free_cluster) {
@@ -2016,6 +2016,20 @@ static int setup_swap_map_and_extents(struct swap_info_struct *p,
 	return nr_extents;
 }
 
+/*
+ * Helper to sys_swapon determining if a given swap
+ * backing device queue supports DISCARD operations.
+ */
+static bool swap_discardable(struct swap_info_struct *si)
+{
+	struct request_queue *q = bdev_get_queue(si->bdev);
+
+	if (!q || !blk_queue_discard(q))
+		return false;
+
+	return true;
+}
+
 SYSCALL_DEFINE2(swapon, const char __user *, specialfile, int, swap_flags)
 {
 	struct swap_info_struct *p;
@@ -2123,8 +2137,37 @@ SYSCALL_DEFINE2(swapon, const char __user *, specialfile, int, swap_flags)
 			p->flags |= SWP_SOLIDSTATE;
 			p->cluster_next = 1 + (prandom_u32() % p->highest_bit);
 		}
-		if ((swap_flags & SWAP_FLAG_DISCARD) && discard_swap(p) == 0)
-			p->flags |= SWP_DISCARDABLE;
+
+		if ((swap_flags & SWAP_FLAG_DISCARD) && swap_discardable(p)) {
+			/*
+			 * When discard is enabled for swap, with no particular
+			 * policy flagged, we set all swap discard flags here
+			 * in order to sustain backward compatibility with
+			 * older swapon(8) releases.
+			 */
+			p->flags |= (SWP_DISCARDABLE | SWP_AREA_DISCARD |
+				     SWP_PAGE_DISCARD);
+
+			/*
+			 * By flagging sys_swapon, a sysadmin can tell us to
+			 * either do sinle-time area discards only, or to just
+			 * perform discards for released swap page-clusters.
+			 * Now it's time to adjust the p->flags accordingly.
+			 */
+			if (swap_flags & SWAP_FLAG_DISCARD_ONCE)
+				p->flags &= ~SWP_PAGE_DISCARD;
+			else if (swap_flags & SWAP_FLAG_DISCARD_PAGES)
+				p->flags &= ~SWP_AREA_DISCARD;
+
+			/* issue a swapon-time discard if it's still required */
+			if (p->flags & SWP_AREA_DISCARD) {
+				int err = discard_swap(p);
+				if (unlikely(err))
+					printk(KERN_ERR
+					       "swapon: discard_swap(%p): %d\n",
+						p, err);
+			}
+		}
 	}
 
 	mutex_lock(&swapon_mutex);
@@ -2135,11 +2178,13 @@ SYSCALL_DEFINE2(swapon, const char __user *, specialfile, int, swap_flags)
 	enable_swap_info(p, prio, swap_map, frontswap_map);
 
 	printk(KERN_INFO "Adding %uk swap on %s.  "
-			"Priority:%d extents:%d across:%lluk %s%s%s\n",
+			"Priority:%d extents:%d across:%lluk %s%s%s%s%s\n",
 		p->pages<<(PAGE_SHIFT-10), name->name, p->prio,
 		nr_extents, (unsigned long long)span<<(PAGE_SHIFT-10),
 		(p->flags & SWP_SOLIDSTATE) ? "SS" : "",
 		(p->flags & SWP_DISCARDABLE) ? "D" : "",
+		(p->flags & SWP_AREA_DISCARD) ? "s" : "",
+		(p->flags & SWP_PAGE_DISCARD) ? "c" : "",
 		(frontswap_map) ? "FS" : "");
 
 	mutex_unlock(&swapon_mutex);
-- 
1.8.1.4

--
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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 02/02] swapon: allow a more flexible swap discard policy

[Rafael Aquini]

Introduce the necessary changes to swapon(8) allowing a sysadmin to leverage
the new changes introduced to sys_swapon by "swap: discard while swapping
only if SWAP_FLAG_DISCARD_PAGES", therefore allowing a more flexible set of
choices when selection the discard policy for mounted swap areas.
This patch introduces the following optional arguments to the already
existent swapon(8) "--discard" option, in order to allow a discard type to 
be selected at swapon time:
 * once    : only single-time area discards are issued. (swapon)
 * pages   : discard freed pages before they are reused.
If no policy is selected both discard types are enabled. (default)

Signed-off-by: Rafael Aquini <aquini@redhat.com>
---
 sys-utils/swapon.8 | 24 +++++++++++++------
 sys-utils/swapon.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++--------
 2 files changed, 77 insertions(+), 17 deletions(-)

diff --git a/sys-utils/swapon.8 b/sys-utils/swapon.8
index 385bf5a..17f7970 100644
--- a/sys-utils/swapon.8
+++ b/sys-utils/swapon.8
@@ -112,15 +112,25 @@ All devices marked as ``swap'' in
 are made available, except for those with the ``noauto'' option.
 Devices that are already being used as swap are silently skipped.
 .TP
-.B "\-d, \-\-discard"
-Discard freed swap pages before they are reused, if the swap
-device supports the discard or trim operation.  This may improve
-performance on some Solid State Devices, but often it does not.
+.B "\-d, \-\-discard\fR [\fIpolicy\fR]"
+Enable swap discards, if the swap backing device supports the discard or
+trim operation. This may improve performance on some Solid State Devices,
+but often it does not. The long option \-\-discard allows one to select
+between two available swap discard policies:
+.BI \-\-discard=once
+to perform a single-time discard operation for the whole swap area at swapon;
+or
+.BI \-\-discard=pages
+to discard freed swap pages before they are reused, while swapping.
+If no policy is selected, the default behavior is to enable both discard types.
 The
 .I /etc/fstab
-mount option
-.BI discard
-may be also used to enable discard flag.
+mount options
+.BI discard,
+.BI discard=once,
+or
+.BI discard=pages
+may be also used to enable discard flags.
 .TP
 .B "\-e, \-\-ifexists"
 Silently skip devices that do not exist.
diff --git a/sys-utils/swapon.c b/sys-utils/swapon.c
index f1e2433..8a90bfe 100644
--- a/sys-utils/swapon.c
+++ b/sys-utils/swapon.c
@@ -34,9 +34,20 @@
 #endif
 
 #ifndef SWAP_FLAG_DISCARD
-# define SWAP_FLAG_DISCARD	0x10000 /* discard swap cluster after use */
+# define SWAP_FLAG_DISCARD	0x10000 /* enable discard for swap */
 #endif
 
+#ifndef SWAP_FLAG_DISCARD_ONCE
+# define SWAP_FLAG_DISCARD_ONCE 0x20000 /* discard swap area at swapon-time */
+#endif
+
+#ifndef SWAP_FLAG_DISCARD_PAGES
+# define SWAP_FLAG_DISCARD_PAGES 0x40000 /* discard page-clusters after use */
+#endif
+
+#define SWAP_FLAGS_DISCARD_VALID (SWAP_FLAG_DISCARD | SWAP_FLAG_DISCARD_ONCE | \
+				  SWAP_FLAG_DISCARD_PAGES)
+
 #ifndef SWAP_FLAG_PREFER
 # define SWAP_FLAG_PREFER	0x8000	/* set if swap priority specified */
 #endif
@@ -70,7 +81,7 @@ enum {
 
 static int all;
 static int priority = -1;	/* non-prioritized swap by default */
-static int discard;
+static int discard = 0;		/* don't send swap discards by default */
 
 /* If true, don't complain if the device/file doesn't exist */
 static int ifexists;
@@ -570,8 +581,22 @@ static int do_swapon(const char *orig_special, int prio,
 			   << SWAP_FLAG_PRIO_SHIFT);
 	}
 #endif
-	if (fl_discard)
-		flags |= SWAP_FLAG_DISCARD;
+	/*
+	 * Validate the discard flags passed and set them
+	 * accordingly before calling sys_swapon.
+	 */
+	if (fl_discard && !(fl_discard & ~SWAP_FLAGS_DISCARD_VALID)) {
+		/*
+		 * If we get here with both discard policy flags set,
+		 * we just need to tell the kernel to enable discards
+		 * and it will do correctly, just as we expect.
+		 */
+		if ((fl_discard & SWAP_FLAG_DISCARD_ONCE) &&
+		    (fl_discard & SWAP_FLAG_DISCARD_PAGES))
+			flags |= SWAP_FLAG_DISCARD;
+		else
+			flags |= fl_discard;
+	}
 
 	status = swapon(special, flags);
 	if (status < 0)
@@ -611,12 +636,22 @@ static int swapon_all(void)
 	while (mnt_table_find_next_fs(tb, itr, match_swap, NULL, &fs) == 0) {
 		/* defaults */
 		int pri = priority, dsc = discard, nofail = ifexists;
-		char *p, *src;
+		char *p, *src, *dscarg;
 
 		if (mnt_fs_get_option(fs, "noauto", NULL, NULL) == 0)
 			continue;
-		if (mnt_fs_get_option(fs, "discard", NULL, NULL) == 0)
-			dsc = 1;
+		if (mnt_fs_get_option(fs, "discard", &dscarg, NULL) == 0) {
+			dsc |= SWAP_FLAG_DISCARD;
+			if (dscarg) {
+				/* only single-time discards are wanted */
+				if (strcmp(dscarg, "once") == 0)
+					dsc |= SWAP_FLAG_DISCARD_ONCE;
+
+				/* do discard for every released swap page */
+				if (strcmp(dscarg, "pages") == 0)
+					dsc |= SWAP_FLAG_DISCARD_PAGES;
+			}
+		}
 		if (mnt_fs_get_option(fs, "nofail", NULL, NULL) == 0)
 			nofail = 1;
 		if (mnt_fs_get_option(fs, "pri", &p, NULL) == 0 && p)
@@ -647,7 +682,7 @@ static void __attribute__ ((__noreturn__)) usage(FILE * out)
 
 	fputs(USAGE_OPTIONS, out);
 	fputs(_(" -a, --all              enable all swaps from /etc/fstab\n"
-		" -d, --discard          discard freed pages before they are reused\n"
+		" -d, --discard[=policy] enable swap discards, if supported by device\n"
 		" -e, --ifexists         silently skip devices that do not exist\n"
 		" -f, --fixpgsz          reinitialize the swap space if necessary\n"
 		" -p, --priority <prio>  specify the priority of the swap device\n"
@@ -672,6 +707,11 @@ static void __attribute__ ((__noreturn__)) usage(FILE * out)
 		" <device>               name of device to be used\n"
 		" <file>                 name of file to be used\n"), out);
 
+	fputs(_("\nAvailable discard policy types (for --discard):\n"
+		" once	  : only single-time area discards are issued. (swapon)\n"
+		" pages	  : discard freed pages before they are reused.\n"
+		" * if no policy is selected both discard types are enabled. (default)\n"), out);
+
 	fputs(_("\nAvailable columns (for --show):\n"), out);
 	for (i = 0; i < NCOLS; i++)
 		fprintf(out, " %4s  %s\n", infos[i].name, _(infos[i].help));
@@ -696,7 +736,7 @@ int main(int argc, char *argv[])
 
 	static const struct option long_opts[] = {
 		{ "priority", 1, 0, 'p' },
-		{ "discard",  0, 0, 'd' },
+		{ "discard",  2, 0, 'd' },
 		{ "ifexists", 0, 0, 'e' },
 		{ "summary",  0, 0, 's' },
 		{ "fixpgsz",  0, 0, 'f' },
@@ -739,7 +779,17 @@ int main(int argc, char *argv[])
 			add_uuid(optarg);
 			break;
 		case 'd':
-			discard = 1;
+			discard |= SWAP_FLAG_DISCARD;
+
+			if (optarg) {
+				/* only single-time discards are wanted */
+				if (strcmp(optarg, "once") == 0)
+					discard |= SWAP_FLAG_DISCARD_ONCE;
+
+				/* do discard for every released swap page */
+				if (strcmp(optarg, "pages") == 0)
+					discard |= SWAP_FLAG_DISCARD_PAGES;
+			}
 			break;
 		case 'e':               /* ifexists */
 		        ifexists = 1;
-- 
1.8.1.4

[PATCH 02/02] swapon: allow a more flexible swap discard policy

[Rafael Aquini]

Introduce the necessary changes to swapon(8) allowing a sysadmin to leverage
the new changes introduced to sys_swapon by "swap: discard while swapping
only if SWAP_FLAG_DISCARD_PAGES", therefore allowing a more flexible set of
choices when selection the discard policy for mounted swap areas.
This patch introduces the following optional arguments to the already
existent swapon(8) "--discard" option, in order to allow a discard type to 
be selected at swapon time:
 * once    : only single-time area discards are issued. (swapon)
 * pages   : discard freed pages before they are reused.
If no policy is selected both discard types are enabled. (default)

Signed-off-by: Rafael Aquini <aquini@redhat.com>
---
 sys-utils/swapon.8 | 24 +++++++++++++------
 sys-utils/swapon.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++--------
 2 files changed, 77 insertions(+), 17 deletions(-)

diff --git a/sys-utils/swapon.8 b/sys-utils/swapon.8
index 385bf5a..17f7970 100644
--- a/sys-utils/swapon.8
+++ b/sys-utils/swapon.8
@@ -112,15 +112,25 @@ All devices marked as ``swap'' in
 are made available, except for those with the ``noauto'' option.
 Devices that are already being used as swap are silently skipped.
 .TP
-.B "\-d, \-\-discard"
-Discard freed swap pages before they are reused, if the swap
-device supports the discard or trim operation.  This may improve
-performance on some Solid State Devices, but often it does not.
+.B "\-d, \-\-discard\fR [\fIpolicy\fR]"
+Enable swap discards, if the swap backing device supports the discard or
+trim operation. This may improve performance on some Solid State Devices,
+but often it does not. The long option \-\-discard allows one to select
+between two available swap discard policies:
+.BI \-\-discard=once
+to perform a single-time discard operation for the whole swap area at swapon;
+or
+.BI \-\-discard=pages
+to discard freed swap pages before they are reused, while swapping.
+If no policy is selected, the default behavior is to enable both discard types.
 The
 .I /etc/fstab
-mount option
-.BI discard
-may be also used to enable discard flag.
+mount options
+.BI discard,
+.BI discard=once,
+or
+.BI discard=pages
+may be also used to enable discard flags.
 .TP
 .B "\-e, \-\-ifexists"
 Silently skip devices that do not exist.
diff --git a/sys-utils/swapon.c b/sys-utils/swapon.c
index f1e2433..8a90bfe 100644
--- a/sys-utils/swapon.c
+++ b/sys-utils/swapon.c
@@ -34,9 +34,20 @@
 #endif
 
 #ifndef SWAP_FLAG_DISCARD
-# define SWAP_FLAG_DISCARD	0x10000 /* discard swap cluster after use */
+# define SWAP_FLAG_DISCARD	0x10000 /* enable discard for swap */
 #endif
 
+#ifndef SWAP_FLAG_DISCARD_ONCE
+# define SWAP_FLAG_DISCARD_ONCE 0x20000 /* discard swap area at swapon-time */
+#endif
+
+#ifndef SWAP_FLAG_DISCARD_PAGES
+# define SWAP_FLAG_DISCARD_PAGES 0x40000 /* discard page-clusters after use */
+#endif
+
+#define SWAP_FLAGS_DISCARD_VALID (SWAP_FLAG_DISCARD | SWAP_FLAG_DISCARD_ONCE | \
+				  SWAP_FLAG_DISCARD_PAGES)
+
 #ifndef SWAP_FLAG_PREFER
 # define SWAP_FLAG_PREFER	0x8000	/* set if swap priority specified */
 #endif
@@ -70,7 +81,7 @@ enum {
 
 static int all;
 static int priority = -1;	/* non-prioritized swap by default */
-static int discard;
+static int discard = 0;		/* don't send swap discards by default */
 
 /* If true, don't complain if the device/file doesn't exist */
 static int ifexists;
@@ -570,8 +581,22 @@ static int do_swapon(const char *orig_special, int prio,
 			   << SWAP_FLAG_PRIO_SHIFT);
 	}
 #endif
-	if (fl_discard)
-		flags |= SWAP_FLAG_DISCARD;
+	/*
+	 * Validate the discard flags passed and set them
+	 * accordingly before calling sys_swapon.
+	 */
+	if (fl_discard && !(fl_discard & ~SWAP_FLAGS_DISCARD_VALID)) {
+		/*
+		 * If we get here with both discard policy flags set,
+		 * we just need to tell the kernel to enable discards
+		 * and it will do correctly, just as we expect.
+		 */
+		if ((fl_discard & SWAP_FLAG_DISCARD_ONCE) &&
+		    (fl_discard & SWAP_FLAG_DISCARD_PAGES))
+			flags |= SWAP_FLAG_DISCARD;
+		else
+			flags |= fl_discard;
+	}
 
 	status = swapon(special, flags);
 	if (status < 0)
@@ -611,12 +636,22 @@ static int swapon_all(void)
 	while (mnt_table_find_next_fs(tb, itr, match_swap, NULL, &fs) == 0) {
 		/* defaults */
 		int pri = priority, dsc = discard, nofail = ifexists;
-		char *p, *src;
+		char *p, *src, *dscarg;
 
 		if (mnt_fs_get_option(fs, "noauto", NULL, NULL) == 0)
 			continue;
-		if (mnt_fs_get_option(fs, "discard", NULL, NULL) == 0)
-			dsc = 1;
+		if (mnt_fs_get_option(fs, "discard", &dscarg, NULL) == 0) {
+			dsc |= SWAP_FLAG_DISCARD;
+			if (dscarg) {
+				/* only single-time discards are wanted */
+				if (strcmp(dscarg, "once") == 0)
+					dsc |= SWAP_FLAG_DISCARD_ONCE;
+
+				/* do discard for every released swap page */
+				if (strcmp(dscarg, "pages") == 0)
+					dsc |= SWAP_FLAG_DISCARD_PAGES;
+			}
+		}
 		if (mnt_fs_get_option(fs, "nofail", NULL, NULL) == 0)
 			nofail = 1;
 		if (mnt_fs_get_option(fs, "pri", &p, NULL) == 0 && p)
@@ -647,7 +682,7 @@ static void __attribute__ ((__noreturn__)) usage(FILE * out)
 
 	fputs(USAGE_OPTIONS, out);
 	fputs(_(" -a, --all              enable all swaps from /etc/fstab\n"
-		" -d, --discard          discard freed pages before they are reused\n"
+		" -d, --discard[=policy] enable swap discards, if supported by device\n"
 		" -e, --ifexists         silently skip devices that do not exist\n"
 		" -f, --fixpgsz          reinitialize the swap space if necessary\n"
 		" -p, --priority <prio>  specify the priority of the swap device\n"
@@ -672,6 +707,11 @@ static void __attribute__ ((__noreturn__)) usage(FILE * out)
 		" <device>               name of device to be used\n"
 		" <file>                 name of file to be used\n"), out);
 
+	fputs(_("\nAvailable discard policy types (for --discard):\n"
+		" once	  : only single-time area discards are issued. (swapon)\n"
+		" pages	  : discard freed pages before they are reused.\n"
+		" * if no policy is selected both discard types are enabled. (default)\n"), out);
+
 	fputs(_("\nAvailable columns (for --show):\n"), out);
 	for (i = 0; i < NCOLS; i++)
 		fprintf(out, " %4s  %s\n", infos[i].name, _(infos[i].help));
@@ -696,7 +736,7 @@ int main(int argc, char *argv[])
 
 	static const struct option long_opts[] = {
 		{ "priority", 1, 0, 'p' },
-		{ "discard",  0, 0, 'd' },
+		{ "discard",  2, 0, 'd' },
 		{ "ifexists", 0, 0, 'e' },
 		{ "summary",  0, 0, 's' },
 		{ "fixpgsz",  0, 0, 'f' },
@@ -739,7 +779,17 @@ int main(int argc, char *argv[])
 			add_uuid(optarg);
 			break;
 		case 'd':
-			discard = 1;
+			discard |= SWAP_FLAG_DISCARD;
+
+			if (optarg) {
+				/* only single-time discards are wanted */
+				if (strcmp(optarg, "once") == 0)
+					discard |= SWAP_FLAG_DISCARD_ONCE;
+
+				/* do discard for every released swap page */
+				if (strcmp(optarg, "pages") == 0)
+					discard |= SWAP_FLAG_DISCARD_PAGES;
+			}
 			break;
 		case 'e':               /* ifexists */
 		        ifexists = 1;
-- 
1.8.1.4

--
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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 01/02] swap: discard while swapping only if SWAP_FLAG_DISCARD_PAGES

[KOSAKI Motohiro]

> +                       /*
> +                        * By flagging sys_swapon, a sysadmin can tell us to
> +                        * either do sinle-time area discards only, or to just
> +                        * perform discards for released swap page-clusters.
> +                        * Now it's time to adjust the p->flags accordingly.
> +                        */
> +                       if (swap_flags & SWAP_FLAG_DISCARD_ONCE)
> +                               p->flags &= ~SWP_PAGE_DISCARD;
> +                       else if (swap_flags & SWAP_FLAG_DISCARD_PAGES)
> +                               p->flags &= ~SWP_AREA_DISCARD;

When using old swapon(8), this code turn off both flags, right?

Re: [PATCH 01/02] swap: discard while swapping only if SWAP_FLAG_DISCARD_PAGES

[KOSAKI Motohiro]

> +                       /*
> +                        * By flagging sys_swapon, a sysadmin can tell us to
> +                        * either do sinle-time area discards only, or to just
> +                        * perform discards for released swap page-clusters.
> +                        * Now it's time to adjust the p->flags accordingly.
> +                        */
> +                       if (swap_flags & SWAP_FLAG_DISCARD_ONCE)
> +                               p->flags &= ~SWP_PAGE_DISCARD;
> +                       else if (swap_flags & SWAP_FLAG_DISCARD_PAGES)
> +                               p->flags &= ~SWP_AREA_DISCARD;

When using old swapon(8), this code turn off both flags, right?

--
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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 01/02] swap: discard while swapping only if SWAP_FLAG_DISCARD_PAGES

[Rafael Aquini]

On Sun, May 26, 2013 at 07:44:56AM -0400, KOSAKI Motohiro wrote:
> > +                       /*
> > +                        * By flagging sys_swapon, a sysadmin can tell us to
> > +                        * either do sinle-time area discards only, or to just
> > +                        * perform discards for released swap page-clusters.
> > +                        * Now it's time to adjust the p->flags accordingly.
> > +                        */
> > +                       if (swap_flags & SWAP_FLAG_DISCARD_ONCE)
> > +                               p->flags &= ~SWP_PAGE_DISCARD;
> > +                       else if (swap_flags & SWAP_FLAG_DISCARD_PAGES)
> > +                               p->flags &= ~SWP_AREA_DISCARD;
> 
> When using old swapon(8), this code turn off both flags, right?

As the flag that enables swap discards SWAP_FLAG_DISCARD remains meaning the
same it meant before, when using old swapon(8) (SWP_PAGE_DISCARD|SWP_AREA_DISCARD)
will remain flagged when discard is enabled, so we keep doing discards the same way 
we did before (at swapon, and for every released page-cluster). 
The flags are removed orthogonally only when the new swapon(8) selects one of the
particular discard policy available by using either SWAP_FLAG_DISCARD_ONCE,
or SWAP_FLAG_DISCARD_PAGES flags.

Re: [PATCH 01/02] swap: discard while swapping only if SWAP_FLAG_DISCARD_PAGES

[Rafael Aquini]

On Sun, May 26, 2013 at 07:44:56AM -0400, KOSAKI Motohiro wrote:
> > +                       /*
> > +                        * By flagging sys_swapon, a sysadmin can tell us to
> > +                        * either do sinle-time area discards only, or to just
> > +                        * perform discards for released swap page-clusters.
> > +                        * Now it's time to adjust the p->flags accordingly.
> > +                        */
> > +                       if (swap_flags & SWAP_FLAG_DISCARD_ONCE)
> > +                               p->flags &= ~SWP_PAGE_DISCARD;
> > +                       else if (swap_flags & SWAP_FLAG_DISCARD_PAGES)
> > +                               p->flags &= ~SWP_AREA_DISCARD;
> 
> When using old swapon(8), this code turn off both flags, right?

As the flag that enables swap discards SWAP_FLAG_DISCARD remains meaning the
same it meant before, when using old swapon(8) (SWP_PAGE_DISCARD|SWP_AREA_DISCARD)
will remain flagged when discard is enabled, so we keep doing discards the same way 
we did before (at swapon, and for every released page-cluster). 
The flags are removed orthogonally only when the new swapon(8) selects one of the
particular discard policy available by using either SWAP_FLAG_DISCARD_ONCE,
or SWAP_FLAG_DISCARD_PAGES flags.

--
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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 01/02] swap: discard while swapping only if SWAP_FLAG_DISCARD_PAGES

[KOSAKI Motohiro]

On Sun, May 26, 2013 at 9:52 AM, Rafael Aquini <aquini@redhat.com> wrote:
> On Sun, May 26, 2013 at 07:44:56AM -0400, KOSAKI Motohiro wrote:
>> > +                       /*
>> > +                        * By flagging sys_swapon, a sysadmin can tell us to
>> > +                        * either do sinle-time area discards only, or to just
>> > +                        * perform discards for released swap page-clusters.
>> > +                        * Now it's time to adjust the p->flags accordingly.
>> > +                        */
>> > +                       if (swap_flags & SWAP_FLAG_DISCARD_ONCE)
>> > +                               p->flags &= ~SWP_PAGE_DISCARD;
>> > +                       else if (swap_flags & SWAP_FLAG_DISCARD_PAGES)
>> > +                               p->flags &= ~SWP_AREA_DISCARD;
>>
>> When using old swapon(8), this code turn off both flags, right
>
 > As the flag that enables swap discards SWAP_FLAG_DISCARD remains meaning the
> same it meant before, when using old swapon(8) (SWP_PAGE_DISCARD|SWP_AREA_DISCARD)

But old swapon(8) don't use neigher SWAP_FLAG_DISCARD_ONCE nor
SWAP_FLAG_DISCARD_PAGES.  It uses only SWAP_FLAG_DISCARD. So, this
condition disables both SWP_PAGE_DISCARD and SWP_AREA_DISCARD.

And you changed that SWP_DISCARDABLE is not checked in IO path  at all.

>-               if (si->flags & SWP_DISCARDABLE) {
>+               if (si->flags & SWP_PAGE_DISCARD) {

I suggest new swapon(8) don't pass SWP_DISCARDABLE and kernel handle
SWP_DISCARDABLE as (SWAP_FLAG_DISCARD_ONCE | SWAP_FLAG_DISCARD_PAGES).

Optionally, warn SWP_DISCARDABLE is a good idea.


> will remain flagged when discard is enabled, so we keep doing discards the same way
> we did before (at swapon, and for every released page-cluster).
> The flags are removed orthogonally only when the new swapon(8) selects one of the
> particular discard policy available by using either SWAP_FLAG_DISCARD_ONCE,
> or SWAP_FLAG_DISCARD_PAGES flags.
>

Re: [PATCH 01/02] swap: discard while swapping only if SWAP_FLAG_DISCARD_PAGES

[KOSAKI Motohiro]

On Sun, May 26, 2013 at 9:52 AM, Rafael Aquini <aquini@redhat.com> wrote:
> On Sun, May 26, 2013 at 07:44:56AM -0400, KOSAKI Motohiro wrote:
>> > +                       /*
>> > +                        * By flagging sys_swapon, a sysadmin can tell us to
>> > +                        * either do sinle-time area discards only, or to just
>> > +                        * perform discards for released swap page-clusters.
>> > +                        * Now it's time to adjust the p->flags accordingly.
>> > +                        */
>> > +                       if (swap_flags & SWAP_FLAG_DISCARD_ONCE)
>> > +                               p->flags &= ~SWP_PAGE_DISCARD;
>> > +                       else if (swap_flags & SWAP_FLAG_DISCARD_PAGES)
>> > +                               p->flags &= ~SWP_AREA_DISCARD;
>>
>> When using old swapon(8), this code turn off both flags, right
>
 > As the flag that enables swap discards SWAP_FLAG_DISCARD remains meaning the
> same it meant before, when using old swapon(8) (SWP_PAGE_DISCARD|SWP_AREA_DISCARD)

But old swapon(8) don't use neigher SWAP_FLAG_DISCARD_ONCE nor
SWAP_FLAG_DISCARD_PAGES.  It uses only SWAP_FLAG_DISCARD. So, this
condition disables both SWP_PAGE_DISCARD and SWP_AREA_DISCARD.

And you changed that SWP_DISCARDABLE is not checked in IO path  at all.

>-               if (si->flags & SWP_DISCARDABLE) {
>+               if (si->flags & SWP_PAGE_DISCARD) {

I suggest new swapon(8) don't pass SWP_DISCARDABLE and kernel handle
SWP_DISCARDABLE as (SWAP_FLAG_DISCARD_ONCE | SWAP_FLAG_DISCARD_PAGES).

Optionally, warn SWP_DISCARDABLE is a good idea.


> will remain flagged when discard is enabled, so we keep doing discards the same way
> we did before (at swapon, and for every released page-cluster).
> The flags are removed orthogonally only when the new swapon(8) selects one of the
> particular discard policy available by using either SWAP_FLAG_DISCARD_ONCE,
> or SWAP_FLAG_DISCARD_PAGES flags.
>

--
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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 01/02] swap: discard while swapping only if SWAP_FLAG_DISCARD_PAGES

[Rafael Aquini]

On Sun, May 26, 2013 at 10:55:32AM -0400, KOSAKI Motohiro wrote:
> On Sun, May 26, 2013 at 9:52 AM, Rafael Aquini <aquini@redhat.com> wrote:
> > On Sun, May 26, 2013 at 07:44:56AM -0400, KOSAKI Motohiro wrote:
> >> > +                       /*
> >> > +                        * By flagging sys_swapon, a sysadmin can tell us to
> >> > +                        * either do sinle-time area discards only, or to just
> >> > +                        * perform discards for released swap page-clusters.
> >> > +                        * Now it's time to adjust the p->flags accordingly.
> >> > +                        */
> >> > +                       if (swap_flags & SWAP_FLAG_DISCARD_ONCE)
> >> > +                               p->flags &= ~SWP_PAGE_DISCARD;
> >> > +                       else if (swap_flags & SWAP_FLAG_DISCARD_PAGES)
> >> > +                               p->flags &= ~SWP_AREA_DISCARD;
> >>
> >> When using old swapon(8), this code turn off both flags, right
> >
>  > As the flag that enables swap discards SWAP_FLAG_DISCARD remains meaning the
> > same it meant before, when using old swapon(8) (SWP_PAGE_DISCARD|SWP_AREA_DISCARD)
> 
> But old swapon(8) don't use neigher SWAP_FLAG_DISCARD_ONCE nor
> SWAP_FLAG_DISCARD_PAGES.  It uses only SWAP_FLAG_DISCARD. So, this
> condition disables both SWP_PAGE_DISCARD and SWP_AREA_DISCARD.
>

This condition _only_ disables one of the new flags orthogonally if swapon(8)
flags a policy to sys_swapon. As old swapon(8) can only flag SWAP_FLAG_DISCARD,
the original behavior is kept. Nothing will change when one is using an old
swapon(8) with this changeset. 

 
> And you changed that SWP_DISCARDABLE is not checked in IO path  at all.
> 
> >-               if (si->flags & SWP_DISCARDABLE) {
> >+               if (si->flags & SWP_PAGE_DISCARD) {
> 

And this is exactly what this change is about -- only enabling that particular
I/O path if we've been told to discard swap page-clusters. Notice that having
SWP_PAGE_DISCARD flagged already implies SWP_DISCARDABLE.


> I suggest new swapon(8) don't pass SWP_DISCARDABLE and kernel handle
> SWP_DISCARDABLE as (SWAP_FLAG_DISCARD_ONCE | SWAP_FLAG_DISCARD_PAGES).

As the old swapon(8) case can only pass SWAP_FLAG_DISCARD along, this change
would nullify the backwards compatibility, wouldn't it?



> 
> Optionally, warn SWP_DISCARDABLE is a good idea.
> 
> 
> > will remain flagged when discard is enabled, so we keep doing discards the same way
> > we did before (at swapon, and for every released page-cluster).
> > The flags are removed orthogonally only when the new swapon(8) selects one of the
> > particular discard policy available by using either SWAP_FLAG_DISCARD_ONCE,
> > or SWAP_FLAG_DISCARD_PAGES flags.
> >

Re: [PATCH 01/02] swap: discard while swapping only if SWAP_FLAG_DISCARD_PAGES

[Rafael Aquini]

On Sun, May 26, 2013 at 10:55:32AM -0400, KOSAKI Motohiro wrote:
> On Sun, May 26, 2013 at 9:52 AM, Rafael Aquini <aquini@redhat.com> wrote:
> > On Sun, May 26, 2013 at 07:44:56AM -0400, KOSAKI Motohiro wrote:
> >> > +                       /*
> >> > +                        * By flagging sys_swapon, a sysadmin can tell us to
> >> > +                        * either do sinle-time area discards only, or to just
> >> > +                        * perform discards for released swap page-clusters.
> >> > +                        * Now it's time to adjust the p->flags accordingly.
> >> > +                        */
> >> > +                       if (swap_flags & SWAP_FLAG_DISCARD_ONCE)
> >> > +                               p->flags &= ~SWP_PAGE_DISCARD;
> >> > +                       else if (swap_flags & SWAP_FLAG_DISCARD_PAGES)
> >> > +                               p->flags &= ~SWP_AREA_DISCARD;
> >>
> >> When using old swapon(8), this code turn off both flags, right
> >
>  > As the flag that enables swap discards SWAP_FLAG_DISCARD remains meaning the
> > same it meant before, when using old swapon(8) (SWP_PAGE_DISCARD|SWP_AREA_DISCARD)
> 
> But old swapon(8) don't use neigher SWAP_FLAG_DISCARD_ONCE nor
> SWAP_FLAG_DISCARD_PAGES.  It uses only SWAP_FLAG_DISCARD. So, this
> condition disables both SWP_PAGE_DISCARD and SWP_AREA_DISCARD.
>

This condition _only_ disables one of the new flags orthogonally if swapon(8)
flags a policy to sys_swapon. As old swapon(8) can only flag SWAP_FLAG_DISCARD,
the original behavior is kept. Nothing will change when one is using an old
swapon(8) with this changeset. 

 
> And you changed that SWP_DISCARDABLE is not checked in IO path  at all.
> 
> >-               if (si->flags & SWP_DISCARDABLE) {
> >+               if (si->flags & SWP_PAGE_DISCARD) {
> 

And this is exactly what this change is about -- only enabling that particular
I/O path if we've been told to discard swap page-clusters. Notice that having
SWP_PAGE_DISCARD flagged already implies SWP_DISCARDABLE.


> I suggest new swapon(8) don't pass SWP_DISCARDABLE and kernel handle
> SWP_DISCARDABLE as (SWAP_FLAG_DISCARD_ONCE | SWAP_FLAG_DISCARD_PAGES).

As the old swapon(8) case can only pass SWAP_FLAG_DISCARD along, this change
would nullify the backwards compatibility, wouldn't it?



> 
> Optionally, warn SWP_DISCARDABLE is a good idea.
> 
> 
> > will remain flagged when discard is enabled, so we keep doing discards the same way
> > we did before (at swapon, and for every released page-cluster).
> > The flags are removed orthogonally only when the new swapon(8) selects one of the
> > particular discard policy available by using either SWAP_FLAG_DISCARD_ONCE,
> > or SWAP_FLAG_DISCARD_PAGES flags.
> >

--
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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 01/02] swap: discard while swapping only if SWAP_FLAG_DISCARD_PAGES

[KOSAKI Motohiro]

On Sun, May 26, 2013 at 11:30 AM, Rafael Aquini <aquini@redhat.com> wrote:
> On Sun, May 26, 2013 at 10:55:32AM -0400, KOSAKI Motohiro wrote:
>> On Sun, May 26, 2013 at 9:52 AM, Rafael Aquini <aquini@redhat.com> wrote:
>> > On Sun, May 26, 2013 at 07:44:56AM -0400, KOSAKI Motohiro wrote:
>> >> > +                       /*
>> >> > +                        * By flagging sys_swapon, a sysadmin can tell us to
>> >> > +                        * either do sinle-time area discards only, or to just
>> >> > +                        * perform discards for released swap page-clusters.
>> >> > +                        * Now it's time to adjust the p->flags accordingly.
>> >> > +                        */
>> >> > +                       if (swap_flags & SWAP_FLAG_DISCARD_ONCE)
>> >> > +                               p->flags &= ~SWP_PAGE_DISCARD;
>> >> > +                       else if (swap_flags & SWAP_FLAG_DISCARD_PAGES)
>> >> > +                               p->flags &= ~SWP_AREA_DISCARD;
>> >>
>> >> When using old swapon(8), this code turn off both flags, right
>> >
>>  > As the flag that enables swap discards SWAP_FLAG_DISCARD remains meaning the
>> > same it meant before, when using old swapon(8) (SWP_PAGE_DISCARD|SWP_AREA_DISCARD)
>>
>> But old swapon(8) don't use neigher SWAP_FLAG_DISCARD_ONCE nor
>> SWAP_FLAG_DISCARD_PAGES.  It uses only SWAP_FLAG_DISCARD. So, this
>> condition disables both SWP_PAGE_DISCARD and SWP_AREA_DISCARD.
>>
>
> This condition _only_ disables one of the new flags orthogonally if swapon(8)
> flags a policy to sys_swapon. As old swapon(8) can only flag SWAP_FLAG_DISCARD,
> the original behavior is kept. Nothing will change when one is using an old
> swapon(8) with this changeset.

Aha, got it. I misunderstood your code.
Thank you.

Acked-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>

Re: [PATCH 01/02] swap: discard while swapping only if SWAP_FLAG_DISCARD_PAGES

[KOSAKI Motohiro]

On Sun, May 26, 2013 at 11:30 AM, Rafael Aquini <aquini@redhat.com> wrote:
> On Sun, May 26, 2013 at 10:55:32AM -0400, KOSAKI Motohiro wrote:
>> On Sun, May 26, 2013 at 9:52 AM, Rafael Aquini <aquini@redhat.com> wrote:
>> > On Sun, May 26, 2013 at 07:44:56AM -0400, KOSAKI Motohiro wrote:
>> >> > +                       /*
>> >> > +                        * By flagging sys_swapon, a sysadmin can tell us to
>> >> > +                        * either do sinle-time area discards only, or to just
>> >> > +                        * perform discards for released swap page-clusters.
>> >> > +                        * Now it's time to adjust the p->flags accordingly.
>> >> > +                        */
>> >> > +                       if (swap_flags & SWAP_FLAG_DISCARD_ONCE)
>> >> > +                               p->flags &= ~SWP_PAGE_DISCARD;
>> >> > +                       else if (swap_flags & SWAP_FLAG_DISCARD_PAGES)
>> >> > +                               p->flags &= ~SWP_AREA_DISCARD;
>> >>
>> >> When using old swapon(8), this code turn off both flags, right
>> >
>>  > As the flag that enables swap discards SWAP_FLAG_DISCARD remains meaning the
>> > same it meant before, when using old swapon(8) (SWP_PAGE_DISCARD|SWP_AREA_DISCARD)
>>
>> But old swapon(8) don't use neigher SWAP_FLAG_DISCARD_ONCE nor
>> SWAP_FLAG_DISCARD_PAGES.  It uses only SWAP_FLAG_DISCARD. So, this
>> condition disables both SWP_PAGE_DISCARD and SWP_AREA_DISCARD.
>>
>
> This condition _only_ disables one of the new flags orthogonally if swapon(8)
> flags a policy to sys_swapon. As old swapon(8) can only flag SWAP_FLAG_DISCARD,
> the original behavior is kept. Nothing will change when one is using an old
> swapon(8) with this changeset.

Aha, got it. I misunderstood your code.
Thank you.

Acked-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>

--
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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 02/02] swapon: allow a more flexible swap discard policy

[KOSAKI Motohiro]

(5/26/13 12:31 AM), Rafael Aquini wrote:
> Introduce the necessary changes to swapon(8) allowing a sysadmin to leverage
> the new changes introduced to sys_swapon by "swap: discard while swapping
> only if SWAP_FLAG_DISCARD_PAGES", therefore allowing a more flexible set of
> choices when selection the discard policy for mounted swap areas.
> This patch introduces the following optional arguments to the already
> existent swapon(8) "--discard" option, in order to allow a discard type to 
> be selected at swapon time:
>  * once    : only single-time area discards are issued. (swapon)
>  * pages   : discard freed pages before they are reused.
> If no policy is selected both discard types are enabled. (default)
> 
> Signed-off-by: Rafael Aquini <aquini@redhat.com>

Acked-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>

Re: [PATCH 02/02] swapon: allow a more flexible swap discard policy

[KOSAKI Motohiro]

(5/26/13 12:31 AM), Rafael Aquini wrote:
> Introduce the necessary changes to swapon(8) allowing a sysadmin to leverage
> the new changes introduced to sys_swapon by "swap: discard while swapping
> only if SWAP_FLAG_DISCARD_PAGES", therefore allowing a more flexible set of
> choices when selection the discard policy for mounted swap areas.
> This patch introduces the following optional arguments to the already
> existent swapon(8) "--discard" option, in order to allow a discard type to 
> be selected at swapon time:
>  * once    : only single-time area discards are issued. (swapon)
>  * pages   : discard freed pages before they are reused.
> If no policy is selected both discard types are enabled. (default)
> 
> Signed-off-by: Rafael Aquini <aquini@redhat.com>

Acked-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>



--
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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 01/02] swap: discard while swapping only if SWAP_FLAG_DISCARD_PAGES

[Shaohua Li]

On Sun, May 26, 2013 at 01:31:55AM -0300, Rafael Aquini wrote:
> This patch introduces SWAP_FLAG_DISCARD_PAGES and SWAP_FLAG_DISCARD_ONCE
> new flags to allow more flexibe swap discard policies being flagged through
> swapon(8). The default behavior is to keep both single-time, or batched, area
> discards (SWAP_FLAG_DISCARD_ONCE) and fine-grained discards for page-clusters
> (SWAP_FLAG_DISCARD_PAGES) enabled, in order to keep consistentcy with older
> kernel behavior, as well as maintain compatibility with older swapon(8).
> However, through the new introduced flags the best suitable discard policy 
> can be selected accordingly to any given swap device constraint.

I'm sorry to response this thread so later. I thought if we just want to
discard the swap partition once at swapon, we really should do it in swapon
tool. The swapon tool can detect the swap device supports discard, any swap
partition is empty at swapon, and we have ioctl to do discard in userspace, so
we have no problem to do discard at the tool. If we don't want to do discard at
all, let the tool handles the option. Kernel is not the place to handle the
complexity.

Thanks,
Shaohua

Re: [PATCH 01/02] swap: discard while swapping only if SWAP_FLAG_DISCARD_PAGES

[Shaohua Li]

On Sun, May 26, 2013 at 01:31:55AM -0300, Rafael Aquini wrote:
> This patch introduces SWAP_FLAG_DISCARD_PAGES and SWAP_FLAG_DISCARD_ONCE
> new flags to allow more flexibe swap discard policies being flagged through
> swapon(8). The default behavior is to keep both single-time, or batched, area
> discards (SWAP_FLAG_DISCARD_ONCE) and fine-grained discards for page-clusters
> (SWAP_FLAG_DISCARD_PAGES) enabled, in order to keep consistentcy with older
> kernel behavior, as well as maintain compatibility with older swapon(8).
> However, through the new introduced flags the best suitable discard policy 
> can be selected accordingly to any given swap device constraint.

I'm sorry to response this thread so later. I thought if we just want to
discard the swap partition once at swapon, we really should do it in swapon
tool. The swapon tool can detect the swap device supports discard, any swap
partition is empty at swapon, and we have ioctl to do discard in userspace, so
we have no problem to do discard at the tool. If we don't want to do discard at
all, let the tool handles the option. Kernel is not the place to handle the
complexity.

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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 02/02] swapon: allow a more flexible swap discard policy

[Karel Zak]

On Sun, May 26, 2013 at 01:31:56AM -0300, Rafael Aquini wrote:
>  sys-utils/swapon.8 | 24 +++++++++++++------
>  sys-utils/swapon.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++--------
>  2 files changed, 77 insertions(+), 17 deletions(-)

 Applied, thanks.

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH 02/02] swapon: allow a more flexible swap discard policy

[Karel Zak]

On Sun, May 26, 2013 at 01:31:56AM -0300, Rafael Aquini wrote:
>  sys-utils/swapon.8 | 24 +++++++++++++------
>  sys-utils/swapon.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++--------
>  2 files changed, 77 insertions(+), 17 deletions(-)

 Applied, thanks.

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

--
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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>