nilfs2: implement tracking of live blocks

This patch introduces the nilfs_palloc_scan_entries() function, which takes an inode of one of nilfs' meta data files and iterates through all of its entries. For each entry the callback function pointer that is given as a parameter is called. The data parameter is passed to the callback function, so that it may receive parameters and return results.

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

---
 fs/nilfs2/alloc.c | 121 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 fs/nilfs2/alloc.h |   6 +++
 2 files changed, 127 insertions(+)

fs/nilfs2/alloc.c +19 -0

index 741fd02..0edd85a 100644
@@ -545,6 +545,127 @@ int nilfs_palloc_prepare_alloc_entry(struct inode *inode,
 }
 
 /**
+ * nilfs_palloc_scan_entries - scan through every entry and execute dofunc
+ * @inode: inode of metadata file using this allocator
+ * @dofunc: function executed for every entry
+ * @data: data pointer passed to dofunc
+ *
+ * Description: nilfs_palloc_scan_entries() walks through every allocated entry
+ * of a metadata file and executes dofunc on it. It passes a data pointer to
+ * dofunc, which can be used as an input parameter or for returning of results.
+ *
+ * Return Value: On success, 0 is returned. On error, a
+ * negative error code is returned.
+ */
+int nilfs_palloc_scan_entries(struct inode *inode,
+			      void (*dofunc)(struct inode *,
+					     struct nilfs_palloc_req *,
+					     void *),
+			      void *data)
+{
+	struct buffer_head *desc_bh, *bitmap_bh;

fs/nilfs2/alloc.c +4 -0

+			return 0;
+		else if (ret < 0)
+			return ret;
+		req.pr_desc_bh = desc_bh;

fs/nilfs2/alloc.c +22 -0

+			} else if (ret < 0)
+				goto out_desc;
+
+			req.pr_bitmap_bh = bitmap_bh;
+			bitmap_kaddr = kmap(bitmap_bh->b_page);
+			bitmap = bitmap_kaddr + bh_offset(bitmap_bh);
+			/* entry blkoff is always bigger than 0 */
+			blkoff = 0;
+			pos = 0;
+
+			for (j = 0; j < m; ++j, ++pos) {
+				pos = nilfs_find_next_bit(bitmap,
+						entries_per_group, pos);
+
+				if (pos >= entries_per_group)
+					break;
+
+				/* found an entry */
+				req.pr_entry_nr =
+					entries_per_group * group + pos;
+
+				prev_blkoff = blkoff;

fs/nilfs2/alloc.c +30 -0

+					if (ret < 0)
+						goto out_entry;
+				}
+
+				dofunc(inode, &req, data);
+			}
+
+			if (blkoff)
+				brelse(req.pr_entry_bh);
+			kunmap(bitmap_bh->b_page);
+			brelse(bitmap_bh);
+		}
+
+		kunmap(desc_bh->b_page);
+		brelse(desc_bh);
+	}
+
+	return 0;
+
+out_entry:
+	kunmap(bitmap_bh->b_page);
+	brelse(bitmap_bh);
+
+out_desc:
+	kunmap(desc_bh->b_page);
+	brelse(desc_bh);
+	return ret;
+}
+
+/**
  * nilfs_palloc_commit_alloc_entry - finish allocation of a persistent object
  * @inode: inode of metadata file using this allocator
  * @req: nilfs_palloc_req structure exchanged for the allocation

fs/nilfs2/alloc.h +1 -0

index 4bd6451..0592035 100644
@@ -77,6 +77,7 @@ int nilfs_palloc_freev(struct inode *, __u64 *, size_t);
 #define nilfs_set_bit_atomic		ext2_set_bit_atomic
 #define nilfs_clear_bit_atomic		ext2_clear_bit_atomic
 #define nilfs_find_next_zero_bit	find_next_zero_bit_le
+#define nilfs_find_next_bit			find_next_bit_le
 
 /**
  * struct nilfs_bh_assoc - block offset and buffer head association

Quoted 46 lines · Andreas Rohner

This patch introduces the nilfs_palloc_scan_entries() function, which takes an inode of one of nilfs' meta data files and iterates through all of its entries. For each entry the callback function pointer that is given as a parameter is called. The data parameter is passed to the callback function, so that it may receive parameters and return results.

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

---
 fs/nilfs2/alloc.c | 121 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 fs/nilfs2/alloc.h |   6 +++
 2 files changed, 127 insertions(+)

fs/nilfs2/alloc.c +25 -0

index 741fd02..0edd85a 100644
@@ -545,6 +545,127 @@ int nilfs_palloc_prepare_alloc_entry(struct inode *inode,
 }
 
 /**
+ * nilfs_palloc_scan_entries - scan through every entry and execute dofunc
+ * @inode: inode of metadata file using this allocator
+ * @dofunc: function executed for every entry
+ * @data: data pointer passed to dofunc
+ *
+ * Description: nilfs_palloc_scan_entries() walks through every allocated entry
+ * of a metadata file and executes dofunc on it. It passes a data pointer to
+ * dofunc, which can be used as an input parameter or for returning of results.
+ *
+ * Return Value: On success, 0 is returned. On error, a
+ * negative error code is returned.
+ */
+int nilfs_palloc_scan_entries(struct inode *inode,
+			      void (*dofunc)(struct inode *,
+					     struct nilfs_palloc_req *,
+					     void *),
+			      void *data)
+{
+	struct buffer_head *desc_bh, *bitmap_bh;
+	struct nilfs_palloc_group_desc *desc;
+	struct nilfs_palloc_req req;
+	unsigned char *bitmap;
+	void *desc_kaddr, *bitmap_kaddr;
+	unsigned long group, maxgroup, ngroups;
+	unsigned long n, m, entries_per_group, groups_per_desc_block;

fs/nilfs2/alloc.c int nilfs_palloc_prepare_alloc_entry(struct inode *inode, · line 576

Quoted 4 lines

fs/nilfs2/alloc.c +4 -0

+	unsigned long i, j, pos;
+	unsigned long blkoff, prev_blkoff;
+	int ret;
+

I think that it really makes sense to split this function's code between several small functions. It improves code style and readability of function. Moreover, it makes function more easy understandable.

fs/nilfs2/alloc.c int nilfs_palloc_prepare_alloc_entry(struct inode *inode, · line 584

Quoted 8 lines

fs/nilfs2/alloc.c +8 -0

+	ngroups = nilfs_palloc_groups_count(inode);
+	maxgroup = ngroups - 1;
+	entries_per_group = nilfs_palloc_entries_per_group(inode);
+	groups_per_desc_block = nilfs_palloc_groups_per_desc_block(inode);
+
+	for (group = 0; group < ngroups;) {
+		ret = nilfs_palloc_get_desc_block(inode, group, 0, &desc_bh);
+		if (ret == -ENOENT)

I suggest to add comment here.

Quoted 4 lines

fs/nilfs2/alloc.c +4 -0

+			return 0;
+		else if (ret < 0)
+			return ret;
+		req.pr_desc_bh = desc_bh;

fs/nilfs2/alloc.c int nilfs_palloc_prepare_alloc_entry(struct inode *inode, · line 598

Quoted 10 lines

fs/nilfs2/alloc.c +10 -0

+		desc_kaddr = kmap(desc_bh->b_page);
+		desc = nilfs_palloc_block_get_group_desc(inode, group,
+							 desc_bh, desc_kaddr);
+		n = nilfs_palloc_rest_groups_in_desc_block(inode, group,
+							   maxgroup);
+
+		for (i = 0; i < n; i++, desc++, group++) {
+			m = entries_per_group -
+					nilfs_palloc_group_desc_nfrees(inode,
+							group, desc);

Looks weird. It makes sense to split on several functions or to use variable.

fs/nilfs2/alloc.c int nilfs_palloc_prepare_alloc_entry(struct inode *inode, · line 603

Quoted 5 lines

fs/nilfs2/alloc.c +5 -0

+			if (!m)
+				continue;
+
+			ret = nilfs_palloc_get_bitmap_block(
+				inode, group, 0, &bitmap_bh);

Ditto. Looks weird.

fs/nilfs2/alloc.c int nilfs_palloc_prepare_alloc_entry(struct inode *inode, · line 606

Quoted 3 lines

fs/nilfs2/alloc.c +3 -0

+			if (ret == -ENOENT) {
+				ret = 0;
+				goto out_desc;

It needs to add comment here. Otherwise, it looks weird because anyway we go to out_desc. Maybe to combine:

if (unlikely(ret < 0)) {
	if (ret == -ENOENT)
		ret = 0;
	goto out_desc;
}

Anyway, it needs to comment why we assign zero for the case of -ENOENT.

Quoted 27 lines

fs/nilfs2/alloc.c +27 -0

+			} else if (ret < 0)
+				goto out_desc;
+
+			req.pr_bitmap_bh = bitmap_bh;
+			bitmap_kaddr = kmap(bitmap_bh->b_page);
+			bitmap = bitmap_kaddr + bh_offset(bitmap_bh);
+			/* entry blkoff is always bigger than 0 */
+			blkoff = 0;
+			pos = 0;
+
+			for (j = 0; j < m; ++j, ++pos) {
+				pos = nilfs_find_next_bit(bitmap,
+						entries_per_group, pos);
+
+				if (pos >= entries_per_group)
+					break;
+
+				/* found an entry */
+				req.pr_entry_nr =
+					entries_per_group * group + pos;
+
+				prev_blkoff = blkoff;
+				blkoff = nilfs_palloc_entry_blkoff(inode,
+							req.pr_entry_nr);
+
+				if (blkoff != prev_blkoff) {
+					if (prev_blkoff)

fs/nilfs2/alloc.c int nilfs_palloc_prepare_alloc_entry(struct inode *inode, · line 638

Quoted 5 lines

fs/nilfs2/alloc.c +5 -0

+						brelse(req.pr_entry_bh);
+
+					ret = nilfs_palloc_get_entry_block(
+							inode, req.pr_entry_nr,
+							0, &req.pr_entry_bh);

Ahhhh. Look weird. :) Split on small functions with clear names, anyway. It really improves the code from any point of view.

Thanks,
Vyacheslav Dubeyko.

Quoted 55 lines

fs/nilfs2/alloc.c +30 -0

+					if (ret < 0)
+						goto out_entry;
+				}
+
+				dofunc(inode, &req, data);
+			}
+
+			if (blkoff)
+				brelse(req.pr_entry_bh);
+			kunmap(bitmap_bh->b_page);
+			brelse(bitmap_bh);
+		}
+
+		kunmap(desc_bh->b_page);
+		brelse(desc_bh);
+	}
+
+	return 0;
+
+out_entry:
+	kunmap(bitmap_bh->b_page);
+	brelse(bitmap_bh);
+
+out_desc:
+	kunmap(desc_bh->b_page);
+	brelse(desc_bh);
+	return ret;
+}
+
+/**
  * nilfs_palloc_commit_alloc_entry - finish allocation of a persistent object
  * @inode: inode of metadata file using this allocator
  * @req: nilfs_palloc_req structure exchanged for the allocation

fs/nilfs2/alloc.h +6 -0

index 4bd6451..0592035 100644
@@ -77,6 +77,7 @@ int nilfs_palloc_freev(struct inode *, __u64 *, size_t);
 #define nilfs_set_bit_atomic		ext2_set_bit_atomic
 #define nilfs_clear_bit_atomic		ext2_clear_bit_atomic
 #define nilfs_find_next_zero_bit	find_next_zero_bit_le
+#define nilfs_find_next_bit			find_next_bit_le
 
 /**
  * struct nilfs_bh_assoc - block offset and buffer head association

@@ -106,5 +107,10 @@ void nilfs_palloc_setup_cache(struct inode *inode,
 			      struct nilfs_palloc_cache *cache);
 void nilfs_palloc_clear_cache(struct inode *inode);
 void nilfs_palloc_destroy_cache(struct inode *inode);
+int nilfs_palloc_scan_entries(struct inode *,
+			      void (*dofunc)(struct inode *,
+					     struct nilfs_palloc_req *,
+					     void *),
+			      void *);
 

fs/nilfs2/alloc.h void nilfs_palloc_setup_cache(struct inode *inode, · line 116

Quoted 1 line

fs/nilfs2/alloc.h +0 -0

 #endif	/* _NILFS_ALLOC_H */

This patch adds an additional timestamp to the segment usage information that indicates the last time the usage information was changed. So su_lastmod indicates the last time the segment itself was modified and su_lastdec indicates the last time the usage information itself was changed.

This is important information for the GC, because it needs to avoid selecting segments for cleaning that are created (su_lastmod) outside of the protection period, but the blocks got reclaimable (su_nblocks is decremented) within the protection period. Without that information the GC policy has to assume, that there are reclaimble blocks, only to find out, that they are protected by the protection period.

This patch also introduces nilfs_sufile_add_segment_usage(), which can be used to increment or decrement the value of su_nblocks of a specific segment.

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

---

fs/nilfs2/sufile.c        | 86 +++++++++++++++++++++++++++++++++++++++++++++--
fs/nilfs2/sufile.h        | 18 ++++++++++
include/linux/nilfs2_fs.h |  7 ++++
3 files changed, 109 insertions(+), 2 deletions(-)

fs/nilfs2/sufile.c +3 -1

index 2a869c3..0886938 100644
@@ -453,6 +453,8 @@ void nilfs_sufile_do_scrap(struct inode *sufile, __u64 segnum,
 	su->su_lastmod = cpu_to_le64(0);
 	su->su_nblocks = cpu_to_le32(0);
 	su->su_flags = cpu_to_le32(1UL << NILFS_SEGMENT_USAGE_DIRTY);
+	if (nilfs_sufile_lastdec_supported(sufile))
+		su->su_lastdec = cpu_to_le64(0);
 	kunmap_atomic(kaddr);
 
 	nilfs_sufile_mod_counter(header_bh, clean ? (u64)-1 : 0, dirty ? 0 : 1);

@@ -482,7 +484,7 @@ void nilfs_sufile_do_free(struct inode *sufile, __u64 segnum,
 	WARN_ON(!nilfs_segment_usage_dirty(su));
 
 	sudirty = nilfs_segment_usage_dirty(su);
-	nilfs_segment_usage_set_clean(su);
+	nilfs_sufile_segment_usage_set_clean(sufile, su);
 	kunmap_atomic(kaddr);
 	mark_buffer_dirty(su_bh);
 

fs/nilfs2/sufile.c +25 -0

+ * @sufile: inode of segment usage file
+ * @segnum: segment number
+ * @value: value to add to su_nblocks
+ * @dectime: current time
+ *
+ * Description: nilfs_sufile_add_segment_usage() adds a signed value to the
+ * su_nblocks field of the segment usage information of @segnum. It ensures
+ * that the result is bigger than 0 and smaller or equal to the maximum number
+ * of blocks per segment
+ *
+ * Return Value: On success, 0 is returned. On error, one of the following
+ * negative error codes is returned.
+ *
+ * %-ENOMEM - Insufficient memory available.
+ *
+ * %-EIO - I/O error
+ *
+ * %-ENOENT - the specified block does not exist (hole block)
+ */
+int nilfs_sufile_add_segment_usage(struct inode *sufile, __u64 segnum,
+				   __s64 value, time_t dectime)
+{
+	struct the_nilfs *nilfs = sufile->i_sb->s_fs_info;
+	struct buffer_head *bh;
+	struct nilfs_segment_usage *su;

fs/nilfs2/sufile.c +25 -1

+		value = nilfs->ns_blocks_per_segment;
+
+	if (value == le32_to_cpu(su->su_nblocks)) {
+		kunmap_atomic(kaddr);
+		goto out_brelse;
+	}
+
+	su->su_nblocks = cpu_to_le32(value);
+	if (dectime && nilfs_sufile_lastdec_supported(sufile))
+		su->su_lastdec = cpu_to_le64(dectime);
+	kunmap_atomic(kaddr);
+
+	mark_buffer_dirty(bh);
+	nilfs_mdt_mark_dirty(sufile);
+
+out_brelse:
+	brelse(bh);
+out_sem:
+	up_write(&NILFS_MDT(sufile)->mi_sem);
+	return ret;
+}
+
+/**
 * nilfs_sufile_get_stat - get segment usage statistics
 * @sufile: inode of segment usage file
 * @stat: pointer to a structure of segment usage statistics

@@ -698,7 +769,8 @@ static int nilfs_sufile_truncate_range(struct inode *sufile,
 		nc = 0;
 		for (su = su2, j = 0; j < n; j++, su = (void *)su + susz) {
 			if (nilfs_segment_usage_error(su)) {
-				nilfs_segment_usage_set_clean(su);
+				nilfs_sufile_segment_usage_set_clean(sufile,
+						su);
 				nc++;
 			}
 		}

fs/nilfs2/sufile.c +6 -0

+				else
+					si->sui_lastdec = 0;
+			}
 		}
 		kunmap_atomic(kaddr);
 		brelse(su_bh);

@@ -935,6 +1014,9 @@ ssize_t nilfs_sufile_set_suinfo(struct inode *sufile, void *buf,
 		if (nilfs_suinfo_update_lastmod(sup))
 			su->su_lastmod = cpu_to_le64(sup->sup_sui.sui_lastmod);
 
+		if (nilfs_suinfo_update_lastdec(sup))
+			su->su_lastdec = cpu_to_le64(sup->sup_sui.sui_lastdec);
+
 		if (nilfs_suinfo_update_nblocks(sup))
 			su->su_nblocks = cpu_to_le32(sup->sup_sui.sui_nblocks);
 

fs/nilfs2/sufile.h +18 -0

index b8afd72..e5455d2 100644
@@ -28,6 +28,23 @@
 #include <linux/nilfs2_fs.h>
 #include "mdt.h"
 
+static inline int
+nilfs_sufile_lastdec_supported(const struct inode *sufile)
+{
+	return NILFS_MDT(sufile)->mi_entry_size ==
+			sizeof(struct nilfs_segment_usage);
+}
+
+static inline void
+nilfs_sufile_segment_usage_set_clean(const struct inode *sufile,
+		struct nilfs_segment_usage *su)
+{
+	su->su_lastmod = cpu_to_le64(0);
+	su->su_nblocks = cpu_to_le32(0);
+	su->su_flags = cpu_to_le32(0);
+	if (nilfs_sufile_lastdec_supported(sufile))
+		su->su_lastdec = cpu_to_le64(0);
+}
 
 static inline unsigned long nilfs_sufile_get_nsegments(struct inode *sufile)
 {

@@ -41,6 +58,7 @@ int nilfs_sufile_alloc(struct inode *, __u64 *);
 int nilfs_sufile_mark_dirty(struct inode *sufile, __u64 segnum);
 int nilfs_sufile_set_segment_usage(struct inode *sufile, __u64 segnum,
 				   unsigned long nblocks, time_t modtime);
+int nilfs_sufile_add_segment_usage(struct inode *, __u64, __s64, time_t);
 int nilfs_sufile_get_stat(struct inode *, struct nilfs_sustat *);
 ssize_t nilfs_sufile_get_suinfo(struct inode *, __u64, void *, unsigned,
 				size_t);

include/linux/nilfs2_fs.h +0 -0

index ff3fea3..ca269ad 100644

include/linux/nilfs2_fs.h +1 -0

 };
 
 #define NILFS_MIN_SEGMENT_USAGE_SIZE	16

@@ -663,6 +665,7 @@ nilfs_segment_usage_set_clean(struct nilfs_segment_usage *su)
 	su->su_lastmod = cpu_to_le64(0);
 	su->su_nblocks = cpu_to_le32(0);
 	su->su_flags = cpu_to_le32(0);
+	su->su_lastdec = cpu_to_le64(0);
 }
 

static inline int

include/linux/nilfs2_fs.h +4 -0

@@ -694,11 +697,13 @@ struct nilfs_sufile_header {
 * @sui_lastmod: timestamp of last modification
 * @sui_nblocks: number of written blocks in segment
 * @sui_flags: segment usage flags
+ * @sui_lastdec: last decrement of sui_nblocks timestamp
 */
 struct nilfs_suinfo {
 	__u64 sui_lastmod;
 	__u32 sui_nblocks;
 	__u32 sui_flags;
+	__u64 sui_lastdec;
 };
 
 #define NILFS_SUINFO_FNS(flag, name)					\

@@ -736,6 +741,7 @@ enum {
 	NILFS_SUINFO_UPDATE_LASTMOD,
 	NILFS_SUINFO_UPDATE_NBLOCKS,
 	NILFS_SUINFO_UPDATE_FLAGS,
+	NILFS_SUINFO_UPDATE_LASTDEC,
 	__NR_NILFS_SUINFO_UPDATE_FIELDS,
 };
 

@@ -759,6 +765,7 @@ nilfs_suinfo_update_##name(const struct nilfs_suinfo_update *sup)	\
 NILFS_SUINFO_UPDATE_FNS(LASTMOD, lastmod)
 NILFS_SUINFO_UPDATE_FNS(NBLOCKS, nblocks)
 NILFS_SUINFO_UPDATE_FNS(FLAGS, flags)
+NILFS_SUINFO_UPDATE_FNS(LASTDEC, lastdec)
 
 enum {
 	NILFS_CHECKPOINT,

Signature · 6 lines

1.9.0

--
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 6 lines · Andreas Rohner

This patch adds an additional timestamp to the segment usage information that indicates the last time the usage information was changed. So su_lastmod indicates the last time the segment itself was modified and su_lastdec indicates the last time the usage information itself was changed.

What will we have if user changes time?
What sequence will we have after such "malicious" action? Did you test such situation?

Quoted 40 lines

This is important information for the GC, because it needs to avoid selecting segments for cleaning that are created (su_lastmod) outside of the protection period, but the blocks got reclaimable (su_nblocks is decremented) within the protection period. Without that information the GC policy has to assume, that there are reclaimble blocks, only to find out, that they are protected by the protection period.

This patch also introduces nilfs_sufile_add_segment_usage(), which can be used to increment or decrement the value of su_nblocks of a specific segment.

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

---

fs/nilfs2/sufile.c        | 86 +++++++++++++++++++++++++++++++++++++++++++++--
fs/nilfs2/sufile.h        | 18 ++++++++++
include/linux/nilfs2_fs.h |  7 ++++
3 files changed, 109 insertions(+), 2 deletions(-)

fs/nilfs2/sufile.c +3 -1

index 2a869c3..0886938 100644
@@ -453,6 +453,8 @@ void nilfs_sufile_do_scrap(struct inode *sufile, __u64 segnum,
 	su->su_lastmod = cpu_to_le64(0);
 	su->su_nblocks = cpu_to_le32(0);
 	su->su_flags = cpu_to_le32(1UL << NILFS_SEGMENT_USAGE_DIRTY);
+	if (nilfs_sufile_lastdec_supported(sufile))
+		su->su_lastdec = cpu_to_le64(0);
 	kunmap_atomic(kaddr);
 
 	nilfs_sufile_mod_counter(header_bh, clean ? (u64)-1 : 0, dirty ? 0 : 1);

@@ -482,7 +484,7 @@ void nilfs_sufile_do_free(struct inode *sufile, __u64 segnum,
 	WARN_ON(!nilfs_segment_usage_dirty(su));
 
 	sudirty = nilfs_segment_usage_dirty(su);
-	nilfs_segment_usage_set_clean(su);
+	nilfs_sufile_segment_usage_set_clean(sufile, su);
 	kunmap_atomic(kaddr);
 	mark_buffer_dirty(su_bh);
 

fs/nilfs2/sufile.c int nilfs_sufile_set_segment_usage(struct inode *sufile, __u64 segnum, · line 554

Quoted 5 lines

fs/nilfs2/sufile.c +1 -0

@@ -549,6 +551,75 @@ int nilfs_sufile_set_segment_usage(struct inode *sufile, __u64 segnum,
 }
 
 /**
+ * nilfs_sufile_add_segment_usage - decrement usage of a segment

I feel cultural dissonance about this name. Add or decrement? :) Decrement and add are different operations for me.

Quoted 29 lines

fs/nilfs2/sufile.c +29 -0

+ * @sufile: inode of segment usage file
+ * @segnum: segment number
+ * @value: value to add to su_nblocks
+ * @dectime: current time
+ *
+ * Description: nilfs_sufile_add_segment_usage() adds a signed value to the
+ * su_nblocks field of the segment usage information of @segnum. It ensures
+ * that the result is bigger than 0 and smaller or equal to the maximum number
+ * of blocks per segment
+ *
+ * Return Value: On success, 0 is returned. On error, one of the following
+ * negative error codes is returned.
+ *
+ * %-ENOMEM - Insufficient memory available.
+ *
+ * %-EIO - I/O error
+ *
+ * %-ENOENT - the specified block does not exist (hole block)
+ */
+int nilfs_sufile_add_segment_usage(struct inode *sufile, __u64 segnum,
+				   __s64 value, time_t dectime)
+{
+	struct the_nilfs *nilfs = sufile->i_sb->s_fs_info;
+	struct buffer_head *bh;
+	struct nilfs_segment_usage *su;
+	void *kaddr;
+	int ret;
+
+	if (value == 0)

fs/nilfs2/sufile.c int nilfs_sufile_set_segment_usage(struct inode *sufile, __u64 segnum, · line 589

Quoted 6 lines

fs/nilfs2/sufile.c +6 -0

+		return 0;
+
+	down_write(&NILFS_MDT(sufile)->mi_sem);
+
+	ret = nilfs_sufile_get_segment_usage_block(sufile, segnum, 0, &bh);
+	if (ret < 0)

Maybe it needs to use unlikely() here.

fs/nilfs2/sufile.c int nilfs_sufile_set_segment_usage(struct inode *sufile, __u64 segnum, · line 596

Quoted 7 lines

fs/nilfs2/sufile.c +7 -0

+		goto out_sem;
+
+	kaddr = kmap_atomic(bh->b_page);
+	su = nilfs_sufile_block_get_segment_usage(sufile, segnum, bh, kaddr);
+	WARN_ON(nilfs_segment_usage_error(su));
+
+	value += le32_to_cpu(su->su_nblocks);

Decrement. Really? :)

fs/nilfs2/sufile.c int nilfs_sufile_set_segment_usage(struct inode *sufile, __u64 segnum, · line 599

Quoted 3 lines

fs/nilfs2/sufile.c +3 -0

+	if (value < 0)
+		value = 0;
+	if (value > nilfs->ns_blocks_per_segment)

maybe "else if" here?

Quoted 38 lines

fs/nilfs2/sufile.c +25 -1

+		value = nilfs->ns_blocks_per_segment;
+
+	if (value == le32_to_cpu(su->su_nblocks)) {
+		kunmap_atomic(kaddr);
+		goto out_brelse;
+	}
+
+	su->su_nblocks = cpu_to_le32(value);
+	if (dectime && nilfs_sufile_lastdec_supported(sufile))
+		su->su_lastdec = cpu_to_le64(dectime);
+	kunmap_atomic(kaddr);
+
+	mark_buffer_dirty(bh);
+	nilfs_mdt_mark_dirty(sufile);
+
+out_brelse:
+	brelse(bh);
+out_sem:
+	up_write(&NILFS_MDT(sufile)->mi_sem);
+	return ret;
+}
+
+/**
 * nilfs_sufile_get_stat - get segment usage statistics
 * @sufile: inode of segment usage file
 * @stat: pointer to a structure of segment usage statistics

@@ -698,7 +769,8 @@ static int nilfs_sufile_truncate_range(struct inode *sufile,
 		nc = 0;
 		for (su = su2, j = 0; j < n; j++, su = (void *)su + susz) {
 			if (nilfs_segment_usage_error(su)) {
-				nilfs_segment_usage_set_clean(su);
+				nilfs_sufile_segment_usage_set_clean(sufile,
+						su);
 				nc++;
 			}
 		}

@@ -858,6 +930,13 @@ ssize_t nilfs_sufile_get_suinfo(struct inode *sufile, __u64 segnum, void *buf,
 			if (nilfs_segment_is_active(nilfs, segnum + j))

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

Quoted 6 lines

fs/nilfs2/sufile.c +4 -0

 				si->sui_flags |=
 					(1UL << NILFS_SEGMENT_USAGE_ACTIVE);
+			if (sisz >= sizeof(struct nilfs_suinfo)) {
+				if (susz >= sizeof(struct nilfs_segment_usage))
+					si->sui_lastdec =
+						le64_to_cpu(su->su_lastdec);

Is it really impossible to place assignment on one line?

Quoted 64 lines

fs/nilfs2/sufile.c +6 -0

+				else
+					si->sui_lastdec = 0;
+			}
 		}
 		kunmap_atomic(kaddr);
 		brelse(su_bh);

@@ -935,6 +1014,9 @@ ssize_t nilfs_sufile_set_suinfo(struct inode *sufile, void *buf,
 		if (nilfs_suinfo_update_lastmod(sup))
 			su->su_lastmod = cpu_to_le64(sup->sup_sui.sui_lastmod);
 
+		if (nilfs_suinfo_update_lastdec(sup))
+			su->su_lastdec = cpu_to_le64(sup->sup_sui.sui_lastdec);
+
 		if (nilfs_suinfo_update_nblocks(sup))
 			su->su_nblocks = cpu_to_le32(sup->sup_sui.sui_nblocks);
 

fs/nilfs2/sufile.h +18 -0

index b8afd72..e5455d2 100644
@@ -28,6 +28,23 @@
 #include <linux/nilfs2_fs.h>
 #include "mdt.h"
 
+static inline int
+nilfs_sufile_lastdec_supported(const struct inode *sufile)
+{
+	return NILFS_MDT(sufile)->mi_entry_size ==
+			sizeof(struct nilfs_segment_usage);
+}
+
+static inline void
+nilfs_sufile_segment_usage_set_clean(const struct inode *sufile,
+		struct nilfs_segment_usage *su)
+{
+	su->su_lastmod = cpu_to_le64(0);
+	su->su_nblocks = cpu_to_le32(0);
+	su->su_flags = cpu_to_le32(0);
+	if (nilfs_sufile_lastdec_supported(sufile))
+		su->su_lastdec = cpu_to_le64(0);
+}
 
 static inline unsigned long nilfs_sufile_get_nsegments(struct inode *sufile)
 {

@@ -41,6 +58,7 @@ int nilfs_sufile_alloc(struct inode *, __u64 *);
 int nilfs_sufile_mark_dirty(struct inode *sufile, __u64 segnum);
 int nilfs_sufile_set_segment_usage(struct inode *sufile, __u64 segnum,
 				   unsigned long nblocks, time_t modtime);
+int nilfs_sufile_add_segment_usage(struct inode *, __u64, __s64, time_t);
 int nilfs_sufile_get_stat(struct inode *, struct nilfs_sustat *);
 ssize_t nilfs_sufile_get_suinfo(struct inode *, __u64, void *, unsigned,
 				size_t);

include/linux/nilfs2_fs.h +1 -0

index ff3fea3..ca269ad 100644
@@ -614,11 +614,13 @@ struct nilfs_cpfile_header {
 * @su_lastmod: last modified timestamp
 * @su_nblocks: number of blocks in segment
 * @su_flags: flags
+ * @su_lastdec: last decrement of su_nblocks timestamp
 */
 struct nilfs_segment_usage {
 	__le64 su_lastmod;

include/linux/nilfs2_fs.h struct nilfs_cpfile_header { · line 623

Quoted 3 lines

include/linux/nilfs2_fs.h +1 -0

 	__le32 su_nblocks;
 	__le32 su_flags;
+	__le64 su_lastdec;

So, this change makes on-disk layout incompatible with previous one. Am I correct? At first it needs to be fully confident that we really need in changing in this place. Secondly, it needs to add incompatible flag for s_feature_incompat field of superblock and maybe mount option.

The su_lastdec sounds not very good for my taste.

Thanks,
Vyacheslav Dubeyko.

Quoted 44 lines

include/linux/nilfs2_fs.h +1 -0

 };
 
 #define NILFS_MIN_SEGMENT_USAGE_SIZE	16

@@ -663,6 +665,7 @@ nilfs_segment_usage_set_clean(struct nilfs_segment_usage *su)
 	su->su_lastmod = cpu_to_le64(0);
 	su->su_nblocks = cpu_to_le32(0);
 	su->su_flags = cpu_to_le32(0);
+	su->su_lastdec = cpu_to_le64(0);
 }
 

static inline int

include/linux/nilfs2_fs.h +4 -0

@@ -694,11 +697,13 @@ struct nilfs_sufile_header {
 * @sui_lastmod: timestamp of last modification
 * @sui_nblocks: number of written blocks in segment
 * @sui_flags: segment usage flags
+ * @sui_lastdec: last decrement of sui_nblocks timestamp
 */
 struct nilfs_suinfo {
 	__u64 sui_lastmod;
 	__u32 sui_nblocks;
 	__u32 sui_flags;
+	__u64 sui_lastdec;
 };
 
 #define NILFS_SUINFO_FNS(flag, name)					\

@@ -736,6 +741,7 @@ enum {
 	NILFS_SUINFO_UPDATE_LASTMOD,
 	NILFS_SUINFO_UPDATE_NBLOCKS,
 	NILFS_SUINFO_UPDATE_FLAGS,
+	NILFS_SUINFO_UPDATE_LASTDEC,
 	__NR_NILFS_SUINFO_UPDATE_FIELDS,
 };
 

@@ -759,6 +765,7 @@ nilfs_suinfo_update_##name(const struct nilfs_suinfo_update *sup)	\
 NILFS_SUINFO_UPDATE_FNS(LASTMOD, lastmod)
 NILFS_SUINFO_UPDATE_FNS(NBLOCKS, nblocks)
 NILFS_SUINFO_UPDATE_FNS(FLAGS, flags)
+NILFS_SUINFO_UPDATE_FNS(LASTDEC, lastdec)
 
 enum {
 	NILFS_CHECKPOINT,

Signature · 1 line

1.9.0

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

Quoted 13 lines

include/linux/nilfs2_fs.h +1 -0

index ff3fea3..ca269ad 100644
@@ -614,11 +614,13 @@ struct nilfs_cpfile_header {
 * @su_lastmod: last modified timestamp
 * @su_nblocks: number of blocks in segment
 * @su_flags: flags
+ * @su_lastdec: last decrement of su_nblocks timestamp
 */
 struct nilfs_segment_usage {
 	__le64 su_lastmod;

Quoted 8 lines

include/linux/nilfs2_fs.h struct nilfs_cpfile_header { · line 623

Quoted 3 lines

include/linux/nilfs2_fs.h +1 -0

 	__le32 su_nblocks;
 	__le32 su_flags;
+	__le64 su_lastdec;

So, this change makes on-disk layout incompatible with previous one. Am I correct? At first it needs to be fully confident that we really need in changing in this place. Secondly, it needs to add incompatible flag for s_feature_incompat field of superblock and maybe mount option.

No it IS compatible. NILFS uses the entry sizes stored in the super block. Notice, that the code does not depend on sizeof(struct nilfs_suinfo) or sizeof(struct nilfs_segment_usage). So an old kernel can read a file system with su_lastdec and a new kernel can read an old file system without su_lastdec.

Quoted 22 lines · Andreas Rohner

Quoted 13 lines

Quoted 13 lines

include/linux/nilfs2_fs.h +1 -0

index ff3fea3..ca269ad 100644
@@ -614,11 +614,13 @@ struct nilfs_cpfile_header {
 * @su_lastmod: last modified timestamp
 * @su_nblocks: number of blocks in segment
 * @su_flags: flags
+ * @su_lastdec: last decrement of su_nblocks timestamp
 */
 struct nilfs_segment_usage {
 	__le64 su_lastmod;

Quoted 8 lines

include/linux/nilfs2_fs.h struct nilfs_cpfile_header { · line 623

Quoted 3 lines

include/linux/nilfs2_fs.h +1 -0

 	__le32 su_nblocks;
 	__le32 su_flags;
+	__le64 su_lastdec;

So, this change makes on-disk layout incompatible with previous one. Am I correct? At first it needs to be fully confident that we really need in changing in this place. Secondly, it needs to add incompatible flag for s_feature_incompat field of superblock and maybe mount option.

Quoted 5 lines

No it IS compatible. NILFS uses the entry sizes stored in the super block. Notice, that the code does not depend on sizeof(struct nilfs_suinfo) or sizeof(struct nilfs_segment_usage). So an old kernel can read a file system with su_lastdec and a new kernel can read an old file system without su_lastdec.

But, anyway, I think that you add some new feature by this and previous patches. I suppose that it makes sense to add specially dedicated flag or flags in s_feature_xxx field of superblock. If feature is compatible with previous state of driver then flag can be added for s_feature_compat field.

Thanks,
Vyacheslav Dubeyko.

Quoted 37 lines · Vyacheslav Dubeyko

Quoted 22 lines · Andreas Rohner

Quoted 13 lines

Quoted 13 lines

include/linux/nilfs2_fs.h +1 -0

index ff3fea3..ca269ad 100644
@@ -614,11 +614,13 @@ struct nilfs_cpfile_header {
 * @su_lastmod: last modified timestamp
 * @su_nblocks: number of blocks in segment
 * @su_flags: flags
+ * @su_lastdec: last decrement of su_nblocks timestamp
 */
 struct nilfs_segment_usage {
 	__le64 su_lastmod;

Quoted 8 lines

include/linux/nilfs2_fs.h struct nilfs_cpfile_header { · line 623

Quoted 3 lines

include/linux/nilfs2_fs.h +1 -0

 	__le32 su_nblocks;
 	__le32 su_flags;
+	__le64 su_lastdec;

So, this change makes on-disk layout incompatible with previous one. Am I correct? At first it needs to be fully confident that we really need in changing in this place. Secondly, it needs to add incompatible flag for s_feature_incompat field of superblock and maybe mount option.

Quoted 5 lines

No it IS compatible. NILFS uses the entry sizes stored in the super block. Notice, that the code does not depend on sizeof(struct nilfs_suinfo) or sizeof(struct nilfs_segment_usage). So an old kernel can read a file system with su_lastdec and a new kernel can read an old file system without su_lastdec.

But, anyway, I think that you add some new feature by this and previous patches. I suppose that it makes sense to add specially dedicated flag or flags in s_feature_xxx field of superblock. If feature is compatible with previous state of driver then flag can be added for s_feature_compat field.

Quoted 2 lines

Thanks,
Vyacheslav Dubeyko.

This is important thing. Please evaluate backward compatibility and forward compatibility of modifications, and properly add one of incompat, compat_ro, or compat flags as Vyacheslav mentioned. It will be a focal point of early stage review.

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

Quoted 11 lines · Vyacheslav Dubeyko

Quoted 8 lines · Andreas Rohner

This patch adds an additional timestamp to the segment usage information that indicates the last time the usage information was changed. So su_lastmod indicates the last time the segment itself was modified and su_lastdec indicates the last time the usage information itself was changed.

Quoted 3 lines

What will we have if user changes time?
What sequence will we have after such "malicious" action? Did you test such situation?

The timestamp is just a hint for the userspace GC. If the hint is wrong the result would be that the GC is less efficient for a while. After a while it would go back to normal. You have the same problem with the already existing su_lastmod timestamp.

Quoted 13 lines · Andreas Rohner

Quoted 11 lines · Vyacheslav Dubeyko

Quoted 8 lines · Andreas Rohner

This patch adds an additional timestamp to the segment usage information that indicates the last time the usage information was changed. So su_lastmod indicates the last time the segment itself was modified and su_lastdec indicates the last time the usage information itself was changed.

Quoted 9 lines

Quoted 3 lines

What will we have if user changes time?
What sequence will we have after such "malicious" action? Did you test such situation?

The timestamp is just a hint for the userspace GC. If the hint is wrong the result would be that the GC is less efficient for a while. After a while it would go back to normal. You have the same problem with the already existing su_lastmod timestamp.

But I worry about such thing. Previously, we had complains of users about different issues with timestamp policy of GC. And I had hope that namely new GC policies can resolve such GC disadvantage. So, what have we again? The same issue of GC?

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

To accurately count the number of live blocks in a segment, it is important to take snapshots into account, because snapshots can protect reclaimable blocks from being cleaned.

This patch uses the previously reserved de_rsv field of the nilfs_dat_entry struct to store one of the snapshots the corresponding block belongs to. One block can belong to many snapshots, but because the snapshots are stored in a sorted linked list, it is easy to check if a block belongs to any other snapshot given the previous and the next snapshot. For example if the current snapshot (in de_ss) is being removed and neither the previous nor the next snapshot is in the range of de_start to de_end, then it is guaranteed that the block doesn't belong to any other snapshot and is reclaimable. On the other hand if lets say the previous snapshot is in the range of de_start to de_end, we simply set de_ss to the previous snapshot and the block is not reclaimable.

To implement this every DAT entry is scanned at snapshot creation/deletion time and updated if needed.

To avoid too many update
operations only potentially reclaimable blocks are ever updated. For example if there are some deleted files and the checkpoint to which these files belong is turned into a snapshot, then su_nblocks is incremented for these blocks, which reverses the decrement that happened when the files were deleted. If after some time this snapshot is deleted, su_nblocks is decremented again to reverse the increment at creation time.

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

---
 fs/nilfs2/cpfile.c        |  7 ++++
 fs/nilfs2/dat.c           | 86 +++++++++++++++++++++++++++++++++++++++++++++++
 fs/nilfs2/dat.h           | 26 ++++++++++++++
 include/linux/nilfs2_fs.h |  4 +--
 4 files changed, 121 insertions(+), 2 deletions(-)

fs/nilfs2/cpfile.c +7 -0

index 0d58075..29952f5 100644
@@ -28,6 +28,7 @@
 #include <linux/nilfs2_fs.h>
 #include "mdt.h"
 #include "cpfile.h"
+#include "sufile.h"
 
 
 static inline unsigned long

@@ -584,6 +585,7 @@ static int nilfs_cpfile_set_snapshot(struct inode *cpfile, __u64 cno)
 	struct nilfs_cpfile_header *header;
 	struct nilfs_checkpoint *cp;
 	struct nilfs_snapshot_list *list;
+	struct the_nilfs *nilfs = cpfile->i_sb->s_fs_info;
 	__u64 curr, prev;
 	unsigned long curr_blkoff, prev_blkoff;
 	void *kaddr;

@@ -681,6 +683,8 @@ static int nilfs_cpfile_set_snapshot(struct inode *cpfile, __u64 cno)
 	mark_buffer_dirty(header_bh);
 	nilfs_mdt_mark_dirty(cpfile);
 
+	nilfs_dat_scan_inc_ss(nilfs->ns_dat, cno);
+
 	brelse(prev_bh);
 
  out_curr:

@@ -703,6 +707,7 @@ static int nilfs_cpfile_clear_snapshot(struct inode *cpfile, __u64 cno)
 	struct nilfs_cpfile_header *header;
 	struct nilfs_checkpoint *cp;
 	struct nilfs_snapshot_list *list;
+	struct the_nilfs *nilfs = cpfile->i_sb->s_fs_info;
 	__u64 next, prev;
 	void *kaddr;
 	int ret;

@@ -784,6 +789,8 @@ static int nilfs_cpfile_clear_snapshot(struct inode *cpfile, __u64 cno)
 	mark_buffer_dirty(header_bh);
 	nilfs_mdt_mark_dirty(cpfile);
 
+	nilfs_dat_scan_dec_ss(nilfs->ns_dat, cno, prev, next);
+
 	brelse(prev_bh);
 
  out_next:

fs/nilfs2/dat.c +15 -0

index 0d5fada..89a4a5f 100644
@@ -28,6 +28,7 @@
 #include "mdt.h"
 #include "alloc.h"
 #include "dat.h"
+#include "sufile.h"
 
 
 #define NILFS_CNO_MIN	((__u64)1)

@@ -97,6 +98,7 @@ void nilfs_dat_commit_alloc(struct inode *dat, struct nilfs_palloc_req *req)
 	entry->de_start = cpu_to_le64(NILFS_CNO_MIN);
 	entry->de_end = cpu_to_le64(NILFS_CNO_MAX);
 	entry->de_blocknr = cpu_to_le64(0);
+	entry->de_ss = cpu_to_le64(0);
 	kunmap_atomic(kaddr);
 
 	nilfs_palloc_commit_alloc_entry(dat, req);

@@ -121,6 +123,7 @@ static void nilfs_dat_commit_free(struct inode *dat,
 	entry->de_start = cpu_to_le64(NILFS_CNO_MIN);
 	entry->de_end = cpu_to_le64(NILFS_CNO_MIN);
 	entry->de_blocknr = cpu_to_le64(0);
+	entry->de_ss = cpu_to_le64(0);
 	kunmap_atomic(kaddr);
 
 	nilfs_dat_commit_entry(dat, req);

@@ -201,6 +204,7 @@ void nilfs_dat_commit_end(struct inode *dat, struct nilfs_palloc_req *req,
 		WARN_ON(start > end);
 	}
 	entry->de_end = cpu_to_le64(end);
+	entry->de_ss = cpu_to_le64(NILFS_CNO_MAX);
 	blocknr = le64_to_cpu(entry->de_blocknr);
 	kunmap_atomic(kaddr);
 

@@ -365,6 +369,8 @@ int nilfs_dat_move(struct inode *dat, __u64 vblocknr, sector_t blocknr)
 	}
 	WARN_ON(blocknr == 0);
 	entry->de_blocknr = cpu_to_le64(blocknr);
+	if (entry->de_ss == cpu_to_le64(NILFS_CNO_MAX))
+		entry->de_ss = cpu_to_le64(0);
 	kunmap_atomic(kaddr);
 
 	mark_buffer_dirty(entry_bh);

@@ -430,6 +436,86 @@ int nilfs_dat_translate(struct inode *dat, __u64 vblocknr, sector_t *blocknrp)
 	return ret;
 }
 
+void nilfs_dat_do_scan_dec(struct inode *dat, struct nilfs_palloc_req *req,
+			   void *data)
+{
+	struct nilfs_dat_entry *entry;
+	__u64 start, end, prev_ss;
+	__u64 *ssp = data, ss = ssp[0], prev = ssp[1], next = ssp[2];
+	sector_t blocknr;
+	void *kaddr;
+	struct the_nilfs *nilfs;

fs/nilfs2/dat.c +29 -0

+
+			if (prev_ss != NILFS_CNO_MAX)
+				prev_ss = le64_to_cpu(entry->de_ss);
+			kunmap_atomic(kaddr);
+			mark_buffer_dirty(req->pr_entry_bh);
+			nilfs_mdt_mark_dirty(dat);
+		} else
+			kunmap_atomic(kaddr);
+
+		if (prev_ss == 0) {
+			nilfs = dat->i_sb->s_fs_info;
+			nilfs_sufile_add_segment_usage(nilfs->ns_sufile,
+				nilfs_get_segnum_of_block(nilfs, blocknr),
+				-1, 0);
+		}
+	} else
+		kunmap_atomic(kaddr);
+}
+
+void nilfs_dat_do_scan_inc(struct inode *dat, struct nilfs_palloc_req *req,
+			   void *data)
+{
+	struct nilfs_dat_entry *entry;
+	__u64 start, end, prev_ss;
+	__u64 *ssp = data, ss = *ssp;
+	sector_t blocknr;
+	void *kaddr;
+	struct the_nilfs *nilfs;
+

fs/nilfs2/dat.c +5 -0

+				1, 0);
+	} else
+		kunmap_atomic(kaddr);
+}
+
 ssize_t nilfs_dat_get_vinfo(struct inode *dat, void *buf, unsigned visz,
 			    size_t nvi)
 {

fs/nilfs2/dat.h +26 -0

index cbd8e97..92a187e 100644
@@ -55,5 +55,31 @@ ssize_t nilfs_dat_get_vinfo(struct inode *, void *, unsigned, size_t);
 
 int nilfs_dat_read(struct super_block *sb, size_t entry_size,
 		   struct nilfs_inode *raw_inode, struct inode **inodep);
+void nilfs_dat_do_scan_dec(struct inode *, struct nilfs_palloc_req *, void *);
+void nilfs_dat_do_scan_inc(struct inode *, struct nilfs_palloc_req *, void *);
+
+/**
+ * nilfs_dat_scan_dec_ss - scan all dat entries for a checkpoint dec suinfo
+ * @dat: inode of dat file
+ * @cno: snapshot number
+ * @prev: previous snapshot number
+ * @next: next snapshot number
+ */
+static inline int nilfs_dat_scan_dec_ss(struct inode *dat, __u64 cno,
+					__u64 prev, __u64 next)
+{
+	__u64 data[3] = { cno, prev, next };
+	return nilfs_palloc_scan_entries(dat, nilfs_dat_do_scan_dec, data);
+}
+
+/**
+ * nilfs_dat_scan_dec_ss - scan all dat entries for a checkpoint inc suinfo
+ * @dat: inode of dat file
+ * @cno: snapshot number
+ */
+static inline int nilfs_dat_scan_inc_ss(struct inode *dat, __u64 cno)
+{
+	return nilfs_palloc_scan_entries(dat, nilfs_dat_do_scan_inc, &cno);
+}
 
 #endif	/* _NILFS_DAT_H */

include/linux/nilfs2_fs.h +1 -1

index ca269ad..ba9ebe02 100644
@@ -475,13 +475,13 @@ struct nilfs_palloc_group_desc {
  * @de_blocknr: block number
  * @de_start: start checkpoint number
  * @de_end: end checkpoint number
- * @de_rsv: reserved for future use
+ * @de_ss: one of the snapshots the block belongs to

Quoted 17 lines · Andreas Rohner

To accurately count the number of live blocks in a segment, it is important to take snapshots into account, because snapshots can protect reclaimable blocks from being cleaned.

This patch uses the previously reserved de_rsv field of the nilfs_dat_entry struct to store one of the snapshots the corresponding block belongs to. One block can belong to many snapshots, but because the snapshots are stored in a sorted linked list, it is easy to check if a block belongs to any other snapshot given the previous and the next snapshot. For example if the current snapshot (in de_ss) is being removed and neither the previous nor the next snapshot is in the range of de_start to de_end, then it is guaranteed that the block doesn't belong to any other snapshot and is reclaimable. On the other hand if lets say the previous snapshot is in the range of de_start to de_end, we simply set de_ss to the previous snapshot and the block is not reclaimable.

Quoted 2 lines

To implement this every DAT entry is scanned at snapshot creation/deletion time and updated if needed.

It is well known problem of NILFS2 that deletion is very slow operation for big files because of necessity to update DAT file (de_end: end checkpoint number). So, how your addition does affect this disadvantage?

Quoted 126 lines

To avoid too many update
operations only potentially reclaimable blocks are ever updated. For example if there are some deleted files and the checkpoint to which these files belong is turned into a snapshot, then su_nblocks is incremented for these blocks, which reverses the decrement that happened when the files were deleted. If after some time this snapshot is deleted, su_nblocks is decremented again to reverse the increment at creation time.

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

---
 fs/nilfs2/cpfile.c        |  7 ++++
 fs/nilfs2/dat.c           | 86 +++++++++++++++++++++++++++++++++++++++++++++++
 fs/nilfs2/dat.h           | 26 ++++++++++++++
 include/linux/nilfs2_fs.h |  4 +--
 4 files changed, 121 insertions(+), 2 deletions(-)

fs/nilfs2/cpfile.c +7 -0

index 0d58075..29952f5 100644
@@ -28,6 +28,7 @@
 #include <linux/nilfs2_fs.h>
 #include "mdt.h"
 #include "cpfile.h"
+#include "sufile.h"
 
 
 static inline unsigned long

@@ -584,6 +585,7 @@ static int nilfs_cpfile_set_snapshot(struct inode *cpfile, __u64 cno)
 	struct nilfs_cpfile_header *header;
 	struct nilfs_checkpoint *cp;
 	struct nilfs_snapshot_list *list;
+	struct the_nilfs *nilfs = cpfile->i_sb->s_fs_info;
 	__u64 curr, prev;
 	unsigned long curr_blkoff, prev_blkoff;
 	void *kaddr;

@@ -681,6 +683,8 @@ static int nilfs_cpfile_set_snapshot(struct inode *cpfile, __u64 cno)
 	mark_buffer_dirty(header_bh);
 	nilfs_mdt_mark_dirty(cpfile);
 
+	nilfs_dat_scan_inc_ss(nilfs->ns_dat, cno);
+
 	brelse(prev_bh);
 
  out_curr:

@@ -703,6 +707,7 @@ static int nilfs_cpfile_clear_snapshot(struct inode *cpfile, __u64 cno)
 	struct nilfs_cpfile_header *header;
 	struct nilfs_checkpoint *cp;
 	struct nilfs_snapshot_list *list;
+	struct the_nilfs *nilfs = cpfile->i_sb->s_fs_info;
 	__u64 next, prev;
 	void *kaddr;
 	int ret;

@@ -784,6 +789,8 @@ static int nilfs_cpfile_clear_snapshot(struct inode *cpfile, __u64 cno)
 	mark_buffer_dirty(header_bh);
 	nilfs_mdt_mark_dirty(cpfile);
 
+	nilfs_dat_scan_dec_ss(nilfs->ns_dat, cno, prev, next);
+
 	brelse(prev_bh);
 
  out_next:

fs/nilfs2/dat.c +20 -0

index 0d5fada..89a4a5f 100644
@@ -28,6 +28,7 @@
 #include "mdt.h"
 #include "alloc.h"
 #include "dat.h"
+#include "sufile.h"
 
 
 #define NILFS_CNO_MIN	((__u64)1)

@@ -97,6 +98,7 @@ void nilfs_dat_commit_alloc(struct inode *dat, struct nilfs_palloc_req *req)
 	entry->de_start = cpu_to_le64(NILFS_CNO_MIN);
 	entry->de_end = cpu_to_le64(NILFS_CNO_MAX);
 	entry->de_blocknr = cpu_to_le64(0);
+	entry->de_ss = cpu_to_le64(0);
 	kunmap_atomic(kaddr);
 
 	nilfs_palloc_commit_alloc_entry(dat, req);

@@ -121,6 +123,7 @@ static void nilfs_dat_commit_free(struct inode *dat,
 	entry->de_start = cpu_to_le64(NILFS_CNO_MIN);
 	entry->de_end = cpu_to_le64(NILFS_CNO_MIN);
 	entry->de_blocknr = cpu_to_le64(0);
+	entry->de_ss = cpu_to_le64(0);
 	kunmap_atomic(kaddr);
 
 	nilfs_dat_commit_entry(dat, req);

@@ -201,6 +204,7 @@ void nilfs_dat_commit_end(struct inode *dat, struct nilfs_palloc_req *req,
 		WARN_ON(start > end);
 	}
 	entry->de_end = cpu_to_le64(end);
+	entry->de_ss = cpu_to_le64(NILFS_CNO_MAX);
 	blocknr = le64_to_cpu(entry->de_blocknr);
 	kunmap_atomic(kaddr);
 

@@ -365,6 +369,8 @@ int nilfs_dat_move(struct inode *dat, __u64 vblocknr, sector_t blocknr)
 	}
 	WARN_ON(blocknr == 0);
 	entry->de_blocknr = cpu_to_le64(blocknr);
+	if (entry->de_ss == cpu_to_le64(NILFS_CNO_MAX))
+		entry->de_ss = cpu_to_le64(0);
 	kunmap_atomic(kaddr);
 
 	mark_buffer_dirty(entry_bh);

@@ -430,6 +436,86 @@ int nilfs_dat_translate(struct inode *dat, __u64 vblocknr, sector_t *blocknrp)
 	return ret;
 }
 
+void nilfs_dat_do_scan_dec(struct inode *dat, struct nilfs_palloc_req *req,
+			   void *data)
+{
+	struct nilfs_dat_entry *entry;
+	__u64 start, end, prev_ss;
+	__u64 *ssp = data, ss = ssp[0], prev = ssp[1], next = ssp[2];
+	sector_t blocknr;
+	void *kaddr;
+	struct the_nilfs *nilfs;
+
+	kaddr = kmap_atomic(req->pr_entry_bh->b_page);
+	entry = nilfs_palloc_block_get_entry(dat, req->pr_entry_nr,
+					     req->pr_entry_bh, kaddr);
+	start = le64_to_cpu(entry->de_start);

fs/nilfs2/dat.c int nilfs_dat_translate(struct inode *dat, __u64 vblocknr, sector_t *blocknrp) · line 457

Quoted 5 lines

fs/nilfs2/dat.c +5 -0

+	end = le64_to_cpu(entry->de_end);
+	blocknr = le64_to_cpu(entry->de_blocknr);
+	prev_ss = le64_to_cpu(entry->de_ss);
+
+	if (blocknr != 0 && end != NILFS_CNO_MAX && ss >= start && ss < end) {

I think that it makes sense to use small functions with clear names about what we check.

fs/nilfs2/dat.c int nilfs_dat_translate(struct inode *dat, __u64 vblocknr, sector_t *blocknrp) · line 464

Quoted 7 lines

fs/nilfs2/dat.c +7 -0

+		if (prev_ss == ss || prev_ss == NILFS_CNO_MAX) {
+			if (prev && prev >= start && prev < end)
+				entry->de_ss = cpu_to_le64(prev);
+			else if (next && next >= start && next < end)
+				entry->de_ss = cpu_to_le64(next);
+			else
+				entry->de_ss = cpu_to_le64(0);

Ditto.

Quoted 33 lines

fs/nilfs2/dat.c +33 -0

+
+			if (prev_ss != NILFS_CNO_MAX)
+				prev_ss = le64_to_cpu(entry->de_ss);
+			kunmap_atomic(kaddr);
+			mark_buffer_dirty(req->pr_entry_bh);
+			nilfs_mdt_mark_dirty(dat);
+		} else
+			kunmap_atomic(kaddr);
+
+		if (prev_ss == 0) {
+			nilfs = dat->i_sb->s_fs_info;
+			nilfs_sufile_add_segment_usage(nilfs->ns_sufile,
+				nilfs_get_segnum_of_block(nilfs, blocknr),
+				-1, 0);
+		}
+	} else
+		kunmap_atomic(kaddr);
+}
+
+void nilfs_dat_do_scan_inc(struct inode *dat, struct nilfs_palloc_req *req,
+			   void *data)
+{
+	struct nilfs_dat_entry *entry;
+	__u64 start, end, prev_ss;
+	__u64 *ssp = data, ss = *ssp;
+	sector_t blocknr;
+	void *kaddr;
+	struct the_nilfs *nilfs;
+
+	kaddr = kmap_atomic(req->pr_entry_bh->b_page);
+	entry = nilfs_palloc_block_get_entry(dat, req->pr_entry_nr,
+			req->pr_entry_bh, kaddr);
+	start = le64_to_cpu(entry->de_start);

fs/nilfs2/dat.c int nilfs_dat_translate(struct inode *dat, __u64 vblocknr, sector_t *blocknrp) · line 503

Quoted 6 lines

fs/nilfs2/dat.c +6 -0

+	end = le64_to_cpu(entry->de_end);
+	blocknr = le64_to_cpu(entry->de_blocknr);
+	prev_ss = le64_to_cpu(entry->de_ss);
+
+	if (blocknr != 0 && end != NILFS_CNO_MAX && ss >= start && ss < end &&
+			(prev_ss == 0 || prev_ss == NILFS_CNO_MAX)) {

Ditto. Moreover, you repeat this check.

Quoted 4 lines

fs/nilfs2/dat.c +4 -0

+
+		entry->de_ss = cpu_to_le64(ss);
+
+		kunmap_atomic(kaddr);

fs/nilfs2/dat.c int nilfs_dat_translate(struct inode *dat, __u64 vblocknr, sector_t *blocknrp) · line 477

Quoted 6 lines

fs/nilfs2/dat.c +6 -0

+		mark_buffer_dirty(req->pr_entry_bh);
+		nilfs_mdt_mark_dirty(dat);
+
+		nilfs = dat->i_sb->s_fs_info;
+		nilfs_sufile_add_segment_usage(nilfs->ns_sufile,
+				nilfs_get_segnum_of_block(nilfs, blocknr),

Looks weird. Maybe, variable?

Thanks,
Vyacheslav Dubeyko.

Quoted 63 lines

fs/nilfs2/dat.c +5 -0

+				1, 0);
+	} else
+		kunmap_atomic(kaddr);
+}
+
 ssize_t nilfs_dat_get_vinfo(struct inode *dat, void *buf, unsigned visz,
 			    size_t nvi)
 {

fs/nilfs2/dat.h +26 -0

index cbd8e97..92a187e 100644
@@ -55,5 +55,31 @@ ssize_t nilfs_dat_get_vinfo(struct inode *, void *, unsigned, size_t);
 
 int nilfs_dat_read(struct super_block *sb, size_t entry_size,
 		   struct nilfs_inode *raw_inode, struct inode **inodep);
+void nilfs_dat_do_scan_dec(struct inode *, struct nilfs_palloc_req *, void *);
+void nilfs_dat_do_scan_inc(struct inode *, struct nilfs_palloc_req *, void *);
+
+/**
+ * nilfs_dat_scan_dec_ss - scan all dat entries for a checkpoint dec suinfo
+ * @dat: inode of dat file
+ * @cno: snapshot number
+ * @prev: previous snapshot number
+ * @next: next snapshot number
+ */
+static inline int nilfs_dat_scan_dec_ss(struct inode *dat, __u64 cno,
+					__u64 prev, __u64 next)
+{
+	__u64 data[3] = { cno, prev, next };
+	return nilfs_palloc_scan_entries(dat, nilfs_dat_do_scan_dec, data);
+}
+
+/**
+ * nilfs_dat_scan_dec_ss - scan all dat entries for a checkpoint inc suinfo
+ * @dat: inode of dat file
+ * @cno: snapshot number
+ */
+static inline int nilfs_dat_scan_inc_ss(struct inode *dat, __u64 cno)
+{
+	return nilfs_palloc_scan_entries(dat, nilfs_dat_do_scan_inc, &cno);
+}
 
 #endif	/* _NILFS_DAT_H */

include/linux/nilfs2_fs.h +2 -2

index ca269ad..ba9ebe02 100644
@@ -475,13 +475,13 @@ struct nilfs_palloc_group_desc {
  * @de_blocknr: block number
  * @de_start: start checkpoint number
  * @de_end: end checkpoint number
- * @de_rsv: reserved for future use
+ * @de_ss: one of the snapshots the block belongs to
  */
 struct nilfs_dat_entry {
 	__le64 de_blocknr;
 	__le64 de_start;
 	__le64 de_end;
-	__le64 de_rsv;
+	__le64 de_ss;
 };
 

include/linux/nilfs2_fs.h struct nilfs_palloc_group_desc { · line 487

Quoted 1 line

include/linux/nilfs2_fs.h +0 -0

 #define NILFS_MIN_DAT_ENTRY_SIZE	32

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

fs/nilfs2/dat.c int nilfs_dat_translate(struct inode *dat, __u64 vblocknr, sector_t *blocknrp) · line 503

Quoted 3 lines

fs/nilfs2/dat.c +3 -0

+
+	if (blocknr != 0 && end != NILFS_CNO_MAX && ss >= start && ss < end &&
+			(prev_ss == 0 || prev_ss == NILFS_CNO_MAX)) {

Ditto. Moreover, you repeat this check.

What do you mean? Where do I repeat this check?

fs/nilfs2/dat.c int nilfs_dat_translate(struct inode *dat, __u64 vblocknr, sector_t *blocknrp) · line 457

Quoted 1 line

fs/nilfs2/dat.c +1 -0

+	if (blocknr != 0 && end != NILFS_CNO_MAX && ss >= start && ss < end) {

This patch introduces new flags for nilfs_vdesc to indicate the reason a block is alive. So if the block would be reclaimable, but must be treated as if it were alive, because it is part of a snapshot, then the snapshot flag is set.

Additionally a new ioctl() is added, which enables the userspace GC to perform a cleanup operation after setting the number of blocks with NILFS_IOCTL_SET_SUINFO. It sets DAT entries with de_ss values of NILFS_CNO_MAX to 0. NILFS_CNO_MAX indicates, that the corresponding block belongs to some snapshot, but was already decremented by a previous deletion operation. If the segment usage info is changed with NILFS_IOCTL_SET_SUINFO and the number of blocks is updated, then these blocks would never be decremented and there are scenarios where the corresponding segments would starve (never be cleaned). To prevent that they must be reset to 0.

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

---
 fs/nilfs2/dat.c           |  63 ++++++++++++++++++++++++++++
 fs/nilfs2/dat.h           |   1 +
 fs/nilfs2/ioctl.c         | 103 +++++++++++++++++++++++++++++++++++++++++++++-
 include/linux/nilfs2_fs.h |  52 ++++++++++++++++++++++-
 4 files changed, 216 insertions(+), 3 deletions(-)

fs/nilfs2/dat.c +10 -0

index 89a4a5f..7adb15d 100644
@@ -382,6 +382,69 @@ int nilfs_dat_move(struct inode *dat, __u64 vblocknr, sector_t blocknr)
 }
 
 /**
+ * nilfs_dat_clean_snapshot_flag - check flags used by snapshots
+ * @dat: DAT file inode
+ * @vblocknr: virtual block number
+ *
+ * Description: nilfs_dat_clean_snapshot_flag() changes the flags from
+ * NILFS_CNO_MAX to 0 if necessary, so that segment usage is accurately
+ * counted. NILFS_CNO_MAX indicates, that the corresponding block belongs
+ * to some snapshot, but was already decremented. If the segment usage info
+ * is changed with NILFS_IOCTL_SET_SUINFO and the number of blocks is updated,
+ * then these blocks would never be decremented and there are scenarios where

fs/nilfs2/dat.c +26 -0

+{
+	struct buffer_head *entry_bh;
+	struct nilfs_dat_entry *entry;
+	void *kaddr;
+	int ret;
+
+	ret = nilfs_palloc_get_entry_block(dat, vblocknr, 0, &entry_bh);
+	if (ret < 0)
+		return ret;
+
+	/*
+	 * The given disk block number (blocknr) is not yet written to
+	 * the device at this point.
+	 *
+	 * To prevent nilfs_dat_translate() from returning the
+	 * uncommitted block number, this makes a copy of the entry
+	 * buffer and redirects nilfs_dat_translate() to the copy.
+	 */
+	if (!buffer_nilfs_redirected(entry_bh)) {
+		ret = nilfs_mdt_freeze_buffer(dat, entry_bh);
+		if (ret) {
+			brelse(entry_bh);
+			return ret;
+		}
+	}
+

fs/nilfs2/dat.c +7 -0

+
+	brelse(entry_bh);
+
+	return 0;
+}
+
+/**
  * nilfs_dat_translate - translate a virtual block number to a block number
  * @dat: DAT file inode
  * @vblocknr: virtual block number

fs/nilfs2/dat.h +1 -0

index 92a187e..a528024 100644
@@ -51,6 +51,7 @@ void nilfs_dat_abort_update(struct inode *, struct nilfs_palloc_req *,
 int nilfs_dat_mark_dirty(struct inode *, __u64);
 int nilfs_dat_freev(struct inode *, __u64 *, size_t);
 int nilfs_dat_move(struct inode *, __u64, sector_t);
+int nilfs_dat_clean_snapshot_flag(struct inode *, __u64);
 ssize_t nilfs_dat_get_vinfo(struct inode *, void *, unsigned, size_t);
 
 int nilfs_dat_read(struct super_block *sb, size_t entry_size,

fs/nilfs2/ioctl.c +1 -1

index 422fb54..0b62bf4 100644
@@ -578,7 +578,7 @@ static int nilfs_ioctl_move_inode_block(struct inode *inode,
 	struct buffer_head *bh;
 	int ret;
 
-	if (vdesc->vd_flags == 0)
+	if (nilfs_vdesc_data(vdesc))
 		ret = nilfs_gccache_submit_read_data(
 			inode, vdesc->vd_offset, vdesc->vd_blocknr,
 			vdesc->vd_vblocknr, &bh);

fs/nilfs2/ioctl.c +91 -0

+ * @inode: inode object
+ * @filp: file object
+ * @cmd: ioctl's request code
+ * @argp: pointer on argument from userspace
+ *
+ * Description: nilfs_ioctl_clean_snapshot_flags() sets DAT entries with de_ss
+ * values of NILFS_CNO_MAX to 0. NILFS_CNO_MAX indicates, that the
+ * corresponding block belongs to some snapshot, but was already decremented.
+ * If the segment usage info is changed with NILFS_IOCTL_SET_SUINFO and the
+ * number of blocks is updated, then these blocks would never be decremented
+ * and there are scenarios where the corresponding segments would starve (never
+ * be cleaned).
+ *
+ * Return Value: On success, 0 is returned or error code, otherwise.
+ */
+static int nilfs_ioctl_clean_snapshot_flags(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;
+	struct nilfs_vdesc *vdesc;
+	size_t len, i;
+	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(struct nilfs_argv)))
+		goto out;
+
+	ret = -EINVAL;
+	if (argv.v_size != sizeof(struct nilfs_vdesc))
+		goto out;
+	if (argv.v_nmembs > UINT_MAX / sizeof(struct nilfs_vdesc))
+		goto out;
+
+	len = argv.v_size * argv.v_nmembs;
+	if (!len) {
+		ret = 0;
+		goto out;
+	}
+
+	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;
+	}
+
+	ret = nilfs_transaction_begin(inode->i_sb, &ti, 0);
+	if (unlikely(ret))
+		goto out_free;
+
+	for (i = 0, vdesc = kbuf; i < argv.v_nmembs; ++i, ++vdesc) {
+		if (nilfs_vdesc_snapshot(vdesc)) {
+			ret = nilfs_dat_clean_snapshot_flag(nilfs->ns_dat,
+					vdesc->vd_vblocknr);
+			if (ret) {
+				nilfs_transaction_abort(inode->i_sb);
+				goto out_free;
+			}
+		}
+	}
+
+	nilfs_transaction_commit(inode->i_sb);
+
+out_free:
+	vfree(kbuf);
+out:
+	mnt_drop_write_file(filp);
+	return ret;
+}
+
+/**
  * nilfs_ioctl_sync - make a checkpoint
  * @inode: inode object
  * @filp: file object

@@ -1332,6 +1430,8 @@ long nilfs_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 		return nilfs_ioctl_get_bdescs(inode, filp, cmd, argp);
 	case NILFS_IOCTL_CLEAN_SEGMENTS:
 		return nilfs_ioctl_clean_segments(inode, filp, cmd, argp);
+	case NILFS_IOCTL_CLEAN_SNAPSHOT_FLAGS:
+		return nilfs_ioctl_clean_snapshot_flags(inode, filp, cmd, argp);
 	case NILFS_IOCTL_SYNC:
 		return nilfs_ioctl_sync(inode, filp, cmd, argp);
 	case NILFS_IOCTL_RESIZE:

include/linux/nilfs2_fs.h +35 -0

+};
+
+#define NILFS_VDESC_FNS(flag, name)					\
+static inline void							\
+nilfs_vdesc_set_##name(struct nilfs_vdesc *vdesc)			\
+{									\
+	vdesc->vd_flags = NILFS_VDESC_##flag;				\
+}									\
+static inline int							\
+nilfs_vdesc_##name(const struct nilfs_vdesc *vdesc)			\
+{									\
+	return vdesc->vd_flags == NILFS_VDESC_##flag;			\
+}
+
+#define NILFS_VDESC_FNS2(flag, name)					\
+static inline void							\
+nilfs_vdesc_set_##name(struct nilfs_vdesc *vdesc)			\
+{									\
+	vdesc->vd_flags2 |= (1UL << NILFS_VDESC_##flag);		\
+}									\
+static inline void							\
+nilfs_vdesc_clear_##name(struct nilfs_vdesc *vdesc)			\
+{									\
+	vdesc->vd_flags2 &= ~(1UL << NILFS_VDESC_##flag);		\
+}									\
+static inline int							\
+nilfs_vdesc_##name(const struct nilfs_vdesc *vdesc)			\
+{									\
+	return !!(vdesc->vd_flags2 & (1UL << NILFS_VDESC_##flag));	\
+}
+
+NILFS_VDESC_FNS(DATA, data)
+NILFS_VDESC_FNS(NODE, node)
+NILFS_VDESC_FNS2(SNAPSHOT, snapshot)
+
 /**
  * struct nilfs_bdesc - descriptor of disk block number
  * @bd_ino: inode number

Quoted 5 lines · Andreas Rohner

This patch introduces new flags for nilfs_vdesc to indicate the reason a block is alive. So if the block would be reclaimable, but must be treated as if it were alive, because it is part of a snapshot, then the snapshot flag is set.

I suppose that I don't quite follow your idea. As far as I can judge, every block in DAT file has: (1) de_start: start checkpoint number; (2)
de_end: end checkpoint number. So, while one of checkpoint number is snapshot number then we know that this block lives in snapshot. Am I correct? Why do we need in special flags?

Quoted 41 lines

Additionally a new ioctl() is added, which enables the userspace GC to perform a cleanup operation after setting the number of blocks with NILFS_IOCTL_SET_SUINFO. It sets DAT entries with de_ss values of NILFS_CNO_MAX to 0. NILFS_CNO_MAX indicates, that the corresponding block belongs to some snapshot, but was already decremented by a previous deletion operation. If the segment usage info is changed with NILFS_IOCTL_SET_SUINFO and the number of blocks is updated, then these blocks would never be decremented and there are scenarios where the corresponding segments would starve (never be cleaned). To prevent that they must be reset to 0.

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

---
 fs/nilfs2/dat.c           |  63 ++++++++++++++++++++++++++++
 fs/nilfs2/dat.h           |   1 +
 fs/nilfs2/ioctl.c         | 103 +++++++++++++++++++++++++++++++++++++++++++++-
 include/linux/nilfs2_fs.h |  52 ++++++++++++++++++++++-
 4 files changed, 216 insertions(+), 3 deletions(-)

fs/nilfs2/dat.c +14 -0

index 89a4a5f..7adb15d 100644
@@ -382,6 +382,69 @@ int nilfs_dat_move(struct inode *dat, __u64 vblocknr, sector_t blocknr)
 }
 
 /**
+ * nilfs_dat_clean_snapshot_flag - check flags used by snapshots
+ * @dat: DAT file inode
+ * @vblocknr: virtual block number
+ *
+ * Description: nilfs_dat_clean_snapshot_flag() changes the flags from
+ * NILFS_CNO_MAX to 0 if necessary, so that segment usage is accurately
+ * counted. NILFS_CNO_MAX indicates, that the corresponding block belongs
+ * to some snapshot, but was already decremented. If the segment usage info
+ * is changed with NILFS_IOCTL_SET_SUINFO and the number of blocks is updated,
+ * then these blocks would never be decremented and there are scenarios where
+ * the corresponding segments would starve (never be cleaned).
+ *
+ * Return Value: On success, 0 is returned. On error, one of the following
+ * negative error codes is returned.

fs/nilfs2/dat.c int nilfs_dat_move(struct inode *dat, __u64 vblocknr, sector_t blocknr) · line 404

Quoted 6 lines

fs/nilfs2/dat.c +6 -0

+ *
+ * %-EIO - I/O error.
+ *
+ * %-ENOMEM - Insufficient amount of memory available.
+ */
+int nilfs_dat_clean_snapshot_flag(struct inode *dat, __u64 vblocknr)

Sounds likewise we clear flag. It can be confusing name.

Quoted 30 lines

fs/nilfs2/dat.c +30 -0

+{
+	struct buffer_head *entry_bh;
+	struct nilfs_dat_entry *entry;
+	void *kaddr;
+	int ret;
+
+	ret = nilfs_palloc_get_entry_block(dat, vblocknr, 0, &entry_bh);
+	if (ret < 0)
+		return ret;
+
+	/*
+	 * The given disk block number (blocknr) is not yet written to
+	 * the device at this point.
+	 *
+	 * To prevent nilfs_dat_translate() from returning the
+	 * uncommitted block number, this makes a copy of the entry
+	 * buffer and redirects nilfs_dat_translate() to the copy.
+	 */
+	if (!buffer_nilfs_redirected(entry_bh)) {
+		ret = nilfs_mdt_freeze_buffer(dat, entry_bh);
+		if (ret) {
+			brelse(entry_bh);
+			return ret;
+		}
+	}
+
+	kaddr = kmap_atomic(entry_bh->b_page);
+	entry = nilfs_palloc_block_get_entry(dat, vblocknr, entry_bh, kaddr);
+	if (entry->de_ss == cpu_to_le64(NILFS_CNO_MAX)) {
+		entry->de_ss = cpu_to_le64(0);

fs/nilfs2/dat.c int nilfs_dat_move(struct inode *dat, __u64 vblocknr, sector_t blocknr) · line 440

Quoted 6 lines

fs/nilfs2/dat.c +6 -0

+		kunmap_atomic(kaddr);
+		mark_buffer_dirty(entry_bh);
+		nilfs_mdt_mark_dirty(dat);
+	} else {
+		kunmap_atomic(kaddr);
+	}

Brackets are unnecessary here.

Quoted 38 lines

fs/nilfs2/dat.c +7 -0

+
+	brelse(entry_bh);
+
+	return 0;
+}
+
+/**
  * nilfs_dat_translate - translate a virtual block number to a block number
  * @dat: DAT file inode
  * @vblocknr: virtual block number

fs/nilfs2/dat.h +1 -0

index 92a187e..a528024 100644
@@ -51,6 +51,7 @@ void nilfs_dat_abort_update(struct inode *, struct nilfs_palloc_req *,
 int nilfs_dat_mark_dirty(struct inode *, __u64);
 int nilfs_dat_freev(struct inode *, __u64 *, size_t);
 int nilfs_dat_move(struct inode *, __u64, sector_t);
+int nilfs_dat_clean_snapshot_flag(struct inode *, __u64);
 ssize_t nilfs_dat_get_vinfo(struct inode *, void *, unsigned, size_t);
 
 int nilfs_dat_read(struct super_block *sb, size_t entry_size,

fs/nilfs2/ioctl.c +1 -1

index 422fb54..0b62bf4 100644
@@ -578,7 +578,7 @@ static int nilfs_ioctl_move_inode_block(struct inode *inode,
 	struct buffer_head *bh;
 	int ret;
 
-	if (vdesc->vd_flags == 0)
+	if (nilfs_vdesc_data(vdesc))
 		ret = nilfs_gccache_submit_read_data(
 			inode, vdesc->vd_offset, vdesc->vd_blocknr,
 			vdesc->vd_vblocknr, &bh);

@@ -662,6 +662,14 @@ static int nilfs_ioctl_move_blocks(struct super_block *sb,
 		}
 

fs/nilfs2/ioctl.c static int nilfs_ioctl_move_blocks(struct super_block *sb, · line 669

Quoted 6 lines

fs/nilfs2/ioctl.c +5 -0

 		do {
+			/*
+			 * old user space tools to not initialize vd_flags2
+			 * check if it contains invalid flags
+			 */
+			if (vdesc->vd_flags2 &

"vd_flags2" is really bad naming. Completely obscure.

fs/nilfs2/ioctl.c static int nilfs_ioctl_move_blocks(struct super_block *sb, · line 670

Quoted 1 line

fs/nilfs2/ioctl.c +1 -0

+					(~0UL << __NR_NILFS_VDESC_FIELDS))

Looks weird.

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

Quoted 10 lines

fs/nilfs2/ioctl.c +3 -0

+				vdesc->vd_flags2 = 0;
+
 			ret = nilfs_ioctl_move_inode_block(inode, vdesc,
 							   &buffers);
 			if (unlikely(ret < 0)) {

@@ -984,6 +992,96 @@ out:
 }
 
 /**
+ * nilfs_ioctl_clean_snapshot_flags - clean dat entries with invalid de_ss

Ditto. Sounds likewise clearing of flag.

Quoted 102 lines

fs/nilfs2/ioctl.c +91 -0

+ * @inode: inode object
+ * @filp: file object
+ * @cmd: ioctl's request code
+ * @argp: pointer on argument from userspace
+ *
+ * Description: nilfs_ioctl_clean_snapshot_flags() sets DAT entries with de_ss
+ * values of NILFS_CNO_MAX to 0. NILFS_CNO_MAX indicates, that the
+ * corresponding block belongs to some snapshot, but was already decremented.
+ * If the segment usage info is changed with NILFS_IOCTL_SET_SUINFO and the
+ * number of blocks is updated, then these blocks would never be decremented
+ * and there are scenarios where the corresponding segments would starve (never
+ * be cleaned).
+ *
+ * Return Value: On success, 0 is returned or error code, otherwise.
+ */
+static int nilfs_ioctl_clean_snapshot_flags(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;
+	struct nilfs_vdesc *vdesc;
+	size_t len, i;
+	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(struct nilfs_argv)))
+		goto out;
+
+	ret = -EINVAL;
+	if (argv.v_size != sizeof(struct nilfs_vdesc))
+		goto out;
+	if (argv.v_nmembs > UINT_MAX / sizeof(struct nilfs_vdesc))
+		goto out;
+
+	len = argv.v_size * argv.v_nmembs;
+	if (!len) {
+		ret = 0;
+		goto out;
+	}
+
+	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;
+	}
+
+	ret = nilfs_transaction_begin(inode->i_sb, &ti, 0);
+	if (unlikely(ret))
+		goto out_free;
+
+	for (i = 0, vdesc = kbuf; i < argv.v_nmembs; ++i, ++vdesc) {
+		if (nilfs_vdesc_snapshot(vdesc)) {
+			ret = nilfs_dat_clean_snapshot_flag(nilfs->ns_dat,
+					vdesc->vd_vblocknr);
+			if (ret) {
+				nilfs_transaction_abort(inode->i_sb);
+				goto out_free;
+			}
+		}
+	}
+
+	nilfs_transaction_commit(inode->i_sb);
+
+out_free:
+	vfree(kbuf);
+out:
+	mnt_drop_write_file(filp);
+	return ret;
+}
+
+/**
  * nilfs_ioctl_sync - make a checkpoint
  * @inode: inode object
  * @filp: file object

@@ -1332,6 +1430,8 @@ long nilfs_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 		return nilfs_ioctl_get_bdescs(inode, filp, cmd, argp);
 	case NILFS_IOCTL_CLEAN_SEGMENTS:
 		return nilfs_ioctl_clean_segments(inode, filp, cmd, argp);
+	case NILFS_IOCTL_CLEAN_SNAPSHOT_FLAGS:
+		return nilfs_ioctl_clean_snapshot_flags(inode, filp, cmd, argp);
 	case NILFS_IOCTL_SYNC:
 		return nilfs_ioctl_sync(inode, filp, cmd, argp);
 	case NILFS_IOCTL_RESIZE:

fs/nilfs2/ioctl.c long nilfs_compat_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) · line 1471

Quoted 4 lines

fs/nilfs2/ioctl.c +1 -0

@@ -1368,6 +1468,7 @@ long nilfs_compat_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 	case NILFS_IOCTL_GET_VINFO:
 	case NILFS_IOCTL_GET_BDESCS:
 	case NILFS_IOCTL_CLEAN_SEGMENTS:
+	case NILFS_IOCTL_CLEAN_SNAPSHOT_FLAGS:

Sounds for me that we clean all snapshot's flags.

include/linux/nilfs2_fs.h struct nilfs_vinfo { · line 866

Quoted 13 lines

fs/nilfs2/ioctl.c +0 -0

 	case NILFS_IOCTL_SYNC:
 	case NILFS_IOCTL_RESIZE:
 	case NILFS_IOCTL_SET_ALLOC_RANGE:

include/linux/nilfs2_fs.h +1 -1

index ba9ebe02..30ddc86 100644
@@ -863,7 +863,7 @@ struct nilfs_vinfo {
  * @vd_blocknr: disk block number
  * @vd_offset: logical block offset inside a file
  * @vd_flags: flags (data or node block)
- * @vd_pad: padding
+ * @vd_flags2: additional flags

Ditto. Weird name.

include/linux/nilfs2_fs.h struct nilfs_vdesc { · line 876

Quoted 9 lines

include/linux/nilfs2_fs.h +1 -1

  */
 struct nilfs_vdesc {
 	__u64 vd_ino;

@@ -873,9 +873,55 @@ struct nilfs_vdesc {
 	__u64 vd_blocknr;
 	__u64 vd_offset;
 	__u32 vd_flags;
-	__u32 vd_pad;
+	/* vd_flags2 needed because of backwards compatibility */

Completely, misunderstand comment. Usually, it keeps old fields for backward compatibility. But this flag is new.

include/linux/nilfs2_fs.h struct nilfs_vdesc { · line 880

Quoted 4 lines

include/linux/nilfs2_fs.h +2 -0

+	__u32 vd_flags2;
 };
 
+/* vdesc flags */

To be honest, I misunderstand why such number of flags and why namely such flags? Comments are really necessary.

include/linux/nilfs2_fs.h struct nilfs_vdesc { · line 884

Quoted 4 lines

include/linux/nilfs2_fs.h +4 -0

+enum {
+	NILFS_VDESC_DATA,
+	NILFS_VDESC_NODE,
+	/* ... */

What does it mean?

include/linux/nilfs2_fs.h struct nilfs_vdesc { · line 889

Quoted 5 lines

include/linux/nilfs2_fs.h +5 -0

+};
+enum {
+	NILFS_VDESC_SNAPSHOT,
+	__NR_NILFS_VDESC_FIELDS,
+	/* ... */

What does it mean?

Quoted 13 lines

include/linux/nilfs2_fs.h struct nilfs_vdesc { · line 876

Quoted 10 lines

*/
struct nilfs_vdesc {

include/linux/nilfs2_fs.h +1 -1

 	__u64 vd_ino;

@@ -873,9 +873,55 @@ struct nilfs_vdesc {
 	__u64 vd_blocknr;
 	__u64 vd_offset;
 	__u32 vd_flags;
-	__u32 vd_pad;
+	/* vd_flags2 needed because of backwards compatibility */

Completely, misunderstand comment. Usually, it keeps old fields for backward compatibility. But this flag is new.

I will rewrite the comment. I need vd_flags2 because I can't use vd_flags because of backwards compatibility.

Quoted 1 line

include/linux/nilfs2_fs.h struct nilfs_vdesc { · line 877

Quoted 1 line

include/linux/nilfs2_fs.h +1 -0

+	__u32 vd_flags2;

Quoted 7 lines

include/linux/nilfs2_fs.h struct nilfs_vdesc { · line 880

Quoted 3 lines

include/linux/nilfs2_fs.h +1 -0

 };
 
+/* vdesc flags */

To be honest, I misunderstand why such number of flags and why namely such flags? Comments are really necessary.

Quoted 6 lines

include/linux/nilfs2_fs.h struct nilfs_vdesc { · line 884

Quoted 4 lines

include/linux/nilfs2_fs.h +4 -0

+enum {
+	NILFS_VDESC_DATA,
+	NILFS_VDESC_NODE,
+	/* ... */

What does it mean?

NILFS_VDESC_DATA = 0 and NILFS_VDESC_NODE = 1. This represents the type of block. These two already existed, in the previous version, but they were not explicit. See "[Patch 4/4] nilfs-utils: add extra flags to nilfs_vdesc and update sui_nblocks":

(quoted diff) +2 -2

@@ -148,17 +149,19 @@ static int nilfs_acc_blocks_file(struct nilfs_file
 *file,
-				vdesc->vd_flags = 0;	/* data */
+				nilfs_vdesc_set_data(vdesc);
 			} else {
 				vdesc->vd_vblocknr =
 					le64_to_cpu(*(__le64 *)blk.b_binfo);
-				vdesc->vd_flags = 1;	/* node */
+				nilfs_vdesc_set_node(vdesc);
 			}
 
 >> +};
 >> +enum {
 >> +	NILFS_VDESC_SNAPSHOT,
 >> +	__NR_NILFS_VDESC_FIELDS,

Quoted 3 lines

Review struct nilfs_vdesc { · line 885

Quoted 1 line

(quoted diff) +1 -0

+	/* ... */

What does it mean?

Quoted 14 lines · Andreas Rohner

Quoted 13 lines

include/linux/nilfs2_fs.h struct nilfs_vdesc { · line 876

Quoted 10 lines

*/
struct nilfs_vdesc {

include/linux/nilfs2_fs.h +1 -1

 	__u64 vd_ino;

@@ -873,9 +873,55 @@ struct nilfs_vdesc {
 	__u64 vd_blocknr;
 	__u64 vd_offset;
 	__u32 vd_flags;
-	__u32 vd_pad;
+	/* vd_flags2 needed because of backwards compatibility */

Completely, misunderstand comment. Usually, it keeps old fields for backward compatibility. But this flag is new.

Quoted 4 lines

I will rewrite the comment. I need vd_flags2 because I can't use vd_flags because of backwards compatibility.

Quoted 1 line

include/linux/nilfs2_fs.h struct nilfs_vdesc { · line 877

Quoted 1 line

include/linux/nilfs2_fs.h +1 -0

+	__u32 vd_flags2;

What about vd_blk_state instead of vd_flags2?

Quoted 34 lines

Quoted 7 lines

include/linux/nilfs2_fs.h struct nilfs_vdesc { · line 880

Quoted 3 lines

include/linux/nilfs2_fs.h +1 -0

 };
 
+/* vdesc flags */

To be honest, I misunderstand why such number of flags and why namely such flags? Comments are really necessary.

Quoted 6 lines

include/linux/nilfs2_fs.h struct nilfs_vdesc { · line 884

Quoted 4 lines

include/linux/nilfs2_fs.h +4 -0

+enum {
+	NILFS_VDESC_DATA,
+	NILFS_VDESC_NODE,
+	/* ... */

What does it mean?

NILFS_VDESC_DATA = 0 and NILFS_VDESC_NODE = 1. This represents the type of block. These two already existed, in the previous version, but they were not explicit. See "[Patch 4/4] nilfs-utils: add extra flags to nilfs_vdesc and update sui_nblocks":

(quoted diff) +2 -2

@@ -148,17 +149,19 @@ static int nilfs_acc_blocks_file(struct nilfs_file
 *file,
-				vdesc->vd_flags = 0;	/* data */
+				nilfs_vdesc_set_data(vdesc);
 			} else {
 				vdesc->vd_vblocknr =
 					le64_to_cpu(*(__le64 *)blk.b_binfo);
-				vdesc->vd_flags = 1;	/* node */
+				nilfs_vdesc_set_node(vdesc);
 			}
 
 >> +};
 >> +enum {
 >> +	NILFS_VDESC_SNAPSHOT,
 >> +	__NR_NILFS_VDESC_FIELDS,

Quoted 3 lines

Quoted 3 lines

Review struct nilfs_vdesc { · line 885

Quoted 1 line

(quoted diff) +1 -0

+	/* ... */

What does it mean?

I asked here about strange comment. What does it mean?

Moreover, I slightly confused by NILFS_VDESC_SNAPSHOT. Is it bit-based flag? I mean NILFS_VDESC_SNAPSHOT = (1 << 0). Or am I incorrect?