nilfs-utils: skip inefficient gc operations

With this option the user can specify the minimal number of free/dead blocks before a segment can be cleaned. This is a threshold for the GC to prevent needless moving of data. If there are less free blocks to gain from cleaning a segment than the specified number, the GC will abort and try again with a different segment.

Will your patches be smart enough to re-claim space if the only available free blocks are under threshold?

Scenario: FS is running out of free space, and the free blocks to be reclaimed are scattered through segments in small numbers. Several segments have a small number of free blocks, but in each segment the amount is under the threshold.

With this option the user can specify the minimal number of free/dead blocks before a segment can be cleaned. This is a threshold for the GC to prevent needless moving of data. If there are less free blocks to gain from cleaning a segment than the specified number, the GC will abort and try again with a different segment.

With this option the user can specify the minimal number of free/dead blocks before a segment can be cleaned.

This is a threshold for the GC
to prevent needless moving of data. If there are less free blocks to gain from cleaning a segment than the specified number, the GC will abort and try again with a different segment.

By setting it to 0 this feature is effectively disabled.

Signed-off-by Andreas Rohner <[email protected]>

---

include/nilfs_cleaner.h           | 19 ++++++++++---------
sbin/cleanerd/cldconfig.c         | 40 +++++++++++++++++++++++++++++++++++++++
sbin/cleanerd/cldconfig.h         |  4 ++++
sbin/cleanerd/cleanerd.c          | 23 ++++++++++++++++++++++
sbin/cleanerd/nilfs_cleanerd.conf |  9 +++++++++
sbin/nilfs-clean/nilfs-clean.c    | 18 ++++++++++++++----
6 files changed, 100 insertions(+), 13 deletions(-)

include/nilfs_cleaner.h +1 -8

index 0bf02aa..42c4aa7 100644
@@ -46,17 +46,18 @@ struct nilfs_cleaner_args {
 	uint64_t start_segnum;	/* start segment number */
 	uint64_t nsegs;		/* number of segments */
 	uint32_t runtime; /* runtime in seconds */
-	uint32_t pad2;
+	uint32_t min_free_blocks_threshold;
 };
 /* valid flags */
-#define NILFS_CLEANER_ARG_PROTECTION_PERIOD	(1 << 0)
-#define NILFS_CLEANER_ARG_NSEGMENTS_PER_CLEAN	(1 << 1)
-#define NILFS_CLEANER_ARG_CLEANING_INTERVAL	(1 << 2)
-#define NILFS_CLEANER_ARG_USAGE_RATE_THRESHOLD	(1 << 3) /* reserved */
-#define NILFS_CLEANER_ARG_START_SEGNUM		(1 << 4) /* reserved */
-#define NILFS_CLEANER_ARG_NSEGS			(1 << 5) /* reserved */
-#define NILFS_CLEANER_ARG_NPASSES		(1 << 6) /* reserved */

include/nilfs_cleaner.h +0 -0

 enum {
 	NILFS_CLEANER_STATUS_IDLE,

sbin/cleanerd/cldconfig.c +0 -0

index 270d360..4d7589c 100644
@@ -456,6 +456,34 @@ nilfs_cldconfig_handle_mc_nsegments_per_clean(struct nilfs_cldconfig *config,
 }
 

static int

sbin/cleanerd/cldconfig.c +28 -0

+nilfs_cldconfig_handle_min_free_blocks(struct nilfs_cldconfig *config,
+				       char **tokens, size_t ntoks,
+				       struct nilfs *nilfs)
+{
+	unsigned long n;
+
+	if (nilfs_cldconfig_get_ulong_argument(tokens, ntoks, &n) < 0)
+		return 0;
+
+	config->cf_min_free_blocks_threshold = n;
+	return 0;
+}
+
+static int
+nilfs_cldconfig_handle_mc_min_free_blocks(struct nilfs_cldconfig *config,
+					  char **tokens, size_t ntoks,
+					  struct nilfs *nilfs)
+{
+	unsigned long n;
+
+	if (nilfs_cldconfig_get_ulong_argument(tokens, ntoks, &n) < 0)
+		return 0;
+
+	config->cf_mc_min_free_blocks_threshold = n;
+	return 0;
+}
+
+static int

nilfs_cldconfig_handle_cleaning_interval(struct nilfs_cldconfig *config,
char **tokens, size_t ntoks, struct nilfs *nilfs)

sbin/cleanerd/cldconfig.c +12 -0

@@ -576,6 +604,14 @@ nilfs_cldconfig_keyword_table[] = {
 		"log_priority", 2, 2,
 		nilfs_cldconfig_handle_log_priority
 	},
+	{
+		"min_free_blocks_threshold", 2, 2,
+		nilfs_cldconfig_handle_min_free_blocks
+	},
+	{
+		"mc_min_free_blocks_threshold", 2, 2,
+		nilfs_cldconfig_handle_mc_min_free_blocks
+	},
 };
 
 #define NILFS_CLDCONFIG_NKEYWORDS			\

@@ -641,6 +677,10 @@ static void nilfs_cldconfig_set_default(struct nilfs_cldconfig *config,
 	config->cf_retry_interval.tv_usec = 0;
 	config->cf_use_mmap = NILFS_CLDCONFIG_USE_MMAP;
 	config->cf_log_priority = NILFS_CLDCONFIG_LOG_PRIORITY;
+	config->cf_min_free_blocks_threshold =
+		NILFS_CLDCONFIG_MIN_FREE_BLOCKS_THRESHOLD;
+	config->cf_mc_min_free_blocks_threshold =
+		NILFS_CLDCONFIG_MC_MIN_FREE_BLOCKS_THRESHOLD;
 }
 
 static inline int iseol(int c)

sbin/cleanerd/cldconfig.h +2 -0

index 188ce9b..49ee6be 100644
@@ -102,6 +102,8 @@ struct nilfs_cldconfig {
 	struct timeval cf_retry_interval;
 	int cf_use_mmap;
 	int cf_log_priority;
+	unsigned long cf_min_free_blocks_threshold;
+	unsigned long cf_mc_min_free_blocks_threshold;
 };
 
 #define NILFS_CLDCONFIG_SELECTION_POLICY_IMPORTANCE	\

Quoted 2 lines · Andreas Rohner

With this option the user can specify the minimal number of free/dead blocks before a segment can be cleaned.

"Free or dead" or "free and dead"? What do you mean by free blocks? "Free blocks" sounds for me as blocks that I can allocate and to use immediately without any operations. If you mean invalid blocks (blocks that it was updated in this segment) then I need to do garbage collection operation for making it free. So, free blocks are really confusing term. And "dead blocks" sounds for me not very good.

Quoted 41 lines

This is a threshold for the GC
to prevent needless moving of data. If there are less free blocks to gain from cleaning a segment than the specified number, the GC will abort and try again with a different segment.

By setting it to 0 this feature is effectively disabled.

Signed-off-by Andreas Rohner <[email protected]>

---

include/nilfs_cleaner.h           | 19 ++++++++++---------
sbin/cleanerd/cldconfig.c         | 40 +++++++++++++++++++++++++++++++++++++++
sbin/cleanerd/cldconfig.h         |  4 ++++
sbin/cleanerd/cleanerd.c          | 23 ++++++++++++++++++++++
sbin/cleanerd/nilfs_cleanerd.conf |  9 +++++++++
sbin/nilfs-clean/nilfs-clean.c    | 18 ++++++++++++++----
6 files changed, 100 insertions(+), 13 deletions(-)

include/nilfs_cleaner.h +5 -9

index 0bf02aa..42c4aa7 100644
@@ -46,17 +46,18 @@ struct nilfs_cleaner_args {
 	uint64_t start_segnum;	/* start segment number */
 	uint64_t nsegs;		/* number of segments */
 	uint32_t runtime; /* runtime in seconds */
-	uint32_t pad2;
+	uint32_t min_free_blocks_threshold;
 };
 /* valid flags */
-#define NILFS_CLEANER_ARG_PROTECTION_PERIOD	(1 << 0)
-#define NILFS_CLEANER_ARG_NSEGMENTS_PER_CLEAN	(1 << 1)
-#define NILFS_CLEANER_ARG_CLEANING_INTERVAL	(1 << 2)
-#define NILFS_CLEANER_ARG_USAGE_RATE_THRESHOLD	(1 << 3) /* reserved */
-#define NILFS_CLEANER_ARG_START_SEGNUM		(1 << 4) /* reserved */
-#define NILFS_CLEANER_ARG_NSEGS			(1 << 5) /* reserved */
-#define NILFS_CLEANER_ARG_NPASSES		(1 << 6) /* reserved */
-#define NILFS_CLEANER_ARG_RUNTIME		(1 << 7) /* reserved */
+#define NILFS_CLEANER_ARG_PROTECTION_PERIOD		(1 << 0)
+#define NILFS_CLEANER_ARG_NSEGMENTS_PER_CLEAN		(1 << 1)
+#define NILFS_CLEANER_ARG_CLEANING_INTERVAL		(1 << 2)
+#define NILFS_CLEANER_ARG_USAGE_RATE_THRESHOLD		(1 << 3) /* reserved */

include/nilfs_cleaner.h struct nilfs_cleaner_args { · line 60

Quoted 5 lines

include/nilfs_cleaner.h +5 -0

+#define NILFS_CLEANER_ARG_START_SEGNUM			(1 << 4) /* reserved */
+#define NILFS_CLEANER_ARG_NSEGS				(1 << 5) /* reserved */
+#define NILFS_CLEANER_ARG_NPASSES			(1 << 6) /* reserved */
+#define NILFS_CLEANER_ARG_RUNTIME			(1 << 7) /* reserved */
+#define NILFS_CLEANER_ARG_MIN_FREE_BLOCKS_THRESHOLD	(1 << 8)

What about NILFS_CLEANER_ARG_MIN_FBLK_THRESHOLD?
But free blocks really confuse me.

include/nilfs_cleaner.h struct nilfs_cldconfig { · line 126

Quoted 7 lines

include/nilfs_cleaner.h +2 -0

 #define NILFS_CLDCONFIG_SELECTION_POLICY_IMPORTANCE	\

@@ -120,6 +122,8 @@ struct nilfs_cldconfig {
 #define NILFS_CLDCONFIG_RETRY_INTERVAL			60
 #define NILFS_CLDCONFIG_USE_MMAP			1
 #define NILFS_CLDCONFIG_LOG_PRIORITY			LOG_INFO
+#define NILFS_CLDCONFIG_MIN_FREE_BLOCKS_THRESHOLD	256
+#define NILFS_CLDCONFIG_MC_MIN_FREE_BLOCKS_THRESHOLD	128

Why 256 or 128? What is good? What is bad?
I am a user. How can I define what value is appropriate?
How should I think for choosing some value?
For example, I want to have the fastest performance as possible. How can I define an appropriate value? Or, maybe, I want to clean as many segments as possible. How should I think?

Or, maybe, I want combination of GC policies.

This patch adds the custom error value EGCTRYAGAIN, which signals to cleanerd to retry the GC process as soon as possible with no timeout.

If the GC decides not to do any real work and only changes a few metadata bytes, there is no need for a timeout. Furthermore if the GC is running, there is probably not enough free space on the device and it is crucial to retry quickly.

Signed-off-by Andreas Rohner <[email protected]>

---
include/nilfs_gc.h | 2 ++
sbin/cleanerd/cleanerd.c | 10 +++++++++-

2 files changed, 11 insertions(+), 1 deletion(-)

include/nilfs_gc.h +0 -0

index 72e9947..7628ce1 100644

Quoted 3 lines · Andreas Rohner

This patch adds the custom error value EGCTRYAGAIN, which signals to cleanerd to retry the GC process as soon as possible with no timeout.

Quoted 5 lines

If the GC decides not to do any real work and only changes a few metadata bytes, there is no need for a timeout. Furthermore if the GC is running, there is probably not enough free space on the device and it is crucial to retry quickly.

What does it defend us from eating 100% of CPU?

Quoted 12 lines

Signed-off-by Andreas Rohner <[email protected]>

---
include/nilfs_gc.h | 2 ++
sbin/cleanerd/cleanerd.c | 10 +++++++++-

2 files changed, 11 insertions(+), 1 deletion(-)

include/nilfs_gc.h +0 -0

index 72e9947..7628ce1 100644
@@ -27,4 +27,6 @@ static inline int nilfs_suinfo_reclaimable(const struct nilfs_suinfo *si)
 

include/nilfs_gc.h static inline int nilfs_suinfo_reclaimable(const struct nilfs_suinfo *si) · line 30

Quoted 3 lines

include/nilfs_gc.h +1 -0

 extern void (*nilfs_gc_logger)(int priority, const char *fmt, ...);
 
+#define EGCTRYAGAIN 513

Why do you declare a new constant? What does it means value of 513? What bad in EAGAIN?

nilfs_reclaim_segment gathers all the necessary information to decide, how many reclaimable blocks there are in the segments that are to be cleaned.

By enabling the passing of the threshold value
minblocks, nilfs_reclaim_segment can check the actual value against the threshold.

Signed-off-by Andreas Rohner <[email protected]>

---
 include/nilfs_gc.h               | 4 ++--
 lib/gc.c                         | 5 +++--
 sbin/cleanerd/cleanerd.c         | 5 +++--
 sbin/nilfs-resize/nilfs-resize.c | 2 +-
 4 files changed, 9 insertions(+), 7 deletions(-)

include/nilfs_gc.h +0 -0

index 7628ce1..f9fbde4 100644

include/nilfs_gc.h +0 -0

 static inline int nilfs_suinfo_reclaimable(const struct nilfs_suinfo *si)

lib/gc.c +3 -2

index 1152299..0b0e2d6 100644
@@ -607,10 +607,11 @@ static int nilfs_toss_bdescs(struct nilfs_vector *bdescv)
  * @nsegs: size of the @segnums array
  * @protseq: start of sequence number of protected segments
  * @protcno: start checkpoint number of protected period
+ * @minblocks: minimal number of free blocks in a segment
  */
 ssize_t nilfs_reclaim_segment(struct nilfs *nilfs,
-			      __u64 *segnums, size_t nsegs,
-			      __u64 protseq, nilfs_cno_t protcno)
+			      __u64 *segnums, size_t nsegs, __u64 protseq,
+			      nilfs_cno_t protcno, unsigned long minblocks)
 {
 	struct nilfs_vector *vdescv, *bdescv, *periodv, *vblocknrv;
 	sigset_t sigset, oldset, waitset;

sbin/cleanerd/cleanerd.c +3 -2

index 575656b..6dd9d24 100644
@@ -1372,8 +1372,9 @@ static ssize_t nilfs_cleanerd_clean_segments(struct nilfs_cleanerd *cleanerd,
 	}
 	cleanerd->no_timeout = 0;
 
-	ret = nilfs_reclaim_segment(cleanerd->nilfs, segnums, nsegs,
-				    protseq, protcno);
+	ret = nilfs_reclaim_segment(cleanerd->nilfs, segnums,
+			nsegs, protseq, protcno,
+			nilfs_cleanerd_min_free_blocks_threshold(cleanerd));
 	if (ret > 0) {
 		for (i = 0; i < ret; i++)
 			syslog(LOG_DEBUG, "segment %llu cleaned",

sbin/nilfs-resize/nilfs-resize.c +1 -1

index c8b0868..2850268 100644
@@ -595,7 +595,7 @@ static ssize_t nilfs_resize_move_segments(struct nilfs *nilfs,
 			return -1;
 
 		ret = nilfs_reclaim_segment(nilfs, snp, nc,
-					    sustat.ss_prot_seq, 0);
+					    sustat.ss_prot_seq, 0, 0);
 		if (ret < 0)
 			return -1;
 

Signature · 6 lines

1.8.5.3

--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

lib/gc.c +11 -1

index 0b0e2d6..ebbe0ca 100644
@@ -29,6 +29,10 @@
 #include <syslog.h>
 #endif	/* HAVE_SYSLOG_H */
 
+#if HAVE_SYS_TIME_H
+#include <sys/time.h>
+#endif	/* HAVE_SYS_TIME */
+
 #include <errno.h>
 #include <assert.h>
 #include <stdarg.h>

@@ -615,7 +619,10 @@ ssize_t nilfs_reclaim_segment(struct nilfs *nilfs,
 {
 	struct nilfs_vector *vdescv, *bdescv, *periodv, *vblocknrv;
 	sigset_t sigset, oldset, waitset;
-	ssize_t n, ret = -1;
+	ssize_t n, i, ret = -1;
+	__u32 freeblocks;
+	struct nilfs_suinfo_update *supv;
+	struct timeval tv;
 
 	if (nsegs == 0)
 		return 0;

@@ -678,6 +685,41 @@ ssize_t nilfs_reclaim_segment(struct nilfs *nilfs,
 		goto out_lock;
 	}
 
+	freeblocks = (nilfs_get_blocks_per_segment(nilfs) * n)
+				- (nilfs_vector_get_size(vdescv)
+				+ nilfs_vector_get_size(bdescv));

On Jan 21, 2014, at 4:59 PM, Andreas Rohner wrote:

[snip]

Quoted 31 lines

lib/gc.c +8 -1

index 0b0e2d6..ebbe0ca 100644
@@ -29,6 +29,10 @@
 #include <syslog.h>
 #endif	/* HAVE_SYSLOG_H */
 
+#if HAVE_SYS_TIME_H
+#include <sys/time.h>
+#endif	/* HAVE_SYS_TIME */
+
 #include <errno.h>
 #include <assert.h>
 #include <stdarg.h>

@@ -615,7 +619,10 @@ ssize_t nilfs_reclaim_segment(struct nilfs *nilfs,
 {
 	struct nilfs_vector *vdescv, *bdescv, *periodv, *vblocknrv;
 	sigset_t sigset, oldset, waitset;
-	ssize_t n, ret = -1;
+	ssize_t n, i, ret = -1;
+	__u32 freeblocks;
+	struct nilfs_suinfo_update *supv;
+	struct timeval tv;
 
 	if (nsegs == 0)
 		return 0;

@@ -678,6 +685,41 @@ ssize_t nilfs_reclaim_segment(struct nilfs *nilfs,
 		goto out_lock;
 	}
 

lib/gc.c ssize_t nilfs_reclaim_segment(struct nilfs *nilfs, · line 700

Quoted 13 lines

lib/gc.c +13 -0

+	freeblocks = (nilfs_get_blocks_per_segment(nilfs) * n)
+				- (nilfs_vector_get_size(vdescv)
+				+ nilfs_vector_get_size(bdescv));
+
+	/* if there are less free blocks than the
+	 * minimal threshold try to update suinfo
+	 * instead of cleaning */
+	if (freeblocks < minblocks * n) {
+		ret = gettimeofday(&tv, NULL);
+		if (ret < 0)
+			goto out_lock;
+
+		supv = malloc(sizeof(struct nilfs_suinfo_update) * n);

Is it enough to allocate memory without zeroing?

Do you free allocated memory in the case of error?

Quoted 2 lines

Do you free allocated memory in the case of error?

Yes I immediately free it before I check the return value. Could you please read the code a bit more carefully, before you ask such questions?

With this ioctl the segment usage information in the SUFILE can be updated from userspace.

This is useful, because it allows the GC to modify and update segment usage entries for specific segments, which enables it to avoid unnecessary write operations.

If a segment needs to be cleaned, but there is no or very little free space to be gained, the cleaning operation basically degrades to needless expensive copying of data. In the end the only thing that changes is the location of the data and a timestamp in the segment usage info. With this ioctl the GC can skip the copying and update the segment usage entries directly instead.

Additionally this patch implements a simple check in nilfs_reclaim_segment. If the number of free blocks that can be gained by cleaning the segments is below the threshold of minblocks, it simply updates the segment usage information instead.

Signed-off-by Andreas Rohner <[email protected]>

---
 include/nilfs.h     |  2 ++
 include/nilfs2_fs.h | 41 +++++++++++++++++++++++++++++++++++++++++
 lib/gc.c            | 44 +++++++++++++++++++++++++++++++++++++++++++-
 lib/nilfs.c         | 26 ++++++++++++++++++++++++++
 4 files changed, 112 insertions(+), 1 deletion(-)

include/nilfs.h +2 -0

index 56286a9..1e9b034 100644
@@ -299,6 +299,8 @@ int nilfs_delete_checkpoint(struct nilfs *, nilfs_cno_t);
 int nilfs_get_cpstat(const struct nilfs *, struct nilfs_cpstat *);
 ssize_t nilfs_get_suinfo(const struct nilfs *, __u64, struct nilfs_suinfo *,
 			 size_t);
+ssize_t nilfs_set_suinfo(const struct nilfs *, struct nilfs_suinfo_update *,
+			 size_t);
 int nilfs_get_sustat(const struct nilfs *, struct nilfs_sustat *);
 ssize_t nilfs_get_vinfo(const struct nilfs *, struct nilfs_vinfo *, size_t);
 ssize_t nilfs_get_bdescs(const struct nilfs *, struct nilfs_bdesc *, size_t);

include/nilfs2_fs.h +41 -0

index e674f44..181c482 100644
@@ -713,6 +713,45 @@ static inline int nilfs_suinfo_clean(const struct nilfs_suinfo *si)
 	return !si->sui_flags;
 }
 
+/**
+ * nilfs_suinfo_update - segment usage information update
+ * @sup_segnum: segment number
+ * @sup_flags: flags for which fields are active in sup_sui
+ * @sup_sui: segment usage information
+ */
+struct nilfs_suinfo_update {
+	__u64 sup_segnum;
+	__u32 sup_flags;
+	struct nilfs_suinfo sup_sui;
+};
+
+enum {
+	NILFS_SUINFO_UPDATE_LASTMOD,
+	NILFS_SUINFO_UPDATE_NBLOCKS,
+	NILFS_SUINFO_UPDATE_FLAGS,
+};
+
+#define NILFS_SUINFO_UPDATE_FNS(flag, name)				\
+static inline void							\
+nilfs_suinfo_update_set_##name(struct nilfs_suinfo_update *sup)		\
+{									\
+	sup->sup_flags |= 1UL << NILFS_SUINFO_UPDATE_##flag;		\
+}									\
+static inline void							\
+nilfs_suinfo_update_clear_##name(struct nilfs_suinfo_update *sup)	\
+{									\
+	sup->sup_flags &= ~(1UL << NILFS_SUINFO_UPDATE_##flag);		\
+}									\
+static inline int							\
+nilfs_suinfo_update_##name(const struct nilfs_suinfo_update *sup)	\
+{									\
+	return !!(sup->sup_flags & (1UL << NILFS_SUINFO_UPDATE_##flag));\
+}
+
+NILFS_SUINFO_UPDATE_FNS(LASTMOD, lastmod)
+NILFS_SUINFO_UPDATE_FNS(NBLOCKS, nblocks)
+NILFS_SUINFO_UPDATE_FNS(FLAGS, flags)
+
 /* ioctl */
 enum {
 	NILFS_CHECKPOINT,

@@ -867,5 +906,7 @@ struct nilfs_bdesc {
 	_IOW(NILFS_IOCTL_IDENT, 0x8B, __u64)
 #define NILFS_IOCTL_SET_ALLOC_RANGE  \
 	_IOW(NILFS_IOCTL_IDENT, 0x8C, __u64[2])
+#define NILFS_IOCTL_SET_SUINFO  \
+	_IOW(NILFS_IOCTL_IDENT, 0x8D, struct nilfs_argv)
 
 #endif	/* _LINUX_NILFS_FS_H */

lib/gc.c +31 -1

index 0b0e2d6..ebbe0ca 100644
@@ -29,6 +29,10 @@
 #include <syslog.h>
 #endif	/* HAVE_SYSLOG_H */
 
+#if HAVE_SYS_TIME_H
+#include <sys/time.h>
+#endif	/* HAVE_SYS_TIME */
+
 #include <errno.h>
 #include <assert.h>
 #include <stdarg.h>

@@ -615,7 +619,10 @@ ssize_t nilfs_reclaim_segment(struct nilfs *nilfs,
 {
 	struct nilfs_vector *vdescv, *bdescv, *periodv, *vblocknrv;
 	sigset_t sigset, oldset, waitset;
-	ssize_t n, ret = -1;
+	ssize_t n, i, ret = -1;
+	__u32 freeblocks;
+	struct nilfs_suinfo_update *supv;
+	struct timeval tv;
 
 	if (nsegs == 0)
 		return 0;

@@ -678,6 +685,41 @@ ssize_t nilfs_reclaim_segment(struct nilfs *nilfs,
 		goto out_lock;
 	}
 
+	freeblocks = (nilfs_get_blocks_per_segment(nilfs) * n)
+				- (nilfs_vector_get_size(vdescv)
+				+ nilfs_vector_get_size(bdescv));
+
+	/* if there are less free blocks than the
+	 * minimal threshold try to update suinfo
+	 * instead of cleaning */
+	if (freeblocks < minblocks * n) {
+		ret = gettimeofday(&tv, NULL);
+		if (ret < 0)
+			goto out_lock;
+
+		supv = malloc(sizeof(struct nilfs_suinfo_update) * n);
+		if (supv == NULL) {
+			ret = -1;
+			goto out_lock;
+		}
+
+		for (i = 0; i < n; ++i) {
+			supv[i].sup_segnum = segnums[i];
+			supv[i].sup_flags = 0;
+			nilfs_suinfo_update_set_lastmod(&supv[i]);
+			supv[i].sup_sui.sui_lastmod = tv.tv_sec;

lib/gc.c +2 -0

+	}
+
 	ret = nilfs_clean_segments(nilfs,
 				   nilfs_vector_get_data(vdescv),
 				   nilfs_vector_get_size(vdescv),

lib/nilfs.c +0 -0

index 7265830..dbd03e6 100644

lib/nilfs.c +24 -0

+ * @sup: an array of nilfs_suinfo_update structs
+ * @nsup: number of elements in sup
+ */
+ssize_t nilfs_set_suinfo(const struct nilfs *nilfs,
+			 struct nilfs_suinfo_update *sup, size_t nsup)
+{
+	struct nilfs_argv argv;
+
+	if (nilfs->n_iocfd < 0) {
+		errno = EBADF;
+		return -1;
+	}
+
+	argv.v_base = (unsigned long)sup;
+	argv.v_nmembs = nsup;
+	argv.v_size = sizeof(struct nilfs_suinfo_update);
+	argv.v_index = 0;
+	argv.v_flags = 0;
+	if (ioctl(nilfs->n_iocfd, NILFS_IOCTL_SET_SUINFO, &argv) < 0)
+		return -1;
+	return argv.v_nmembs;
+}
+
+/**
  * nilfs_get_sustat -
  * @nilfs:
  * @sustat:

Signature · 6 lines

1.8.5.3

--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Quoted 23 lines

lib/gc.c +23 -0

+	freeblocks = (nilfs_get_blocks_per_segment(nilfs) * n)
+				- (nilfs_vector_get_size(vdescv)
+				+ nilfs_vector_get_size(bdescv));
+
+	/* if there are less free blocks than the
+	 * minimal threshold try to update suinfo
+	 * instead of cleaning */
+	if (freeblocks < minblocks * n) {
+		ret = gettimeofday(&tv, NULL);
+		if (ret < 0)
+			goto out_lock;
+
+		supv = malloc(sizeof(struct nilfs_suinfo_update) * n);
+		if (supv == NULL) {
+			ret = -1;
+			goto out_lock;
+		}
+
+		for (i = 0; i < n; ++i) {
+			supv[i].sup_segnum = segnums[i];
+			supv[i].sup_flags = 0;
+			nilfs_suinfo_update_set_lastmod(&supv[i]);
+			supv[i].sup_sui.sui_lastmod = tv.tv_sec;

lib/gc.c ssize_t nilfs_reclaim_segment(struct nilfs *nilfs, · line 720

Quoted 10 lines

lib/gc.c +10 -0

+		}
+
+		ret = nilfs_set_suinfo(nilfs, supv, n);
+		free(supv);
+		if (ret >= 0) {
+			/* success, tell caller
+			 * to try another segment/s */
+			ret = -EGCTRYAGAIN;
+			goto out_lock;
+		}

You should design the new reclaim function so that it turns off the logic using nilfs_set_suinfo() once nilfs_set_suinfo() returned a status code < 0 and errno was ENOTTY.

This fallback logic is needed to keep compatibility for old kernel.

In addition, whether applying the optimization or not, should be selectable in /etc/nilfs_cleanerd.conf.

Quoted 5 lines · Ryusuke Konishi

You should design the new reclaim function so that it turns off the logic using nilfs_set_suinfo() once nilfs_set_suinfo() returned a status code < 0 and errno was ENOTTY.

This fallback logic is needed to keep compatibility for old kernel.

Thanks for your review. I am quite devastated how much needs to be fixed :). I implemented most of your suggestions. I will test them tomorrow and compose a new version of the patch set.

Quoted 2 lines

In addition, whether applying the optimization or not, should be selectable in /etc/nilfs_cleanerd.conf.

You can set the two options to 0, which would completely disable the optimization.

# Minimum number of free blocks before a segment

# can be cleaned. Set to 0 to disable it.
min_free_blocks_threshold	0

# Minimum number of free blocks before a segment

# can be cleaned. Set to 0 to disable it.
# if clean segments < min_clean_segments
mc_min_free_blocks_threshold	0

Is that enough or should I add an additional switch, like the use_mmap switch?

Quoted 8 lines · Ryusuke Konishi

lib/gc.c ssize_t nilfs_reclaim_segment(struct nilfs *nilfs, · line 720

Quoted 8 lines

lib/gc.c +8 -0

+		ret = nilfs_set_suinfo(nilfs, supv, n);
+		free(supv);
+		if (ret >= 0) {
+			/* success, tell caller
+			 * to try another segment/s */
+			ret = -EGCTRYAGAIN;
+			goto out_lock;
+		}

Quoted 9 lines · Andreas Rohner

Quoted 8 lines · Ryusuke Konishi

Review

Quoted 8 lines

(quoted diff) +8 -0

+		ret = nilfs_set_suinfo(nilfs, supv, n);
+		free(supv);
+		if (ret >= 0) {
+			/* success, tell caller
+			 * to try another segment/s */
+			ret = -EGCTRYAGAIN;
+			goto out_lock;
+		}

Returning -EGCTRYAGAIN looks confusing, since posix error code Exxxxx is usually stored in errno variable in userland. It also looks a bit hacky.

How about handling this as follows ?

- Add a new structure to return result of GC, for instance, as follows:

struct nilfs_reclaim_status {
unsigned int ncleanedsegs; /* number of segments cleaned
with nilfs_clean_segments() */
unsigned int ndeferredsegs;/* number of segments deferred
with nilfs_set_suinfo() */
unsigned long nliveblocks; /* sum of live virtual blocks and live
DAT blocks */
unsigned long ndeadblocks; /* number of reclaimable blocks */
unsigned long nlivevblocks; /* live virtual blocks */
unsigned long nfreevblocks; /* freed virtual block addresses */
...
};

- Use the structure as an argument of the "new" API function (i.e. variant of nilfs_reclaim_segment() )

- Rewrite the variant API function so that it returns a success value
(0) also for the EGCTRYAGAIN case.

- Store other status information in nilfs_reclaim_status structures.

And, I think adding nilfs_set_suinfo() API should be a separate patch.

This patch introduces the nilfs_sufile_set_suinfo function, which expects an array of nilfs_suinfo_update structures and updates the segment usage information accordingly.

This is basically a helper function for the newly introduced NILFS_IOCTL_SET_SUINFO ioctl.

Signed-off-by Andreas Rohner <[email protected]>

---
 fs/nilfs2/sufile.c | 91 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 fs/nilfs2/sufile.h |  2 +-
 2 files changed, 92 insertions(+), 1 deletion(-)

fs/nilfs2/sufile.c +0 -0

index 3127e9f..81c3438 100644

fs/nilfs2/sufile.c +35 -0

+ * @sufile: inode of segment usage file
+ * @buf: array of suinfo
+ * @supsz: byte size of suinfo
+ * @nsup: size of suinfo array
+ *
+ * Description: Takes an array of nilfs_suinfo_update structs and updates
+ * segment usage accordingly.
+ *
+ * Return Value: On success, 0 is returned and .... On error, one of the
+ * following negative error codes is returned.
+ *
+ * %-EIO - I/O error.
+ *
+ * %-ENOMEM - Insufficient amount of memory available.
+ *
+ * %-EINVAL - Invalid segment number in input
+ */
+ssize_t nilfs_sufile_set_suinfo(struct inode *sufile, void *buf,
+				unsigned supsz, size_t nsup)
+{
+	struct buffer_head *bh;
+	struct nilfs_suinfo_update *sup, *supv = buf;
+	struct nilfs_segment_usage *su;
+	void *kaddr;
+	unsigned long blkoff, prev_blkoff;
+	size_t nerr = 0;
+	int ret = 0;
+
+	if (unlikely(nsup == 0))
+		goto out;
+
+	down_write(&NILFS_MDT(sufile)->mi_sem);
+	for (sup = supv; sup < supv + nsup; sup++) {
+		if (unlikely(sup->sup_segnum >=
+				nilfs_sufile_get_nsegments(sufile))) {

fs/nilfs2/sufile.c +10 -0

+
+	sup = supv;
+	blkoff = nilfs_sufile_get_blkoff(sufile, sup->sup_segnum);
+	ret = nilfs_mdt_get_block(sufile, blkoff, 1, NULL, &bh);
+	if (ret < 0)
+		goto out_sem;
+
+	for (;;) {
+		kaddr = kmap_atomic(bh->b_page);
+		su = nilfs_sufile_block_get_segment_usage(

fs/nilfs2/sufile.c +7 -0

+ out_sem:
+	up_write(&NILFS_MDT(sufile)->mi_sem);
+ out:
+	return ret;
+}
+
+/**
  * nilfs_sufile_read - read or get sufile inode
  * @sb: super block instance
  * @susize: size of a segment usage entry

fs/nilfs2/sufile.h +1 -1

index e84bc5b..7aa9d3e 100644
@@ -44,7 +44,7 @@ int nilfs_sufile_set_segment_usage(struct inode *sufile, __u64 segnum,
 int nilfs_sufile_get_stat(struct inode *, struct nilfs_sustat *);
 ssize_t nilfs_sufile_get_suinfo(struct inode *, __u64, void *, unsigned,
 				size_t);
-
+ssize_t nilfs_sufile_set_suinfo(struct inode *, void *, unsigned , size_t);
 int nilfs_sufile_updatev(struct inode *, __u64 *, size_t, int, size_t *,
 			 void (*dofunc)(struct inode *, __u64,
 					struct buffer_head *,

Signature · 6 lines

1.8.5.3

--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Quoted 18 lines · Andreas Rohner

This patch introduces the nilfs_sufile_set_suinfo function, which expects an array of nilfs_suinfo_update structures and updates the segment usage information accordingly.

This is basically a helper function for the newly introduced NILFS_IOCTL_SET_SUINFO ioctl.

Signed-off-by Andreas Rohner <[email protected]>

---
 fs/nilfs2/sufile.c | 91 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 fs/nilfs2/sufile.h |  2 +-
 2 files changed, 92 insertions(+), 1 deletion(-)

fs/nilfs2/sufile.c +0 -0

index 3127e9f..81c3438 100644

fs/nilfs2/sufile.c ssize_t nilfs_sufile_get_suinfo(struct inode *sufile, __u64 segnum, void *buf, · line 873

Quoted 4 lines

fs/nilfs2/sufile.c +1 -0

@@ -870,6 +870,97 @@ ssize_t nilfs_sufile_get_suinfo(struct inode *sufile, __u64 segnum, void *buf,
 }
 
 /**
+ * nilfs_sufile_set_suinfo -

Please fill the summary line of function even though nilfs_sufile_get_suinfo() function lacks it.

* nilfs_sufile_set_suinfo - update segment usage information

Quoted 35 lines

fs/nilfs2/sufile.c +35 -0

+ * @sufile: inode of segment usage file
+ * @buf: array of suinfo
+ * @supsz: byte size of suinfo
+ * @nsup: size of suinfo array
+ *
+ * Description: Takes an array of nilfs_suinfo_update structs and updates
+ * segment usage accordingly.
+ *
+ * Return Value: On success, 0 is returned and .... On error, one of the
+ * following negative error codes is returned.
+ *
+ * %-EIO - I/O error.
+ *
+ * %-ENOMEM - Insufficient amount of memory available.
+ *
+ * %-EINVAL - Invalid segment number in input
+ */
+ssize_t nilfs_sufile_set_suinfo(struct inode *sufile, void *buf,
+				unsigned supsz, size_t nsup)
+{
+	struct buffer_head *bh;
+	struct nilfs_suinfo_update *sup, *supv = buf;
+	struct nilfs_segment_usage *su;
+	void *kaddr;
+	unsigned long blkoff, prev_blkoff;
+	size_t nerr = 0;
+	int ret = 0;
+
+	if (unlikely(nsup == 0))
+		goto out;
+
+	down_write(&NILFS_MDT(sufile)->mi_sem);
+	for (sup = supv; sup < supv + nsup; sup++) {
+		if (unlikely(sup->sup_segnum >=
+				nilfs_sufile_get_nsegments(sufile))) {

fs/nilfs2/sufile.c ssize_t nilfs_sufile_get_suinfo(struct inode *sufile, __u64 segnum, void *buf, · line 918

Quoted 10 lines

fs/nilfs2/sufile.c +10 -0

+			printk(KERN_WARNING
+			       "%s: invalid segment number: %llu\n", __func__,
+			       (unsigned long long)sup->sup_segnum);
+			nerr++;
+		}
+	}
+	if (nerr > 0) {
+		ret = -EINVAL;
+		goto out_sem;
+	}

Seems that you can exit from inside the loop:

	for (sup = supv; sup < supv + nsup; sup++) {
		if (unlikely(sup->sup_segnum >=
				nilfs_sufile_get_nsegments(sufile))) {
			printk(KERN_WARNING
			       "%s: invalid segment number: %llu\n", __func__,
			       (unsigned long long)sup->sup_segnum);
			ret = -EINVAL;
			goto out_sem;
		}
	}

Quoted 10 lines

fs/nilfs2/sufile.c +10 -0

+
+	sup = supv;
+	blkoff = nilfs_sufile_get_blkoff(sufile, sup->sup_segnum);
+	ret = nilfs_mdt_get_block(sufile, blkoff, 1, NULL, &bh);
+	if (ret < 0)
+		goto out_sem;
+
+	for (;;) {
+		kaddr = kmap_atomic(bh->b_page);
+		su = nilfs_sufile_block_get_segment_usage(

fs/nilfs2/sufile.c ssize_t nilfs_sufile_get_suinfo(struct inode *sufile, __u64 segnum, void *buf, · line 938

Quoted 10 lines

fs/nilfs2/sufile.c +10 -0

+			sufile, sup->sup_segnum, bh, kaddr);
+
+		if (nilfs_suinfo_update_lastmod(sup))
+			su->su_lastmod = cpu_to_le64(sup->sup_sui.sui_lastmod);
+
+		if (nilfs_suinfo_update_nblocks(sup))
+			su->su_nblocks = cpu_to_le32(sup->sup_sui.sui_nblocks);
+
+		if (nilfs_suinfo_update_flags(sup))
+			su->su_flags = cpu_to_le32(sup->sup_sui.sui_flags);

You must also update the counter value of sh_ncleansegs, sh_ndirtysegs in the nilfs_sufile_header struct of sufile header block based on the change of these flags.

In addition, you should ignore NILFS_SEGMENT_USAGE_ACTIVE_FLAG as below because this flag is a virtual flag set by running nilfs kernel code. The flag bit should not be written to sufile on disk.

          (sui_flags & ~(1UL << NILFS_SEGMENT_USAGE_ACTIVE))

fs/nilfs2/sufile.c ssize_t nilfs_sufile_get_suinfo(struct inode *sufile, __u64 segnum, void *buf, · line 942

Quoted 4 lines

fs/nilfs2/sufile.c +4 -0

+
+		kunmap_atomic(kaddr);
+
+		if (++sup >= supv + nsup)

You must not use the automatic pointer operation of C language for nilfs_suinfo_update because it may be extended in a future release. This "++sup" breaks forward compatibility of the ioctl.

You should do this by:

                sup = (void *)sup + supsz;

Note that pointer type is automatically converted for "void *" type variables.

fs/nilfs2/sufile.c ssize_t nilfs_sufile_get_suinfo(struct inode *sufile, __u64 segnum, void *buf, · line 949

Quoted 7 lines

fs/nilfs2/sufile.c +7 -0

+			break;
+		prev_blkoff = blkoff;
+		blkoff = nilfs_sufile_get_blkoff(sufile, sup->sup_segnum);
+		if (blkoff == prev_blkoff)
+			continue;
+
+		/* get different block */

You must mark bh that this function changed as dirty with mark_buffer_dirty(bh). Otherwise, the change will be gone.

fs/nilfs2/sufile.c ssize_t nilfs_sufile_get_suinfo(struct inode *sufile, __u64 segnum, void *buf, · line 956

Quoted 7 lines

fs/nilfs2/sufile.c +7 -0

+		brelse(bh);
+		ret = nilfs_mdt_get_block(sufile, blkoff, 1, NULL, &bh);
+		if (unlikely(ret < 0))
+			goto out_sem;
+	}
+	brelse(bh);
+

The sufile should be marked as dirty (if any change happened):

    nilfs_mdt_mark_dirty(sufile);

Quoted 77 lines · Ryusuke Konishi

Quoted 18 lines · Andreas Rohner

This patch introduces the nilfs_sufile_set_suinfo function, which expects an array of nilfs_suinfo_update structures and updates the segment usage information accordingly.

This is basically a helper function for the newly introduced NILFS_IOCTL_SET_SUINFO ioctl.

Signed-off-by Andreas Rohner <[email protected]>

---
 fs/nilfs2/sufile.c | 91 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 fs/nilfs2/sufile.h |  2 +-
 2 files changed, 92 insertions(+), 1 deletion(-)

fs/nilfs2/sufile.c +0 -0

index 3127e9f..81c3438 100644

fs/nilfs2/sufile.c ssize_t nilfs_sufile_get_suinfo(struct inode *sufile, __u64 segnum, void *buf, · line 873

Quoted 4 lines

fs/nilfs2/sufile.c +1 -0

@@ -870,6 +870,97 @@ ssize_t nilfs_sufile_get_suinfo(struct inode *sufile, __u64 segnum, void *buf,
 }
 
 /**
+ * nilfs_sufile_set_suinfo -

Please fill the summary line of function even though nilfs_sufile_get_suinfo() function lacks it.

* nilfs_sufile_set_suinfo - update segment usage information

Quoted 35 lines

fs/nilfs2/sufile.c +35 -0

+ * @sufile: inode of segment usage file
+ * @buf: array of suinfo
+ * @supsz: byte size of suinfo
+ * @nsup: size of suinfo array
+ *
+ * Description: Takes an array of nilfs_suinfo_update structs and updates
+ * segment usage accordingly.
+ *
+ * Return Value: On success, 0 is returned and .... On error, one of the
+ * following negative error codes is returned.
+ *
+ * %-EIO - I/O error.
+ *
+ * %-ENOMEM - Insufficient amount of memory available.
+ *
+ * %-EINVAL - Invalid segment number in input
+ */
+ssize_t nilfs_sufile_set_suinfo(struct inode *sufile, void *buf,
+				unsigned supsz, size_t nsup)
+{
+	struct buffer_head *bh;
+	struct nilfs_suinfo_update *sup, *supv = buf;
+	struct nilfs_segment_usage *su;
+	void *kaddr;
+	unsigned long blkoff, prev_blkoff;
+	size_t nerr = 0;
+	int ret = 0;
+
+	if (unlikely(nsup == 0))
+		goto out;
+
+	down_write(&NILFS_MDT(sufile)->mi_sem);
+	for (sup = supv; sup < supv + nsup; sup++) {
+		if (unlikely(sup->sup_segnum >=
+				nilfs_sufile_get_nsegments(sufile))) {

fs/nilfs2/sufile.c ssize_t nilfs_sufile_get_suinfo(struct inode *sufile, __u64 segnum, void *buf, · line 918

Quoted 10 lines

fs/nilfs2/sufile.c +10 -0

+			printk(KERN_WARNING
+			       "%s: invalid segment number: %llu\n", __func__,
+			       (unsigned long long)sup->sup_segnum);
+			nerr++;
+		}
+	}
+	if (nerr > 0) {
+		ret = -EINVAL;
+		goto out_sem;
+	}

Seems that you can exit from inside the loop:

Quoted 9 lines

	for (sup = supv; sup < supv + nsup; sup++) {
		if (unlikely(sup->sup_segnum >=
				nilfs_sufile_get_nsegments(sufile))) {
			printk(KERN_WARNING
			       "%s: invalid segment number: %llu\n", __func__,
			       (unsigned long long)sup->sup_segnum);
			ret = -EINVAL;
			goto out_sem;
		}

Quoted 1 line

	}

Yes. I copied that from nilfs_sufile_updatev.

Quoted 25 lines

Quoted 10 lines

fs/nilfs2/sufile.c +10 -0

+
+	sup = supv;
+	blkoff = nilfs_sufile_get_blkoff(sufile, sup->sup_segnum);
+	ret = nilfs_mdt_get_block(sufile, blkoff, 1, NULL, &bh);
+	if (ret < 0)
+		goto out_sem;
+
+	for (;;) {
+		kaddr = kmap_atomic(bh->b_page);
+		su = nilfs_sufile_block_get_segment_usage(

fs/nilfs2/sufile.c ssize_t nilfs_sufile_get_suinfo(struct inode *sufile, __u64 segnum, void *buf, · line 938

Quoted 10 lines

fs/nilfs2/sufile.c +10 -0

+			sufile, sup->sup_segnum, bh, kaddr);
+
+		if (nilfs_suinfo_update_lastmod(sup))
+			su->su_lastmod = cpu_to_le64(sup->sup_sui.sui_lastmod);
+
+		if (nilfs_suinfo_update_nblocks(sup))
+			su->su_nblocks = cpu_to_le32(sup->sup_sui.sui_nblocks);
+
+		if (nilfs_suinfo_update_flags(sup))
+			su->su_flags = cpu_to_le32(sup->sup_sui.sui_flags);

You must also update the counter value of sh_ncleansegs, sh_ndirtysegs in the nilfs_sufile_header struct of sufile header block based on the change of these flags.

Quoted 5 lines

In addition, you should ignore NILFS_SEGMENT_USAGE_ACTIVE_FLAG as below because this flag is a virtual flag set by running nilfs kernel code. The flag bit should not be written to sufile on disk.

          (sui_flags & ~(1UL << NILFS_SEGMENT_USAGE_ACTIVE))

Yes. I didn't think of that.

Quoted 9 lines

fs/nilfs2/sufile.c ssize_t nilfs_sufile_get_suinfo(struct inode *sufile, __u64 segnum, void *buf, · line 942

Quoted 4 lines

fs/nilfs2/sufile.c +4 -0

+
+		kunmap_atomic(kaddr);
+
+		if (++sup >= supv + nsup)

You must not use the automatic pointer operation of C language for nilfs_suinfo_update because it may be extended in a future release. This "++sup" breaks forward compatibility of the ioctl.

Quoted 6 lines

You should do this by:

                sup = (void *)sup + supsz;

Note that pointer type is automatically converted for "void *" type variables.

Makes sense. I guess I blindly copied that from nilfs_sufile_updatev, but with a simple array of __u64 it works of course. With a structure, that is subject to change, not so much any more.

Quoted 22 lines

fs/nilfs2/sufile.c ssize_t nilfs_sufile_get_suinfo(struct inode *sufile, __u64 segnum, void *buf, · line 949

Quoted 7 lines

fs/nilfs2/sufile.c +7 -0

+			break;
+		prev_blkoff = blkoff;
+		blkoff = nilfs_sufile_get_blkoff(sufile, sup->sup_segnum);
+		if (blkoff == prev_blkoff)
+			continue;
+
+		/* get different block */

You must mark bh that this function changed as dirty with mark_buffer_dirty(bh). Otherwise, the change will be gone.

fs/nilfs2/sufile.c ssize_t nilfs_sufile_get_suinfo(struct inode *sufile, __u64 segnum, void *buf, · line 956

Quoted 7 lines

fs/nilfs2/sufile.c +7 -0

+		brelse(bh);
+		ret = nilfs_mdt_get_block(sufile, blkoff, 1, NULL, &bh);
+		if (unlikely(ret < 0))
+			goto out_sem;
+	}
+	brelse(bh);
+

The sufile should be marked as dirty (if any change happened):

Quoted 1 line

    nilfs_mdt_mark_dirty(sufile);

Of course. I am sorry that's a really stupid error. I will fix it right away.

Thanks for your review. I am really embarrassed about all the stupid avoidable errors.

br,
Andreas Rohner
--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

With this ioctl the segment usage information in the SUFILE can be updated from userspace.

This is useful, because it allows the userspace GC to modify and update segment usage entries for specific segments, which enables it to avoid unnecessary write operations.

If a segment needs to be cleaned, but there is no or very little free space to be gained, the cleaning operation basically degrades to needless expensive copying of data. In the end the only thing that changes is the location of the data and a timestamp in the segment usage info. With this ioctl the GC can skip the copying and update the segment usage entries directly instead.

Signed-off-by Andreas Rohner <[email protected]>

---
 fs/nilfs2/ioctl.c         | 50 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/nilfs2_fs.h |  2 ++
 2 files changed, 52 insertions(+)

fs/nilfs2/ioctl.c +25 -0

index b44bdb2..c6a3faf 100644
@@ -273,6 +273,13 @@ nilfs_ioctl_do_get_suinfo(struct the_nilfs *nilfs, __u64 *posp, int flags,
 	return ret;
 }
 
+static ssize_t
+nilfs_ioctl_do_set_suinfo(struct the_nilfs *nilfs, __u64 *posp, int flags,
+			  void *buf, size_t size, size_t nmembs)
+{
+	return nilfs_sufile_set_suinfo(nilfs->ns_sufile, buf, size, nmembs);
+}
+
 static int nilfs_ioctl_get_sustat(struct inode *inode, struct file *filp,
 				  unsigned int cmd, void __user *argp)
 {

@@ -767,6 +774,44 @@ out:
 	return ret;
 }
 
+static int nilfs_ioctl_set_info(struct inode *inode, struct file *filp,
+				unsigned int cmd, void __user *argp,
+				size_t membsz,
+				ssize_t (*dofunc)(struct the_nilfs *,
+						  __u64 *, int,
+						  void *, size_t, size_t))
+{
+	struct the_nilfs *nilfs = inode->i_sb->s_fs_info;
+	struct nilfs_transaction_info ti;
+	struct nilfs_argv argv;
+	int ret;
+
+	if (!capable(CAP_SYS_ADMIN))
+		return -EPERM;
+
+	ret = mnt_want_write_file(filp);
+	if (ret)
+		return ret;

fs/nilfs2/ioctl.c +15 -0

+	if (unlikely(ret < 0))
+		nilfs_transaction_abort(inode->i_sb);
+	else
+		nilfs_transaction_commit(inode->i_sb); /* never fails */
+
+out:
+	mnt_drop_write_file(filp);
+	return ret;
+}
+
 static int nilfs_ioctl_get_info(struct inode *inode, struct file *filp,
 				unsigned int cmd, void __user *argp,
 				size_t membsz,

@@ -820,6 +865,10 @@ long nilfs_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 		return nilfs_ioctl_get_info(inode, filp, cmd, argp,
 					    sizeof(struct nilfs_suinfo),
 					    nilfs_ioctl_do_get_suinfo);
+	case NILFS_IOCTL_SET_SUINFO:
+		return nilfs_ioctl_set_info(inode, filp, cmd, argp,
+					    sizeof(struct nilfs_suinfo_update),
+					    nilfs_ioctl_do_set_suinfo);
 	case NILFS_IOCTL_GET_SUSTAT:
 		return nilfs_ioctl_get_sustat(inode, filp, cmd, argp);
 	case NILFS_IOCTL_GET_VINFO:

@@ -859,6 +908,7 @@ long nilfs_compat_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 	case NILFS_IOCTL_GET_CPINFO:
 	case NILFS_IOCTL_GET_CPSTAT:
 	case NILFS_IOCTL_GET_SUINFO:
+	case NILFS_IOCTL_SET_SUINFO:
 	case NILFS_IOCTL_GET_SUSTAT:
 	case NILFS_IOCTL_GET_VINFO:
 	case NILFS_IOCTL_GET_BDESCS:

include/linux/nilfs2_fs.h +2 -0

index 2ee1594..528e2c5 100644
@@ -902,5 +902,7 @@ struct nilfs_bdesc {
 	_IOW(NILFS_IOCTL_IDENT, 0x8B, __u64)
 #define NILFS_IOCTL_SET_ALLOC_RANGE  \
 	_IOW(NILFS_IOCTL_IDENT, 0x8C, __u64[2])
+#define NILFS_IOCTL_SET_SUINFO  \
+	_IOW(NILFS_IOCTL_IDENT, 0x8D, struct nilfs_argv)
 
 #endif	/* _LINUX_NILFS_FS_H */

Signature · 6 lines

1.8.5.3

--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Quoted 60 lines · Andreas Rohner

With this ioctl the segment usage information in the SUFILE can be updated from userspace.

This is useful, because it allows the userspace GC to modify and update segment usage entries for specific segments, which enables it to avoid unnecessary write operations.

If a segment needs to be cleaned, but there is no or very little free space to be gained, the cleaning operation basically degrades to needless expensive copying of data. In the end the only thing that changes is the location of the data and a timestamp in the segment usage info. With this ioctl the GC can skip the copying and update the segment usage entries directly instead.

Signed-off-by Andreas Rohner <[email protected]>

---
 fs/nilfs2/ioctl.c         | 50 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/nilfs2_fs.h |  2 ++
 2 files changed, 52 insertions(+)

fs/nilfs2/ioctl.c +25 -0

index b44bdb2..c6a3faf 100644
@@ -273,6 +273,13 @@ nilfs_ioctl_do_get_suinfo(struct the_nilfs *nilfs, __u64 *posp, int flags,
 	return ret;
 }
 
+static ssize_t
+nilfs_ioctl_do_set_suinfo(struct the_nilfs *nilfs, __u64 *posp, int flags,
+			  void *buf, size_t size, size_t nmembs)
+{
+	return nilfs_sufile_set_suinfo(nilfs->ns_sufile, buf, size, nmembs);
+}
+
 static int nilfs_ioctl_get_sustat(struct inode *inode, struct file *filp,
 				  unsigned int cmd, void __user *argp)
 {

@@ -767,6 +774,44 @@ out:
 	return ret;
 }
 
+static int nilfs_ioctl_set_info(struct inode *inode, struct file *filp,
+				unsigned int cmd, void __user *argp,
+				size_t membsz,
+				ssize_t (*dofunc)(struct the_nilfs *,
+						  __u64 *, int,
+						  void *, size_t, size_t))
+{
+	struct the_nilfs *nilfs = inode->i_sb->s_fs_info;
+	struct nilfs_transaction_info ti;
+	struct nilfs_argv argv;
+	int ret;
+
+	if (!capable(CAP_SYS_ADMIN))
+		return -EPERM;
+
+	ret = mnt_want_write_file(filp);
+	if (ret)
+		return ret;

fs/nilfs2/ioctl.c out: · line 804

Quoted 10 lines

fs/nilfs2/ioctl.c +10 -0

+
+	ret = -EFAULT;
+	if (copy_from_user(&argv, argp, sizeof(argv)))
+		goto out;
+
+	if (argv.v_size < membsz)
+		return -EINVAL;
+
+	nilfs_transaction_begin(inode->i_sb, &ti, 0);
+	ret = nilfs_ioctl_wrap_copy(nilfs, &argv, _IOC_DIR(cmd), dofunc);

Hmm, we have a problem here.

nilfs_ioctl_wrap_copy() cannot be used between nilfs_transcation_begin() and nilfs_transaction_end() since nilfs_ioctl_wrap_copy() calls copy_from_user() and/or copy_to_user(). Unfortunately there is a lock order restriction for these functions.

Let's use vmalloc() like nilfs_ioctl_clean_segments(). The following is a sample code for this:

	size_t len;
	void __user *base;
	void *kbuf;
	int ret;

	...
	ret = -EINVAL;
	if (argv.v_size < sizeof(struct nilfs_suinfo_update))
		goto out;

	< range check of argv.v_nmembs >

	len = argv.v_size * argv.v_nmembs;
	base = (void __user *)(unsigned long)argv.v_base;
	kbuf = vmalloc(len);
	if (!kbuf) {
		ret = -ENOMEM;
		goto out;
	}

	if (copy_from_user(kbuf, base, len)) {
		ret = -EFAULT;
		goto out_free;
	}

	nilfs_transaction_begin(inode->i_sb, &ti, 0);
	ret = < call nilfs_sufile_set_suinfo > ;
	if (unlikely(ret < 0))
		nilfs_transcation_abort(inode->i_sb);
	else
		nilfs_transcation_commit(inode->i_sb);

out_free:

	vfree(kbuf);
out:
	mnt_drop_write_file(filp);
	...

Quoted 114 lines · Ryusuke Konishi

Quoted 60 lines · Andreas Rohner

With this ioctl the segment usage information in the SUFILE can be updated from userspace.

This is useful, because it allows the userspace GC to modify and update segment usage entries for specific segments, which enables it to avoid unnecessary write operations.

If a segment needs to be cleaned, but there is no or very little free space to be gained, the cleaning operation basically degrades to needless expensive copying of data. In the end the only thing that changes is the location of the data and a timestamp in the segment usage info. With this ioctl the GC can skip the copying and update the segment usage entries directly instead.

Signed-off-by Andreas Rohner <[email protected]>

---
 fs/nilfs2/ioctl.c         | 50 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/nilfs2_fs.h |  2 ++
 2 files changed, 52 insertions(+)

fs/nilfs2/ioctl.c +25 -0

index b44bdb2..c6a3faf 100644
@@ -273,6 +273,13 @@ nilfs_ioctl_do_get_suinfo(struct the_nilfs *nilfs, __u64 *posp, int flags,
 	return ret;
 }
 
+static ssize_t
+nilfs_ioctl_do_set_suinfo(struct the_nilfs *nilfs, __u64 *posp, int flags,
+			  void *buf, size_t size, size_t nmembs)
+{
+	return nilfs_sufile_set_suinfo(nilfs->ns_sufile, buf, size, nmembs);
+}
+
 static int nilfs_ioctl_get_sustat(struct inode *inode, struct file *filp,
 				  unsigned int cmd, void __user *argp)
 {

@@ -767,6 +774,44 @@ out:
 	return ret;
 }
 
+static int nilfs_ioctl_set_info(struct inode *inode, struct file *filp,
+				unsigned int cmd, void __user *argp,
+				size_t membsz,
+				ssize_t (*dofunc)(struct the_nilfs *,
+						  __u64 *, int,
+						  void *, size_t, size_t))
+{
+	struct the_nilfs *nilfs = inode->i_sb->s_fs_info;
+	struct nilfs_transaction_info ti;
+	struct nilfs_argv argv;
+	int ret;
+
+	if (!capable(CAP_SYS_ADMIN))
+		return -EPERM;
+
+	ret = mnt_want_write_file(filp);
+	if (ret)
+		return ret;

fs/nilfs2/ioctl.c out: · line 804

Quoted 10 lines

fs/nilfs2/ioctl.c +10 -0

+
+	ret = -EFAULT;
+	if (copy_from_user(&argv, argp, sizeof(argv)))
+		goto out;
+
+	if (argv.v_size < membsz)
+		return -EINVAL;
+
+	nilfs_transaction_begin(inode->i_sb, &ti, 0);
+	ret = nilfs_ioctl_wrap_copy(nilfs, &argv, _IOC_DIR(cmd), dofunc);

Hmm, we have a problem here.

nilfs_ioctl_wrap_copy() cannot be used between nilfs_transcation_begin() and nilfs_transaction_end() since nilfs_ioctl_wrap_copy() calls copy_from_user() and/or copy_to_user(). Unfortunately there is a lock order restriction for these functions.

Let's use vmalloc() like nilfs_ioctl_clean_segments(). The following is a sample code for this:

	size_t len;
	void __user *base;
	void *kbuf;
	int ret;

	...
	ret = -EINVAL;
	if (argv.v_size < sizeof(struct nilfs_suinfo_update))
		goto out;

	< range check of argv.v_nmembs >

	len = argv.v_size * argv.v_nmembs;
	base = (void __user *)(unsigned long)argv.v_base;
	kbuf = vmalloc(len);
	if (!kbuf) {
		ret = -ENOMEM;
		goto out;
	}

	if (copy_from_user(kbuf, base, len)) {
		ret = -EFAULT;
		goto out_free;
	}

	nilfs_transaction_begin(inode->i_sb, &ti, 0);
	ret = < call nilfs_sufile_set_suinfo > ;
	if (unlikely(ret < 0))
		nilfs_transcation_abort(inode->i_sb);
	else
		nilfs_transcation_commit(inode->i_sb);

Quoted 6 lines

out_free:

	vfree(kbuf);
out:
	mnt_drop_write_file(filp);
	...

Then we don't need the ugly nilfs_suinfo_update structure. I can just send an array of struct nilfs_argv[2]. First element has the segment numbers and second element has the nilfs_suinfo structures and flags are in v_flags. Would that be acceptable?

br,
Andreas Rohner
--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

This adds struct nilfs_suinfo_update and some flags. It contains the information needed to update one entry of segment usage information in the SUFILE. The flags specify, which fields need to be updated.

This can be used to update segment usage information from userspace with an ioctl.

Signed-off-by Andreas Rohner <[email protected]>

---
 include/linux/nilfs2_fs.h | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

include/linux/nilfs2_fs.h +1 -0

index 9875576..2ee1594 100644
@@ -709,6 +709,45 @@ static inline int nilfs_suinfo_clean(const struct nilfs_suinfo *si)
 	return !si->sui_flags;
 }
 
+/**

include/linux/nilfs2_fs.h +28 -0

+
+enum {
+	NILFS_SUINFO_UPDATE_LASTMOD,
+	NILFS_SUINFO_UPDATE_NBLOCKS,
+	NILFS_SUINFO_UPDATE_FLAGS,
+};
+
+#define NILFS_SUINFO_UPDATE_FNS(flag, name)				\
+static inline void							\
+nilfs_suinfo_update_set_##name(struct nilfs_suinfo_update *sup)		\
+{									\
+	sup->sup_flags |= 1UL << NILFS_SUINFO_UPDATE_##flag;		\
+}									\
+static inline void							\
+nilfs_suinfo_update_clear_##name(struct nilfs_suinfo_update *sup)	\
+{									\
+	sup->sup_flags &= ~(1UL << NILFS_SUINFO_UPDATE_##flag);		\
+}									\
+static inline int							\
+nilfs_suinfo_update_##name(const struct nilfs_suinfo_update *sup)	\
+{									\
+	return !!(sup->sup_flags & (1UL << NILFS_SUINFO_UPDATE_##flag));\
+}
+
+NILFS_SUINFO_UPDATE_FNS(LASTMOD, lastmod)
+NILFS_SUINFO_UPDATE_FNS(NBLOCKS, nblocks)
+NILFS_SUINFO_UPDATE_FNS(FLAGS, flags)
+
 /* ioctl */
 enum {
 	NILFS_CHECKPOINT,

Signature · 6 lines

1.8.5.3

--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Quoted 21 lines · Andreas Rohner

This adds struct nilfs_suinfo_update and some flags. It contains the information needed to update one entry of segment usage information in the SUFILE. The flags specify, which fields need to be updated.

This can be used to update segment usage information from userspace with an ioctl.

Signed-off-by Andreas Rohner <[email protected]>

---
 include/linux/nilfs2_fs.h | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

include/linux/nilfs2_fs.h +1 -0

index 9875576..2ee1594 100644
@@ -709,6 +709,45 @@ static inline int nilfs_suinfo_clean(const struct nilfs_suinfo *si)
 	return !si->sui_flags;
 }
 
+/**

include/linux/nilfs2_fs.h static inline int nilfs_suinfo_clean(const struct nilfs_suinfo *si) · line 722

Quoted 10 lines

include/linux/nilfs2_fs.h +10 -0

+ * nilfs_suinfo_update - segment usage information update
+ * @sup_segnum: segment number
+ * @sup_flags: flags for which fields are active in sup_sui
+ * @sup_sui: segment usage information
+ */
+struct nilfs_suinfo_update {
+	__u64 sup_segnum;
+	__u32 sup_flags;
+	struct nilfs_suinfo sup_sui;
+};

This structure is not aligned on 8 byte boundary. You need a 4 byte pad to make the alignment architecture independent since the structure already has some 64-bit variables.

If you do not so, the ioctl will lose compatibility, for instance, between 32 bit userland programs and 64 bit kernel.

struct nilfs_suinfo_update {
	__u64 sup_segnum;
	__u32 sup_flags;
	__u32 sup_reserved;
	struct nilfs_suinfo sup_sui;
};

Do you really need different suinfo update flags per segment ?

If it's not so, we don't have to add nilfs_suinfo_update structure. v_flags of nilfs_argv structure looks to be available for that purpose.

It's just a confirmation. Basically, I think this extension is acceptable.

Quoted 2 lines · Ryusuke Konishi

On Tue, 21 Jan 2014 15:00:28 +0100, Andreas Rohner wrote:
Do you really need different suinfo update flags per segment ?

No.

Quoted 2 lines

If it's not so, we don't have to add nilfs_suinfo_update structure. v_flags of nilfs_argv structure looks to be available for that purpose.

Yes v_flags is available, but I need nilfs_suinfo_update to contain the segment number. I can't use v_index for the segment number like with GET_SUINFO, because the segment numbers are not necessarily sequential.

If I use v_index I could effectively only use the ioctl to update one segment at a time. So I thought, since I need nilfs_suinfo_update anyway, I might as well put the flags there. But with the extra alignment it gets a bit ugly, so I tend to agree with you to put the flags into v_flags now.

Maybe I missed something, but how would you propose to send the segment numbers, only using nilfs_suinfo and nilfs_argv?

It is certainly possible to use v_index of nilfs_argv and update one segment at a time. It shouldn't be a problem, because there are at most

#define NILFS_CLDCONFIG_NSEGMENTS_PER_CLEAN_MAX 32

segments to be updated.

Quoted 2 lines

It's just a confirmation. Basically, I think this extension is acceptable.

Cool. Thanks!

br,
Andreas Rohner
--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Hi,

This is the second version of this patch set. It replaces the kind of hacky use of v_flags with a proper implementation of NILFS_IOCTL_SET_SUINFO ioctl.

v1->v2
* Implementation of NILFS_IOCTL_SET_SUINFO
* Added mc_min_free_blocks_threshold config option (if clean segments < min_clean_segments)
* Added new command line param for nilfs-clean
* Update man- and config-files
* Simpler benchmark

This patch set implements a small new feature and there shouldn't be any compatibility issues. It enables the GC to check how much free space can be gained from cleaning a segment and if it is less than a certain threshold it will abort the operation and try a different segment. Although no blocks need to be moved, the SUFILE entry of the corresponding segment needs to be updated to avoid an infinite loop.

Not having your super-secret test suite or disk space to run it, I went the other direction, using the commonly available fs_mark utility to make many tiny writes with 16 threads. My initial opinion is that your new GC code fixes some obvious lag when a filesystem is populated and nilfs_cleanerd starts to do its work.

However, for reasons of code
or simple mathematics, the file system hits end-of-space a bit earlier than does the unpatched code. I'll have to build some kernels, live with the system, and otherwise generate lots of checkpoints to know if this is a problem. IOW, I need to find out for myself if I need to make a slightly larger filesystem to do the same things using a patched NILFS2.

Hi Michael,

Quoted 5 lines · Michael L. Semon

Not having your super-secret test suite or disk space to run it, I went the other direction, using the commonly available fs_mark utility to make many tiny writes with 16 threads. My initial opinion is that your new GC code fixes some obvious lag when a filesystem is populated and nilfs_cleanerd starts to do its work.

Thanks for testing my code. It is not a super-secret test suite :), it's just a few 100 lines of crappy C code I am embarrassed to publish. Thanks for pointing out fs_mark, I didn't know it. From what I can see it seems to be the perfect tool to test the GC. I will repeat my measurements with fs_mark over the weekend.

Quoted 7 lines

However, for reasons of code
or simple mathematics, the file system hits end-of-space a bit earlier than does the unpatched code. I'll have to build some kernels, live with the system, and otherwise generate lots of checkpoints to know if this is a problem. IOW, I need to find out for myself if I need to make a slightly larger filesystem to do the same things using a patched NILFS2.

Maybe my default values are a bit too high. 256 blocks are about 1 MB with 4k blocks. So if you use the current default settings, you could potentially loose 1/8 of your free space.

Hi,

This is the second version of this patch set. It replaces the kind of hacky use of v_flags with a proper implementation of NILFS_IOCTL_SET_SUINFO ioctl.

v1->v2
* Implementation of NILFS_IOCTL_SET_SUINFO
* Added mc_min_free_blocks_threshold config option (if clean segments < min_clean_segments)
* Added new command line param for nilfs-clean
* Update man- and config-files
* Simpler benchmark

This patch set implements a small new feature and there shouldn't be any compatibility issues. It enables the GC to check how much free space can be gained from cleaning a segment and if it is less than a certain threshold it will abort the operation and try a different segment.

Although no blocks need to be moved, the SUFILE entry of the corresponding segment needs to be updated to avoid an infinite loop.

This is potentially useful for all gc policies, but it is especially beneficial for the timestamp policy.

Lets assume for example a NILFS2
volume with 20% static files and lets assume these static files are in the oldest segments. The current timestamp policy will select the oldest segments and, since the data is static, move them mostly unchanged to new segments. After a while they will become the oldest segments again. Then timestamp will move them again. These moving operations are expensive and unnecessary.

I used a simple benchmark to test the patch set (only a few lines of C). I used a 100 GB partition and performed the following steps:

1. Write a 20 GB file
2. Write a 50 GB file
3. Overwrite chunks of 1 MB within the 50 GB file at random
4. Repeat step 3 until 60 GB of data is written

Steps 3 and 4 are only perfomed to get the GC started. So the benchmark writes a 130 GB in total to a 100 GB partition.

HHD:

   Timestamp GB Written: 340.7574
   Timestamp GB Read:    208.2935
   Timestamp Runtime:    7787.546s

   Patched GB Written:   313.2566
   Patched GB Read:      182.6389
   Patched Runtime:      7410.892s

SSD:

   Timestamp GB Written: 679.3901
   Timestamp GB Read:    242.59
   Timestamp Runtime:    3022.081s

   Patched GB Written:   500.0095
   Patched GB Read:      157.475   
   Patched Runtime:      2313.448

The results for the HDD clearly show, that about 20 GB less data has been written and read in the patched version. It is reasonable to assume, that these 20 GB are the static data.

The speed of the GC was tuned to the HDD. It was probably too aggressive for the much faster SSD. That is probably the reason why the difference in GB written and read is much higher than 20 GB.

Quoted 6 lines · Andreas Rohner

Hi,

This is the second version of this patch set. It replaces the kind of hacky use of v_flags with a proper implementation of NILFS_IOCTL_SET_SUINFO ioctl.

Quoted 7 lines

v1->v2
* Implementation of NILFS_IOCTL_SET_SUINFO
* Added mc_min_free_blocks_threshold config option (if clean segments < min_clean_segments)
* Added new command line param for nilfs-clean
* Update man- and config-files
* Simpler benchmark

If you are talking about something then it should be in patchset. Otherwise, why do you mention about it?

Quoted 5 lines

This patch set implements a small new feature and there shouldn't be any compatibility issues. It enables the GC to check how much free space can be gained from cleaning a segment and if it is less than a certain threshold it will abort the operation and try a different segment.

When you have cleaned a segment then you can use the whole one. So, if segment has 8 MB in size then it will be available 8 MB free space. The phrase "It enables the GC to check how much free space can be gained from cleaning a segment" really confuses me. Because I always know how much space I gain after cleaning every segment. I suppose that you mean something different. Am I correct?

Quoted 5 lines

Although no blocks need to be moved, the SUFILE entry of the corresponding segment needs to be updated to avoid an infinite loop.

This is potentially useful for all gc policies, but it is especially beneficial for the timestamp policy.

I completely misunderstand this statement. What do you mean?

Quoted 16 lines

Lets assume for example a NILFS2
volume with 20% static files and lets assume these static files are in the oldest segments. The current timestamp policy will select the oldest segments and, since the data is static, move them mostly unchanged to new segments. After a while they will become the oldest segments again. Then timestamp will move them again. These moving operations are expensive and unnecessary.

I used a simple benchmark to test the patch set (only a few lines of C). I used a 100 GB partition and performed the following steps:

1. Write a 20 GB file
2. Write a 50 GB file
3. Overwrite chunks of 1 MB within the 50 GB file at random
4. Repeat step 3 until 60 GB of data is written

Quoted 2 lines

Steps 3 and 4 are only perfomed to get the GC started. So the benchmark writes a 130 GB in total to a 100 GB partition.

How is it possible to save 130 GB in 100 GB partition? Are you magician? :)

Quoted 23 lines

HHD:

   Timestamp GB Written: 340.7574
   Timestamp GB Read:    208.2935
   Timestamp Runtime:    7787.546s

   Patched GB Written:   313.2566
   Patched GB Read:      182.6389
   Patched Runtime:      7410.892s

SSD:

   Timestamp GB Written: 679.3901
   Timestamp GB Read:    242.59
   Timestamp Runtime:    3022.081s

   Patched GB Written:   500.0095
   Patched GB Read:      157.475   
   Patched Runtime:      2313.448

The results for the HDD clearly show, that about 20 GB less data has been written and read in the patched version. It is reasonable to assume, that these 20 GB are the static data.

Quoted 3 lines

The speed of the GC was tuned to the HDD. It was probably too aggressive for the much faster SSD. That is probably the reason why the difference in GB written and read is much higher than 20 GB.

I misunderstand completely what you mean here.

23 ÑÎ×. 2014 Ç., × 22:12, Andreas Rohner <[email protected]>:

Quoted 7 lines

Quoted 6 lines

This patch set implements a small new feature and there shouldn't be any compatibility issues. It enables the GC to check how much free space can be gained from cleaning a segment and if it is less than a certain threshold it will abort the operation and try a different segment.

Quoted 6 lines

When you have cleaned a segment then you can use the whole one. So, if segment has 8 MB in size then it will be available 8 MB free space. The phrase "It enables the GC to check how much free space can be gained from cleaning a segment" really confuses me. Because I always know how much space I gain after cleaning every segment. I suppose that you mean something different. Am I correct?

You have to move the live blocks to a new segment, so you gain only (8 MB - live_blocks) of free space.

Quoted 6 lines

Quoted 5 lines

Although no blocks need to be moved, the SUFILE entry of the corresponding segment needs to be updated to avoid an infinite loop.

This is potentially useful for all gc policies, but it is especially beneficial for the timestamp policy.

Quoted 1 line

I completely misunderstand this statement. What do you mean?

Well the timestamp policy always selects the oldest segment. If the oldest segment is below the threshold it won't be cleaned. If we don't change the timestamp it will be immediately selected again and it probably will still be below the threshold and so on in an infinite loop.

Quoted 5 lines

Quoted 4 lines

The speed of the GC was tuned to the HDD. It was probably too aggressive for the much faster SSD. That is probably the reason why the difference in GB written and read is much higher than 20 GB.

Quoted 1 line

I misunderstand completely what you mean here.

You need to be a bit more specific. These are the results of my benchmark. The GB Written and GB Read values are calculated by simply importing /proc/diskstats into R (You subtract the values before the benchmark from those after the benchmark).

The patched version writes less and reads less. Pretty simple.

Quoted 16 lines

23 янв. 2014 г., в 22:12, Andreas Rohner <[email protected]>:

Quoted 13 lines

Quoted 6 lines

This patch set implements a small new feature and there shouldn't be any compatibility issues. It enables the GC to check how much free space can be gained from cleaning a segment and if it is less than a certain threshold it will abort the operation and try a different segment.

When you have cleaned a segment then you can use the whole one. So, if segment has 8 MB in size then it will be available 8 MB free space. The phrase "It enables the GC to check how much free space can be gained from cleaning a segment" really confuses me. Because I always know how much space I gain after cleaning every segment. I suppose that you mean something different. Am I correct?

Quoted 2 lines

You have to move the live blocks to a new segment, so you gain only (8 MB - live_blocks) of free space.

You always will have 8 MB after cleaning (garbage collection). So, all segments are identical from the free space point of view. What does GC check? And how can GC distinguish segments on the basis of free space? All cleaned segments return 8 MB free space for further allocation. So, all used segments will be over any threshold.

Quoted 8 lines

Quoted 7 lines

Quoted 5 lines

Although no blocks need to be moved, the SUFILE entry of the corresponding segment needs to be updated to avoid an infinite loop.

This is potentially useful for all gc policies, but it is especially beneficial for the timestamp policy.

I completely misunderstand this statement. What do you mean?

Quoted 4 lines

Well the timestamp policy always selects the oldest segment. If the oldest segment is below the threshold it won't be cleaned. If we don't change the timestamp it will be immediately selected again and it probably will still be below the threshold and so on in an infinite loop.

I understand your approach. But I misunderstand about what usefulness you are talking. What is it?

Quoted 7 lines

Quoted 6 lines

Quoted 4 lines

The speed of the GC was tuned to the HDD. It was probably too aggressive for the much faster SSD. That is probably the reason why the difference in GB written and read is much higher than 20 GB.

I misunderstand completely what you mean here.

Quoted 6 lines

You need to be a bit more specific. These are the results of my benchmark. The GB Written and GB Read values are calculated by simply importing /proc/diskstats into R (You subtract the values before the benchmark from those after the benchmark).

The patched version writes less and reads less. Pretty simple.

Here I asked about aggressiveness related to SSD. What do you mean?

Quoted 8 lines

Quoted 2 lines

You have to move the live blocks to a new segment, so you gain only (8 MB - live_blocks) of free space.

You always will have 8 MB after cleaning (garbage collection). So, all segments are identical from the free space point of view. What does GC check? And how can GC distinguish segments on the basis of free space? All cleaned segments return 8 MB free space for further allocation. So, all used segments will be over any threshold.

I am sorry if I wasn't clear enough. The invalidated blocks in a segment are basically unusable free space, that needs to be garbage collected. If you clean a segment you only gain the space of the invalidated blocks, from the perspective of the whole file system. Of course the whole segment is free after cleaning, but the live blocks were moved somewhere else and occupy space there. So from the perspective of the whole file system you only really gained the amount of space occupied by the invalidated blocks.

Quoted 13 lines

Quoted 12 lines

Quoted 7 lines

Quoted 5 lines

Although no blocks need to be moved, the SUFILE entry of the corresponding segment needs to be updated to avoid an infinite loop.

This is potentially useful for all gc policies, but it is especially beneficial for the timestamp policy.

I completely misunderstand this statement. What do you mean?

Well the timestamp policy always selects the oldest segment. If the oldest segment is below the threshold it won't be cleaned. If we don't change the timestamp it will be immediately selected again and it probably will still be below the threshold and so on in an infinite loop.

Quoted 3 lines

I understand your approach. But I misunderstand about what usefulness you are talking. What is it?

It helps to avoid unnecessary copy operations of data.

Quoted 9 lines · Andreas Rohner

Quoted 8 lines

Quoted 2 lines

You have to move the live blocks to a new segment, so you gain only (8 MB - live_blocks) of free space.

You always will have 8 MB after cleaning (garbage collection). So, all segments are identical from the free space point of view. What does GC check? And how can GC distinguish segments on the basis of free space? All cleaned segments return 8 MB free space for further allocation. So, all used segments will be over any threshold.

Quoted 9 lines

I am sorry if I wasn't clear enough. The invalidated blocks in a segment are basically unusable free space, that needs to be garbage collected. If you clean a segment you only gain the space of the invalidated blocks, from the perspective of the whole file system. Of course the whole segment is free after cleaning, but the live blocks were moved somewhere else and occupy space there. So from the perspective of the whole file system you only really gained the amount of space occupied by the invalidated blocks.

I suggest not to use "free blocks" term for the case of invalidated blocks. Because it is really confusing way. I suppose that we can use term "invalid blocks" or "dead blocks". But I think that "dead blocks" is not good way too. Because, from my point of view, invalidated blocks are blocks that keep previous state of actual blocks. Dead block sounds for me likewise unusable or bad block.

Quoted 22 lines · Vyacheslav Dubeyko

Quoted 9 lines · Andreas Rohner

Quoted 8 lines

Quoted 2 lines

You have to move the live blocks to a new segment, so you gain only (8 MB - live_blocks) of free space.

You always will have 8 MB after cleaning (garbage collection). So, all segments are identical from the free space point of view. What does GC check? And how can GC distinguish segments on the basis of free space? All cleaned segments return 8 MB free space for further allocation. So, all used segments will be over any threshold.

Quoted 9 lines

I am sorry if I wasn't clear enough. The invalidated blocks in a segment are basically unusable free space, that needs to be garbage collected. If you clean a segment you only gain the space of the invalidated blocks, from the perspective of the whole file system. Of course the whole segment is free after cleaning, but the live blocks were moved somewhere else and occupy space there. So from the perspective of the whole file system you only really gained the amount of space occupied by the invalidated blocks.

Quoted 6 lines

I suggest not to use "free blocks" term for the case of invalidated blocks. Because it is really confusing way. I suppose that we can use term "invalid blocks" or "dead blocks". But I think that "dead blocks" is not good way too. Because, from my point of view, invalidated blocks are blocks that keep previous state of actual blocks. Dead block sounds for me likewise unusable or bad block.

"free blocks" term sounds confusing. We often use terms "live" and "dead" for garbage collection, but "dead blocks" also sounds unfit to me as Vyacheslav pointed out.

How about "min reclaimable blocks" or "min collectable blocks" ?