nilfs-utils: new feature to skip inefficient gc

Hi,

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

This is potentially useful for all gc policies, but it is especially beneficial for the timestamp policy. Lets assume for example a NILFS2 volume with 20% static files, which is not unreasonable, and lets assume these static files are in the oldest segments. So the current timestamp policy will select the oldest segments and since the data is static it needs to be moved to new segments. Now it is in the newest segments, but after a while they will become the oldest segments again. Then timestamp will move them again. These moving operations are expensive and unnecessary.

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

SSD:

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

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

HDD:

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

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

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

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

Best regards,
Andreas Rohner

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

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

1.8.5.3

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

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

By setting it to 0 this feature is effectively disabled.

---
 sbin/cleanerd/cldconfig.c | 20 ++++++++++++++++++++
 sbin/cleanerd/cldconfig.h |  2 ++
 2 files changed, 22 insertions(+)

index 270d360..9a55914 100644
@@ -456,6 +456,20 @@ nilfs_cldconfig_handle_mc_nsegments_per_clean(struct nilfs_cldconfig *config,
 }
 
 static int
+nilfs_cldconfig_handle_min_free_blocks(struct nilfs_cldconfig *config,
+				       char **tokens, size_t ntoks,
+				       struct nilfs *nilfs)
+{
+	unsigned long n;
+
+	if (nilfs_cldconfig_get_ulong_argument(tokens, ntoks, &n) < 0)
+		return 0;
+
+	config->cf_min_free_blocks_for_cleaning = n;
+	return 0;
+}
+
+static int
 nilfs_cldconfig_handle_cleaning_interval(struct nilfs_cldconfig *config,
 					 char **tokens, size_t ntoks,
 					 struct nilfs *nilfs)

@@ -576,6 +590,10 @@ nilfs_cldconfig_keyword_table[] = {
 		"log_priority", 2, 2,
 		nilfs_cldconfig_handle_log_priority
 	},
+	{
+		"min_free_blocks_for_cleaning", 2, 2,
+		nilfs_cldconfig_handle_min_free_blocks
+	},
 };
 
 #define NILFS_CLDCONFIG_NKEYWORDS			\

@@ -641,6 +659,8 @@ static void nilfs_cldconfig_set_default(struct nilfs_cldconfig *config,
 	config->cf_retry_interval.tv_usec = 0;
 	config->cf_use_mmap = NILFS_CLDCONFIG_USE_MMAP;
 	config->cf_log_priority = NILFS_CLDCONFIG_LOG_PRIORITY;
+	config->cf_min_free_blocks_for_cleaning =
+		NILFS_CLDCONFIG_MIN_FREE_BLOCKS_FOR_CLEANING;
 }
 
 static inline int iseol(int c)

index 188ce9b..71a0642 100644
@@ -102,6 +102,7 @@ struct nilfs_cldconfig {
 	struct timeval cf_retry_interval;
 	int cf_use_mmap;
 	int cf_log_priority;
+	unsigned long cf_min_free_blocks_for_cleaning;
 };
 
 #define NILFS_CLDCONFIG_SELECTION_POLICY_IMPORTANCE	\

@@ -120,6 +121,7 @@ struct nilfs_cldconfig {
 #define NILFS_CLDCONFIG_RETRY_INTERVAL			60
 #define NILFS_CLDCONFIG_USE_MMAP			1
 #define NILFS_CLDCONFIG_LOG_PRIORITY			LOG_INFO
+#define NILFS_CLDCONFIG_MIN_FREE_BLOCKS_FOR_CLEANING		128
 
 #define NILFS_CLDCONFIG_NSEGMENTS_PER_CLEAN_MAX	32
 

1.8.5.3

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

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

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

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

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

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

Thanks,
Vyacheslav Dubeyko.

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

Hi Vyacheslav,

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

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

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

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

Thanks for reviewing my patches,

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

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

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

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

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

Thanks,
Vyacheslav Dubeyko.

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

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

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

Hi Vyacheslav,

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

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

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

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

if (!nilfs_segment_usage_clean(su))

	continue;

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

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

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

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

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

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

---
 include/nilfs_gc.h       |  2 ++
 sbin/cleanerd/cleanerd.c | 10 +++++++++-
 2 files changed, 11 insertions(+), 1 deletion(-)

index 72e9947..7628ce1 100644
@@ -27,4 +27,6 @@ static inline int nilfs_suinfo_reclaimable(const struct nilfs_suinfo *si)
 
 extern void (*nilfs_gc_logger)(int priority, const char *fmt, ...);
 
+#define EGCTRYAGAIN 513
+
 #endif /* NILFS_GC_H */

index 86dfcf7..ab8124f 100644
@@ -167,6 +167,7 @@ struct nilfs_cleanerd {
 	int running;
 	int fallback;
 	int retry_cleaning;
+	int no_timeout;
 	int shutdown;
 	long ncleansegs;
 	struct timeval cleaning_interval;

@@ -873,7 +874,7 @@ static int nilfs_cleanerd_recalc_interval(struct nilfs_cleanerd *cleanerd,
 	interval = nilfs_cleanerd_cleaning_interval(cleanerd);
 	/* timercmp() does not work for '>=' or '<='. */
 	/* curr >= target */
-	if (!timercmp(&curr, &cleanerd->target, <)) {
+	if (!timercmp(&curr, &cleanerd->target, <) || cleanerd->no_timeout) {
 		cleanerd->timeout.tv_sec = 0;
 		cleanerd->timeout.tv_usec = 0;
 		timeradd(&curr, interval, &cleanerd->target);

@@ -1348,6 +1349,7 @@ static ssize_t nilfs_cleanerd_clean_segments(struct nilfs_cleanerd *cleanerd,
 		       "number: %m");
 		goto out;
 	}
+	cleanerd->no_timeout = 0;
 
 	ret = nilfs_reclaim_segment(cleanerd->nilfs, segnums, nsegs,
 				    protseq, protcno);

@@ -1373,6 +1375,11 @@ static ssize_t nilfs_cleanerd_clean_segments(struct nilfs_cleanerd *cleanerd,
 			cleanerd->retry_cleaning = 0;
 		}
 
+	} else if (ret == -EGCTRYAGAIN) {
+		cleanerd->fallback = 0;
+		cleanerd->retry_cleaning = 1;
+		cleanerd->no_timeout = 1;
+		ret = 0;
 	} else if (ret < 0 && errno == ENOMEM) {
 		nilfs_cleanerd_reduce_ncleansegs_for_retry(cleanerd);
 		cleanerd->fallback = 1;

@@ -1415,6 +1422,7 @@ static int nilfs_cleanerd_clean_loop(struct nilfs_cleanerd *cleanerd)
 	cleanerd->running = 1;
 	cleanerd->fallback = 0;
 	cleanerd->retry_cleaning = 0;
+	cleanerd->no_timeout = 0;
 	nilfs_cnoconv_reset(cleanerd->cnoconv);
 	nilfs_gc_logger = syslog;
 

1.8.5.3

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

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

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

Could you describe your vision how it will work?

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

Hi Vyacheslav,

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

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

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

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

Best regards,
Andreas Rohner

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

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

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

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

index 7628ce1..f9fbde4 100644
@@ -15,8 +15,8 @@
 #include "nilfs.h"
 
 ssize_t nilfs_reclaim_segment(struct nilfs *nilfs,
-			      __u64 *segnums, size_t nsegs,
-			      __u64 protseq, nilfs_cno_t protcno);
+			      __u64 *segnums, size_t nsegs, __u64 protseq,
+			      nilfs_cno_t protcno, unsigned long minblocks);
 
 
 static inline int nilfs_suinfo_reclaimable(const struct nilfs_suinfo *si)

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

index ab8124f..76a79de 100644
@@ -1351,8 +1351,9 @@ static ssize_t nilfs_cleanerd_clean_segments(struct nilfs_cleanerd *cleanerd,
 	}
 	cleanerd->no_timeout = 0;
 
-	ret = nilfs_reclaim_segment(cleanerd->nilfs, segnums, nsegs,
-				    protseq, protcno);
+	ret = nilfs_reclaim_segment(cleanerd->nilfs, segnums,
+			nsegs, protseq, protcno,
+			cleanerd->config.cf_min_free_blocks_for_cleaning);
 	if (ret > 0) {
 		for (i = 0; i < ret; i++)
 			syslog(LOG_DEBUG, "segment %llu cleaned",

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

1.8.5.3

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

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

NILFS_CLEAN_SEGMENTS_DEFAULT: Normal GC operation

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

Additionally this patch implements a simple check in nilfs_reclaim_segment. If the number of free blocks that can be gained by cleaning the segments is below the threshold of minblocks, it sets the NILFS_CLEAN_SEGMENTS_UPDATE_SEGUSG flag. This causes the kernel to leave the segment alone, not move any blocks, and only update the SUFILE.

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

---
 include/nilfs.h     |  2 +-
 include/nilfs2_fs.h |  6 ++++++
 lib/gc.c            | 16 ++++++++++++++--
 lib/nilfs.c         |  3 ++-
 4 files changed, 23 insertions(+), 4 deletions(-)

index 56286a9..5ca8bec 100644
@@ -304,7 +304,7 @@ ssize_t nilfs_get_vinfo(const struct nilfs *, struct nilfs_vinfo *, size_t);
 ssize_t nilfs_get_bdescs(const struct nilfs *, struct nilfs_bdesc *, size_t);
 int nilfs_clean_segments(struct nilfs *, struct nilfs_vdesc *, size_t,
 			 struct nilfs_period *, size_t, __u64 *, size_t,
-			 struct nilfs_bdesc *, size_t, __u64 *, size_t);
+			 struct nilfs_bdesc *, size_t, __u64 *, size_t, int);
 int nilfs_sync(const struct nilfs *, nilfs_cno_t *);
 int nilfs_resize(struct nilfs *nilfs, off_t size);
 int nilfs_set_alloc_range(struct nilfs *nilfs, off_t start, off_t end);

index e674f44..1ac558f 100644
@@ -731,6 +731,12 @@ struct nilfs_cpmode {
 	__u32 cm_pad;
 };
 
+/* values for v_flags */
+enum {
+	NILFS_CLEAN_SEGMENTS_DEFAULT,
+	NILFS_CLEAN_SEGMENTS_UPDATE_SEGUSG,
+};
+
 /**
  * struct nilfs_argv - argument vector
  * @v_base: pointer on data array from userspace

index 0b0e2d6..cbae0f3 100644
@@ -616,6 +616,8 @@ ssize_t nilfs_reclaim_segment(struct nilfs *nilfs,
 	struct nilfs_vector *vdescv, *bdescv, *periodv, *vblocknrv;
 	sigset_t sigset, oldset, waitset;
 	ssize_t n, ret = -1;
+	__u32 freeblocks;
+	int cleaning_flags = NILFS_CLEAN_SEGMENTS_DEFAULT;
 
 	if (nsegs == 0)
 		return 0;

@@ -678,6 +680,13 @@ ssize_t nilfs_reclaim_segment(struct nilfs *nilfs,
 		goto out_lock;
 	}
 
+	freeblocks = (nilfs_get_blocks_per_segment(nilfs) * n)
+				- (nilfs_vector_get_size(vdescv)
+				+ nilfs_vector_get_size(bdescv));
+
+	if (freeblocks < minblocks * n)
+		cleaning_flags = NILFS_CLEAN_SEGMENTS_UPDATE_SEGUSG;
+
 	ret = nilfs_clean_segments(nilfs,
 				   nilfs_vector_get_data(vdescv),
 				   nilfs_vector_get_size(vdescv),

@@ -687,12 +696,15 @@ ssize_t nilfs_reclaim_segment(struct nilfs *nilfs,
 				   nilfs_vector_get_size(vblocknrv),
 				   nilfs_vector_get_data(bdescv),
 				   nilfs_vector_get_size(bdescv),
-				   segnums, n);
+				   segnums, n, cleaning_flags);
 	if (ret < 0) {
 		nilfs_gc_logger(LOG_ERR, "cannot clean segments: %s",
 				strerror(errno));
 	} else {
-		ret = n;
+		if (cleaning_flags == NILFS_CLEAN_SEGMENTS_UPDATE_SEGUSG)
+			ret = -EGCTRYAGAIN;
+		else
+			ret = n;
 	}
 
 out_lock:

index ebe50b8..50eed5e 100644
@@ -627,7 +627,7 @@ int nilfs_clean_segments(struct nilfs *nilfs,
 			 struct nilfs_period *periods, size_t nperiods,
 			 __u64 *vblocknrs, size_t nvblocknrs,
 			 struct nilfs_bdesc *bdescs, size_t nbdescs,
-			 __u64 *segnums, size_t nsegs)
+			 __u64 *segnums, size_t nsegs, int flags)
 {
 	struct nilfs_argv argv[5];
 

@@ -639,6 +639,7 @@ int nilfs_clean_segments(struct nilfs *nilfs,
 	argv[0].v_base = (unsigned long)vdescs;
 	argv[0].v_nmembs = nvdescs;
 	argv[0].v_size = sizeof(struct nilfs_vdesc);
+	argv[0].v_flags = flags;
 	argv[1].v_base = (unsigned long)periods;
 	argv[1].v_nmembs = nperiods;
 	argv[1].v_size = sizeof(struct nilfs_period);

1.8.5.3

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

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

NILFS_CLEAN_SEGMENTS_DEFAULT: Normal GC operation

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

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

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

---
 fs/nilfs2/ioctl.c         | 58 ++++++++++++++++++++++++++++++++++++++++-------
 include/linux/nilfs2_fs.h |  6 +++++
 2 files changed, 56 insertions(+), 8 deletions(-)

index b44bdb2..52b967a 100644
@@ -571,6 +571,42 @@ int nilfs_ioctl_prepare_clean_segments(struct the_nilfs *nilfs,
 	return ret;
 }
 
+static int nilfs_ioctl_update_segment_usage(struct super_block *sb,
+				struct nilfs_argv argv[5], void *bufs[5])
+{
+	size_t nmembs;
+	struct the_nilfs *nilfs = sb->s_fs_info;
+	struct inode *sufile = nilfs->ns_sufile;
+	struct nilfs_suinfo si;
+	__u64 *segnums;
+	int i, ret;
+	struct nilfs_transaction_info ti;
+
+	ret = nilfs_transaction_begin(sb, &ti, 0);
+	if (unlikely(ret))
+		return ret;
+
+	nmembs = argv[4].v_nmembs;
+	for (i = 0, segnums = bufs[4]; i < nmembs; ++i) {
+		ret = nilfs_sufile_get_suinfo(sufile, segnums[i],
+				&si, sizeof(struct nilfs_suinfo), 1);
+		if (unlikely(ret < 0))
+			goto failure;
+
+		ret = nilfs_sufile_set_segment_usage(sufile, segnums[i],
+				si.sui_nblocks, nilfs->ns_ctime);
+		if (unlikely(ret < 0))
+			goto failure;
+	}
+
+	nilfs_transaction_commit(sb);
+	return ret;
+
+ failure:
+	nilfs_transaction_abort(sb);
+	return ret;
+}
+
 static int nilfs_ioctl_clean_segments(struct inode *inode, struct file *filp,
 				      unsigned int cmd, void __user *argp)
 {

@@ -660,14 +696,20 @@ static int nilfs_ioctl_clean_segments(struct inode *inode, struct file *filp,
 		goto out_free;
 	}
 
-	ret = nilfs_ioctl_move_blocks(inode->i_sb, &argv[0], kbufs[0]);
-	if (ret < 0)
-		printk(KERN_ERR "NILFS: GC failed during preparation: "
-			"cannot read source blocks: err=%d\n", ret);
-	else {
-		if (nilfs_sb_need_update(nilfs))
-			set_nilfs_discontinued(nilfs);
-		ret = nilfs_clean_segments(inode->i_sb, argv, kbufs);
+	if (argv[0].v_flags == NILFS_CLEAN_SEGMENTS_UPDATE_SEGUSG) {
+		/* only update segment usage */
+		ret = nilfs_ioctl_update_segment_usage(inode->i_sb, argv,
+				kbufs);
+	} else {
+		ret = nilfs_ioctl_move_blocks(inode->i_sb, &argv[0], kbufs[0]);
+		if (ret < 0)
+			printk(KERN_ERR "NILFS: GC failed during preparation: "
+				"cannot read source blocks: err=%d\n", ret);
+		else {
+			if (nilfs_sb_need_update(nilfs))
+				set_nilfs_discontinued(nilfs);
+			ret = nilfs_clean_segments(inode->i_sb, argv, kbufs);
+		}
 	}
 
 	nilfs_remove_all_gcinodes(nilfs);

index 9875576..420be0b 100644
@@ -727,6 +727,12 @@ struct nilfs_cpmode {
 	__u32 cm_pad;
 };
 
+/* values for v_flags */
+enum {
+	NILFS_CLEAN_SEGMENTS_DEFAULT,
+	NILFS_CLEAN_SEGMENTS_UPDATE_SEGUSG,
+};
+
 /**
  * struct nilfs_argv - argument vector
  * @v_base: pointer on data array from userspace

1.8.5.3

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

Hi Andreas,

Thank you for posting the patch.

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

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

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

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

Additional comments are as follows:

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

Thanks,
Ryusuke Konishi

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

Hi Ryusuke,

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

Ah yes I didn't think of that.

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

Best regards,
Andreas Rohner

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

Hi Ryusuke,

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

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

Hi Andreas,

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

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

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

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

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

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

Thanks,
Vyacheslav Dubeyko.

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

Hi Vyacheslav,

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

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

Thanks

Thanks again

Best regards,
Andreas Rohner

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

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

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

With the best regards,
Vyacheslav Dubeyko.

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