[PATCH v3 0/4] nilfs-utils: shortcut for certain GC operations

[PATCH v3 0/4] nilfs-utils: shortcut for certain GC operations

[Andreas Rohner]

Hi,

This is the third version of this patch set. It basically fixes a 
lot of bugs.

v2->v3
* Alignment in nilfs_suinfo_update
* Add missing comments
* nilfs2: Extra validity checks in nilfs_sufile_set_suinfo
* nilfs2: Update sh_ncleansegs, sh_ndirtysegs if flags change
* nilfs2: Fix bugs in nilfs_sufile_set_suinfo
* nilfs2: Use vmalloc instead of ioctl_wrap_copy
* nilfs-utils: Separate nilfs_relaim_segments_with_threshold
* nilfs-utils: Allow different units for min_reclaimable_blocks
* nilfs-utils: Introduce flag to disable the optimization
* nilfs-utils: Disable optimization if kernel retruns ENOTTY
* nilfs-utils: Remove EGCTRYAGAIN error return code
* nilfs-utils: Rename option to min_reclaimable_blocks
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-file

This patch set adds an optimized version of 
nilfs_reclaim_segments, which has an additional parameter 
min_reclaimable. This parameter specifies the minimum number of 
reclaimable blocks in a segment, before it can be cleaned. If a 
segment is below this threshold, it is considered to be not worth 
cleaning, because all the live blocks would need to be moved to a 
new segment, which is expensive, and the number of reclaimable 
blocks is too low. But it is still necessary to update the segment 
usage information to turn the old segment into a new one.
    
This is basically a shortcut to cleaning the segment. It is still 
necessary to read the segment summary information, but the writing 
of the live blocks can be skipped if it's not worth it.

Additionally new options are introduced for the configuration file 
and nilfs-clean to allow the user to specify the threshold. 

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.

The benchmarks are currently running. I will give you the results
shortly.

Regards,
Andreas Rohner

---
Andreas Rohner (4):
  nilfs-utils: cldconfig add an option to set min. reclaimable blocks
  nilfs-utils: nilfs-clean add cmdline param min-reclaimable-blocks
  nilfs-utils: add suport for NILFS_IOCTL_SET_SUINFO ioctl
  nilfs-utils: add optimized version of nilfs_reclaim_segments

 include/nilfs.h                   |   9 +-
 include/nilfs2_fs.h               |  43 ++++++++++
 include/nilfs_cleaner.h           |  19 +++--
 include/nilfs_gc.h                |   4 +
 lib/gc.c                          | 112 ++++++++++++++++++++-----
 lib/nilfs.c                       |  60 ++++++++++++++
 man/nilfs-clean.8                 |   4 +
 man/nilfs_cleanerd.conf.5         |  22 +++++
 sbin/cleanerd/cldconfig.c         | 168 ++++++++++++++++++++++++++++----------
 sbin/cleanerd/cldconfig.h         |   8 ++
 sbin/cleanerd/cleanerd.c          |  39 ++++++++-
 sbin/cleanerd/nilfs_cleanerd.conf |  13 +++
 sbin/nilfs-clean/nilfs-clean.c    |  44 ++++++++--
 13 files changed, 466 insertions(+), 79 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 v3 1/4] nilfs-utils: cldconfig add an option to set min. reclaimable blocks

[Andreas Rohner]

With this option the user can specify the minimum number of
reclaimable blocks of a segment, before it can be cleaned. This is
a threshold for the GC to prevent needless moving of data. If
there are less reclaimable blocks in a segment than the specified
number, the GC will abort and try again with a different segment.

If there are less clean segments than min_clean_segments,
mc_min_reclaimable_blocks is used instead of
min_reclaimable_blocks. This allows for more flexibility in
configuring the GC.

The number of blocks can be specified in percent of a segment or
in bytes.

If the use_set_suinfo switch is not set, the optimization is
completely disabled.

Signed-off-by: Andreas Rohner <andreas.rohner-hi6Y0CQ0nG0@public.gmane.org>
---
 include/nilfs.h                   |   7 +-
 lib/nilfs.c                       |  28 +++++++
 man/nilfs_cleanerd.conf.5         |  22 +++++
 sbin/cleanerd/cldconfig.c         | 168 ++++++++++++++++++++++++++++----------
 sbin/cleanerd/cldconfig.h         |   8 ++
 sbin/cleanerd/cleanerd.c          |  15 ++++
 sbin/cleanerd/nilfs_cleanerd.conf |  13 +++
 7 files changed, 218 insertions(+), 43 deletions(-)

diff --git a/include/nilfs.h b/include/nilfs.h
index 56286a9..b5f85d3 100644
--- a/include/nilfs.h
+++ b/include/nilfs.h
@@ -128,7 +128,8 @@ struct nilfs {
 #define NILFS_OPEN_RDWR		0x0008	/* Open NILFS API in read/write mode */
 #define NILFS_OPEN_GCLK		0x1000	/* Open GC lock primitive */
 
-#define NILFS_OPT_MMAP	0x01
+#define NILFS_OPT_MMAP		0x01
+#define NILFS_OPT_SET_SUINFO	0x02
 
 
 struct nilfs *nilfs_open(const char *, const char *, int);
@@ -140,6 +141,10 @@ void nilfs_opt_clear_mmap(struct nilfs *);
 int nilfs_opt_set_mmap(struct nilfs *);
 int nilfs_opt_test_mmap(struct nilfs *);
 
+void nilfs_opt_clear_set_suinfo(struct nilfs *);
+int nilfs_opt_set_set_suinfo(struct nilfs *);
+int nilfs_opt_test_set_suinfo(struct nilfs *);
+
 nilfs_cno_t nilfs_get_oldest_cno(struct nilfs *);
 
 struct nilfs_super_block *nilfs_get_sb(struct nilfs *);
diff --git a/lib/nilfs.c b/lib/nilfs.c
index c1771e8..93822de 100644
--- a/lib/nilfs.c
+++ b/lib/nilfs.c
@@ -288,6 +288,34 @@ int nilfs_opt_test_mmap(struct nilfs *nilfs)
 	return !!(nilfs->n_opts & NILFS_OPT_MMAP);
 }
 
+/**
+ * nilfs_opt_set_set_suinfo - set set_suinfo option
+ * @nilfs: nilfs object
+ */
+int nilfs_opt_set_set_suinfo(struct nilfs *nilfs)
+{
+	nilfs->n_opts |= NILFS_OPT_SET_SUINFO;
+	return 0;
+}
+
+/**
+ * nilfs_opt_clear_set_suinfo - clear set_suinfo option
+ * @nilfs: nilfs object
+ */
+void nilfs_opt_clear_set_suinfo(struct nilfs *nilfs)
+{
+	nilfs->n_opts &= ~NILFS_OPT_SET_SUINFO;
+}
+
+/**
+ * nilfs_opt_test_set_suinfo - test whether set_suinfo option is set or not
+ * @nilfs: nilfs object
+ */
+int nilfs_opt_test_set_suinfo(struct nilfs *nilfs)
+{
+	return !!(nilfs->n_opts & NILFS_OPT_SET_SUINFO);
+}
+
 static int nilfs_open_sem(struct nilfs *nilfs)
 {
 	char semnambuf[NAME_MAX - 4];
diff --git a/man/nilfs_cleanerd.conf.5 b/man/nilfs_cleanerd.conf.5
index 8f3fcd2..4315ecd 100644
--- a/man/nilfs_cleanerd.conf.5
+++ b/man/nilfs_cleanerd.conf.5
@@ -79,6 +79,28 @@ Specify whether to use \fBmmap\fP(2) for reading segments.  At
 present, this option is enabled if supported regardless of this
 directive.
 .TP
+.B use_set_suinfo
+Specify whether to use the set_suinfo ioctl if it is supported. This is
+necessary for the \fBmin_reclaimable_blocks\fP feature. By disabling this
+switch \fBmin_reclaimable_blocks\fP is also disabled.
+.TP
+.B min_reclaimable_blocks
+Specify the minimum number of reclaimable blocks in a segment before
+it can be cleaned.
+.TP
+.B mc_min_reclaimable_blocks
+Specify the minimum number of reclaimable blocks in a segment before
+it can be cleaned. if clean segments < min_clean_segments.
+.PP
+\fBmin_reclaimable_blocks\fP and \fBmc_min_reclaimable_blocks\fP may
+be followed by a percent sign or the following multiplicative suffixes:
+kB 1000, K 1024, MB 1000*1000, M 1024*1024, GB 1000*1000*1000, G
+1024*1024*1024, and so on for T, P, E.  If the argument is followed by
+a percent sign, it represents the ratio of blocks in a segment.
+.PP
+The default values of \fBmin_reclaimable_blocks\fP and
+\fBmc_min_reclaimable_blocks\fP are 5 percent and 1 percent respectively.
+.TP
 .B log_priority
 Gives the verbosity level that is used when logging messages from
 \fBnilfs_cleanerd\fP(8).  The possible values are: \fBemerg\fP,
diff --git a/sbin/cleanerd/cldconfig.c b/sbin/cleanerd/cldconfig.c
index 270d360..b69f060 100644
--- a/sbin/cleanerd/cldconfig.c
+++ b/sbin/cleanerd/cldconfig.c
@@ -278,6 +278,54 @@ nilfs_cldconfig_handle_protection_period(struct nilfs_cldconfig *config,
 }
 
 static unsigned long long
+nilfs_convert_units_to_bytes(struct nilfs_param *param) {
+	unsigned long long bytes = param->num;
+
+	switch (param->unit) {
+	case NILFS_SIZE_UNIT_KB:
+		bytes *= 1000ULL;
+		break;
+	case NILFS_SIZE_UNIT_KIB:
+		bytes <<= 10;
+		break;
+	case NILFS_SIZE_UNIT_MB:
+		bytes *= 1000000ULL;
+		break;
+	case NILFS_SIZE_UNIT_MIB:
+		bytes <<= 20;
+		break;
+	case NILFS_SIZE_UNIT_GB:
+		bytes *= 1000000000ULL;
+		break;
+	case NILFS_SIZE_UNIT_GIB:
+		bytes <<= 30;
+		break;
+	case NILFS_SIZE_UNIT_TB:
+		bytes *= 1000000000000ULL;
+		break;
+	case NILFS_SIZE_UNIT_TIB:
+		bytes <<= 40;
+		break;
+	case NILFS_SIZE_UNIT_PB:
+		bytes *= 1000000000000000ULL;
+		break;
+	case NILFS_SIZE_UNIT_PIB:
+		bytes <<= 50;
+		break;
+	case NILFS_SIZE_UNIT_EB:
+		bytes *= 1000000000000000000ULL;
+		break;
+	case NILFS_SIZE_UNIT_EIB:
+		bytes <<= 60;
+		break;
+	default:
+		assert(0);
+	}
+
+	return bytes;
+}
+
+static unsigned long long
 nilfs_convert_size_to_nsegments(struct nilfs *nilfs, struct nilfs_param *param)
 {
 	unsigned long long ret, segment_size, bytes;
@@ -287,48 +335,7 @@ nilfs_convert_size_to_nsegments(struct nilfs *nilfs, struct nilfs_param *param)
 	} else if (param->unit == NILFS_SIZE_UNIT_PERCENT) {
 		ret = (nilfs_get_nsegments(nilfs) * param->num + 99) / 100;
 	} else {
-		bytes = param->num;
-
-		switch (param->unit) {
-		case NILFS_SIZE_UNIT_KB:
-			bytes *= 1000ULL;
-			break;
-		case NILFS_SIZE_UNIT_KIB:
-			bytes <<= 10;
-			break;
-		case NILFS_SIZE_UNIT_MB:
-			bytes *= 1000000ULL;
-			break;
-		case NILFS_SIZE_UNIT_MIB:
-			bytes <<= 20;
-			break;
-		case NILFS_SIZE_UNIT_GB:
-			bytes *= 1000000000ULL;
-			break;
-		case NILFS_SIZE_UNIT_GIB:
-			bytes <<= 30;
-			break;
-		case NILFS_SIZE_UNIT_TB:
-			bytes *= 1000000000000ULL;
-			break;
-		case NILFS_SIZE_UNIT_TIB:
-			bytes <<= 40;
-			break;
-		case NILFS_SIZE_UNIT_PB:
-			bytes *= 1000000000000000ULL;
-			break;
-		case NILFS_SIZE_UNIT_PIB:
-			bytes <<= 50;
-			break;
-		case NILFS_SIZE_UNIT_EB:
-			bytes *= 1000000000000000000ULL;
-			break;
-		case NILFS_SIZE_UNIT_EIB:
-			bytes <<= 60;
-			break;
-		default:
-			assert(0);
-		}
+		bytes = nilfs_convert_units_to_bytes(param);
 		segment_size = nilfs_get_block_size(nilfs) *
 			nilfs_get_blocks_per_segment(nilfs);
 		ret = (bytes + segment_size - 1) / segment_size;
@@ -455,6 +462,52 @@ nilfs_cldconfig_handle_mc_nsegments_per_clean(struct nilfs_cldconfig *config,
 	return 0;
 }
 
+static unsigned long long
+nilfs_convert_size_to_blocks_per_segment(struct nilfs *nilfs,
+					 struct nilfs_param *param)
+{
+	unsigned long long ret, segment_size, block_size, bytes;
+
+	if (param->unit == NILFS_SIZE_UNIT_NONE) {
+		ret = param->num;
+	} else if (param->unit == NILFS_SIZE_UNIT_PERCENT) {
+		ret = (nilfs_get_blocks_per_segment(nilfs) * param->num) / 100;
+	} else {
+		block_size = nilfs_get_block_size(nilfs);
+		segment_size = block_size *
+				nilfs_get_blocks_per_segment(nilfs);
+		bytes = nilfs_convert_units_to_bytes(param) % segment_size;
+		ret = (bytes + block_size - 1) / block_size;
+	}
+	return ret;
+}
+
+static int
+nilfs_cldconfig_handle_min_reclaimable_blocks(struct nilfs_cldconfig *config,
+					      char **tokens, size_t ntoks,
+					      struct nilfs *nilfs)
+{
+	struct nilfs_param param;
+
+	if (nilfs_cldconfig_get_size_argument(tokens, ntoks, &param) == 0)
+		config->cf_min_reclaimable_blocks =
+			nilfs_convert_size_to_blocks_per_segment(nilfs, &param);
+	return 0;
+}
+
+static int
+nilfs_cldconfig_handle_mc_min_reclaimable_blocks(struct nilfs_cldconfig *config,
+						 char **tokens, size_t ntoks,
+						 struct nilfs *nilfs)
+{
+	struct nilfs_param param;
+
+	if (nilfs_cldconfig_get_size_argument(tokens, ntoks, &param) == 0)
+		config->cf_mc_min_reclaimable_blocks =
+			nilfs_convert_size_to_blocks_per_segment(nilfs, &param);
+	return 0;
+}
+
 static int
 nilfs_cldconfig_handle_cleaning_interval(struct nilfs_cldconfig *config,
 					 char **tokens, size_t ntoks,
@@ -490,6 +543,14 @@ static int nilfs_cldconfig_handle_use_mmap(struct nilfs_cldconfig *config,
 	return 0;
 }
 
+static int nilfs_cldconfig_handle_use_set_suinfo(struct nilfs_cldconfig *config,
+						 char **tokens, size_t ntoks,
+						 struct nilfs *nilfs)
+{
+	config->cf_use_set_suinfo = 1;
+	return 0;
+}
+
 static const struct nilfs_cldconfig_log_priority
 nilfs_cldconfig_log_priority_table[] = {
 	{"emerg",	LOG_EMERG},
@@ -576,6 +637,18 @@ nilfs_cldconfig_keyword_table[] = {
 		"log_priority", 2, 2,
 		nilfs_cldconfig_handle_log_priority
 	},
+	{
+		"min_reclaimable_blocks", 2, 2,
+		nilfs_cldconfig_handle_min_reclaimable_blocks
+	},
+	{
+		"mc_min_reclaimable_blocks", 2, 2,
+		nilfs_cldconfig_handle_mc_min_reclaimable_blocks
+	},
+	{
+		"use_set_suinfo", 1, 1,
+		nilfs_cldconfig_handle_use_set_suinfo
+	},
 };
 
 #define NILFS_CLDCONFIG_NKEYWORDS			\
@@ -640,7 +713,18 @@ static void nilfs_cldconfig_set_default(struct nilfs_cldconfig *config,
 	config->cf_retry_interval.tv_sec = NILFS_CLDCONFIG_RETRY_INTERVAL;
 	config->cf_retry_interval.tv_usec = 0;
 	config->cf_use_mmap = NILFS_CLDCONFIG_USE_MMAP;
+	config->cf_use_set_suinfo = NILFS_CLDCONFIG_USE_SET_SUINFO;
 	config->cf_log_priority = NILFS_CLDCONFIG_LOG_PRIORITY;
+
+	param.num = NILFS_CLDCONFIG_MIN_RECLAIMABLE_BLOCKS;
+	param.unit = NILFS_CLDCONFIG_MIN_RECLAIMABLE_BLOCKS_UNIT;
+	config->cf_min_reclaimable_blocks =
+		nilfs_convert_size_to_blocks_per_segment(nilfs, &param);
+
+	param.num = NILFS_CLDCONFIG_MC_MIN_RECLAIMABLE_BLOCKS;
+	param.unit = NILFS_CLDCONFIG_MC_MIN_RECLAIMABLE_BLOCKS_UNIT;
+	config->cf_mc_min_reclaimable_blocks =
+		nilfs_convert_size_to_blocks_per_segment(nilfs, &param);
 }
 
 static inline int iseol(int c)
diff --git a/sbin/cleanerd/cldconfig.h b/sbin/cleanerd/cldconfig.h
index 188ce9b..e7ac72e 100644
--- a/sbin/cleanerd/cldconfig.h
+++ b/sbin/cleanerd/cldconfig.h
@@ -101,7 +101,10 @@ struct nilfs_cldconfig {
 	struct timeval cf_mc_cleaning_interval;
 	struct timeval cf_retry_interval;
 	int cf_use_mmap;
+	int cf_use_set_suinfo;
 	int cf_log_priority;
+	unsigned long cf_min_reclaimable_blocks;
+	unsigned long cf_mc_min_reclaimable_blocks;
 };
 
 #define NILFS_CLDCONFIG_SELECTION_POLICY_IMPORTANCE	\
@@ -119,7 +122,12 @@ struct nilfs_cldconfig {
 #define NILFS_CLDCONFIG_MC_CLEANING_INTERVAL		1
 #define NILFS_CLDCONFIG_RETRY_INTERVAL			60
 #define NILFS_CLDCONFIG_USE_MMAP			1
+#define NILFS_CLDCONFIG_USE_SET_SUINFO			0
 #define NILFS_CLDCONFIG_LOG_PRIORITY			LOG_INFO
+#define NILFS_CLDCONFIG_MIN_RECLAIMABLE_BLOCKS		5
+#define NILFS_CLDCONFIG_MIN_RECLAIMABLE_BLOCKS_UNIT	NILFS_SIZE_UNIT_PERCENT
+#define NILFS_CLDCONFIG_MC_MIN_RECLAIMABLE_BLOCKS	1
+#define NILFS_CLDCONFIG_MC_MIN_RECLAIMABLE_BLOCKS_UNIT	NILFS_SIZE_UNIT_PERCENT
 
 #define NILFS_CLDCONFIG_NSEGMENTS_PER_CLEAN_MAX	32
 
diff --git a/sbin/cleanerd/cleanerd.c b/sbin/cleanerd/cleanerd.c
index 742ab98..bf8b8b0 100644
--- a/sbin/cleanerd/cleanerd.c
+++ b/sbin/cleanerd/cleanerd.c
@@ -172,6 +172,7 @@ struct nilfs_cleanerd {
 	struct timeval cleaning_interval;
 	struct timeval target;
 	struct timeval timeout;
+	unsigned long min_reclaimable_blocks;
 	__u64 prev_nongc_ctime;
 	mqd_t recvq;
 	char *recvq_name;
@@ -246,6 +247,12 @@ static int nilfs_cleanerd_config(struct nilfs_cleanerd *cleanerd,
 	else
 		nilfs_opt_clear_mmap(cleanerd->nilfs);
 #endif	/* HAVE_MMAP */
+
+	if (cleanerd->config.cf_use_set_suinfo)
+		nilfs_opt_set_set_suinfo(cleanerd->nilfs);
+	else
+		nilfs_opt_clear_set_suinfo(cleanerd->nilfs);
+
 	nilfs_cleanerd_set_log_priority(cleanerd);
 
 	if (protection_period != ULONG_MAX) {
@@ -273,6 +280,8 @@ static int nilfs_cleanerd_reconfig(struct nilfs_cleanerd *cleanerd,
 	} else {
 		cleanerd->ncleansegs = config->cf_nsegments_per_clean;
 		cleanerd->cleaning_interval = config->cf_cleaning_interval;
+		cleanerd->min_reclaimable_blocks =
+				config->cf_min_reclaimable_blocks;
 		syslog(LOG_INFO, "configuration file reloaded");
 	}
 	return ret;
@@ -1238,10 +1247,14 @@ static int nilfs_cleanerd_handle_clean_check(struct nilfs_cleanerd *cleanerd,
 		/* disk space is close to limit -- accelerate cleaning */
 		cleanerd->ncleansegs = config->cf_mc_nsegments_per_clean;
 		cleanerd->cleaning_interval = config->cf_mc_cleaning_interval;
+		cleanerd->min_reclaimable_blocks =
+				config->cf_mc_min_reclaimable_blocks;
 	} else {
 		/* continue to run */
 		cleanerd->ncleansegs = config->cf_nsegments_per_clean;
 		cleanerd->cleaning_interval = config->cf_cleaning_interval;
+		cleanerd->min_reclaimable_blocks =
+				config->cf_min_reclaimable_blocks;
 	}
 
 	return 0; /* do gc */
@@ -1427,6 +1440,8 @@ static int nilfs_cleanerd_clean_loop(struct nilfs_cleanerd *cleanerd)
 
 	cleanerd->ncleansegs = cleanerd->config.cf_nsegments_per_clean;
 	cleanerd->cleaning_interval = cleanerd->config.cf_cleaning_interval;
+	cleanerd->min_reclaimable_blocks =
+			cleanerd->config.cf_min_reclaimable_blocks;
 
 
 	if (nilfs_cleanerd_automatic_suspend(cleanerd))
diff --git a/sbin/cleanerd/nilfs_cleanerd.conf b/sbin/cleanerd/nilfs_cleanerd.conf
index 26872aa..f88a57a 100644
--- a/sbin/cleanerd/nilfs_cleanerd.conf
+++ b/sbin/cleanerd/nilfs_cleanerd.conf
@@ -51,6 +51,19 @@ mc_cleaning_interval	1
 # Retry interval in seconds.
 retry_interval		60
 
+# Specify the minimum number of reclaimable blocks in a segment
+# before it can be cleaned.
+min_reclaimable_blocks	5%
+
+# Specify the minimum number of reclaimable blocks in a segment
+# before it can be cleaned.
+# if clean segments < min_clean_segments
+mc_min_reclaimable_blocks	1%
+
+# enable set_suinfo ioctl if supported
+# (needed for min_reclaimable_blocks)
+use_set_suinfo
+
 # Use mmap when reading segments if supported.
 use_mmap
 
-- 
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 v3 2/4] nilfs-utils: nilfs-clean add cmdline param min-reclaimable-blocks

[Andreas Rohner]

This patch adds a command line parameter for nilfs-clean to allow
the manual override of the min_reclaimable_blocks option.

Signed-off-by: Andreas Rohner <andreas.rohner-hi6Y0CQ0nG0@public.gmane.org>
---
 include/nilfs_cleaner.h        | 19 +++++++++---------
 man/nilfs-clean.8              |  4 ++++
 sbin/cleanerd/cleanerd.c       | 19 ++++++++++++++++++
 sbin/nilfs-clean/nilfs-clean.c | 44 ++++++++++++++++++++++++++++++++++++------
 4 files changed, 71 insertions(+), 15 deletions(-)

diff --git a/include/nilfs_cleaner.h b/include/nilfs_cleaner.h
index 0bf02aa..874c17a 100644
--- a/include/nilfs_cleaner.h
+++ b/include/nilfs_cleaner.h
@@ -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_reclaimable_blocks;
 };
 /* 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 */
+#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_RECLAIMABLE_BLOCKS	(1 << 8)
 
 enum {
 	NILFS_CLEANER_STATUS_IDLE,
diff --git a/man/nilfs-clean.8 b/man/nilfs-clean.8
index 04e11c6..94267e1 100644
--- a/man/nilfs-clean.8
+++ b/man/nilfs-clean.8
@@ -43,6 +43,10 @@ Display cleaner status.
 \fB\-h\fR, \fB\-\-help\fR
 Display help message and exit.
 .TP
+\fB\-m\fR, \fB\-\-min\-reclaimable\-blocks=\fICOUNT\fR
+Specify the minimum number of reclaimable blocks in a segment before
+it can be cleaned.
+.TP
 \fB\-p\fR, \fB\-\-protection-period=\fIinterval\fR
 Set protection period for a cleaner run.  The \fIinterval\fR parameter
 is an integer value and specifies the minimum time that deleted or
diff --git a/sbin/cleanerd/cleanerd.c b/sbin/cleanerd/cleanerd.c
index bf8b8b0..13018b2 100644
--- a/sbin/cleanerd/cleanerd.c
+++ b/sbin/cleanerd/cleanerd.c
@@ -185,6 +185,7 @@ struct nilfs_cleanerd {
 	long mm_ncleansegs;
 	struct timeval mm_protection_period;
 	struct timeval mm_cleaning_interval;
+	unsigned long mm_min_reclaimable_blocks;
 };
 
 /**
@@ -468,6 +469,14 @@ nilfs_cleanerd_protection_period(struct nilfs_cleanerd *cleanerd)
 		&cleanerd->config.cf_protection_period;
 }
 
+static unsigned long
+nilfs_cleanerd_min_reclaimable_blocks(struct nilfs_cleanerd *cleanerd)
+{
+	return cleanerd->running == 2 ?
+		cleanerd->mm_min_reclaimable_blocks :
+		cleanerd->min_reclaimable_blocks;
+}
+
 static void
 nilfs_cleanerd_reduce_ncleansegs_for_retry(struct nilfs_cleanerd *cleanerd)
 {
@@ -1010,6 +1019,16 @@ static int nilfs_cleanerd_cmd_run(struct nilfs_cleanerd *cleanerd,
 		cleanerd->mm_cleaning_interval =
 			cleanerd->cleaning_interval;
 	}
+	/* minimal reclaimable blocks */
+	if ((req2->args.valid & NILFS_CLEANER_ARG_MIN_RECLAIMABLE_BLOCKS)
+		&& req2->args.min_reclaimable_blocks <=
+			nilfs_get_blocks_per_segment(cleanerd->nilfs)) {
+		cleanerd->mm_min_reclaimable_blocks =
+			req2->args.min_reclaimable_blocks;
+	} else {
+		cleanerd->mm_min_reclaimable_blocks =
+			cleanerd->min_reclaimable_blocks;
+	}
 	/* number of passes */
 	if (req2->args.valid & NILFS_CLEANER_ARG_NPASSES) {
 		if (!req2->args.npasses)
diff --git a/sbin/nilfs-clean/nilfs-clean.c b/sbin/nilfs-clean/nilfs-clean.c
index 55abd55..f77fdf7 100644
--- a/sbin/nilfs-clean/nilfs-clean.c
+++ b/sbin/nilfs-clean/nilfs-clean.c
@@ -77,6 +77,7 @@ const static struct option long_option[] = {
 	{"stop", no_argument, NULL, 'b'},
 	{"suspend", no_argument, NULL, 's'},
 	{"speed", required_argument, NULL, 'S'},
+	{"min-reclaimable-blocks", required_argument, NULL, 'm'},
 	{"verbose", no_argument, NULL, 'v'},
 	{"version", no_argument, NULL, 'V'},
 	{NULL, 0, NULL, 0}
@@ -90,6 +91,9 @@ const static struct option long_option[] = {
 	"  -l, --status\t\tdisplay cleaner status\n"			\
 	"  -p, --protection-period=SECONDS\n"				\
 	"               \t\tspecify protection period\n"		\
+	"  -m, --min-reclaimable-blocks=COUNT\n"			\
+	"               \t\tset minimum number of reclaimable blocks\n"	\
+	"               \t\tbefore a segment can be cleaned\n"		\
 	"  -q, --quit\t\tshutdown cleaner\n"				\
 	"  -r, --resume\t\tresume cleaner\n"				\
 	"  -s, --suspend\t\tsuspend cleaner\n"				\
@@ -98,9 +102,10 @@ const static struct option long_option[] = {
 	"  -v, --verbose\t\tverbose mode\n"				\
 	"  -V, --version\t\tdisplay version and exit\n"
 #else
-#define NILFS_CLEAN_USAGE						\
-	"Usage: %s [-b] [-c [conffile]] [-h] [-l] [-p protection-period]" \
-	"          [-q] [-r] [-s] [-S gc-speed] [-v] [-V] [device]\n"
+#define NILFS_CLEAN_USAGE						  \
+	"Usage: %s [-b] [-c [conffile]] [-h] [-l] [-m blocks]\n"	  \
+	"          [-p protection-period] [-q] [-r] [-s] [-S gc-speed]\n" \
+	"          [-v] [-V] [device]\n"
 #endif	/* _GNU_SOURCE */
 
 
@@ -124,6 +129,7 @@ static const char *conffile = NULL;
 static unsigned long protection_period = ULONG_MAX;
 static int nsegments_per_clean = 2;
 static struct timespec cleaning_interval = { 0, 100000000 };   /* 100 msec */
+static unsigned long min_reclaimable_blocks = 20;  /* about 1% with 8MB segs */
 
 static sigjmp_buf nilfs_clean_env;
 static struct nilfs_cleaner *nilfs_cleaner;
@@ -164,9 +170,11 @@ static int nilfs_clean_do_run(struct nilfs_cleaner *cleaner)
 	args.nsegments_per_clean = nsegments_per_clean;
 	args.cleaning_interval = cleaning_interval.tv_sec;
 	args.cleaning_interval_nsec = cleaning_interval.tv_nsec;
+	args.min_reclaimable_blocks = min_reclaimable_blocks;
 	args.valid = (NILFS_CLEANER_ARG_NPASSES |
 		      NILFS_CLEANER_ARG_CLEANING_INTERVAL |
-		      NILFS_CLEANER_ARG_NSEGMENTS_PER_CLEAN);
+		      NILFS_CLEANER_ARG_NSEGMENTS_PER_CLEAN |
+		      NILFS_CLEANER_ARG_MIN_RECLAIMABLE_BLOCKS);
 
 	if (protection_period != ULONG_MAX) {
 		args.protection_period = protection_period;
@@ -426,6 +434,26 @@ failed_too_large:
 	return -1;
 }
 
+static int nilfs_clean_parse_min_reclaimable(const char *arg)
+{
+	unsigned long blocks;
+	char *endptr;
+
+	blocks = strtoul(arg, &endptr, 10);
+	if (endptr == arg || endptr[0] != '\0') {
+		myprintf(_("Error: invalid reclaimable blocks: %s\n"), arg);
+		return -1;
+	}
+
+	if (blocks == ULONG_MAX) {
+		myprintf(_("Error: value too large: %s\n"), arg);
+		return -1;
+	}
+
+	min_reclaimable_blocks = blocks;
+	return 0;
+}
+
 static void nilfs_clean_parse_options(int argc, char *argv[])
 {
 #ifdef _GNU_SOURCE
@@ -434,10 +462,10 @@ static void nilfs_clean_parse_options(int argc, char *argv[])
 	int c;
 
 #ifdef _GNU_SOURCE
-	while ((c = getopt_long(argc, argv, "bc::hlp:qrsS:vV",
+	while ((c = getopt_long(argc, argv, "bc::hlm:p:qrsS:vV",
 				long_option, &option_index)) >= 0) {
 #else
-	while ((c = getopt(argc, argv, "bc::hlp:qrsS:vV")) >= 0) {
+	while ((c = getopt(argc, argv, "bc::hlm:p:qrsS:vV")) >= 0) {
 #endif	/* _GNU_SOURCE */
 		switch (c) {
 		case 'b':
@@ -455,6 +483,10 @@ static void nilfs_clean_parse_options(int argc, char *argv[])
 		case 'l':
 			clean_cmd = NILFS_CLEAN_CMD_INFO;
 			break;
+		case 'm':
+			if (nilfs_clean_parse_min_reclaimable(optarg) < 0)
+				exit(EXIT_FAILURE);
+			break;
 		case 'p':
 			if (nilfs_clean_parse_protection_period(optarg) < 0)
 				exit(EXIT_FAILURE);
-- 
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 v3 3/4] nilfs-utils: add suport for NILFS_IOCTL_SET_SUINFO ioctl

[Andreas Rohner]

This patch adds library support for the NILFS_IOCTL_SET_SUINFO
ioctl. A new function nilfs_set_suinfo and the structure
nilfs_suinfo_update, which contains the update information, are
added.

With this function the segment usage information can be updated
from userspace, which can be used as a shortcut for certain GC
operations.

Signed-off-by: Andreas Rohner <andreas.rohner-hi6Y0CQ0nG0@public.gmane.org>
---
 include/nilfs.h     |  2 ++
 include/nilfs2_fs.h | 43 +++++++++++++++++++++++++++++++++++++++++++
 lib/nilfs.c         | 32 ++++++++++++++++++++++++++++++++
 3 files changed, 77 insertions(+)

diff --git a/include/nilfs.h b/include/nilfs.h
index b5f85d3..af4735d 100644
--- a/include/nilfs.h
+++ b/include/nilfs.h
@@ -304,6 +304,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);
diff --git a/include/nilfs2_fs.h b/include/nilfs2_fs.h
index e674f44..45de858 100644
--- a/include/nilfs2_fs.h
+++ b/include/nilfs2_fs.h
@@ -713,6 +713,47 @@ 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_reserved: reserved necessary for alignment
+ * @sup_sui: segment usage information
+ */
+struct nilfs_suinfo_update {
+	__u64 sup_segnum;
+	__u32 sup_flags;
+	__u32 sup_reserved;
+	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 +908,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 */
diff --git a/lib/nilfs.c b/lib/nilfs.c
index 93822de..25664e1 100644
--- a/lib/nilfs.c
+++ b/lib/nilfs.c
@@ -596,6 +596,38 @@ ssize_t nilfs_get_suinfo(const struct nilfs *nilfs, __u64 segnum,
 }
 
 /**
+ * nilfs_set_suinfo - sets segment usage info
+ * @nilfs: nilfs object
+ * @sup: an array of nilfs_suinfo_update structs
+ * @nsup: number of elements in sup
+ *
+ * Description: Takes an array of nilfs_suinfo_update structs and updates
+ * segment usage info accordingly. Only the fields indicated by sup_flags
+ * are updated.
+ *
+ * Return Value: On success, 0 is returned. On error, -1 is returned.
+ */
+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 0;
+}
+
+/**
  * nilfs_get_sustat - get segment usage statistics
  * @nilfs: nilfs object
  * @sustat: buffer of a nilfs_sustat struct to store statistics in
-- 
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 v3 4/4] nilfs-utils: add optimized version of nilfs_reclaim_segments

[Andreas Rohner]

This patch adds an optimized version of nilfs_reclaim_segments,
which has an additional parameter min_reclaimable. This parameter
specifies the minimum number of reclaimable blocks in a segment,
before it can be cleaned. If a segment is below this threshold, it
is considered to be not worth cleaning, because all the live blocks
would need to be moved to a new segment, which is expensive, and
the number of reclaimable blocks is too low. But it is still
necessary to update the segment usage information to turn the old
segment into a new one.

This is basically a shortcut to cleaning the segment. It is still
necessary to read the segment summary information, but the writing
of the live blocks can be skipped if it's not worth it.

This optimization can be enabled and disabled by the
use_set_suinfo switch in the configuration file. If the kernel
doesn't support the set_suinfo ioctl it returns the error
number ENOTTY, which also permanently disables the optimization.

Signed-off-by: Andreas Rohner <andreas.rohner-hi6Y0CQ0nG0@public.gmane.org>
---
 include/nilfs_gc.h       |   4 ++
 lib/gc.c                 | 112 +++++++++++++++++++++++++++++++++++++++--------
 sbin/cleanerd/cleanerd.c |   5 ++-
 3 files changed, 100 insertions(+), 21 deletions(-)

diff --git a/include/nilfs_gc.h b/include/nilfs_gc.h
index 72e9947..0ee797e 100644
--- a/include/nilfs_gc.h
+++ b/include/nilfs_gc.h
@@ -18,6 +18,10 @@ ssize_t nilfs_reclaim_segment(struct nilfs *nilfs,
 			      __u64 *segnums, size_t nsegs,
 			      __u64 protseq, nilfs_cno_t protcno);
 
+ssize_t nilfs_reclaim_segment_with_threshold(struct nilfs *nilfs,
+				__u64 *segnums, size_t nsegs, __u64 protseq,
+				nilfs_cno_t protcno,
+				unsigned long min_reclaimable);
 
 static inline int nilfs_suinfo_reclaimable(const struct nilfs_suinfo *si)
 {
diff --git a/lib/gc.c b/lib/gc.c
index 1152299..91ce47d 100644
--- a/lib/gc.c
+++ b/lib/gc.c
@@ -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>
@@ -601,20 +605,32 @@ static int nilfs_toss_bdescs(struct nilfs_vector *bdescv)
 }
 
 /**
- * nilfs_reclaim_segment - reclaim segments
+ * nilfs_reclaim_segment_with_threshold - reclaim segments with a threshold
  * @nilfs: nilfs object
  * @segnums: array of segment numbers storing selected segments
  * @nsegs: size of the @segnums array
  * @protseq: start of sequence number of protected segments
  * @protcno: start checkpoint number of protected period
+ * @min_reclaimable: minimal number of reclaimable blocks in a segment
+ *
+ * Description: Reclaim segments only if the number of reclaimable blocks
+ * is bigger than min_reclaimable. To reclaim all segments set
+ * min_reclaimable to 0.
+ *
+ * Return Value: On success, the number of reclaimed segments is returned.
+ * On error, the return value is < 0
  */
-ssize_t nilfs_reclaim_segment(struct nilfs *nilfs,
-			      __u64 *segnums, size_t nsegs,
-			      __u64 protseq, nilfs_cno_t protcno)
+ssize_t nilfs_reclaim_segment_with_threshold(struct nilfs *nilfs,
+				__u64 *segnums, size_t nsegs, __u64 protseq,
+				nilfs_cno_t protcno,
+				unsigned long min_reclaimable)
 {
 	struct nilfs_vector *vdescv, *bdescv, *periodv, *vblocknrv;
 	sigset_t sigset, oldset, waitset;
-	ssize_t n, ret = -1;
+	ssize_t n, i, ret = -1;
+	__u32 reclaimable_blocks;
+	struct nilfs_suinfo_update *supv;
+	struct timeval tv;
 
 	if (nsegs == 0)
 		return 0;
@@ -677,21 +693,63 @@ ssize_t nilfs_reclaim_segment(struct nilfs *nilfs,
 		goto out_lock;
 	}
 
-	ret = nilfs_clean_segments(nilfs,
-				   nilfs_vector_get_data(vdescv),
-				   nilfs_vector_get_size(vdescv),
-				   nilfs_vector_get_data(periodv),
-				   nilfs_vector_get_size(periodv),
-				   nilfs_vector_get_data(vblocknrv),
-				   nilfs_vector_get_size(vblocknrv),
-				   nilfs_vector_get_data(bdescv),
-				   nilfs_vector_get_size(bdescv),
-				   segnums, n);
-	if (ret < 0) {
-		nilfs_gc_logger(LOG_ERR, "cannot clean segments: %s",
-				strerror(errno));
+	reclaimable_blocks = (nilfs_get_blocks_per_segment(nilfs) * n)
+				- (nilfs_vector_get_size(vdescv)
+				+ nilfs_vector_get_size(bdescv));
+
+	/* if there are less reclaimable blocks than the
+	 * minimal threshold try to update suinfo
+	 * instead of cleaning */
+	if (nilfs_opt_test_set_suinfo(nilfs)
+			&& reclaimable_blocks < min_reclaimable * 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;
+		}
+
+		ret = nilfs_set_suinfo(nilfs, supv, n);
+		if (ret == 0) {
+			ret = n;
+		} else if (ret < 0 && errno == ENOTTY) {
+			nilfs_gc_logger(LOG_WARNING,
+					"set_suinfo ioctl is not supported");
+			nilfs_opt_clear_set_suinfo(nilfs);
+			ret = 0;
+		} else {
+			nilfs_gc_logger(LOG_ERR, "cannot set suinfo: %s",
+					strerror(errno));
+		}
+
+		free(supv);
 	} else {
-		ret = n;
+		ret = nilfs_clean_segments(nilfs,
+					   nilfs_vector_get_data(vdescv),
+					   nilfs_vector_get_size(vdescv),
+					   nilfs_vector_get_data(periodv),
+					   nilfs_vector_get_size(periodv),
+					   nilfs_vector_get_data(vblocknrv),
+					   nilfs_vector_get_size(vblocknrv),
+					   nilfs_vector_get_data(bdescv),
+					   nilfs_vector_get_size(bdescv),
+					   segnums, n);
+		if (ret < 0) {
+			nilfs_gc_logger(LOG_ERR, "cannot clean segments: %s",
+					strerror(errno));
+		} else {
+			ret = n;
+		}
 	}
 
 out_lock:
@@ -713,3 +771,19 @@ out_vec:
 
 	return ret;
 }
+
+/**
+ * nilfs_reclaim_segment - reclaim segments
+ * @nilfs: nilfs object
+ * @segnums: array of segment numbers storing selected segments
+ * @nsegs: size of the @segnums array
+ * @protseq: start of sequence number of protected segments
+ * @protcno: start checkpoint number of protected period
+ */
+ssize_t nilfs_reclaim_segment(struct nilfs *nilfs,
+			      __u64 *segnums, size_t nsegs,
+			      __u64 protseq, nilfs_cno_t protcno)
+{
+	return nilfs_reclaim_segment_with_threshold(nilfs, segnums, nsegs,
+			protseq, protcno, 0);
+}
diff --git a/sbin/cleanerd/cleanerd.c b/sbin/cleanerd/cleanerd.c
index 13018b2..b225283 100644
--- a/sbin/cleanerd/cleanerd.c
+++ b/sbin/cleanerd/cleanerd.c
@@ -1384,8 +1384,9 @@ static ssize_t nilfs_cleanerd_clean_segments(struct nilfs_cleanerd *cleanerd,
 		goto out;
 	}
 
-	ret = nilfs_reclaim_segment(cleanerd->nilfs, segnums, nsegs,
-				    protseq, protcno);
+	ret = nilfs_reclaim_segment_with_threshold(cleanerd->nilfs, segnums,
+			nsegs, protseq, protcno,
+			nilfs_cleanerd_min_reclaimable_blocks(cleanerd));
 	if (ret > 0) {
 		for (i = 0; i < ret; i++)
 			syslog(LOG_DEBUG, "segment %llu cleaned",
-- 
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 v3 1/3] nilfs2: add struct nilfs_suinfo_update and flags

[Andreas Rohner]

This patch adds the nilfs_suinfo_update structure, which contains the
information needed to update one segment usage entry. The flags
specify, which fields need to be updated.

Signed-off-by: Andreas Rohner <andreas.rohner-hi6Y0CQ0nG0@public.gmane.org>
---
 include/linux/nilfs2_fs.h | 41 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/include/linux/nilfs2_fs.h b/include/linux/nilfs2_fs.h
index 9875576..7b94449 100644
--- a/include/linux/nilfs2_fs.h
+++ b/include/linux/nilfs2_fs.h
@@ -709,6 +709,47 @@ 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_reserved: reserved necessary for alignment
+ * @sup_sui: segment usage information
+ */
+struct nilfs_suinfo_update {
+	__u64 sup_segnum;
+	__u32 sup_flags;
+	__u32 sup_reserved;
+	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,
-- 
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 v3 2/3] nilfs2: add nilfs_sufile_set_suinfo to update segment usage

[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 <andreas.rohner-hi6Y0CQ0nG0@public.gmane.org>
---
 fs/nilfs2/sufile.c | 129 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 fs/nilfs2/sufile.h |   1 +
 2 files changed, 130 insertions(+)

diff --git a/fs/nilfs2/sufile.c b/fs/nilfs2/sufile.c
index 3127e9f..8922523 100644
--- a/fs/nilfs2/sufile.c
+++ b/fs/nilfs2/sufile.c
@@ -870,6 +870,135 @@ ssize_t nilfs_sufile_get_suinfo(struct inode *sufile, __u64 segnum, void *buf,
 }
 
 /**
+ * nilfs_sufile_set_suinfo - sets segment usage info
+ * @sufile: inode of segment usage file
+ * @buf: array of suinfo_update
+ * @supsz: byte size of suinfo_update
+ * @nsup: size of suinfo_update array
+ *
+ * Description: Takes an array of nilfs_suinfo_update structs and updates
+ * segment usage accordingly. Only the fields indicated by the sup_flags
+ * are updated.
+ *
+ * Return Value: On success, 0 is returned. On error, one of the
+ * following negative error codes is returned.
+ *
+ * %-EIO - I/O error.
+ *
+ * %-ENOMEM - Insufficient amount of memory available.
+ *
+ * %-EINVAL - Invalid values in input (segment number, flags or nblocks)
+ */
+ssize_t nilfs_sufile_set_suinfo(struct inode *sufile, void *buf,
+				unsigned supsz, size_t nsup)
+{
+	struct the_nilfs *nilfs = sufile->i_sb->s_fs_info;
+	struct buffer_head *header_bh, *bh;
+	struct nilfs_suinfo_update *sup, *supend = buf + supsz * nsup;
+	struct nilfs_segment_usage *su;
+	void *kaddr;
+	unsigned long blkoff, prev_blkoff;
+	int ret = 0, ncleansegs, ndirtysegs, cleansi,
+			cleansu, dirtysi, dirtysu;
+
+	if (unlikely(nsup == 0))
+		return ret;
+
+	for (sup = buf; sup < supend; sup = (void *)sup + supsz) {
+		if (sup->sup_segnum >= nilfs->ns_nsegments
+			|| (sup->sup_flags &
+				(~0UL << (NILFS_SUINFO_UPDATE_FLAGS + 1)))
+			|| (nilfs_suinfo_update_nblocks(sup) &&
+				sup->sup_sui.sui_nblocks >
+				nilfs->ns_blocks_per_segment)
+			|| (nilfs_suinfo_update_flags(sup) &&
+				(sup->sup_sui.sui_flags &
+				(~0UL << (NILFS_SEGMENT_USAGE_ERROR + 1)))))
+			return -EINVAL;
+	}
+
+	down_write(&NILFS_MDT(sufile)->mi_sem);
+
+	ret = nilfs_sufile_get_header_block(sufile, &header_bh);
+	if (ret < 0)
+		goto out_sem;
+
+	sup = buf;
+	blkoff = nilfs_sufile_get_blkoff(sufile, sup->sup_segnum);
+	ret = nilfs_mdt_get_block(sufile, blkoff, 1, NULL, &bh);
+	if (ret < 0)
+		goto out_header;
+
+	for (;;) {
+		kaddr = kmap_atomic(bh->b_page);
+		su = nilfs_sufile_block_get_segment_usage(
+			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)) {
+			sup->sup_sui.sui_flags &=
+					~(1UL << NILFS_SEGMENT_USAGE_ACTIVE);
+
+			ncleansegs = 0;
+			ndirtysegs = 0;
+			cleansi = nilfs_suinfo_clean(&sup->sup_sui);
+			cleansu = nilfs_segment_usage_clean(su);
+			dirtysi = nilfs_suinfo_dirty(&sup->sup_sui);
+			dirtysu = nilfs_segment_usage_dirty(su);
+
+			if (cleansi && !cleansu)
+				++ncleansegs;
+			else if (!cleansi && cleansu)
+				--ncleansegs;
+
+			if (dirtysi && !dirtysu)
+				++ndirtysegs;
+			else if (!dirtysi && dirtysu)
+				--ndirtysegs;
+
+			su->su_flags = cpu_to_le32(sup->sup_sui.sui_flags);
+
+			nilfs_sufile_mod_counter(header_bh, ncleansegs,
+					ndirtysegs);
+			NILFS_SUI(sufile)->ncleansegs += ncleansegs;
+		}
+
+		kunmap_atomic(kaddr);
+
+		sup = (void *)sup + supsz;
+		if (sup >= supend)
+			break;
+
+		prev_blkoff = blkoff;
+		blkoff = nilfs_sufile_get_blkoff(sufile, sup->sup_segnum);
+		if (blkoff == prev_blkoff)
+			continue;
+
+		/* get different block */
+		mark_buffer_dirty(bh);
+		brelse(bh);
+		ret = nilfs_mdt_get_block(sufile, blkoff, 1, NULL, &bh);
+		if (unlikely(ret < 0))
+			goto out_mark;
+	}
+	mark_buffer_dirty(bh);
+	brelse(bh);
+
+ out_mark:
+	nilfs_mdt_mark_dirty(sufile);
+ out_header:
+	brelse(header_bh);
+ out_sem:
+	up_write(&NILFS_MDT(sufile)->mi_sem);
+	return ret;
+}
+
+/**
  * nilfs_sufile_read - read or get sufile inode
  * @sb: super block instance
  * @susize: size of a segment usage entry
diff --git a/fs/nilfs2/sufile.h b/fs/nilfs2/sufile.h
index e84bc5b..366003c 100644
--- a/fs/nilfs2/sufile.h
+++ b/fs/nilfs2/sufile.h
@@ -44,6 +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,
-- 
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 v3 3/3] nilfs2: implementation of NILFS_IOCTL_SET_SUINFO ioctl

[Andreas Rohner]

With this ioctl the segment usage entries 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
reclaimable space in it, the cleaning operation basically degrades to
a useless moving operation. In the end the only thing that changes is
the location of the data and a timestamp in the segment usage
information. With this ioctl the GC can skip the cleaning and update
the segment usage entries directly instead.

This is basically a shortcut to cleaning the segment. It is still
necessary to read the segment summary information, but the writing of
the live blocks can be skipped if it's not worth it.

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

diff --git a/fs/nilfs2/ioctl.c b/fs/nilfs2/ioctl.c
index b44bdb2..e97df9c 100644
--- a/fs/nilfs2/ioctl.c
+++ b/fs/nilfs2/ioctl.c
@@ -767,6 +767,64 @@ out:
 	return ret;
 }
 
+static int nilfs_ioctl_set_suinfo(struct inode *inode, struct file *filp,
+				unsigned int cmd, void __user *argp)
+{
+	struct the_nilfs *nilfs = inode->i_sb->s_fs_info;
+	struct nilfs_transaction_info ti;
+	struct nilfs_argv argv;
+	size_t len;
+	void __user *base;
+	void *kbuf;
+	int ret;
+
+	if (!capable(CAP_SYS_ADMIN))
+		return -EPERM;
+
+	ret = mnt_want_write_file(filp);
+	if (ret)
+		return ret;
+
+	ret = -EFAULT;
+	if (copy_from_user(&argv, argp, sizeof(argv)))
+		goto out;
+
+	ret = -EINVAL;
+	if (argv.v_size < sizeof(struct nilfs_suinfo_update))
+		goto out;
+
+	if (argv.v_nmembs > nilfs->ns_nsegments)
+		goto out;
+
+	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 = nilfs_sufile_set_suinfo(nilfs->ns_sufile, kbuf, argv.v_size,
+			argv.v_nmembs);
+	if (unlikely(ret < 0))
+		nilfs_transaction_abort(inode->i_sb);
+	else
+		nilfs_transaction_commit(inode->i_sb); /* never fails */
+
+out_free:
+	vfree(kbuf);
+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 +878,8 @@ 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_suinfo(inode, filp, cmd, argp);
 	case NILFS_IOCTL_GET_SUSTAT:
 		return nilfs_ioctl_get_sustat(inode, filp, cmd, argp);
 	case NILFS_IOCTL_GET_VINFO:
@@ -859,6 +919,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:
diff --git a/include/linux/nilfs2_fs.h b/include/linux/nilfs2_fs.h
index 7b94449..4140f7f 100644
--- a/include/linux/nilfs2_fs.h
+++ b/include/linux/nilfs2_fs.h
@@ -904,5 +904,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 */
-- 
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 v3 0/4] nilfs-utils: shortcut for certain GC operations

[Ryusuke Konishi]

Hi Andreas,
On Mon, 27 Jan 2014 10:58:52 +0100, Andreas Rohner wrote:
> Hi,
> 
> This is the third version of this patch set. It basically fixes a 
> lot of bugs.
> 
> v2->v3
> * Alignment in nilfs_suinfo_update
> * Add missing comments
> * nilfs2: Extra validity checks in nilfs_sufile_set_suinfo
> * nilfs2: Update sh_ncleansegs, sh_ndirtysegs if flags change
> * nilfs2: Fix bugs in nilfs_sufile_set_suinfo
> * nilfs2: Use vmalloc instead of ioctl_wrap_copy
> * nilfs-utils: Separate nilfs_relaim_segments_with_threshold
> * nilfs-utils: Allow different units for min_reclaimable_blocks
> * nilfs-utils: Introduce flag to disable the optimization
> * nilfs-utils: Disable optimization if kernel retruns ENOTTY
> * nilfs-utils: Remove EGCTRYAGAIN error return code
> * nilfs-utils: Rename option to min_reclaimable_blocks
> 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-file
> 
> This patch set adds an optimized version of 
> nilfs_reclaim_segments, which has an additional parameter 
> min_reclaimable. This parameter specifies the minimum number of 
> reclaimable blocks in a segment, before it can be cleaned. If a 
> segment is below this threshold, it is considered to be not worth 
> cleaning, because all the live blocks would need to be moved to a 
> new segment, which is expensive, and the number of reclaimable 
> blocks is too low. But it is still necessary to update the segment 
> usage information to turn the old segment into a new one.
>     
> This is basically a shortcut to cleaning the segment. It is still 
> necessary to read the segment summary information, but the writing 
> of the live blocks can be skipped if it's not worth it.
> 
> Additionally new options are introduced for the configuration file 
> and nilfs-clean to allow the user to specify the threshold. 
> 
> 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.
> 
> The benchmarks are currently running. I will give you the results
> shortly.

The kernel patchset didn't apply to the mainline as is because
Vyacheslav's ioctl commentary patch just has been merged.  Please
rebase your patchset on the current master branch next time.  Also,
nilfs_ioctl_set_suinfo() function needs summary comment as for the
current master branch.

Anyway, I will review the kernel patches first, and then look into
the userland patches.

Regards,
Ryusuke Konishi


> Regards,
> Andreas Rohner
> 
> ---
> Andreas Rohner (4):
>   nilfs-utils: cldconfig add an option to set min. reclaimable blocks
>   nilfs-utils: nilfs-clean add cmdline param min-reclaimable-blocks
>   nilfs-utils: add suport for NILFS_IOCTL_SET_SUINFO ioctl
>   nilfs-utils: add optimized version of nilfs_reclaim_segments
> 
>  include/nilfs.h                   |   9 +-
>  include/nilfs2_fs.h               |  43 ++++++++++
>  include/nilfs_cleaner.h           |  19 +++--
>  include/nilfs_gc.h                |   4 +
>  lib/gc.c                          | 112 ++++++++++++++++++++-----
>  lib/nilfs.c                       |  60 ++++++++++++++
>  man/nilfs-clean.8                 |   4 +
>  man/nilfs_cleanerd.conf.5         |  22 +++++
>  sbin/cleanerd/cldconfig.c         | 168 ++++++++++++++++++++++++++++----------
>  sbin/cleanerd/cldconfig.h         |   8 ++
>  sbin/cleanerd/cleanerd.c          |  39 ++++++++-
>  sbin/cleanerd/nilfs_cleanerd.conf |  13 +++
>  sbin/nilfs-clean/nilfs-clean.c    |  44 ++++++++--
>  13 files changed, 466 insertions(+), 79 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
--
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 v3 1/3] nilfs2: add struct nilfs_suinfo_update and flags

[Ryusuke Konishi]

On Mon, 27 Jan 2014 10:59:26 +0100, Andreas Rohner wrote:
> This patch adds the nilfs_suinfo_update structure, which contains the
> information needed to update one segment usage entry. The flags
> specify, which fields need to be updated.
> 
> Signed-off-by: Andreas Rohner <andreas.rohner-hi6Y0CQ0nG0@public.gmane.org>
> ---
>  include/linux/nilfs2_fs.h | 41 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 41 insertions(+)
> 
> diff --git a/include/linux/nilfs2_fs.h b/include/linux/nilfs2_fs.h
> index 9875576..7b94449 100644
> --- a/include/linux/nilfs2_fs.h
> +++ b/include/linux/nilfs2_fs.h
> @@ -709,6 +709,47 @@ 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_reserved: reserved necessary for alignment
> + * @sup_sui: segment usage information
> + */
> +struct nilfs_suinfo_update {
> +	__u64 sup_segnum;
> +	__u32 sup_flags;
> +	__u32 sup_reserved;
> +	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,
> -- 
> 1.8.5.3

This patch looks good to me.


Strictly speaking, these declarations should be inserted in the ioctl
section of nilfs2_fs.h which starts from the comment line /* ioctl */
instead of the disk format section.  But, the current constuction of
nilfs2_fs.h is already broken :(

The insert position you selected seems appropriate for now.

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 v3 0/4] nilfs-utils: shortcut for certain GC operations

[Andreas Rohner]

On 2014-01-27 16:03, Ryusuke Konishi wrote:
> Hi Andreas,
> On Mon, 27 Jan 2014 10:58:52 +0100, Andreas Rohner wrote:
>> Hi,
>>
>> This is the third version of this patch set. It basically fixes a 
>> lot of bugs.
>>
>> v2->v3
>> * Alignment in nilfs_suinfo_update
>> * Add missing comments
>> * nilfs2: Extra validity checks in nilfs_sufile_set_suinfo
>> * nilfs2: Update sh_ncleansegs, sh_ndirtysegs if flags change
>> * nilfs2: Fix bugs in nilfs_sufile_set_suinfo
>> * nilfs2: Use vmalloc instead of ioctl_wrap_copy
>> * nilfs-utils: Separate nilfs_relaim_segments_with_threshold
>> * nilfs-utils: Allow different units for min_reclaimable_blocks
>> * nilfs-utils: Introduce flag to disable the optimization
>> * nilfs-utils: Disable optimization if kernel retruns ENOTTY
>> * nilfs-utils: Remove EGCTRYAGAIN error return code
>> * nilfs-utils: Rename option to min_reclaimable_blocks
>> 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-file
>>
>> This patch set adds an optimized version of 
>> nilfs_reclaim_segments, which has an additional parameter 
>> min_reclaimable. This parameter specifies the minimum number of 
>> reclaimable blocks in a segment, before it can be cleaned. If a 
>> segment is below this threshold, it is considered to be not worth 
>> cleaning, because all the live blocks would need to be moved to a 
>> new segment, which is expensive, and the number of reclaimable 
>> blocks is too low. But it is still necessary to update the segment 
>> usage information to turn the old segment into a new one.
>>     
>> This is basically a shortcut to cleaning the segment. It is still 
>> necessary to read the segment summary information, but the writing 
>> of the live blocks can be skipped if it's not worth it.
>>
>> Additionally new options are introduced for the configuration file 
>> and nilfs-clean to allow the user to specify the threshold. 
>>
>> 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.
>>
>> The benchmarks are currently running. I will give you the results
>> shortly.
> 
> The kernel patchset didn't apply to the mainline as is because
> Vyacheslav's ioctl commentary patch just has been merged.  Please
> rebase your patchset on the current master branch next time.  Also,
> nilfs_ioctl_set_suinfo() function needs summary comment as for the
> current master branch.

Sorry for that, I based it on 3.13. I will rebase it immediateley and
add the summary comment.

> Anyway, I will review the kernel patches first, and then look into
> the userland patches.

Thanks. By the way, is it ok to use one cover letter for both the
userland and kernel patches?

br,
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 v3 3/3] nilfs2: implementation of NILFS_IOCTL_SET_SUINFO ioctl

[Ryusuke Konishi]

On Mon, 27 Jan 2014 10:59:28 +0100, Andreas Rohner wrote:
> With this ioctl the segment usage entries 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
> reclaimable space in it, the cleaning operation basically degrades to
> a useless moving operation. In the end the only thing that changes is
> the location of the data and a timestamp in the segment usage
> information. With this ioctl the GC can skip the cleaning and update
> the segment usage entries directly instead.
> 
> This is basically a shortcut to cleaning the segment. It is still
> necessary to read the segment summary information, but the writing of
> the live blocks can be skipped if it's not worth it.
> 
> Signed-off-by: Andreas Rohner <andreas.rohner-hi6Y0CQ0nG0@public.gmane.org>
> ---
>  fs/nilfs2/ioctl.c         | 61 +++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/nilfs2_fs.h |  2 ++
>  2 files changed, 63 insertions(+)
> 
> diff --git a/fs/nilfs2/ioctl.c b/fs/nilfs2/ioctl.c
> index b44bdb2..e97df9c 100644
> --- a/fs/nilfs2/ioctl.c
> +++ b/fs/nilfs2/ioctl.c
> @@ -767,6 +767,64 @@ out:
>  	return ret;
>  }
>  
> +static int nilfs_ioctl_set_suinfo(struct inode *inode, struct file *filp,
> +				unsigned int cmd, void __user *argp)
> +{
> +	struct the_nilfs *nilfs = inode->i_sb->s_fs_info;
> +	struct nilfs_transaction_info ti;
> +	struct nilfs_argv argv;
> +	size_t len;
> +	void __user *base;
> +	void *kbuf;
> +	int ret;
> +
> +	if (!capable(CAP_SYS_ADMIN))
> +		return -EPERM;
> +
> +	ret = mnt_want_write_file(filp);
> +	if (ret)
> +		return ret;
> +
> +	ret = -EFAULT;
> +	if (copy_from_user(&argv, argp, sizeof(argv)))
> +		goto out;
> +
> +	ret = -EINVAL;
> +	if (argv.v_size < sizeof(struct nilfs_suinfo_update))
> +		goto out;
> +
> +	if (argv.v_nmembs > nilfs->ns_nsegments)
> +		goto out;
> +
> +	len = argv.v_size * argv.v_nmembs;

The following check should be inserted to avoid overflow of variable
"len".

	if (argv.v_nmembs >= UINT_MAX / argv.v_size)
		goto out;

The size argument of vmalloc() is unsigned long, but sizeof(unsinged
long) differs depending on architecture, thus undesirable.

The following check also should be inserted because vmalloc(0) arises
a warning returning NULL.

	if (!len) {
		ret = 0;
		goto out;
	}


Regards,
Ryusuke Konishi

> +	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 = nilfs_sufile_set_suinfo(nilfs->ns_sufile, kbuf, argv.v_size,
> +			argv.v_nmembs);
> +	if (unlikely(ret < 0))
> +		nilfs_transaction_abort(inode->i_sb);
> +	else
> +		nilfs_transaction_commit(inode->i_sb); /* never fails */
> +
> +out_free:
> +	vfree(kbuf);
> +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 +878,8 @@ 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_suinfo(inode, filp, cmd, argp);
>  	case NILFS_IOCTL_GET_SUSTAT:
>  		return nilfs_ioctl_get_sustat(inode, filp, cmd, argp);
>  	case NILFS_IOCTL_GET_VINFO:
> @@ -859,6 +919,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:
> diff --git a/include/linux/nilfs2_fs.h b/include/linux/nilfs2_fs.h
> index 7b94449..4140f7f 100644
> --- a/include/linux/nilfs2_fs.h
> +++ b/include/linux/nilfs2_fs.h
> @@ -904,5 +904,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 */
> -- 
> 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 v3 0/4] nilfs-utils: shortcut for certain GC operations

[Andreas Rohner]

On 2014-01-27 10:58, Andreas Rohner wrote:
> The benchmarks are currently running. I will give you the results
> shortly.

Here are the promised results:

I used a 100GB nilfs2 volume on both a HDD and a SDD and the well known
fs_mark benchmark tool. The benchmark consisted of the following steps:

1. Write a 20GB file (static data)
2. fs_mark -d dir -L 135 -D 16 -t 16 -n 150 -s 131072 -S 1 -w 4096
3. Wait for the cleaner to reach max_clean_segments

The following key configuration options were used:

min_clean_segments        20%
max_clean_segments        22%
nsegments_per_clean       4
mc_nsegments_per_clean    4
cleaning_interval         0.5
mc_cleaning_interval      0.5
min_reclaimable_blocks    5%
mc_min_reclaimable_blocks 1%
use_set_suinfo

HDD:
    Timestamp GB Written: 140.2588
    Timestamp GB Read:    48.06372
    Timestamp Runtime:    4145.151s
    Timestamp Disk Util.: 94%

    Patched GB Written:   120.1527
    Patched GB Read:      28.28576
    Patched Runtime:      3692.105s
    Patched Disk Util.:   93%

SSD:
    Timestamp GB Written: 210.2145
    Timestamp GB Read:    48.79246
    Timestamp Runtime:    3883.966s
    Timestamp Disk Util.: 87%

    Patched GB Written:   168.6009
    Patched GB Read:      28.66516
    Patched Runtime:      3566.425s
    Patched Disk Util.:   90%


The disk utilization is measured after step 2, because after step 3 it
is always 78%.

The results for the HDD show, that the 20 GB of static data were moved
in the case of the normal timestamp policy and they were ignored in case
of the patched timestamp policy.

The results for the SSD were similar, but the difference in GBs written
is 40 GB instead of 20 GB, which is a bit strange.

The value of 1% for mc_min_reclaimable_blocks seems to be ideal, because
it is just enough, so that completely full segments fall below the
threshold and can be skipped.

br,
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 v3 0/4] nilfs-utils: shortcut for certain GC operations

[Ryusuke Konishi]

On Mon, 27 Jan 2014 16:47:32 +0100, Andreas Rohner wrote:
> On 2014-01-27 16:03, Ryusuke Konishi wrote:
>> Hi Andreas,
>> On Mon, 27 Jan 2014 10:58:52 +0100, Andreas Rohner wrote:
>>> Hi,
>>>
>>> This is the third version of this patch set. It basically fixes a 
>>> lot of bugs.
>>>
>>> v2->v3
>>> * Alignment in nilfs_suinfo_update
>>> * Add missing comments
>>> * nilfs2: Extra validity checks in nilfs_sufile_set_suinfo
>>> * nilfs2: Update sh_ncleansegs, sh_ndirtysegs if flags change
>>> * nilfs2: Fix bugs in nilfs_sufile_set_suinfo
>>> * nilfs2: Use vmalloc instead of ioctl_wrap_copy
>>> * nilfs-utils: Separate nilfs_relaim_segments_with_threshold
>>> * nilfs-utils: Allow different units for min_reclaimable_blocks
>>> * nilfs-utils: Introduce flag to disable the optimization
>>> * nilfs-utils: Disable optimization if kernel retruns ENOTTY
>>> * nilfs-utils: Remove EGCTRYAGAIN error return code
>>> * nilfs-utils: Rename option to min_reclaimable_blocks
>>> 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-file
>>>
>>> This patch set adds an optimized version of 
>>> nilfs_reclaim_segments, which has an additional parameter 
>>> min_reclaimable. This parameter specifies the minimum number of 
>>> reclaimable blocks in a segment, before it can be cleaned. If a 
>>> segment is below this threshold, it is considered to be not worth 
>>> cleaning, because all the live blocks would need to be moved to a 
>>> new segment, which is expensive, and the number of reclaimable 
>>> blocks is too low. But it is still necessary to update the segment 
>>> usage information to turn the old segment into a new one.
>>>     
>>> This is basically a shortcut to cleaning the segment. It is still 
>>> necessary to read the segment summary information, but the writing 
>>> of the live blocks can be skipped if it's not worth it.
>>>
>>> Additionally new options are introduced for the configuration file 
>>> and nilfs-clean to allow the user to specify the threshold. 
>>>
>>> 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.
>>>
>>> The benchmarks are currently running. I will give you the results
>>> shortly.
>> 
>> The kernel patchset didn't apply to the mainline as is because
>> Vyacheslav's ioctl commentary patch just has been merged.  Please
>> rebase your patchset on the current master branch next time.  Also,
>> nilfs_ioctl_set_suinfo() function needs summary comment as for the
>> current master branch.
> 
> Sorry for that, I based it on 3.13. I will rebase it immediateley and
> add the summary comment.

I already rebased it manually.  Please wait until I complete the
review for all patches.

>> Anyway, I will review the kernel patches first, and then look into
>> the userland patches.
> 
> Thanks. By the way, is it ok to use one cover letter for both the
> userland and kernel patches?

No problem.  In either case, I will add a cover letter when I send
them to upstream.

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 v3 2/3] nilfs2: add nilfs_sufile_set_suinfo to update segment usage

[Ryusuke Konishi]

On Mon, 27 Jan 2014 10:59:27 +0100, Andreas Rohner wrote:
> 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 <andreas.rohner-hi6Y0CQ0nG0@public.gmane.org>
> ---
>  fs/nilfs2/sufile.c | 129 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  fs/nilfs2/sufile.h |   1 +
>  2 files changed, 130 insertions(+)
> 
> diff --git a/fs/nilfs2/sufile.c b/fs/nilfs2/sufile.c
> index 3127e9f..8922523 100644
> --- a/fs/nilfs2/sufile.c
> +++ b/fs/nilfs2/sufile.c
> @@ -870,6 +870,135 @@ ssize_t nilfs_sufile_get_suinfo(struct inode *sufile, __u64 segnum, void *buf,
>  }
>  
>  /**
> + * nilfs_sufile_set_suinfo - sets segment usage info
> + * @sufile: inode of segment usage file
> + * @buf: array of suinfo_update
> + * @supsz: byte size of suinfo_update
> + * @nsup: size of suinfo_update array
> + *
> + * Description: Takes an array of nilfs_suinfo_update structs and updates
> + * segment usage accordingly. Only the fields indicated by the sup_flags
> + * are updated.
> + *
> + * Return Value: On success, 0 is returned. On error, one of the
> + * following negative error codes is returned.
> + *
> + * %-EIO - I/O error.
> + *
> + * %-ENOMEM - Insufficient amount of memory available.
> + *
> + * %-EINVAL - Invalid values in input (segment number, flags or nblocks)
> + */
> +ssize_t nilfs_sufile_set_suinfo(struct inode *sufile, void *buf,
> +				unsigned supsz, size_t nsup)
> +{
> +	struct the_nilfs *nilfs = sufile->i_sb->s_fs_info;
> +	struct buffer_head *header_bh, *bh;
> +	struct nilfs_suinfo_update *sup, *supend = buf + supsz * nsup;
> +	struct nilfs_segment_usage *su;
> +	void *kaddr;
> +	unsigned long blkoff, prev_blkoff;
> +	int ret = 0, ncleansegs, ndirtysegs, cleansi,
> +			cleansu, dirtysi, dirtysu;

This indentation looks peculiar. Why not separate them into two or
more lines?  At least, ncleansegs and ndirtysegs differ from cleansi,
cleansu, dirtysi, dirtysu and ret.  ncleansegs and ndirtysegs sounds
confusing since these local variables do not give a total number.
ncleaned and ndirtied would be better.

	int cleansi, cleansu, dirtysi, dirtysu;
	int ncleaned, ndirtied;
	int ret = 0;

> +
> +	if (unlikely(nsup == 0))
> +		return ret;
> +
> +	for (sup = buf; sup < supend; sup = (void *)sup + supsz) {
> +		if (sup->sup_segnum >= nilfs->ns_nsegments
> +			|| (sup->sup_flags &
> +				(~0UL << (NILFS_SUINFO_UPDATE_FLAGS + 1)))

This looks confusing. It should be clarified as follows:

enum {
        NILFS_SUINFO_UPDATE_LASTMOD,
        NILFS_SUINFO_UPDATE_NBLOCKS,
        NILFS_SUINFO_UPDATE_FLAGS,
	__NR_NILFS_SUINFO_FIELDS
};

		if (sup->sup_segnum >= nilfs->ns_nsegments ||
		    (sup->sup_flags & (~0UL << __NR_NILFS_SUINFO_FIELDS)) ||

> +			|| (nilfs_suinfo_update_nblocks(sup) &&
> +				sup->sup_sui.sui_nblocks >
> +				nilfs->ns_blocks_per_segment)
> +			|| (nilfs_suinfo_update_flags(sup) &&
> +				(sup->sup_sui.sui_flags &
> +				(~0UL << (NILFS_SEGMENT_USAGE_ERROR + 1)))))

Ditto. We need to add a definition to nilfs2_fs.h.

enum {
        NILFS_SEGMENT_USAGE_ACTIVE,
        NILFS_SEGMENT_USAGE_DIRTY,
        NILFS_SEGMENT_USAGE_ERROR,
	__NR_NILFS_SEGMENT_USAGE_FLAGS
};

		    (nilfs_suinfo_update_flags(sup) &&
		     (sup->sup_sui.sui_flags &
		      (~0UL << __NR_NILFS_SEGMENT_USAGE_FLAGS))))

By the way, this will dismiss the capability that userland cleaner
programs uses the rest of su_flags for their own purpose such as GC
optimization.  I think this (rejecting or utilizing it) should be
carefully determined.

Any comments on this?

> +	}
> +
> +	down_write(&NILFS_MDT(sufile)->mi_sem);
> +
> +	ret = nilfs_sufile_get_header_block(sufile, &header_bh);
> +	if (ret < 0)
> +		goto out_sem;
> +
> +	sup = buf;
> +	blkoff = nilfs_sufile_get_blkoff(sufile, sup->sup_segnum);
> +	ret = nilfs_mdt_get_block(sufile, blkoff, 1, NULL, &bh);
> +	if (ret < 0)
> +		goto out_header;
> +
> +	for (;;) {
> +		kaddr = kmap_atomic(bh->b_page);
> +		su = nilfs_sufile_block_get_segment_usage(
> +			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)) {
> +			sup->sup_sui.sui_flags &=
> +					~(1UL << NILFS_SEGMENT_USAGE_ACTIVE);

Your would be better off adding a comment to explain what's this.

  			/*
			 * Active flag is a virtual flag projected by running
			 * nilfs kernel code - drop it not to write it to
			 * disk.
			 */
> +
> +			ncleansegs = 0;
> +			ndirtysegs = 0;
> +			cleansi = nilfs_suinfo_clean(&sup->sup_sui);
> +			cleansu = nilfs_segment_usage_clean(su);
> +			dirtysi = nilfs_suinfo_dirty(&sup->sup_sui);
> +			dirtysu = nilfs_segment_usage_dirty(su);
> +
> +			if (cleansi && !cleansu)
> +				++ncleansegs;
> +			else if (!cleansi && cleansu)
> +				--ncleansegs;
> +
> +			if (dirtysi && !dirtysu)
> +				++ndirtysegs;
> +			else if (!dirtysi && dirtysu)
> +				--ndirtysegs;
> +
> +			su->su_flags = cpu_to_le32(sup->sup_sui.sui_flags);
> +
> +			nilfs_sufile_mod_counter(header_bh, ncleansegs,
> +					ndirtysegs);

Does it work for a negative value without cast of (u64) ?
Please confirm that these counters are updated as you expected.

> +			NILFS_SUI(sufile)->ncleansegs += ncleansegs;

Ditto.  


Regards,
Ryusuke Konishi

> +		}
> +
> +		kunmap_atomic(kaddr);
> +
> +		sup = (void *)sup + supsz;
> +		if (sup >= supend)
> +			break;
> +
> +		prev_blkoff = blkoff;
> +		blkoff = nilfs_sufile_get_blkoff(sufile, sup->sup_segnum);
> +		if (blkoff == prev_blkoff)
> +			continue;
> +
> +		/* get different block */
> +		mark_buffer_dirty(bh);
> +		brelse(bh);
> +		ret = nilfs_mdt_get_block(sufile, blkoff, 1, NULL, &bh);
> +		if (unlikely(ret < 0))
> +			goto out_mark;
> +	}
> +	mark_buffer_dirty(bh);
> +	brelse(bh);
> +
> + out_mark:
> +	nilfs_mdt_mark_dirty(sufile);
> + out_header:
> +	brelse(header_bh);
> + out_sem:
> +	up_write(&NILFS_MDT(sufile)->mi_sem);
> +	return ret;
> +}
> +
> +/**
>   * nilfs_sufile_read - read or get sufile inode
>   * @sb: super block instance
>   * @susize: size of a segment usage entry
> diff --git a/fs/nilfs2/sufile.h b/fs/nilfs2/sufile.h
> index e84bc5b..366003c 100644
> --- a/fs/nilfs2/sufile.h
> +++ b/fs/nilfs2/sufile.h
> @@ -44,6 +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,
> -- 
> 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 v3 2/3] nilfs2: add nilfs_sufile_set_suinfo to update segment usage

[Andreas Rohner]

On 2014-01-27 20:07, Ryusuke Konishi wrote:
> On Mon, 27 Jan 2014 10:59:27 +0100, Andreas Rohner wrote:
>> 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 <andreas.rohner-hi6Y0CQ0nG0@public.gmane.org>
>> ---
>>  fs/nilfs2/sufile.c | 129 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  fs/nilfs2/sufile.h |   1 +
>>  2 files changed, 130 insertions(+)
>>
>> diff --git a/fs/nilfs2/sufile.c b/fs/nilfs2/sufile.c
>> index 3127e9f..8922523 100644
>> --- a/fs/nilfs2/sufile.c
>> +++ b/fs/nilfs2/sufile.c
>> @@ -870,6 +870,135 @@ ssize_t nilfs_sufile_get_suinfo(struct inode *sufile, __u64 segnum, void *buf,
>>  }
>>  
>>  /**
>> + * nilfs_sufile_set_suinfo - sets segment usage info
>> + * @sufile: inode of segment usage file
>> + * @buf: array of suinfo_update
>> + * @supsz: byte size of suinfo_update
>> + * @nsup: size of suinfo_update array
>> + *
>> + * Description: Takes an array of nilfs_suinfo_update structs and updates
>> + * segment usage accordingly. Only the fields indicated by the sup_flags
>> + * are updated.
>> + *
>> + * Return Value: On success, 0 is returned. On error, one of the
>> + * following negative error codes is returned.
>> + *
>> + * %-EIO - I/O error.
>> + *
>> + * %-ENOMEM - Insufficient amount of memory available.
>> + *
>> + * %-EINVAL - Invalid values in input (segment number, flags or nblocks)
>> + */
>> +ssize_t nilfs_sufile_set_suinfo(struct inode *sufile, void *buf,
>> +				unsigned supsz, size_t nsup)
>> +{
>> +	struct the_nilfs *nilfs = sufile->i_sb->s_fs_info;
>> +	struct buffer_head *header_bh, *bh;
>> +	struct nilfs_suinfo_update *sup, *supend = buf + supsz * nsup;
>> +	struct nilfs_segment_usage *su;
>> +	void *kaddr;
>> +	unsigned long blkoff, prev_blkoff;
>> +	int ret = 0, ncleansegs, ndirtysegs, cleansi,
>> +			cleansu, dirtysi, dirtysu;
> 
> This indentation looks peculiar. Why not separate them into two or
> more lines?  At least, ncleansegs and ndirtysegs differ from cleansi,
> cleansu, dirtysi, dirtysu and ret.  ncleansegs and ndirtysegs sounds
> confusing since these local variables do not give a total number.
> ncleaned and ndirtied would be better.
> 
> 	int cleansi, cleansu, dirtysi, dirtysu;
> 	int ncleaned, ndirtied;
> 	int ret = 0;
> 
>> +
>> +	if (unlikely(nsup == 0))
>> +		return ret;
>> +
>> +	for (sup = buf; sup < supend; sup = (void *)sup + supsz) {
>> +		if (sup->sup_segnum >= nilfs->ns_nsegments
>> +			|| (sup->sup_flags &
>> +				(~0UL << (NILFS_SUINFO_UPDATE_FLAGS + 1)))
> 
> This looks confusing. It should be clarified as follows:
> 
> enum {
>         NILFS_SUINFO_UPDATE_LASTMOD,
>         NILFS_SUINFO_UPDATE_NBLOCKS,
>         NILFS_SUINFO_UPDATE_FLAGS,
> 	__NR_NILFS_SUINFO_FIELDS
> };
> 
> 		if (sup->sup_segnum >= nilfs->ns_nsegments ||
> 		    (sup->sup_flags & (~0UL << __NR_NILFS_SUINFO_FIELDS)) ||
> 
>> +			|| (nilfs_suinfo_update_nblocks(sup) &&
>> +				sup->sup_sui.sui_nblocks >
>> +				nilfs->ns_blocks_per_segment)
>> +			|| (nilfs_suinfo_update_flags(sup) &&
>> +				(sup->sup_sui.sui_flags &
>> +				(~0UL << (NILFS_SEGMENT_USAGE_ERROR + 1)))))
> 
> Ditto. We need to add a definition to nilfs2_fs.h.
> 
> enum {
>         NILFS_SEGMENT_USAGE_ACTIVE,
>         NILFS_SEGMENT_USAGE_DIRTY,
>         NILFS_SEGMENT_USAGE_ERROR,
> 	__NR_NILFS_SEGMENT_USAGE_FLAGS
> };
> 
> 		    (nilfs_suinfo_update_flags(sup) &&
> 		     (sup->sup_sui.sui_flags &
> 		      (~0UL << __NR_NILFS_SEGMENT_USAGE_FLAGS))))
> 
> By the way, this will dismiss the capability that userland cleaner
> programs uses the rest of su_flags for their own purpose such as GC
> optimization.  I think this (rejecting or utilizing it) should be
> carefully determined.
> 
> Any comments on this?

Hmm, I think it can't hurt to let the userland cleaner use the su_flags.
As far as I can tell, it shouldn't affect the kernel side.
nilfs_segment_usage_set_clean() would still work and
nilfs_sufile_do_scrap() overwrites the whole su_flags as well.

>> +	}
>> +
>> +	down_write(&NILFS_MDT(sufile)->mi_sem);
>> +
>> +	ret = nilfs_sufile_get_header_block(sufile, &header_bh);
>> +	if (ret < 0)
>> +		goto out_sem;
>> +
>> +	sup = buf;
>> +	blkoff = nilfs_sufile_get_blkoff(sufile, sup->sup_segnum);
>> +	ret = nilfs_mdt_get_block(sufile, blkoff, 1, NULL, &bh);
>> +	if (ret < 0)
>> +		goto out_header;
>> +
>> +	for (;;) {
>> +		kaddr = kmap_atomic(bh->b_page);
>> +		su = nilfs_sufile_block_get_segment_usage(
>> +			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)) {
>> +			sup->sup_sui.sui_flags &=
>> +					~(1UL << NILFS_SEGMENT_USAGE_ACTIVE);
> 
> Your would be better off adding a comment to explain what's this.
> 
>   			/*
> 			 * Active flag is a virtual flag projected by running
> 			 * nilfs kernel code - drop it not to write it to
> 			 * disk.
> 			 */
>> +
>> +			ncleansegs = 0;
>> +			ndirtysegs = 0;
>> +			cleansi = nilfs_suinfo_clean(&sup->sup_sui);
>> +			cleansu = nilfs_segment_usage_clean(su);
>> +			dirtysi = nilfs_suinfo_dirty(&sup->sup_sui);
>> +			dirtysu = nilfs_segment_usage_dirty(su);
>> +
>> +			if (cleansi && !cleansu)
>> +				++ncleansegs;
>> +			else if (!cleansi && cleansu)
>> +				--ncleansegs;
>> +
>> +			if (dirtysi && !dirtysu)
>> +				++ndirtysegs;
>> +			else if (!dirtysi && dirtysu)
>> +				--ndirtysegs;
>> +
>> +			su->su_flags = cpu_to_le32(sup->sup_sui.sui_flags);
>> +
>> +			nilfs_sufile_mod_counter(header_bh, ncleansegs,
>> +					ndirtysegs);
> 
> Does it work for a negative value without cast of (u64) ?
> Please confirm that these counters are updated as you expected.
> 
>> +			NILFS_SUI(sufile)->ncleansegs += ncleansegs;
> 
> Ditto.  

I have tested it and it works. At least on my 64 bit architecture. It is
probably still a good idea to do an explicit cast.

How about I use s64 for ncleaned and ndirtied and move
nilfs_sufile_mod_counter outside the loop?

	s64 ncleaned = 0, ndirtied = 0;

	...

	for (;;) {
		...
	}
	mark_buffer_dirty(bh);
	brelse(bh);

 out_mark:
	if (ncleaned || ndirtied) {
		nilfs_sufile_mod_counter(header_bh, (u64)ncleaned,
				(u64)ndirtied);
		NILFS_SUI(sufile)->ncleansegs += ncleaned;
	}
	nilfs_mdt_mark_dirty(sufile);
 out_header:
	brelse(header_bh);
 out_sem:
	up_write(&NILFS_MDT(sufile)->mi_sem);
	return ret;

Best regards,
Andreas Rohner
> 
> Regards,
> Ryusuke Konishi
> 
>> +		}
>> +
>> +		kunmap_atomic(kaddr);
>> +
>> +		sup = (void *)sup + supsz;
>> +		if (sup >= supend)
>> +			break;
>> +
>> +		prev_blkoff = blkoff;
>> +		blkoff = nilfs_sufile_get_blkoff(sufile, sup->sup_segnum);
>> +		if (blkoff == prev_blkoff)
>> +			continue;
>> +
>> +		/* get different block */
>> +		mark_buffer_dirty(bh);
>> +		brelse(bh);
>> +		ret = nilfs_mdt_get_block(sufile, blkoff, 1, NULL, &bh);
>> +		if (unlikely(ret < 0))
>> +			goto out_mark;
>> +	}
>> +	mark_buffer_dirty(bh);
>> +	brelse(bh);
>> +
>> + out_mark:
>> +	nilfs_mdt_mark_dirty(sufile);
>> + out_header:
>> +	brelse(header_bh);
>> + out_sem:
>> +	up_write(&NILFS_MDT(sufile)->mi_sem);
>> +	return ret;
>> +}
>> +
>> +/**
>>   * nilfs_sufile_read - read or get sufile inode
>>   * @sb: super block instance
>>   * @susize: size of a segment usage entry
>> diff --git a/fs/nilfs2/sufile.h b/fs/nilfs2/sufile.h
>> index e84bc5b..366003c 100644
>> --- a/fs/nilfs2/sufile.h
>> +++ b/fs/nilfs2/sufile.h
>> @@ -44,6 +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,
>> -- 
>> 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 v3 2/3] nilfs2: add nilfs_sufile_set_suinfo to update segment usage

[Ryusuke Konishi]

On Tue, 28 Jan 2014 00:42:35 +0100, Andreas Rohner wrote:
> On 2014-01-27 20:07, Ryusuke Konishi wrote:
>> On Mon, 27 Jan 2014 10:59:27 +0100, Andreas Rohner wrote:
>>> +			|| (nilfs_suinfo_update_flags(sup) &&
>>> +				(sup->sup_sui.sui_flags &
>>> +				(~0UL << (NILFS_SEGMENT_USAGE_ERROR + 1)))))
>> 
>> Ditto. We need to add a definition to nilfs2_fs.h.
>> 
>> enum {
>>         NILFS_SEGMENT_USAGE_ACTIVE,
>>         NILFS_SEGMENT_USAGE_DIRTY,
>>         NILFS_SEGMENT_USAGE_ERROR,
>> 	__NR_NILFS_SEGMENT_USAGE_FLAGS
>> };
>> 
>> 		    (nilfs_suinfo_update_flags(sup) &&
>> 		     (sup->sup_sui.sui_flags &
>> 		      (~0UL << __NR_NILFS_SEGMENT_USAGE_FLAGS))))
>> 
>> By the way, this will dismiss the capability that userland cleaner
>> programs uses the rest of su_flags for their own purpose such as GC
>> optimization.  I think this (rejecting or utilizing it) should be
>> carefully determined.
>> 
>> Any comments on this?
> 
> Hmm, I think it can't hurt to let the userland cleaner use the su_flags.
> As far as I can tell, it shouldn't affect the kernel side.
> nilfs_segment_usage_set_clean() would still work and
> nilfs_sufile_do_scrap() overwrites the whole su_flags as well.

Well, actually the current definition of
nilfs_segment_usage_set_clean() and also nilfs_segment_usage_clean()
are written without compatibility consideration.

It looks to be a separate change if we allow to use the upper bits.
In that case, a bunch of changes and a new feature_compat_ro flag to
deal it as a disk format change, would be needed.

Ok, let's take the above one which protects the upper bits for now.

>>> +			nilfs_sufile_mod_counter(header_bh, ncleansegs,
>>> +					ndirtysegs);
>> 
>> Does it work for a negative value without cast of (u64) ?
>> Please confirm that these counters are updated as you expected.
>> 
>>> +			NILFS_SUI(sufile)->ncleansegs += ncleansegs;
>> 
>> Ditto.  
> 
> I have tested it and it works. At least on my 64 bit architecture. It is
> probably still a good idea to do an explicit cast.
> 
> How about I use s64 for ncleaned and ndirtied and move
> nilfs_sufile_mod_counter outside the loop?

Yes, this one looks better.  In that case, the u64 cast seems
unnecessary.

> 	s64 ncleaned = 0, ndirtied = 0;
> 
> 	...
> 
> 	for (;;) {
> 		...
> 	}
> 	mark_buffer_dirty(bh);
> 	brelse(bh);
> 
>  out_mark:
> 	if (ncleaned || ndirtied) {
> 		nilfs_sufile_mod_counter(header_bh, (u64)ncleaned,
> 				(u64)ndirtied);

> 		NILFS_SUI(sufile)->ncleansegs += ncleaned;

This one looks unclear.

How about defining ncleaned and ndirtied with unsigned long type and
cast them to (u64) for the arguments of nilfs_sufile_mod_counter() ?

Regards,
Ryusuke Konishi


> 	}
> 	nilfs_mdt_mark_dirty(sufile);
>  out_header:
> 	brelse(header_bh);
>  out_sem:
> 	up_write(&NILFS_MDT(sufile)->mi_sem);
> 	return ret;
> 
--
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 v3 2/3] nilfs2: add nilfs_sufile_set_suinfo to update segment usage

[Andreas Rohner]

On 2014-01-28 02:03, Ryusuke Konishi wrote:
> On Tue, 28 Jan 2014 00:42:35 +0100, Andreas Rohner wrote:
>> On 2014-01-27 20:07, Ryusuke Konishi wrote:
>>> On Mon, 27 Jan 2014 10:59:27 +0100, Andreas Rohner wrote:
>>>> +			|| (nilfs_suinfo_update_flags(sup) &&
>>>> +				(sup->sup_sui.sui_flags &
>>>> +				(~0UL << (NILFS_SEGMENT_USAGE_ERROR + 1)))))
>>>
>>> Ditto. We need to add a definition to nilfs2_fs.h.
>>>
>>> enum {
>>>         NILFS_SEGMENT_USAGE_ACTIVE,
>>>         NILFS_SEGMENT_USAGE_DIRTY,
>>>         NILFS_SEGMENT_USAGE_ERROR,
>>> 	__NR_NILFS_SEGMENT_USAGE_FLAGS
>>> };
>>>
>>> 		    (nilfs_suinfo_update_flags(sup) &&
>>> 		     (sup->sup_sui.sui_flags &
>>> 		      (~0UL << __NR_NILFS_SEGMENT_USAGE_FLAGS))))
>>>
>>> By the way, this will dismiss the capability that userland cleaner
>>> programs uses the rest of su_flags for their own purpose such as GC
>>> optimization.  I think this (rejecting or utilizing it) should be
>>> carefully determined.
>>>
>>> Any comments on this?
>>
>> Hmm, I think it can't hurt to let the userland cleaner use the su_flags.
>> As far as I can tell, it shouldn't affect the kernel side.
>> nilfs_segment_usage_set_clean() would still work and
>> nilfs_sufile_do_scrap() overwrites the whole su_flags as well.
> 
> Well, actually the current definition of
> nilfs_segment_usage_set_clean() and also nilfs_segment_usage_clean()
> are written without compatibility consideration.

I actually thought it would be a good idea to wipe the custom flags if a
segment is cleaned, which the current implementation does. So the custom
flags are only valid for dirty segments and a segment is only considered
to be clean with nilfs_segment_usage_clean if there are no custom flags.
I don't think that would be unreasonable, because the GC has no use for
flags on clean segments anyway.

> It looks to be a separate change if we allow to use the upper bits.
> In that case, a bunch of changes and a new feature_compat_ro flag to
> deal it as a disk format change, would be needed.

I think we would only have to define, which flags are reserved for
future use and which are available for the userspace GC. Everything else
would just work.

> Ok, let's take the above one which protects the upper bits for now.

Ok. It is certainly cleaner that way.

>>>> +			nilfs_sufile_mod_counter(header_bh, ncleansegs,
>>>> +					ndirtysegs);
>>>
>>> Does it work for a negative value without cast of (u64) ?
>>> Please confirm that these counters are updated as you expected.
>>>
>>>> +			NILFS_SUI(sufile)->ncleansegs += ncleansegs;
>>>
>>> Ditto.  
>>
>> I have tested it and it works. At least on my 64 bit architecture. It is
>> probably still a good idea to do an explicit cast.
>>
>> How about I use s64 for ncleaned and ndirtied and move
>> nilfs_sufile_mod_counter outside the loop?
> 
> Yes, this one looks better.  In that case, the u64 cast seems
> unnecessary.
> 
>> 	s64 ncleaned = 0, ndirtied = 0;
>>
>> 	...
>>
>> 	for (;;) {
>> 		...
>> 	}
>> 	mark_buffer_dirty(bh);
>> 	brelse(bh);
>>
>>  out_mark:
>> 	if (ncleaned || ndirtied) {
>> 		nilfs_sufile_mod_counter(header_bh, (u64)ncleaned,
>> 				(u64)ndirtied);
> 
>> 		NILFS_SUI(sufile)->ncleansegs += ncleaned;
> 
> This one looks unclear.
> 
> How about defining ncleaned and ndirtied with unsigned long type and
> cast them to (u64) for the arguments of nilfs_sufile_mod_counter() ?

Ok I agree ncleansegs could be 32 bit on 32 bit systems. But why
"unsigned long" and not just "long"? It seems a bit strange to use an
unsigned type for possible negative values and I don't see the problem
of adding a negative number to an unsigned type of the same size.

Additionally if we use "unsigned long" wouldn't a typecast to (u64)
result in a number like 4294967295 rather than 18446744073709551615,
which is equivalent to -1, on a 32 bit system?

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 v3 2/3] nilfs2: add nilfs_sufile_set_suinfo to update segment usage

[Ryusuke Konishi]

On Tue, 28 Jan 2014 05:26:05 +0100, Andreas Rohner wrote:
> On 2014-01-28 02:03, Ryusuke Konishi wrote:
>> On Tue, 28 Jan 2014 00:42:35 +0100, Andreas Rohner wrote:
>>> On 2014-01-27 20:07, Ryusuke Konishi wrote:
>>>> On Mon, 27 Jan 2014 10:59:27 +0100, Andreas Rohner wrote:
>>>>> +			|| (nilfs_suinfo_update_flags(sup) &&
>>>>> +				(sup->sup_sui.sui_flags &
>>>>> +				(~0UL << (NILFS_SEGMENT_USAGE_ERROR + 1)))))
>>>>
>>>> Ditto. We need to add a definition to nilfs2_fs.h.
>>>>
>>>> enum {
>>>>         NILFS_SEGMENT_USAGE_ACTIVE,
>>>>         NILFS_SEGMENT_USAGE_DIRTY,
>>>>         NILFS_SEGMENT_USAGE_ERROR,
>>>> 	__NR_NILFS_SEGMENT_USAGE_FLAGS
>>>> };
>>>>
>>>> 		    (nilfs_suinfo_update_flags(sup) &&
>>>> 		     (sup->sup_sui.sui_flags &
>>>> 		      (~0UL << __NR_NILFS_SEGMENT_USAGE_FLAGS))))
>>>>
>>>> By the way, this will dismiss the capability that userland cleaner
>>>> programs uses the rest of su_flags for their own purpose such as GC
>>>> optimization.  I think this (rejecting or utilizing it) should be
>>>> carefully determined.
>>>>
>>>> Any comments on this?
>>>
>>> Hmm, I think it can't hurt to let the userland cleaner use the su_flags.
>>> As far as I can tell, it shouldn't affect the kernel side.
>>> nilfs_segment_usage_set_clean() would still work and
>>> nilfs_sufile_do_scrap() overwrites the whole su_flags as well.
>> 
>> Well, actually the current definition of
>> nilfs_segment_usage_set_clean() and also nilfs_segment_usage_clean()
>> are written without compatibility consideration.
> 
> I actually thought it would be a good idea to wipe the custom flags if a
> segment is cleaned, which the current implementation does. So the custom
> flags are only valid for dirty segments and a segment is only considered
> to be clean with nilfs_segment_usage_clean if there are no custom flags.
> I don't think that would be unreasonable, because the GC has no use for
> flags on clean segments anyway.

I looked over functions manipulating su_flags again, and came to the
same conclusion.  We can keep consistency even if userland gc programs
utilize the remaining flags for their own purpose.  There are no
compatibility issues at least if we manipulate su_flags of dirty
segments.  In this regard, the current implementation, which wipes the
custom flags when it cleans segment, is not bad, yes.

>> It looks to be a separate change if we allow to use the upper bits.
>> In that case, a bunch of changes and a new feature_compat_ro flag to
>> deal it as a disk format change, would be needed.
> 
> I think we would only have to define, which flags are reserved for
> future use and which are available for the userspace GC. Everything else
> would just work.

Right.  My above comment is completely wrong.  We don't have to add a
new feature-compat flag to use custom flags unless we want to use them
for free segments (in this case, we need to change definition of
nilfs_segment_usage_clean() and nilfs_segment_usage_set_clean())

>> Ok, let's take the above one which protects the upper bits for now.
> 
> Ok. It is certainly cleaner that way.

Let me recant my comment.  Let's change the patch to allow custom
flags.

> 
>>>>> +			nilfs_sufile_mod_counter(header_bh, ncleansegs,
>>>>> +					ndirtysegs);
>>>>
>>>> Does it work for a negative value without cast of (u64) ?
>>>> Please confirm that these counters are updated as you expected.
>>>>
>>>>> +			NILFS_SUI(sufile)->ncleansegs += ncleansegs;
>>>>
>>>> Ditto.  
>>>
>>> I have tested it and it works. At least on my 64 bit architecture. It is
>>> probably still a good idea to do an explicit cast.
>>>
>>> How about I use s64 for ncleaned and ndirtied and move
>>> nilfs_sufile_mod_counter outside the loop?
>> 
>> Yes, this one looks better.  In that case, the u64 cast seems
>> unnecessary.
>> 
>>> 	s64 ncleaned = 0, ndirtied = 0;
>>>
>>> 	...
>>>
>>> 	for (;;) {
>>> 		...
>>> 	}
>>> 	mark_buffer_dirty(bh);
>>> 	brelse(bh);
>>>
>>>  out_mark:
>>> 	if (ncleaned || ndirtied) {
>>> 		nilfs_sufile_mod_counter(header_bh, (u64)ncleaned,
>>> 				(u64)ndirtied);
>> 
>>> 		NILFS_SUI(sufile)->ncleansegs += ncleaned;
>> 
>> This one looks unclear.
>> 
>> How about defining ncleaned and ndirtied with unsigned long type and
>> cast them to (u64) for the arguments of nilfs_sufile_mod_counter() ?
> 
> Ok I agree ncleansegs could be 32 bit on 32 bit systems. But why
> "unsigned long" and not just "long"? It seems a bit strange to use an
> unsigned type for possible negative values and I don't see the problem
> of adding a negative number to an unsigned type of the same size.
> 
> Additionally if we use "unsigned long" wouldn't a typecast to (u64)
> result in a number like 4294967295 rather than 18446744073709551615,
> which is equivalent to -1, on a 32 bit system?

Yes, sorry, I said wrong thing here too.  If we define ncleaned with
unsigned long type, it must be arithmetically extended when we convert
it to 64 bit size.  So, casting with "signed long" is needed like
"(long)ncleaned" before converting it to u64.

>>> 		NILFS_SUI(sufile)->ncleansegs += ncleaned;

would be calculated as "unsigned long" even if the type of ncleaned is
"long" according to the usual arithmetic conversion rule of C99
(6.3.1.8).  And, in this case, we can omit the above typecast
"(long)".

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 v3 2/3] nilfs2: add nilfs_sufile_set_suinfo to update segment usage

[Ryusuke Konishi]

Hi Andreas,
On Tue, 28 Jan 2014 16:39:24 +0900 (JST), Ryusuke Konishi wrote:
> On Tue, 28 Jan 2014 05:26:05 +0100, Andreas Rohner wrote:
>> On 2014-01-28 02:03, Ryusuke Konishi wrote:
>>> On Tue, 28 Jan 2014 00:42:35 +0100, Andreas Rohner wrote:
>>>> On 2014-01-27 20:07, Ryusuke Konishi wrote:
>>>>> On Mon, 27 Jan 2014 10:59:27 +0100, Andreas Rohner wrote:
>>>>>> +			|| (nilfs_suinfo_update_flags(sup) &&
>>>>>> +				(sup->sup_sui.sui_flags &
>>>>>> +				(~0UL << (NILFS_SEGMENT_USAGE_ERROR + 1)))))
>>>>>
>>>>> Ditto. We need to add a definition to nilfs2_fs.h.
>>>>>
>>>>> enum {
>>>>>         NILFS_SEGMENT_USAGE_ACTIVE,
>>>>>         NILFS_SEGMENT_USAGE_DIRTY,
>>>>>         NILFS_SEGMENT_USAGE_ERROR,
>>>>> 	__NR_NILFS_SEGMENT_USAGE_FLAGS
>>>>> };
>>>>>
>>>>> 		    (nilfs_suinfo_update_flags(sup) &&
>>>>> 		     (sup->sup_sui.sui_flags &
>>>>> 		      (~0UL << __NR_NILFS_SEGMENT_USAGE_FLAGS))))
>>>>>
>>>>> By the way, this will dismiss the capability that userland cleaner
>>>>> programs uses the rest of su_flags for their own purpose such as GC
>>>>> optimization.  I think this (rejecting or utilizing it) should be
>>>>> carefully determined.
>>>>>
>>>>> Any comments on this?
>>>>
>>>> Hmm, I think it can't hurt to let the userland cleaner use the su_flags.
>>>> As far as I can tell, it shouldn't affect the kernel side.
>>>> nilfs_segment_usage_set_clean() would still work and
>>>> nilfs_sufile_do_scrap() overwrites the whole su_flags as well.
>>> 
>>> Well, actually the current definition of
>>> nilfs_segment_usage_set_clean() and also nilfs_segment_usage_clean()
>>> are written without compatibility consideration.
>> 
>> I actually thought it would be a good idea to wipe the custom flags if a
>> segment is cleaned, which the current implementation does. So the custom
>> flags are only valid for dirty segments and a segment is only considered
>> to be clean with nilfs_segment_usage_clean if there are no custom flags.
>> I don't think that would be unreasonable, because the GC has no use for
>> flags on clean segments anyway.
> 
> I looked over functions manipulating su_flags again, and came to the
> same conclusion.  We can keep consistency even if userland gc programs
> utilize the remaining flags for their own purpose.  There are no
> compatibility issues at least if we manipulate su_flags of dirty
> segments.  In this regard, the current implementation, which wipes the
> custom flags when it cleans segment, is not bad, yes.
> 
>>> It looks to be a separate change if we allow to use the upper bits.
>>> In that case, a bunch of changes and a new feature_compat_ro flag to
>>> deal it as a disk format change, would be needed.
>> 
>> I think we would only have to define, which flags are reserved for
>> future use and which are available for the userspace GC. Everything else
>> would just work.
> 
> Right.  My above comment is completely wrong.  We don't have to add a
> new feature-compat flag to use custom flags unless we want to use them
> for free segments (in this case, we need to change definition of
> nilfs_segment_usage_clean() and nilfs_segment_usage_set_clean())
> 
>>> Ok, let's take the above one which protects the upper bits for now.
>> 
>> Ok. It is certainly cleaner that way.
> 
> Let me recant my comment.  Let's change the patch to allow custom
> flags.

By the way, can you post the revised version of this series 
(kernel patches) before it gets late ?

I'd like to send these 3 patches to upstream so that they will
be merged into the mainline in the next merge window.

Userland changes can be merged and released early once we finished
review-and-fix process, but it take longer time until kernel patches
are merged and released.

Thanks,
Ryusuke Konishi


>>>>>> +			nilfs_sufile_mod_counter(header_bh, ncleansegs,
>>>>>> +					ndirtysegs);
>>>>>
>>>>> Does it work for a negative value without cast of (u64) ?
>>>>> Please confirm that these counters are updated as you expected.
>>>>>
>>>>>> +			NILFS_SUI(sufile)->ncleansegs += ncleansegs;
>>>>>
>>>>> Ditto.  
>>>>
>>>> I have tested it and it works. At least on my 64 bit architecture. It is
>>>> probably still a good idea to do an explicit cast.
>>>>
>>>> How about I use s64 for ncleaned and ndirtied and move
>>>> nilfs_sufile_mod_counter outside the loop?
>>> 
>>> Yes, this one looks better.  In that case, the u64 cast seems
>>> unnecessary.
>>> 
>>>> 	s64 ncleaned = 0, ndirtied = 0;
>>>>
>>>> 	...
>>>>
>>>> 	for (;;) {
>>>> 		...
>>>> 	}
>>>> 	mark_buffer_dirty(bh);
>>>> 	brelse(bh);
>>>>
>>>>  out_mark:
>>>> 	if (ncleaned || ndirtied) {
>>>> 		nilfs_sufile_mod_counter(header_bh, (u64)ncleaned,
>>>> 				(u64)ndirtied);
>>> 
>>>> 		NILFS_SUI(sufile)->ncleansegs += ncleaned;
>>> 
>>> This one looks unclear.
>>> 
>>> How about defining ncleaned and ndirtied with unsigned long type and
>>> cast them to (u64) for the arguments of nilfs_sufile_mod_counter() ?
>> 
>> Ok I agree ncleansegs could be 32 bit on 32 bit systems. But why
>> "unsigned long" and not just "long"? It seems a bit strange to use an
>> unsigned type for possible negative values and I don't see the problem
>> of adding a negative number to an unsigned type of the same size.
>> 
>> Additionally if we use "unsigned long" wouldn't a typecast to (u64)
>> result in a number like 4294967295 rather than 18446744073709551615,
>> which is equivalent to -1, on a 32 bit system?
> 
> Yes, sorry, I said wrong thing here too.  If we define ncleaned with
> unsigned long type, it must be arithmetically extended when we convert
> it to 64 bit size.  So, casting with "signed long" is needed like
> "(long)ncleaned" before converting it to u64.
> 
>>>> 		NILFS_SUI(sufile)->ncleansegs += ncleaned;
> 
> would be calculated as "unsigned long" even if the type of ncleaned is
> "long" according to the usual arithmetic conversion rule of C99
> (6.3.1.8).  And, in this case, we can omit the above typecast
> "(long)".
> 
> 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
--
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 v3 2/3] nilfs2: add nilfs_sufile_set_suinfo to update segment usage

[Andreas Rohner]

On 2014-01-30 08:57, Ryusuke Konishi wrote:
> Hi Andreas,
> On Tue, 28 Jan 2014 16:39:24 +0900 (JST), Ryusuke Konishi wrote:
>> On Tue, 28 Jan 2014 05:26:05 +0100, Andreas Rohner wrote:
>>> On 2014-01-28 02:03, Ryusuke Konishi wrote:
>>>> On Tue, 28 Jan 2014 00:42:35 +0100, Andreas Rohner wrote:
>>>>> On 2014-01-27 20:07, Ryusuke Konishi wrote:
>>>>>> On Mon, 27 Jan 2014 10:59:27 +0100, Andreas Rohner wrote:
>>>>>>> +			|| (nilfs_suinfo_update_flags(sup) &&
>>>>>>> +				(sup->sup_sui.sui_flags &
>>>>>>> +				(~0UL << (NILFS_SEGMENT_USAGE_ERROR + 1)))))
>>>>>>
>>>>>> Ditto. We need to add a definition to nilfs2_fs.h.
>>>>>>
>>>>>> enum {
>>>>>>         NILFS_SEGMENT_USAGE_ACTIVE,
>>>>>>         NILFS_SEGMENT_USAGE_DIRTY,
>>>>>>         NILFS_SEGMENT_USAGE_ERROR,
>>>>>> 	__NR_NILFS_SEGMENT_USAGE_FLAGS
>>>>>> };
>>>>>>
>>>>>> 		    (nilfs_suinfo_update_flags(sup) &&
>>>>>> 		     (sup->sup_sui.sui_flags &
>>>>>> 		      (~0UL << __NR_NILFS_SEGMENT_USAGE_FLAGS))))
>>>>>>
>>>>>> By the way, this will dismiss the capability that userland cleaner
>>>>>> programs uses the rest of su_flags for their own purpose such as GC
>>>>>> optimization.  I think this (rejecting or utilizing it) should be
>>>>>> carefully determined.
>>>>>>
>>>>>> Any comments on this?
>>>>>
>>>>> Hmm, I think it can't hurt to let the userland cleaner use the su_flags.
>>>>> As far as I can tell, it shouldn't affect the kernel side.
>>>>> nilfs_segment_usage_set_clean() would still work and
>>>>> nilfs_sufile_do_scrap() overwrites the whole su_flags as well.
>>>>
>>>> Well, actually the current definition of
>>>> nilfs_segment_usage_set_clean() and also nilfs_segment_usage_clean()
>>>> are written without compatibility consideration.
>>>
>>> I actually thought it would be a good idea to wipe the custom flags if a
>>> segment is cleaned, which the current implementation does. So the custom
>>> flags are only valid for dirty segments and a segment is only considered
>>> to be clean with nilfs_segment_usage_clean if there are no custom flags.
>>> I don't think that would be unreasonable, because the GC has no use for
>>> flags on clean segments anyway.
>>
>> I looked over functions manipulating su_flags again, and came to the
>> same conclusion.  We can keep consistency even if userland gc programs
>> utilize the remaining flags for their own purpose.  There are no
>> compatibility issues at least if we manipulate su_flags of dirty
>> segments.  In this regard, the current implementation, which wipes the
>> custom flags when it cleans segment, is not bad, yes.
>>
>>>> It looks to be a separate change if we allow to use the upper bits.
>>>> In that case, a bunch of changes and a new feature_compat_ro flag to
>>>> deal it as a disk format change, would be needed.
>>>
>>> I think we would only have to define, which flags are reserved for
>>> future use and which are available for the userspace GC. Everything else
>>> would just work.
>>
>> Right.  My above comment is completely wrong.  We don't have to add a
>> new feature-compat flag to use custom flags unless we want to use them
>> for free segments (in this case, we need to change definition of
>> nilfs_segment_usage_clean() and nilfs_segment_usage_set_clean())
>>
>>>> Ok, let's take the above one which protects the upper bits for now.
>>>
>>> Ok. It is certainly cleaner that way.
>>
>> Let me recant my comment.  Let's change the patch to allow custom
>> flags.
> 
> By the way, can you post the revised version of this series 
> (kernel patches) before it gets late ?
> 
> I'd like to send these 3 patches to upstream so that they will
> be merged into the mainline in the next merge window.
> 
> Userland changes can be merged and released early once we finished
> review-and-fix process, but it take longer time until kernel patches
> are merged and released.
> 
> Thanks,
> Ryusuke Konishi

Sure. I'll send them as soon as possible.

Thanks,
Andreas Rohner

>>>>>>> +			nilfs_sufile_mod_counter(header_bh, ncleansegs,
>>>>>>> +					ndirtysegs);
>>>>>>
>>>>>> Does it work for a negative value without cast of (u64) ?
>>>>>> Please confirm that these counters are updated as you expected.
>>>>>>
>>>>>>> +			NILFS_SUI(sufile)->ncleansegs += ncleansegs;
>>>>>>
>>>>>> Ditto.  
>>>>>
>>>>> I have tested it and it works. At least on my 64 bit architecture. It is
>>>>> probably still a good idea to do an explicit cast.
>>>>>
>>>>> How about I use s64 for ncleaned and ndirtied and move
>>>>> nilfs_sufile_mod_counter outside the loop?
>>>>
>>>> Yes, this one looks better.  In that case, the u64 cast seems
>>>> unnecessary.
>>>>
>>>>> 	s64 ncleaned = 0, ndirtied = 0;
>>>>>
>>>>> 	...
>>>>>
>>>>> 	for (;;) {
>>>>> 		...
>>>>> 	}
>>>>> 	mark_buffer_dirty(bh);
>>>>> 	brelse(bh);
>>>>>
>>>>>  out_mark:
>>>>> 	if (ncleaned || ndirtied) {
>>>>> 		nilfs_sufile_mod_counter(header_bh, (u64)ncleaned,
>>>>> 				(u64)ndirtied);
>>>>
>>>>> 		NILFS_SUI(sufile)->ncleansegs += ncleaned;
>>>>
>>>> This one looks unclear.
>>>>
>>>> How about defining ncleaned and ndirtied with unsigned long type and
>>>> cast them to (u64) for the arguments of nilfs_sufile_mod_counter() ?
>>>
>>> Ok I agree ncleansegs could be 32 bit on 32 bit systems. But why
>>> "unsigned long" and not just "long"? It seems a bit strange to use an
>>> unsigned type for possible negative values and I don't see the problem
>>> of adding a negative number to an unsigned type of the same size.
>>>
>>> Additionally if we use "unsigned long" wouldn't a typecast to (u64)
>>> result in a number like 4294967295 rather than 18446744073709551615,
>>> which is equivalent to -1, on a 32 bit system?
>>
>> Yes, sorry, I said wrong thing here too.  If we define ncleaned with
>> unsigned long type, it must be arithmetically extended when we convert
>> it to 64 bit size.  So, casting with "signed long" is needed like
>> "(long)ncleaned" before converting it to u64.
>>
>>>>> 		NILFS_SUI(sufile)->ncleansegs += ncleaned;
>>
>> would be calculated as "unsigned long" even if the type of ncleaned is
>> "long" according to the usual arithmetic conversion rule of C99
>> (6.3.1.8).  And, in this case, we can omit the above typecast
>> "(long)".
>>
>> 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
> 

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