nilfs-utils: shortcut for certain GC operations

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

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

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

---
 fs/nilfs2/sufile.c | 129 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 fs/nilfs2/sufile.h |   1 +
 2 files changed, 130 insertions(+)

fs/nilfs2/sufile.c +20 -0

index 3127e9f..8922523 100644
@@ -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,

fs/nilfs2/sufile.c +18 -0

+	}
+
+	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);

fs/nilfs2/sufile.c +12 -0

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

fs/nilfs2/sufile.c +33 -0

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

fs/nilfs2/sufile.h +1 -0

index e84bc5b..366003c 100644
@@ -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,

Signature · 6 lines

1.8.5.3

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

Quoted 41 lines · Andreas Rohner

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

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

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

---
 fs/nilfs2/sufile.c | 129 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 fs/nilfs2/sufile.h |   1 +
 2 files changed, 130 insertions(+)

fs/nilfs2/sufile.c +20 -0

index 3127e9f..8922523 100644
@@ -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,

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

Quoted 10 lines

fs/nilfs2/sufile.c +10 -0

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

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

Quoted 8 lines

fs/nilfs2/sufile.c +8 -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)) ||

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

Quoted 6 lines

fs/nilfs2/sufile.c +6 -0

+			|| (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?

Quoted 18 lines

fs/nilfs2/sufile.c +18 -0

+	}
+
+	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);

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

Quoted 10 lines

fs/nilfs2/sufile.c +10 -0

+
+		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.
*/

Quoted 12 lines

fs/nilfs2/sufile.c +12 -0

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

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

Quoted 10 lines

fs/nilfs2/sufile.c +10 -0

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

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

Quoted 1 line

fs/nilfs2/sufile.c +1 -0

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

Ditto.

Regards,
Ryusuke Konishi

Quoted 51 lines

fs/nilfs2/sufile.c +33 -0

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

fs/nilfs2/sufile.h +1 -0

index e84bc5b..366003c 100644
@@ -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,

Signature · 1 line

1.8.5.3

Quoted 4 lines

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

Quoted 23 lines · Ryusuke Konishi

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

Quoted 3 lines · Andreas Rohner

fs/nilfs2/sufile.c +3 -0

+			|| (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.

Quoted 1 line

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.

Quoted 9 lines

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

Quoted 2 lines

fs/nilfs2/sufile.c +2 -0

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

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

Quoted 1 line

fs/nilfs2/sufile.c +1 -0

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

Quoted 26 lines · Andreas Rohner

Quoted 24 lines · Ryusuke Konishi

Review

Quoted 3 lines · Andreas Rohner

(quoted diff) +3 -0

+			|| (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?

Quoted 4 lines

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.

Quoted 10 lines

Quoted 9 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

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

Review

Quoted 1 line

(quoted diff) +1 -0

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

Ditto.

Quoted 5 lines

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.

Quoted 14 lines

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

Quoted 1 line

		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() ?

Quoted 27 lines · Ryusuke Konishi

Quoted 26 lines · Andreas Rohner

Quoted 24 lines · Ryusuke Konishi

Review

Quoted 3 lines · Andreas Rohner

(quoted diff) +3 -0

+			|| (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?

Quoted 8 lines

Quoted 4 lines

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.

Quoted 3 lines

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.

Quoted 1 line

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

Ok. It is certainly cleaner that way.

Quoted 35 lines

Quoted 11 lines

Quoted 10 lines

Review

Quoted 3 lines

(quoted diff) +2 -0

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

Review

Quoted 1 line

(quoted diff) +1 -0

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

Ditto.

Quoted 5 lines

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.

Quoted 14 lines

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

Quoted 6 lines

Quoted 1 line

		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?

Quoted 1 line

Quoted 1 line

		NILFS_SUI(sufile)->ncleansegs += ncleaned;

Quoted 37 lines · Andreas Rohner

Quoted 27 lines · Ryusuke Konishi

Quoted 26 lines · Andreas Rohner

Quoted 24 lines · Ryusuke Konishi

Review

Quoted 3 lines · Andreas Rohner

(quoted diff) +3 -0

+			|| (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?

Quoted 8 lines

Quoted 4 lines

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.

Quoted 6 lines

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.

Quoted 7 lines

Quoted 3 lines

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

Quoted 3 lines

Quoted 1 line

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.

Quoted 41 lines

Quoted 34 lines

Quoted 10 lines

Quoted 9 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

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

Review

Quoted 1 line

(quoted diff) +1 -0

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

Ditto.

Quoted 5 lines

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.

Quoted 14 lines

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

Quoted 6 lines

Quoted 1 line

		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() ?

Quoted 8 lines

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.

Quoted 1 line

Quoted 1 line

Quoted 1 line

		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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Hi Andreas,

Quoted 65 lines · Ryusuke Konishi

Quoted 37 lines · Andreas Rohner

Quoted 27 lines · Ryusuke Konishi

Quoted 26 lines · Andreas Rohner

Quoted 24 lines · Ryusuke Konishi

Review

Quoted 3 lines · Andreas Rohner

(quoted diff) +3 -0

+			|| (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?

Quoted 8 lines

Quoted 4 lines

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.

Quoted 6 lines

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.

Quoted 7 lines

Quoted 3 lines

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

Quoted 6 lines

Quoted 3 lines

Quoted 1 line

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

Quoted 62 lines

Quoted 41 lines

Quoted 34 lines

Quoted 10 lines

Quoted 9 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

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

Review

Quoted 1 line

(quoted diff) +1 -0

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

Ditto.

Quoted 5 lines

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.

Quoted 14 lines

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

Quoted 6 lines

Quoted 1 line

		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() ?

Quoted 8 lines

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.

Quoted 1 line

Quoted 1 line

Quoted 1 line

		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)".

Quoted 6 lines

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

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 <[email protected]>

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

fs/nilfs2/ioctl.c +20 -0

index b44bdb2..e97df9c 100644
@@ -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)))

fs/nilfs2/ioctl.c +28 -0

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

fs/nilfs2/ioctl.c +3 -0

 				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:

include/linux/nilfs2_fs.h +2 -0

index 7b94449..4140f7f 100644
@@ -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 */

Signature · 6 lines

1.8.5.3

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

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 <[email protected]>

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

include/linux/nilfs2_fs.h +41 -0

index 9875576..7b94449 100644
@@ -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,

Signature · 1 line

1.8.5.3

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

Signature · 6 lines

1.8.5.3

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

Hi Andreas,

Quoted 51 lines · 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.

Quoted 2 lines

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.

Quoted 56 lines · Ryusuke Konishi

Hi Andreas,

Quoted 53 lines · 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.

Quoted 5 lines

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.

Quoted 2 lines

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?

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