[PATCH 0/4] nilfs-utils: new feature to skip inefficient gc

[PATCH 0/4] nilfs-utils: new feature to skip inefficient gc

[Andreas Rohner]

Hi,

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, which is not unreasonable, and lets assume these
static files are in the oldest segments. So the current timestamp
policy will select the oldest segments and since the data is static
it needs to be moved to new segments. Now it is in the newest segments,
but after a while they will become the oldest segments again. Then
timestamp will move them again. These moving operations are expensive
and unnecessary.

I designed a benchmark that uses a fresh NILFS2 volume, initialized
with 20% static data. Above that, normal file operations are simulated,
but the static data is never touched. These are the results:

SSD:
    Timestamp GB Written: 1857.88
    Timestamp GB Read:    711.1417
    Timestamp Runtime:    10724.27s

    Patched GB Written:   823.6395
    Patched GB Read:      222.1085
    Patched Runtime:      6374.93s

HDD:
    Timestamp GB Written: 609.026
    Timestamp GB Read:    382.4147
    Timestamp Runtime:    13123.15s

    Patched GB Written:   423.8563
    Patched GB Read:      199.2632
    Patched Runtime:      9460.73s

Admittedly the benchmark is a bit biased to highlight the problem. On a
real system the static data won't be so completely static and so the
performance improvements are probably not so extreme.

This is my first patch set to this mailing list. I hope 
everything is formally correct.

Best regards,
Andreas Rohner

Andreas Rohner (4):
  nilfs-utils: cldconfig add an option to set minimal free blocks
  nilfs-utils: cleanerd: add custom error value to enable fast retry
  nilfs-utils: refactoring of nilfs_reclaim_segment to add minblocks
    param
  nilfs-utils: add support for and define some nilfs_argv flags

 include/nilfs.h                  |  2 +-
 include/nilfs2_fs.h              |  6 ++++++
 include/nilfs_gc.h               |  6 ++++--
 lib/gc.c                         | 21 +++++++++++++++++----
 lib/nilfs.c                      |  3 ++-
 sbin/cleanerd/cldconfig.c        | 20 ++++++++++++++++++++
 sbin/cleanerd/cldconfig.h        |  2 ++
 sbin/cleanerd/cleanerd.c         | 15 ++++++++++++---
 sbin/nilfs-resize/nilfs-resize.c |  2 +-
 9 files changed, 65 insertions(+), 12 deletions(-)

-- 
1.8.5.3

--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/4] nilfs-utils: cldconfig add an option to set minimal free blocks

[Andreas Rohner]

With this option the user can specify the minimal number of free/dead
blocks, which is a threshold for the GC. 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 <andreas.rohner-hi6Y0CQ0nG0@public.gmane.org>
---
 sbin/cleanerd/cldconfig.c | 20 ++++++++++++++++++++
 sbin/cleanerd/cldconfig.h |  2 ++
 2 files changed, 22 insertions(+)

diff --git a/sbin/cleanerd/cldconfig.c b/sbin/cleanerd/cldconfig.c
index 270d360..9a55914 100644
--- a/sbin/cleanerd/cldconfig.c
+++ b/sbin/cleanerd/cldconfig.c
@@ -456,6 +456,20 @@ nilfs_cldconfig_handle_mc_nsegments_per_clean(struct nilfs_cldconfig *config,
 }
 
 static int
+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_for_cleaning = n;
+	return 0;
+}
+
+static int
 nilfs_cldconfig_handle_cleaning_interval(struct nilfs_cldconfig *config,
 					 char **tokens, size_t ntoks,
 					 struct nilfs *nilfs)
@@ -576,6 +590,10 @@ nilfs_cldconfig_keyword_table[] = {
 		"log_priority", 2, 2,
 		nilfs_cldconfig_handle_log_priority
 	},
+	{
+		"min_free_blocks_for_cleaning", 2, 2,
+		nilfs_cldconfig_handle_min_free_blocks
+	},
 };
 
 #define NILFS_CLDCONFIG_NKEYWORDS			\
@@ -641,6 +659,8 @@ 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_for_cleaning =
+		NILFS_CLDCONFIG_MIN_FREE_BLOCKS_FOR_CLEANING;
 }
 
 static inline int iseol(int c)
diff --git a/sbin/cleanerd/cldconfig.h b/sbin/cleanerd/cldconfig.h
index 188ce9b..71a0642 100644
--- a/sbin/cleanerd/cldconfig.h
+++ b/sbin/cleanerd/cldconfig.h
@@ -102,6 +102,7 @@ struct nilfs_cldconfig {
 	struct timeval cf_retry_interval;
 	int cf_use_mmap;
 	int cf_log_priority;
+	unsigned long cf_min_free_blocks_for_cleaning;
 };
 
 #define NILFS_CLDCONFIG_SELECTION_POLICY_IMPORTANCE	\
@@ -120,6 +121,7 @@ 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_FOR_CLEANING		128
 
 #define NILFS_CLDCONFIG_NSEGMENTS_PER_CLEAN_MAX	32
 
-- 
1.8.5.3

--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/4] nilfs-utils: cleanerd: add custom error value to enable fast retry

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

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 <andreas.rohner-hi6Y0CQ0nG0@public.gmane.org>
---
 include/nilfs_gc.h       |  2 ++
 sbin/cleanerd/cleanerd.c | 10 +++++++++-
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/include/nilfs_gc.h b/include/nilfs_gc.h
index 72e9947..7628ce1 100644
--- a/include/nilfs_gc.h
+++ b/include/nilfs_gc.h
@@ -27,4 +27,6 @@ static inline int nilfs_suinfo_reclaimable(const struct nilfs_suinfo *si)
 
 extern void (*nilfs_gc_logger)(int priority, const char *fmt, ...);
 
+#define EGCTRYAGAIN 513
+
 #endif /* NILFS_GC_H */
diff --git a/sbin/cleanerd/cleanerd.c b/sbin/cleanerd/cleanerd.c
index 86dfcf7..ab8124f 100644
--- a/sbin/cleanerd/cleanerd.c
+++ b/sbin/cleanerd/cleanerd.c
@@ -167,6 +167,7 @@ struct nilfs_cleanerd {
 	int running;
 	int fallback;
 	int retry_cleaning;
+	int no_timeout;
 	int shutdown;
 	long ncleansegs;
 	struct timeval cleaning_interval;
@@ -873,7 +874,7 @@ static int nilfs_cleanerd_recalc_interval(struct nilfs_cleanerd *cleanerd,
 	interval = nilfs_cleanerd_cleaning_interval(cleanerd);
 	/* timercmp() does not work for '>=' or '<='. */
 	/* curr >= target */
-	if (!timercmp(&curr, &cleanerd->target, <)) {
+	if (!timercmp(&curr, &cleanerd->target, <) || cleanerd->no_timeout) {
 		cleanerd->timeout.tv_sec = 0;
 		cleanerd->timeout.tv_usec = 0;
 		timeradd(&curr, interval, &cleanerd->target);
@@ -1348,6 +1349,7 @@ static ssize_t nilfs_cleanerd_clean_segments(struct nilfs_cleanerd *cleanerd,
 		       "number: %m");
 		goto out;
 	}
+	cleanerd->no_timeout = 0;
 
 	ret = nilfs_reclaim_segment(cleanerd->nilfs, segnums, nsegs,
 				    protseq, protcno);
@@ -1373,6 +1375,11 @@ static ssize_t nilfs_cleanerd_clean_segments(struct nilfs_cleanerd *cleanerd,
 			cleanerd->retry_cleaning = 0;
 		}
 
+	} else if (ret == -EGCTRYAGAIN) {
+		cleanerd->fallback = 0;
+		cleanerd->retry_cleaning = 1;
+		cleanerd->no_timeout = 1;
+		ret = 0;
 	} else if (ret < 0 && errno == ENOMEM) {
 		nilfs_cleanerd_reduce_ncleansegs_for_retry(cleanerd);
 		cleanerd->fallback = 1;
@@ -1415,6 +1422,7 @@ static int nilfs_cleanerd_clean_loop(struct nilfs_cleanerd *cleanerd)
 	cleanerd->running = 1;
 	cleanerd->fallback = 0;
 	cleanerd->retry_cleaning = 0;
+	cleanerd->no_timeout = 0;
 	nilfs_cnoconv_reset(cleanerd->cnoconv);
 	nilfs_gc_logger = syslog;
 
-- 
1.8.5.3

--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/4] nilfs-utils: refactoring of nilfs_reclaim_segment to add minblocks param

[Andreas Rohner]

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 <andreas.rohner-hi6Y0CQ0nG0@public.gmane.org>
---
 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(-)

diff --git a/include/nilfs_gc.h b/include/nilfs_gc.h
index 7628ce1..f9fbde4 100644
--- a/include/nilfs_gc.h
+++ b/include/nilfs_gc.h
@@ -15,8 +15,8 @@
 #include "nilfs.h"
 
 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);
 
 
 static inline int nilfs_suinfo_reclaimable(const struct nilfs_suinfo *si)
diff --git a/lib/gc.c b/lib/gc.c
index 1152299..0b0e2d6 100644
--- a/lib/gc.c
+++ b/lib/gc.c
@@ -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;
diff --git a/sbin/cleanerd/cleanerd.c b/sbin/cleanerd/cleanerd.c
index ab8124f..76a79de 100644
--- a/sbin/cleanerd/cleanerd.c
+++ b/sbin/cleanerd/cleanerd.c
@@ -1351,8 +1351,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,
+			cleanerd->config.cf_min_free_blocks_for_cleaning);
 	if (ret > 0) {
 		for (i = 0; i < ret; i++)
 			syslog(LOG_DEBUG, "segment %llu cleaned",
diff --git a/sbin/nilfs-resize/nilfs-resize.c b/sbin/nilfs-resize/nilfs-resize.c
index c8b0868..2850268 100644
--- a/sbin/nilfs-resize/nilfs-resize.c
+++ b/sbin/nilfs-resize/nilfs-resize.c
@@ -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;
 
-- 
1.8.5.3

--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4/4] nilfs-utils: add support for and define some nilfs_argv flags

[Andreas Rohner]

By using the v_flags field of the nilfs_argv struct, it is possible to
communicate additional information to the kernel. This patch defines
two new flags, which are to be used with the nilfs_clean_segments
function.

NILFS_CLEAN_SEGMENTS_DEFAULT: Normal GC operation

NILFS_CLEAN_SEGMENTS_UPDATE_SEGUSG: Do not move any blocks, just update
	the segment usage metadata

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 sets
the NILFS_CLEAN_SEGMENTS_UPDATE_SEGUSG flag. This causes the kernel to
leave the segment alone, not move any blocks, and only update the
SUFILE.

This is useful, because if the gain of cleaning a segment is very low,
it is better to leave it alone and try cleaning a different segment.

Signed-off-by: Andreas Rohner <andreas.rohner-hi6Y0CQ0nG0@public.gmane.org>
---
 include/nilfs.h     |  2 +-
 include/nilfs2_fs.h |  6 ++++++
 lib/gc.c            | 16 ++++++++++++++--
 lib/nilfs.c         |  3 ++-
 4 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/include/nilfs.h b/include/nilfs.h
index 56286a9..5ca8bec 100644
--- a/include/nilfs.h
+++ b/include/nilfs.h
@@ -304,7 +304,7 @@ 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);
 int nilfs_clean_segments(struct nilfs *, struct nilfs_vdesc *, size_t,
 			 struct nilfs_period *, size_t, __u64 *, size_t,
-			 struct nilfs_bdesc *, size_t, __u64 *, size_t);
+			 struct nilfs_bdesc *, size_t, __u64 *, size_t, int);
 int nilfs_sync(const struct nilfs *, nilfs_cno_t *);
 int nilfs_resize(struct nilfs *nilfs, off_t size);
 int nilfs_set_alloc_range(struct nilfs *nilfs, off_t start, off_t end);
diff --git a/include/nilfs2_fs.h b/include/nilfs2_fs.h
index e674f44..1ac558f 100644
--- a/include/nilfs2_fs.h
+++ b/include/nilfs2_fs.h
@@ -731,6 +731,12 @@ struct nilfs_cpmode {
 	__u32 cm_pad;
 };
 
+/* values for v_flags */
+enum {
+	NILFS_CLEAN_SEGMENTS_DEFAULT,
+	NILFS_CLEAN_SEGMENTS_UPDATE_SEGUSG,
+};
+
 /**
  * struct nilfs_argv - argument vector
  * @v_base: pointer on data array from userspace
diff --git a/lib/gc.c b/lib/gc.c
index 0b0e2d6..cbae0f3 100644
--- a/lib/gc.c
+++ b/lib/gc.c
@@ -616,6 +616,8 @@ 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;
+	__u32 freeblocks;
+	int cleaning_flags = NILFS_CLEAN_SEGMENTS_DEFAULT;
 
 	if (nsegs == 0)
 		return 0;
@@ -678,6 +680,13 @@ 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 (freeblocks < minblocks * n)
+		cleaning_flags = NILFS_CLEAN_SEGMENTS_UPDATE_SEGUSG;
+
 	ret = nilfs_clean_segments(nilfs,
 				   nilfs_vector_get_data(vdescv),
 				   nilfs_vector_get_size(vdescv),
@@ -687,12 +696,15 @@ ssize_t nilfs_reclaim_segment(struct nilfs *nilfs,
 				   nilfs_vector_get_size(vblocknrv),
 				   nilfs_vector_get_data(bdescv),
 				   nilfs_vector_get_size(bdescv),
-				   segnums, n);
+				   segnums, n, cleaning_flags);
 	if (ret < 0) {
 		nilfs_gc_logger(LOG_ERR, "cannot clean segments: %s",
 				strerror(errno));
 	} else {
-		ret = n;
+		if (cleaning_flags == NILFS_CLEAN_SEGMENTS_UPDATE_SEGUSG)
+			ret = -EGCTRYAGAIN;
+		else
+			ret = n;
 	}
 
 out_lock:
diff --git a/lib/nilfs.c b/lib/nilfs.c
index ebe50b8..50eed5e 100644
--- a/lib/nilfs.c
+++ b/lib/nilfs.c
@@ -627,7 +627,7 @@ int nilfs_clean_segments(struct nilfs *nilfs,
 			 struct nilfs_period *periods, size_t nperiods,
 			 __u64 *vblocknrs, size_t nvblocknrs,
 			 struct nilfs_bdesc *bdescs, size_t nbdescs,
-			 __u64 *segnums, size_t nsegs)
+			 __u64 *segnums, size_t nsegs, int flags)
 {
 	struct nilfs_argv argv[5];
 
@@ -639,6 +639,7 @@ int nilfs_clean_segments(struct nilfs *nilfs,
 	argv[0].v_base = (unsigned long)vdescs;
 	argv[0].v_nmembs = nvdescs;
 	argv[0].v_size = sizeof(struct nilfs_vdesc);
+	argv[0].v_flags = flags;
 	argv[1].v_base = (unsigned long)periods;
 	argv[1].v_nmembs = nperiods;
 	argv[1].v_size = sizeof(struct nilfs_period);
-- 
1.8.5.3

--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] nilfs2: depending on flags, update segment usage instead of cleaning

[Andreas Rohner]

This patch introduces two new flags, which can be used to modify the
behavior of the cleaner.

NILFS_CLEAN_SEGMENTS_DEFAULT: Normal GC operation

NILFS_CLEAN_SEGMENTS_UPDATE_SEGUSG: Do not move any blocks, just update
        the segment usage information

Additionally this patch implements a simple update function, that
modifies the SUFILE in such a way, that old segments effectively become
new segments, without the need to move the blocks around.

This is useful because it allows the userspace GC to avoid
copying segments around needlessly, and still get essentially the same
effekt. This way huge amounts of write operations can be avoided and
the performance improves significantly.

Signed-off-by: Andreas Rohner <andreas.rohner-hi6Y0CQ0nG0@public.gmane.org>
---
 fs/nilfs2/ioctl.c         | 58 ++++++++++++++++++++++++++++++++++++++++-------
 include/linux/nilfs2_fs.h |  6 +++++
 2 files changed, 56 insertions(+), 8 deletions(-)

diff --git a/fs/nilfs2/ioctl.c b/fs/nilfs2/ioctl.c
index b44bdb2..52b967a 100644
--- a/fs/nilfs2/ioctl.c
+++ b/fs/nilfs2/ioctl.c
@@ -571,6 +571,42 @@ int nilfs_ioctl_prepare_clean_segments(struct the_nilfs *nilfs,
 	return ret;
 }
 
+static int nilfs_ioctl_update_segment_usage(struct super_block *sb,
+				struct nilfs_argv argv[5], void *bufs[5])
+{
+	size_t nmembs;
+	struct the_nilfs *nilfs = sb->s_fs_info;
+	struct inode *sufile = nilfs->ns_sufile;
+	struct nilfs_suinfo si;
+	__u64 *segnums;
+	int i, ret;
+	struct nilfs_transaction_info ti;
+
+	ret = nilfs_transaction_begin(sb, &ti, 0);
+	if (unlikely(ret))
+		return ret;
+
+	nmembs = argv[4].v_nmembs;
+	for (i = 0, segnums = bufs[4]; i < nmembs; ++i) {
+		ret = nilfs_sufile_get_suinfo(sufile, segnums[i],
+				&si, sizeof(struct nilfs_suinfo), 1);
+		if (unlikely(ret < 0))
+			goto failure;
+
+		ret = nilfs_sufile_set_segment_usage(sufile, segnums[i],
+				si.sui_nblocks, nilfs->ns_ctime);
+		if (unlikely(ret < 0))
+			goto failure;
+	}
+
+	nilfs_transaction_commit(sb);
+	return ret;
+
+ failure:
+	nilfs_transaction_abort(sb);
+	return ret;
+}
+
 static int nilfs_ioctl_clean_segments(struct inode *inode, struct file *filp,
 				      unsigned int cmd, void __user *argp)
 {
@@ -660,14 +696,20 @@ static int nilfs_ioctl_clean_segments(struct inode *inode, struct file *filp,
 		goto out_free;
 	}
 
-	ret = nilfs_ioctl_move_blocks(inode->i_sb, &argv[0], kbufs[0]);
-	if (ret < 0)
-		printk(KERN_ERR "NILFS: GC failed during preparation: "
-			"cannot read source blocks: err=%d\n", ret);
-	else {
-		if (nilfs_sb_need_update(nilfs))
-			set_nilfs_discontinued(nilfs);
-		ret = nilfs_clean_segments(inode->i_sb, argv, kbufs);
+	if (argv[0].v_flags == NILFS_CLEAN_SEGMENTS_UPDATE_SEGUSG) {
+		/* only update segment usage */
+		ret = nilfs_ioctl_update_segment_usage(inode->i_sb, argv,
+				kbufs);
+	} else {
+		ret = nilfs_ioctl_move_blocks(inode->i_sb, &argv[0], kbufs[0]);
+		if (ret < 0)
+			printk(KERN_ERR "NILFS: GC failed during preparation: "
+				"cannot read source blocks: err=%d\n", ret);
+		else {
+			if (nilfs_sb_need_update(nilfs))
+				set_nilfs_discontinued(nilfs);
+			ret = nilfs_clean_segments(inode->i_sb, argv, kbufs);
+		}
 	}
 
 	nilfs_remove_all_gcinodes(nilfs);
diff --git a/include/linux/nilfs2_fs.h b/include/linux/nilfs2_fs.h
index 9875576..420be0b 100644
--- a/include/linux/nilfs2_fs.h
+++ b/include/linux/nilfs2_fs.h
@@ -727,6 +727,12 @@ struct nilfs_cpmode {
 	__u32 cm_pad;
 };
 
+/* values for v_flags */
+enum {
+	NILFS_CLEAN_SEGMENTS_DEFAULT,
+	NILFS_CLEAN_SEGMENTS_UPDATE_SEGUSG,
+};
+
 /**
  * struct nilfs_argv - argument vector
  * @v_base: pointer on data array from userspace
-- 
1.8.5.3

--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] nilfs2: depending on flags, update segment usage instead of cleaning

[Ryusuke Konishi]

Hi Andreas,

On Sun, 19 Jan 2014 15:02:21 +0100, Andreas Rohner wrote:
> This patch introduces two new flags, which can be used to modify the
> behavior of the cleaner.
> 
> NILFS_CLEAN_SEGMENTS_DEFAULT: Normal GC operation
> 
> NILFS_CLEAN_SEGMENTS_UPDATE_SEGUSG: Do not move any blocks, just update
>         the segment usage information
> 
> Additionally this patch implements a simple update function, that
> modifies the SUFILE in such a way, that old segments effectively become
> new segments, without the need to move the blocks around.
> 
> This is useful because it allows the userspace GC to avoid
> copying segments around needlessly, and still get essentially the same
> effekt. This way huge amounts of write operations can be avoided and
> the performance improves significantly.
> 
> Signed-off-by: Andreas Rohner <andreas.rohner-hi6Y0CQ0nG0@public.gmane.org>

Thank you for posting the patch.

Could you consider adding NILFS_IOCTL_SET_SUINFO instead of extending
v_flags of NILFS_IOCTL_CLEAN_SEGMENTS ?

It is hacky to extend NILFS_IOCTL_CLEAN_SEGMENTS like this, and,
unfortunately, argv[]->v_flags of NILFS_IOCTL_CLEAN_SEGMENTS is not
zero-filled in the current library implementation.  This is our
mistake (so I will fix it soon), but we cannot use these flags for
some time.  Otherwise, existing cleanerds will go wrong when this is
merged into kernel.

Presence of ioctls can be tested with ENOTTY error, so libnilfs
can know whether nilfs in underlying kernel has NILFS_IOCTL_SET_SUINFO
ioctl or not, and we can extend the library keeping compatibility
by using this nature.

A good example of code updating metadata file is
nilfs_ioctl_change_cpmode() even though NILFS_IOCTL_SET_SUINFO will
need nilfs_ioctl_wrap_copy().  It would be helpful for you.

Additional comments are as follows:

- For NILFS_IOCTL_SET_SUINFO, v_flags should be used to define which
  fields (lastmod, nblocks, flags) are modified.  These flags should
  be defined with bit masks.


Thanks,
Ryusuke Konishi

> ---
>  fs/nilfs2/ioctl.c         | 58 ++++++++++++++++++++++++++++++++++++++++-------
>  include/linux/nilfs2_fs.h |  6 +++++
>  2 files changed, 56 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/nilfs2/ioctl.c b/fs/nilfs2/ioctl.c
> index b44bdb2..52b967a 100644
> --- a/fs/nilfs2/ioctl.c
> +++ b/fs/nilfs2/ioctl.c
> @@ -571,6 +571,42 @@ int nilfs_ioctl_prepare_clean_segments(struct the_nilfs *nilfs,
>  	return ret;
>  }
>  
> +static int nilfs_ioctl_update_segment_usage(struct super_block *sb,
> +				struct nilfs_argv argv[5], void *bufs[5])
> +{
> +	size_t nmembs;
> +	struct the_nilfs *nilfs = sb->s_fs_info;
> +	struct inode *sufile = nilfs->ns_sufile;
> +	struct nilfs_suinfo si;
> +	__u64 *segnums;
> +	int i, ret;
> +	struct nilfs_transaction_info ti;
> +
> +	ret = nilfs_transaction_begin(sb, &ti, 0);
> +	if (unlikely(ret))
> +		return ret;
> +
> +	nmembs = argv[4].v_nmembs;
> +	for (i = 0, segnums = bufs[4]; i < nmembs; ++i) {
> +		ret = nilfs_sufile_get_suinfo(sufile, segnums[i],
> +				&si, sizeof(struct nilfs_suinfo), 1);
> +		if (unlikely(ret < 0))
> +			goto failure;
> +
> +		ret = nilfs_sufile_set_segment_usage(sufile, segnums[i],
> +				si.sui_nblocks, nilfs->ns_ctime);
> +		if (unlikely(ret < 0))
> +			goto failure;
> +	}
> +
> +	nilfs_transaction_commit(sb);
> +	return ret;
> +
> + failure:
> +	nilfs_transaction_abort(sb);
> +	return ret;
> +}
> +
>  static int nilfs_ioctl_clean_segments(struct inode *inode, struct file *filp,
>  				      unsigned int cmd, void __user *argp)
>  {
> @@ -660,14 +696,20 @@ static int nilfs_ioctl_clean_segments(struct inode *inode, struct file *filp,
>  		goto out_free;
>  	}
>  
> -	ret = nilfs_ioctl_move_blocks(inode->i_sb, &argv[0], kbufs[0]);
> -	if (ret < 0)
> -		printk(KERN_ERR "NILFS: GC failed during preparation: "
> -			"cannot read source blocks: err=%d\n", ret);
> -	else {
> -		if (nilfs_sb_need_update(nilfs))
> -			set_nilfs_discontinued(nilfs);
> -		ret = nilfs_clean_segments(inode->i_sb, argv, kbufs);
> +	if (argv[0].v_flags == NILFS_CLEAN_SEGMENTS_UPDATE_SEGUSG) {
> +		/* only update segment usage */
> +		ret = nilfs_ioctl_update_segment_usage(inode->i_sb, argv,
> +				kbufs);
> +	} else {
> +		ret = nilfs_ioctl_move_blocks(inode->i_sb, &argv[0], kbufs[0]);
> +		if (ret < 0)
> +			printk(KERN_ERR "NILFS: GC failed during preparation: "
> +				"cannot read source blocks: err=%d\n", ret);
> +		else {
> +			if (nilfs_sb_need_update(nilfs))
> +				set_nilfs_discontinued(nilfs);
> +			ret = nilfs_clean_segments(inode->i_sb, argv, kbufs);
> +		}
>  	}
>  
>  	nilfs_remove_all_gcinodes(nilfs);
> diff --git a/include/linux/nilfs2_fs.h b/include/linux/nilfs2_fs.h
> index 9875576..420be0b 100644
> --- a/include/linux/nilfs2_fs.h
> +++ b/include/linux/nilfs2_fs.h
> @@ -727,6 +727,12 @@ struct nilfs_cpmode {
>  	__u32 cm_pad;
>  };
>  
> +/* values for v_flags */
> +enum {
> +	NILFS_CLEAN_SEGMENTS_DEFAULT,
> +	NILFS_CLEAN_SEGMENTS_UPDATE_SEGUSG,
> +};
> +
>  /**
>   * struct nilfs_argv - argument vector
>   * @v_base: pointer on data array from userspace
> -- 
> 1.8.5.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] nilfs2: depending on flags, update segment usage instead of cleaning

[Andreas Rohner]

Hi Ryusuke,

On 2014-01-19 17:49, Ryusuke Konishi wrote:
> Could you consider adding NILFS_IOCTL_SET_SUINFO instead of extending
> v_flags of NILFS_IOCTL_CLEAN_SEGMENTS ?

Yes sure. I actually considered that writing the patch, but then shyed
away from adding a new ioctl.

> It is hacky to extend NILFS_IOCTL_CLEAN_SEGMENTS like this, and,
> unfortunately, argv[]->v_flags of NILFS_IOCTL_CLEAN_SEGMENTS is not
> zero-filled in the current library implementation.  This is our
> mistake (so I will fix it soon), but we cannot use these flags for
> some time.  Otherwise, existing cleanerds will go wrong when this is
> merged into kernel.

Ah yes I didn't think of that.

> Presence of ioctls can be tested with ENOTTY error, so libnilfs
> can know whether nilfs in underlying kernel has NILFS_IOCTL_SET_SUINFO
> ioctl or not, and we can extend the library keeping compatibility
> by using this nature.
> 
> A good example of code updating metadata file is
> nilfs_ioctl_change_cpmode() even though NILFS_IOCTL_SET_SUINFO will
> need nilfs_ioctl_wrap_copy().  It would be helpful for you.
> 
> Additional comments are as follows:
> 
> - For NILFS_IOCTL_SET_SUINFO, v_flags should be used to define which
>   fields (lastmod, nblocks, flags) are modified.  These flags should
>   be defined with bit masks.

Thank you for your comments. I will try and implement it and come back
with a new version of my patch.

Best regards,
Andreas Rohner

--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/4] nilfs-utils: new feature to skip inefficient gc

[Vyacheslav Dubeyko]

Hi Andreas,

On Sun, 2014-01-19 at 15:01 +0100, Andreas Rohner wrote:
> Hi,
> 
> 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.
> 

Thank you for this patchset. Your efforts in GC direction is very
important for NILFS2 users.

Anyway, I worried about compatibility. I think that it needs to analyze
more deeply how NILFS2 file system driver is ready for this change. I
think that it needs to describe briefly and in more details your
approach in patchset description. Do you try to analyze what potential
side effects can add your pathset? Could you describe your vision of
this patchset sanity?

> 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, which is not unreasonable, and lets assume these
> static files are in the oldest segments. So the current timestamp
> policy will select the oldest segments and since the data is static
> it needs to be moved to new segments. Now it is in the newest segments,
> but after a while they will become the oldest segments again. Then
> timestamp will move them again. These moving operations are expensive
> and unnecessary.
> 
> I designed a benchmark that uses a fresh NILFS2 volume, initialized
> with 20% static data. Above that, normal file operations are simulated,
> but the static data is never touched. These are the results:
> 
> SSD:
>     Timestamp GB Written: 1857.88
>     Timestamp GB Read:    711.1417
>     Timestamp Runtime:    10724.27s
> 
>     Patched GB Written:   823.6395
>     Patched GB Read:      222.1085
>     Patched Runtime:      6374.93s
> 
> HDD:
>     Timestamp GB Written: 609.026
>     Timestamp GB Read:    382.4147
>     Timestamp Runtime:    13123.15s
> 
>     Patched GB Written:   423.8563
>     Patched GB Read:      199.2632
>     Patched Runtime:      9460.73s
> 
> Admittedly the benchmark is a bit biased to highlight the problem. On a
> real system the static data won't be so completely static and so the
> performance improvements are probably not so extreme.
> 

I think that it really needs to test this patchset by xfstests and
fsstress (part of LTP test suite) tools. What do you think?

Moreover, I think that it makes sense to use well-known benchmark suite
(or several) for testing and estimation.

> This is my first patch set to this mailing list. I hope 
> everything is formally correct.
> 
> Best regards,
> Andreas Rohner
> 

Usually, here it is used special delimiter ("--") here. But I don't
think that it is critical.

> Andreas Rohner (4):
>   nilfs-utils: cldconfig add an option to set minimal free blocks
>   nilfs-utils: cleanerd: add custom error value to enable fast retry
>   nilfs-utils: refactoring of nilfs_reclaim_segment to add minblocks
>     param
>   nilfs-utils: add support for and define some nilfs_argv flags
> 
>  include/nilfs.h                  |  2 +-
>  include/nilfs2_fs.h              |  6 ++++++
>  include/nilfs_gc.h               |  6 ++++--
>  lib/gc.c                         | 21 +++++++++++++++++----
>  lib/nilfs.c                      |  3 ++-
>  sbin/cleanerd/cldconfig.c        | 20 ++++++++++++++++++++
>  sbin/cleanerd/cldconfig.h        |  2 ++
>  sbin/cleanerd/cleanerd.c         | 15 ++++++++++++---
>  sbin/nilfs-resize/nilfs-resize.c |  2 +-
>  9 files changed, 65 insertions(+), 12 deletions(-)
> 

And it is expected branch version here, usually. :)

Thanks,
Vyacheslav Dubeyko.


--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] nilfs-utils: cldconfig add an option to set minimal free blocks

[Vyacheslav Dubeyko]

On Sun, 2014-01-19 at 15:01 +0100, Andreas Rohner wrote:

It is expected "From: <author name and name>" and "Subject: <subject>"
in the begin of patch.

> With this option the user can specify the minimal number of free/dead
> blocks, which is a threshold for the GC. 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.
> 

What will be under stress? I mean situation when we processed all
segments that it is valuable for cleaning on threshold basis. Then we
need to clean segments for space allocation but all "dirty" segments not
reasonable choice for cleaning because of threshold.

What will we have in situation when an user selects not proper value of
threshold?

I worried about situation of skipping sibling segments from cleaning. Is
NILFS2 driver really ready for this? Did you think about efficiency of
free space allocation after such cleaning and about file system
consistency? Are you sure that all will be good after such approach of
cleaning? I simply want to be sure that you have analyzed this.

Thanks,
Vyacheslav Dubeyko.


--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/4] nilfs-utils: new feature to skip inefficient gc

[Andreas Rohner]

Hi Vyacheslav,

On 2014-01-20 10:43, Vyacheslav Dubeyko wrote:
> Hi Andreas,
> 
> On Sun, 2014-01-19 at 15:01 +0100, Andreas Rohner wrote:
>> Hi,
>>
>> 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.
>>
> 
> Thank you for this patchset. Your efforts in GC direction is very
> important for NILFS2 users.
> 
> Anyway, I worried about compatibility. I think that it needs to analyze
> more deeply how NILFS2 file system driver is ready for this change. I
> think that it needs to describe briefly and in more details your
> approach in patchset description. Do you try to analyze what potential
> side effects can add your pathset? Could you describe your vision of
> this patchset sanity?

Ok I will try to go into more detail in version 2. It is just a simple
threshold to avoid the worst case behaviour of the timestamp policy. I
don't think, that there should be any compatibility issues, aside from
those pointed out by Ryusuke, which I am going to fix. The only side
effect I can think of is, if the threshold is set too high, and a lot of
segments don't get cleaned.

>> Admittedly the benchmark is a bit biased to highlight the problem. On a
>> real system the static data won't be so completely static and so the
>> performance improvements are probably not so extreme.
>>
> 
> I think that it really needs to test this patchset by xfstests and
> fsstress (part of LTP test suite) tools. What do you think?
> 
> Moreover, I think that it makes sense to use well-known benchmark suite
> (or several) for testing and estimation.

Yes you are right. I will try to use another benchmark. But it is hard
to use well-known benchmarks to test the GC, because they are designed
to test different things. Either not enough data is written and the GC
never starts or too much and the benchmark crashes with a "not enough
space left on the device" error. I don't know if xfstests or fsstress
can be tuned for my purposes, but I will look into it.

>> This is my first patch set to this mailing list. I hope 
>> everything is formally correct.
>>
>> Best regards,
>> Andreas Rohner
>>
> 
> Usually, here it is used special delimiter ("--") here. But I don't
> think that it is critical.

Thanks

> 
> And it is expected branch version here, usually. :)
> 

Thanks again

Best regards,
Andreas Rohner

--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/4] nilfs-utils: cleanerd: add custom error value to enable fast retry

[Vyacheslav Dubeyko]

On Sun, 2014-01-19 at 15:01 +0100, Andreas Rohner wrote:
> 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.

Now GC really decrease performance very frequently. So, you suggest
don't use timeout. :) I am afraid that it can decrease performance more.
Why do you think in opposite manner?

If we doesn't relate with idle system state or I/O load then rigid
timeout can really decrease performance, as far as I can judge. The main
problem of GC that it works in the background of main file system
operations and, as a result, decrease the whole performance. So, how the
"no-timeout" does correlate with main file system operation or with the
whole system load? 

Could you describe your vision how it will work?

I think that it makes sense to have in this patch as declarations as
implementation of concept. Otherwise, it is hard to understand the goal
of such suggestion.

> 
> Signed-off-by: Andreas Rohner <andreas.rohner-hi6Y0CQ0nG0@public.gmane.org>
> ---
>  include/nilfs_gc.h       |  2 ++
>  sbin/cleanerd/cleanerd.c | 10 +++++++++-
>  2 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/include/nilfs_gc.h b/include/nilfs_gc.h
> index 72e9947..7628ce1 100644
> --- a/include/nilfs_gc.h
> +++ b/include/nilfs_gc.h
> @@ -27,4 +27,6 @@ static inline int nilfs_suinfo_reclaimable(const struct nilfs_suinfo *si)
>  
>  extern void (*nilfs_gc_logger)(int priority, const char *fmt, ...);
>  
> +#define EGCTRYAGAIN 513

Is it really need to add special constant? Why don't use EAGAIN?

Thanks,
Vyacheslav Dubeyko.


--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] nilfs-utils: cldconfig add an option to set minimal free blocks

[Andreas Rohner]

Hi Vyacheslav,

On 2014-01-20 11:14, Vyacheslav Dubeyko wrote:
> On Sun, 2014-01-19 at 15:01 +0100, Andreas Rohner wrote:
> 
> It is expected "From: <author name and name>" and "Subject: <subject>"
> in the begin of patch.

Are you sure? I don't see that in other patches and I think this
information is already contained in the email headers. I used "git
send-email" to submit the patch.

>> With this option the user can specify the minimal number of free/dead
>> blocks, which is a threshold for the GC. 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.
>>
> 
> What will be under stress? I mean situation when we processed all
> segments that it is valuable for cleaning on threshold basis. Then we
> need to clean segments for space allocation but all "dirty" segments not
> reasonable choice for cleaning because of threshold.

You have a point here. I could add another option that is used when the
file system is under stress. Just like with cleaning_interval and
mc_cleaning_interval. By setting the mc-option to 0 you could disable
the threshold under stress.

> What will we have in situation when an user selects not proper value of
> threshold?

Well that's the users problem. You could also ask what will happen if
the user selects an improper min_clean_segments value ;)

> I worried about situation of skipping sibling segments from cleaning. Is
> NILFS2 driver really ready for this? Did you think about efficiency of
> free space allocation after such cleaning and about file system
> consistency? Are you sure that all will be good after such approach of
> cleaning? I simply want to be sure that you have analyzed this.

I tested it extensively and I am quite sure, that there are no problems
with that on the driver side.

Thanks for reviewing my patches,

Best regards,
Andreas Rohner
--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/4] nilfs-utils: new feature to skip inefficient gc

[Vyacheslav Dubeyko]

On Mon, 2014-01-20 at 11:37 +0100, Andreas Rohner wrote:

> > I think that it really needs to test this patchset by xfstests and
> > fsstress (part of LTP test suite) tools. What do you think?
> > 
> > Moreover, I think that it makes sense to use well-known benchmark suite
> > (or several) for testing and estimation.
> 
> Yes you are right. I will try to use another benchmark. But it is hard
> to use well-known benchmarks to test the GC, because they are designed
> to test different things. Either not enough data is written and the GC
> never starts or too much and the benchmark crashes with a "not enough
> space left on the device" error.

I don't know concrete reasons of such crashes. But namely efficient GC
policy should resolve such issues, from my point of view. :)

Usually, stable and mature file system should pass through xfstests and
fsstress without any trouble. Namely, such success is our goal, I think.

With the best regards,
Vyacheslav Dubeyko.


--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] nilfs-utils: cldconfig add an option to set minimal free blocks

[Vyacheslav Dubeyko]

On Mon, 2014-01-20 at 11:52 +0100, Andreas Rohner wrote:
> Hi Vyacheslav,
> 
> On 2014-01-20 11:14, Vyacheslav Dubeyko wrote:
> > On Sun, 2014-01-19 at 15:01 +0100, Andreas Rohner wrote:
> > 
> > It is expected "From: <author name and name>" and "Subject: <subject>"
> > in the begin of patch.
> 
> Are you sure? I don't see that in other patches and I think this
> information is already contained in the email headers. I used "git
> send-email" to submit the patch.
> 

It is requirement for kernel patches. You can see it in kernel
documentations.

> >> With this option the user can specify the minimal number of free/dead
> >> blocks, which is a threshold for the GC. 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.
> >>
> > 
> > What will be under stress? I mean situation when we processed all
> > segments that it is valuable for cleaning on threshold basis. Then we
> > need to clean segments for space allocation but all "dirty" segments not
> > reasonable choice for cleaning because of threshold.
> 
> You have a point here. I could add another option that is used when the
> file system is under stress. Just like with cleaning_interval and
> mc_cleaning_interval. By setting the mc-option to 0 you could disable
> the threshold under stress.
> 

I suppose that it should be made by nilfs_cleanerd by itself but not an
user.

> > What will we have in situation when an user selects not proper value of
> > threshold?
> 
> Well that's the users problem. You could also ask what will happen if
> the user selects an improper min_clean_segments value ;)
> 

I don't think that it is good approach not to defend an user from
mistaken decision.

> > I worried about situation of skipping sibling segments from cleaning. Is
> > NILFS2 driver really ready for this? Did you think about efficiency of
> > free space allocation after such cleaning and about file system
> > consistency? Are you sure that all will be good after such approach of
> > cleaning? I simply want to be sure that you have analyzed this.
> 
> I tested it extensively and I am quite sure, that there are no problems
> with that on the driver side.
> 

I want to listen the approach at whole and arguments how it works for
the NILFS2 at whole. Such words as "I tested it and it works" is
dangerous. Did you check your file system by fsck? I suppose that you
don't. So, we need some reasonable description of concept that it prove
your vision.

Thanks,
Vyacheslav Dubeyko.


--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] nilfs-utils: cldconfig add an option to set minimal free blocks

[Ryusuke Konishi]

On Mon, 20 Jan 2014 15:05:08 +0400, Vyacheslav Dubeyko wrote:
> On Mon, 2014-01-20 at 11:52 +0100, Andreas Rohner wrote:
>> Hi Vyacheslav,
>> 
>> On 2014-01-20 11:14, Vyacheslav Dubeyko wrote:
>> > On Sun, 2014-01-19 at 15:01 +0100, Andreas Rohner wrote:
>> > 
>> > It is expected "From: <author name and name>" and "Subject: <subject>"
>> > in the begin of patch.
>> 
>> Are you sure? I don't see that in other patches and I think this
>> information is already contained in the email headers. I used "git
>> send-email" to submit the patch.
>> 
> 
> It is requirement for kernel patches. You can see it in kernel
> documentations.

I think the additional from line and patch title line are only
required when they are different from those in the mail headers.

Regards,
Ryusuke Konishi
--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] nilfs-utils: cldconfig add an option to set minimal free blocks

[Andreas Rohner]

Hi Vyacheslav,

On 2014-01-20 12:05, Vyacheslav Dubeyko wrote:
> I suppose that it should be made by nilfs_cleanerd by itself but not an
> user.
> 
>>> What will we have in situation when an user selects not proper value of
>>> threshold?
>>
>> Well that's the users problem. You could also ask what will happen if
>> the user selects an improper min_clean_segments value ;)
>>
> 
> I don't think that it is good approach not to defend an user from
> mistaken decision.

I guess my point was, that NILFS2 isn't the user friendliest file system
on the planet anyway. Good documentation in the config file and sensible
default values would probably be the best solution.

We could of course set some default value and remove the option from the
config file, but I think it is a powerful option and the user might want
to tweak it to his or her needs.

>>> I worried about situation of skipping sibling segments from cleaning. Is
>>> NILFS2 driver really ready for this? Did you think about efficiency of
>>> free space allocation after such cleaning and about file system
>>> consistency? Are you sure that all will be good after such approach of
>>> cleaning? I simply want to be sure that you have analyzed this.
>>
>> I tested it extensively and I am quite sure, that there are no problems
>> with that on the driver side.
>>
> 
> I want to listen the approach at whole and arguments how it works for
> the NILFS2 at whole. Such words as "I tested it and it works" is
> dangerous. Did you check your file system by fsck? I suppose that you
> don't. So, we need some reasonable description of concept that it prove
> your vision.

Ok I am sorry about that. Let me address all of your points:

If you look at nilfs_sufile_alloc in sufile.c you will see, that the
allocation algorithm basically just remembers the last allocated segment
in sh_last_alloc and then from there sequentially scans the SUFILE for a
clean segment. Thereby it checks the flags of the nilfs_segment_usage
structure and skips it if it is not clean:

if (!nilfs_segment_usage_clean(su))
	continue;

So it is perfectly capable of skipping segments that are left over at
random places.

Could that reduce the performance of the allocation algorithm? Probably
yes, but I don't think it would be significant. Most of the SUFILE will
be in Cache anyway.

I don't see how the file system consistency could be affected by just
updating a timestamp in the SUFILE. The value of su_lastmod is never
used in the driver. It is only set at creation time and used by the GC
to sort the segment list.

Best regards,
Andreas Rohner
--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/4] nilfs-utils: cleanerd: add custom error value to enable fast retry

[Andreas Rohner]

Hi Vyacheslav,

Thank you for reviewing my patch set so thoroughly. I already have a few
new ideas to improve it from our exchange.

On 2014-01-20 11:46, Vyacheslav Dubeyko wrote:
> On Sun, 2014-01-19 at 15:01 +0100, Andreas Rohner wrote:
>> 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.
> 
> Now GC really decrease performance very frequently. So, you suggest
> don't use timeout. :) I am afraid that it can decrease performance more.
> Why do you think in opposite manner?

I don't suggest to eliminate timeout in general. It just adds the option
for nilfs_reclaim_segment to skip the timeout once by returning the
custom error code -EGCTRYAGAIN. This is useful because if the number of
free blocks is below the threshold the cleaner won't write to the device
and won't free any segments, so it is important to retry another segment
quickly. I guess it could be reasonable to use a shorter timeout instead
of no timeout in this case.

> If we doesn't relate with idle system state or I/O load then rigid
> timeout can really decrease performance, as far as I can judge. The main
> problem of GC that it works in the background of main file system
> operations and, as a result, decrease the whole performance. So, how the
> "no-timeout" does correlate with main file system operation or with the
> whole system load? 
> 
> Could you describe your vision how it will work?

Well the GC uses the normal timeout for its normal operation. If the
number of free blocks is below the threshold it skips the current
segment or segments and returns -EGCTRYAGAIN to skip the timeout ONCE.

I thought it is good practice to separate a big patch into logical
units. That's why it is probably not clear what the intent is by just
looking at a single patch in the patch set. I will try to improve my
commit message to make it clearer.

Best regards,
Andreas Rohner

--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] nilfs2: depending on flags, update segment usage instead of cleaning

[Andreas Rohner]

Hi Ryusuke,

On 2014-01-19 17:49, Ryusuke Konishi wrote:
> Additional comments are as follows:
> 
> - For NILFS_IOCTL_SET_SUINFO, v_flags should be used to define which
>   fields (lastmod, nblocks, flags) are modified.  These flags should
>   be defined with bit masks.

I forgot to mention in my cover letter, that I decided not to use the
v_flags field for the flags, because I needed a new structure to contain
the segment number anyway. It just seemed to be a better choice to put
the flags there as well. But if you want I can always change it to use
the v_flags field.

Best regards,
Andreas Rohner
--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html