ocfs2: Add batched discard support

From: Tao Ma <[email protected]>

Add ocfs2_trim_fs to support trimming freed clusters in the volume. A range will be given and all the freed clusters greater than minlen will be discarded to the block layer.

Signed-off-by Tao Ma <[email protected]>

---
 fs/ocfs2/alloc.c |  154 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 fs/ocfs2/alloc.h |    1 +
 2 files changed, 155 insertions(+), 0 deletions(-)

fs/ocfs2/alloc.c +16 -0

index b27a0d8..6e1b3b5 100644
@@ -29,6 +29,7 @@
 #include <linux/highmem.h>
 #include <linux/swap.h>
 #include <linux/quotaops.h>
+#include <linux/blkdev.h>
 
 #include <cluster/masklog.h>
 

@@ -7184,3 +7185,156 @@ out_commit:
 out:
 	return ret;
 }
+
+static int ocfs2_trim_extent(struct super_block *sb,
+			     struct ocfs2_group_desc *gd,
+			     int start, int count)
+{
+	u64 discard;
+
+	count = ocfs2_clusters_to_blocks(sb, count);
+	discard = le64_to_cpu(gd->bg_blkno) +
+			ocfs2_clusters_to_blocks(sb, start);
+
+	return sb_issue_discard(sb, discard, count, GFP_NOFS, 0);
+}
+
+static int ocfs2_trim_group(struct super_block *sb,

fs/ocfs2/alloc.c +21 -0

+
+		if ((next - start) >= minbits) {
+			ret = ocfs2_trim_extent(sb, gd,
+						start, next - start);
+			if (ret < 0) {
+				mlog_errno(ret);
+				break;
+			}
+			count += next - start;
+		}
+		start = next + 1;
+
+		if (fatal_signal_pending(current)) {
+			count = -ERESTARTSYS;
+			break;
+		}
+
+		if ((le16_to_cpu(gd->bg_free_bits_count) - count) < minbits)
+			break;
+	}
+

fs/ocfs2/alloc.c +31 -0

+		return -EINVAL;
+
+	main_bm_inode = ocfs2_get_system_file_inode(osb,
+						    GLOBAL_BITMAP_SYSTEM_INODE,
+						    OCFS2_INVALID_SLOT);
+	if (!main_bm_inode) {
+		ret = -EIO;
+		mlog_errno(ret);
+		goto out;
+	}
+
+	mutex_lock(&main_bm_inode->i_mutex);
+
+	ret = ocfs2_inode_lock(main_bm_inode, &main_bm_bh, 0);
+	if (ret < 0) {
+		mlog_errno(ret);
+		goto out_mutex;
+	}
+	main_bm = (struct ocfs2_dinode *)main_bm_bh->b_data;
+
+	if (start >= le32_to_cpu(main_bm->i_clusters)) {
+		ret = -EINVAL;
+		mlog_errno(ret);
+		goto out_unlock;
+	}
+
+	if (start + len > le32_to_cpu(main_bm->i_clusters))
+		len = le32_to_cpu(main_bm->i_clusters) - start;
+
+	/* Determine first and last group to examine based on start and len */
+	first_group = ocfs2_which_cluster_group(main_bm_inode, start);

fs/ocfs2/alloc.c +37 -0

+
+		ret = ocfs2_read_group_descriptor(main_bm_inode,
+						  main_bm, group,
+						  &gd_bh);
+		if (ret < 0) {
+			mlog_errno(ret);
+			break;
+		}
+
+		gd = (struct ocfs2_group_desc *)gd_bh->b_data;
+		cnt = ocfs2_trim_group(sb, gd, first_bit, last_bit, minlen);
+		brelse(gd_bh);
+		gd_bh = NULL;
+		if (cnt < 0) {
+			ret = cnt;
+			mlog_errno(ret);
+			break;
+		}
+
+		trimmed += cnt;
+		len -= osb->bitmap_cpg - first_bit;
+		first_bit = 0;
+		if (group == osb->first_cluster_group_blkno)
+			group = ocfs2_clusters_to_blocks(sb, osb->bitmap_cpg);
+		else
+			group += ocfs2_clusters_to_blocks(sb, osb->bitmap_cpg);
+	}
+	range->len = trimmed * sb->s_blocksize;
+out_unlock:
+	ocfs2_inode_unlock(main_bm_inode, 0);
+	brelse(main_bm_bh);
+out_mutex:
+	mutex_unlock(&main_bm_inode->i_mutex);
+	iput(main_bm_inode);
+out:
+	return ret;
+}

fs/ocfs2/alloc.h +1 -0

index 3bd08a0..ca381c5 100644
@@ -239,6 +239,7 @@ int ocfs2_find_leaf(struct ocfs2_caching_info *ci,
 		    struct buffer_head **leaf_bh);
 int ocfs2_search_extent_list(struct ocfs2_extent_list *el, u32 v_cluster);
 
+int ocfs2_trim_fs(struct super_block *sb, struct fstrim_range *range);
 /*
  * Helper function to look at the # of clusters in an extent record.
  */

Hi Tao,

Most of codes looks pretty neat to me, few comments inlined below:

Quoted 44 lines · Tao Ma

From: Tao Ma <[email protected]>

Add ocfs2_trim_fs to support trimming freed clusters in the volume. A range will be given and all the freed clusters greater than minlen will be discarded to the block layer.

Signed-off-by Tao Ma <[email protected]>

---
 fs/ocfs2/alloc.c |  154
++++++++++++++++++++++++++++++++++++++++++++++++++++++
 fs/ocfs2/alloc.h |    1 +
 2 files changed, 155 insertions(+), 0 deletions(-)

fs/ocfs2/alloc.c +16 -0

index b27a0d8..6e1b3b5 100644
@@ -29,6 +29,7 @@
 #include <linux/highmem.h>
 #include <linux/swap.h>
 #include <linux/quotaops.h>
+#include <linux/blkdev.h>
 
 #include <cluster/masklog.h>
 

@@ -7184,3 +7185,156 @@ out_commit:
 out:
     return ret;
 }
+
+static int ocfs2_trim_extent(struct super_block *sb,
+                 struct ocfs2_group_desc *gd,
+                 int start, int count)
+{
+    u64 discard;
+
+    count = ocfs2_clusters_to_blocks(sb, count);
+    discard = le64_to_cpu(gd->bg_blkno) +
+            ocfs2_clusters_to_blocks(sb, start);
+
+    return sb_issue_discard(sb, discard, count, GFP_NOFS, 0);
+}
+
+static int ocfs2_trim_group(struct super_block *sb,

fs/ocfs2/alloc.c out_commit: · line 7212

Quoted 10 lines

fs/ocfs2/alloc.c +10 -0

+                struct ocfs2_group_desc *gd,
+                int start, int max, int minbits)
+{
+    int ret = 0, count = 0, next;
+    void *bitmap = gd->bg_bitmap;
+
+    while (start < max) {
+        start = ocfs2_find_next_zero_bit(bitmap, max, start);
+        if (start >= max)
+            break;

   /* What if the 'start' stands within a hole */

   if (ocfs2_test_bit(...)) {
      start = ocfs2_find_next_zero_bit(...);
      if ((start == -1) || (start >= max))
         break;
   }

fs/ocfs2/alloc.c out_commit: · line 7213

Quoted 1 line

fs/ocfs2/alloc.c +1 -0

+        next = ocfs2_find_next_bit(bitmap, max, start);

     next = ocfs2_find_next_bit(...);
   if (next == -1)
      break;

   if (next > max)
      next = max;

Quoted 25 lines

fs/ocfs2/alloc.c +24 -0

+
+        if ((next - start) >= minbits) {
+            ret = ocfs2_trim_extent(sb, gd,
+                        start, next - start);
+            if (ret < 0) {
+                mlog_errno(ret);
+                break;
+            }
+            count += next - start;
+        }
+        start = next + 1;
+
+        if (fatal_signal_pending(current)) {
+            count = -ERESTARTSYS;
+            break;
+        }
+
+        if ((le16_to_cpu(gd->bg_free_bits_count) - count) < minbits)
+            break;
+    }
+
+    if (ret < 0)
+        count = ret;
+

fs/ocfs2/alloc.c out_commit: · line 7244

Quoted 7 lines

fs/ocfs2/alloc.c +7 -0

+    return count;
+}
+
+int ocfs2_trim_fs(struct super_block *sb, struct fstrim_range *range)
+{
+    struct ocfs2_super *osb = OCFS2_SB(sb);
+    u64 start, len, minlen, trimmed, first_group, last_group, group;

why not using u32 start, len, minlen, trimmed;

fs/ocfs2/alloc.c out_commit: · line 7257

Quoted 13 lines

fs/ocfs2/alloc.c +13 -0

+    int ret, cnt, first_bit, last_bit;
+    struct buffer_head *main_bm_bh = NULL;
+    struct inode *main_bm_inode = NULL;
+    struct buffer_head *gd_bh = NULL;
+    struct ocfs2_dinode *main_bm;
+    struct ocfs2_group_desc *gd = NULL;
+
+    if (ocfs2_is_hard_readonly(osb) || ocfs2_is_soft_readonly(osb))
+        return -EROFS;
+
+    start = range->start >> osb->s_clustersize_bits;
+    len = range->len >> osb->s_clustersize_bits;
+    minlen = range->minlen >> osb->s_clustersize_bits;

I guess you may want to count two corner clusters which cover the
'start' and 'end' bytes,
so the appropriate way might be:

   start = range->start >> osb->s_clustersize_bits;
   len = ocfs2_clusters_for_bytes(osb->sb, range->start  + range->len);
   len -= start;

fs/ocfs2/alloc.c out_commit: · line 7260

Quoted 4 lines

fs/ocfs2/alloc.c +3 -0

+    trimmed = 0;
+
+    if (!len || !minlen || minlen >= osb->bitmap_cpg)

'minlen == 0' looks acceptable, which means we allowing discarding
for all size of extents.
and what's more, 'len == 0' may not be harmful enough to issue a 'EINVAL', returning a legal '0'
to userspace immediately is fine.

Quoted 35 lines

fs/ocfs2/alloc.c +30 -0

+        return -EINVAL;
+
+    main_bm_inode = ocfs2_get_system_file_inode(osb,
+                            GLOBAL_BITMAP_SYSTEM_INODE,
+                            OCFS2_INVALID_SLOT);
+    if (!main_bm_inode) {
+        ret = -EIO;
+        mlog_errno(ret);
+        goto out;
+    }
+
+    mutex_lock(&main_bm_inode->i_mutex);
+
+    ret = ocfs2_inode_lock(main_bm_inode, &main_bm_bh, 0);
+    if (ret < 0) {
+        mlog_errno(ret);
+        goto out_mutex;
+    }
+    main_bm = (struct ocfs2_dinode *)main_bm_bh->b_data;
+
+    if (start >= le32_to_cpu(main_bm->i_clusters)) {
+        ret = -EINVAL;
+        mlog_errno(ret);
+        goto out_unlock;
+    }
+
+    if (start + len > le32_to_cpu(main_bm->i_clusters))
+        len = le32_to_cpu(main_bm->i_clusters) - start;
+
+    /* Determine first and last group to examine based on start and

len */

fs/ocfs2/alloc.c +2 -0

+    first_group = ocfs2_which_cluster_group(main_bm_inode, start);
+    if (first_group == osb->first_cluster_group_blkno)

fs/ocfs2/alloc.c out_commit: · line 7301

Quoted 10 lines

fs/ocfs2/alloc.c +9 -1

+        first_bit = start;
+    else
+        first_bit = start - ocfs2_blocks_to_clusters(sb, first_group);
+    last_group = ocfs2_which_cluster_group(main_bm_inode, start + len
- 1);
+    last_bit = osb->bitmap_cpg;
+
+    for (group = first_group; group <= last_group;) {
+        if (first_bit + len >= osb->bitmap_cpg)
+            last_bit = osb->bitmap_cpg - first_bit;

is 'first_bit' and 'last_bit' both represent a local offset within a
cluster group?
just wondering why last_bit wasn't equal to 'osb->bitmap_cpg' in above case(I meant the case
of 'first_bit + len >= osb->bitmap_cpg'

fs/ocfs2/alloc.c

Quoted 2 lines

fs/ocfs2/alloc.c +2 -0

+        else
+            last_bit = start + len;

   why above case is not 'last_bit = first_bit + len';

Quoted 3 lines · Tristan Ye

Hi Tao,

Most of codes looks pretty neat to me, few comments inlined below:

Thanks for the review.

Quoted 64 lines

Quoted 44 lines · Tao Ma

From: Tao Ma <[email protected]>

Add ocfs2_trim_fs to support trimming freed clusters in the volume. A range will be given and all the freed clusters greater than minlen will be discarded to the block layer.

Signed-off-by Tao Ma <[email protected]>

---
 fs/ocfs2/alloc.c |  154
++++++++++++++++++++++++++++++++++++++++++++++++++++++
 fs/ocfs2/alloc.h |    1 +
 2 files changed, 155 insertions(+), 0 deletions(-)

fs/ocfs2/alloc.c +16 -0

index b27a0d8..6e1b3b5 100644
@@ -29,6 +29,7 @@
 #include <linux/highmem.h>
 #include <linux/swap.h>
 #include <linux/quotaops.h>
+#include <linux/blkdev.h>
 
 #include <cluster/masklog.h>
 

@@ -7184,3 +7185,156 @@ out_commit:
 out:
     return ret;
 }
+
+static int ocfs2_trim_extent(struct super_block *sb,
+                 struct ocfs2_group_desc *gd,
+                 int start, int count)
+{
+    u64 discard;
+
+    count = ocfs2_clusters_to_blocks(sb, count);
+    discard = le64_to_cpu(gd->bg_blkno) +
+            ocfs2_clusters_to_blocks(sb, start);
+
+    return sb_issue_discard(sb, discard, count, GFP_NOFS, 0);
+}
+
+static int ocfs2_trim_group(struct super_block *sb,

fs/ocfs2/alloc.c out_commit: · line 7212

Quoted 10 lines

fs/ocfs2/alloc.c +10 -0

+                struct ocfs2_group_desc *gd,
+                int start, int max, int minbits)
+{
+    int ret = 0, count = 0, next;
+    void *bitmap = gd->bg_bitmap;
+
+    while (start < max) {
+        start = ocfs2_find_next_zero_bit(bitmap, max, start);
+        if (start >= max)
+            break;

/* What if the 'start' stands within a hole */

   if (ocfs2_test_bit(...)) {
      start = ocfs2_find_next_zero_bit(...);
      if ((start == -1) || (start >= max))
         break;
   }

Quoted 1 line

fs/ocfs2/alloc.c +1 -0

+        next = ocfs2_find_next_bit(bitmap, max, start);

Quoted 3 lines

     next = ocfs2_find_next_bit(...);
   if (next == -1)
      break;

next will be set to "-1"? sorry, but where do you get it?

Quoted 2 lines

   if (next > max)
      next = max;

again, ocfs2_find_next_bit will return a value larger than 'max'? I am afraid not. Otherwise, it will be nonsense to pass a 'max' to it.

Quoted 25 lines

Quoted 25 lines

fs/ocfs2/alloc.c +24 -0

+
+        if ((next - start) >= minbits) {
+            ret = ocfs2_trim_extent(sb, gd,
+                        start, next - start);
+            if (ret < 0) {
+                mlog_errno(ret);
+                break;
+            }
+            count += next - start;
+        }
+        start = next + 1;
+
+        if (fatal_signal_pending(current)) {
+            count = -ERESTARTSYS;
+            break;
+        }
+
+        if ((le16_to_cpu(gd->bg_free_bits_count) - count) < minbits)
+            break;
+    }
+
+    if (ret < 0)
+        count = ret;
+

Quoted 8 lines

fs/ocfs2/alloc.c out_commit: · line 7244

Quoted 7 lines

fs/ocfs2/alloc.c +7 -0

+    return count;
+}
+
+int ocfs2_trim_fs(struct super_block *sb, struct fstrim_range *range)
+{
+    struct ocfs2_super *osb = OCFS2_SB(sb);
+    u64 start, len, minlen, trimmed, first_group, last_group, group;

why not using u32 start, len, minlen, trimmed;

we may use 64 bit clusters later I guess. And what's more, they will be set by the user later. and it may overflow. Say the user pass a u64 range->len, it will overflow with range->len >> osb->s_clustersize_bits.

Quoted 17 lines

fs/ocfs2/alloc.c out_commit: · line 7257

Quoted 13 lines

fs/ocfs2/alloc.c +13 -0

+    int ret, cnt, first_bit, last_bit;
+    struct buffer_head *main_bm_bh = NULL;
+    struct inode *main_bm_inode = NULL;
+    struct buffer_head *gd_bh = NULL;
+    struct ocfs2_dinode *main_bm;
+    struct ocfs2_group_desc *gd = NULL;
+
+    if (ocfs2_is_hard_readonly(osb) || ocfs2_is_soft_readonly(osb))
+        return -EROFS;
+
+    start = range->start >> osb->s_clustersize_bits;
+    len = range->len >> osb->s_clustersize_bits;
+    minlen = range->minlen >> osb->s_clustersize_bits;

I guess you may want to count two corner clusters which cover the
'start' and 'end' bytes,
so the appropriate way might be:

Quoted 3 lines

   start = range->start >> osb->s_clustersize_bits;
   len = ocfs2_clusters_for_bytes(osb->sb, range->start  + range->len);
   len -= start;

No, I don't want that.. Just want to make it the same as what ext4 did. See ext4_trim_fs for more details.

Quoted 9 lines

fs/ocfs2/alloc.c out_commit: · line 7260

Quoted 4 lines

fs/ocfs2/alloc.c +3 -0

+    trimmed = 0;
+
+    if (!len || !minlen || minlen >= osb->bitmap_cpg)

'minlen == 0' looks acceptable, which means we allowing discarding
for all size of extents.
and what's more, 'len == 0' may not be harmful enough to issue a 'EINVAL', returning a legal '0'
to userspace immediately is fine.

Fair enough. I will change it. Thanks.

Quoted 50 lines

Quoted 34 lines

fs/ocfs2/alloc.c +30 -0

+        return -EINVAL;
+
+    main_bm_inode = ocfs2_get_system_file_inode(osb,
+                            GLOBAL_BITMAP_SYSTEM_INODE,
+                            OCFS2_INVALID_SLOT);
+    if (!main_bm_inode) {
+        ret = -EIO;
+        mlog_errno(ret);
+        goto out;
+    }
+
+    mutex_lock(&main_bm_inode->i_mutex);
+
+    ret = ocfs2_inode_lock(main_bm_inode, &main_bm_bh, 0);
+    if (ret < 0) {
+        mlog_errno(ret);
+        goto out_mutex;
+    }
+    main_bm = (struct ocfs2_dinode *)main_bm_bh->b_data;
+
+    if (start >= le32_to_cpu(main_bm->i_clusters)) {
+        ret = -EINVAL;
+        mlog_errno(ret);
+        goto out_unlock;
+    }
+
+    if (start + len > le32_to_cpu(main_bm->i_clusters))
+        len = le32_to_cpu(main_bm->i_clusters) - start;
+
+    /* Determine first and last group to examine based on start and

len */

fs/ocfs2/alloc.c +2 -0

+    first_group = ocfs2_which_cluster_group(main_bm_inode, start);
+    if (first_group == osb->first_cluster_group_blkno)

fs/ocfs2/alloc.c out_commit: · line 7301

Quoted 10 lines

fs/ocfs2/alloc.c +9 -1

+        first_bit = start;
+    else
+        first_bit = start - ocfs2_blocks_to_clusters(sb, first_group);
+    last_group = ocfs2_which_cluster_group(main_bm_inode, start + len
- 1);
+    last_bit = osb->bitmap_cpg;
+
+    for (group = first_group; group <= last_group;) {
+        if (first_bit + len >= osb->bitmap_cpg)
+            last_bit = osb->bitmap_cpg - first_bit;

is 'first_bit' and 'last_bit' both represent a local offset within a
cluster group?
just wondering why last_bit wasn't equal to 'osb->bitmap_cpg' in above case(I meant the case
of 'first_bit + len >= osb->bitmap_cpg'

Quoted 3 lines

fs/ocfs2/alloc.c

Quoted 2 lines

fs/ocfs2/alloc.c +2 -0

+        else
+            last_bit = start + len;

why above case is not 'last_bit = first_bit + len';

you are right. Thanks.

Regards,
Tao

Quoted 69 lines · Tao Ma

Quoted 3 lines · Tristan Ye

Hi Tao,

Most of codes looks pretty neat to me, few comments inlined below:

Thanks for the review.

Quoted 64 lines

Quoted 44 lines · Tao Ma

From: Tao Ma <[email protected]>

Add ocfs2_trim_fs to support trimming freed clusters in the volume. A range will be given and all the freed clusters greater than minlen will be discarded to the block layer.

Signed-off-by Tao Ma <[email protected]>

---
 fs/ocfs2/alloc.c |  154
++++++++++++++++++++++++++++++++++++++++++++++++++++++
 fs/ocfs2/alloc.h |    1 +
 2 files changed, 155 insertions(+), 0 deletions(-)

fs/ocfs2/alloc.c +16 -0

index b27a0d8..6e1b3b5 100644
@@ -29,6 +29,7 @@
 #include <linux/highmem.h>
 #include <linux/swap.h>
 #include <linux/quotaops.h>
+#include <linux/blkdev.h>
 
 #include <cluster/masklog.h>
 

@@ -7184,3 +7185,156 @@ out_commit:
 out:
     return ret;
 }
+
+static int ocfs2_trim_extent(struct super_block *sb,
+                 struct ocfs2_group_desc *gd,
+                 int start, int count)
+{
+    u64 discard;
+
+    count = ocfs2_clusters_to_blocks(sb, count);
+    discard = le64_to_cpu(gd->bg_blkno) +
+            ocfs2_clusters_to_blocks(sb, start);
+
+    return sb_issue_discard(sb, discard, count, GFP_NOFS, 0);
+}
+
+static int ocfs2_trim_group(struct super_block *sb,

fs/ocfs2/alloc.c out_commit: · line 7212

Quoted 10 lines

fs/ocfs2/alloc.c +10 -0

+                struct ocfs2_group_desc *gd,
+                int start, int max, int minbits)
+{
+    int ret = 0, count = 0, next;
+    void *bitmap = gd->bg_bitmap;
+
+    while (start < max) {
+        start = ocfs2_find_next_zero_bit(bitmap, max, start);
+        if (start >= max)
+            break;

/* What if the 'start' stands within a hole */

   if (ocfs2_test_bit(...)) {
      start = ocfs2_find_next_zero_bit(...);
      if ((start == -1) || (start >= max))
         break;
   }

Quoted 1 line

fs/ocfs2/alloc.c +1 -0

+        next = ocfs2_find_next_bit(bitmap, max, start);

Quoted 8 lines

Quoted 3 lines

     next = ocfs2_find_next_bit(...);
   if (next == -1)
      break;

next will be set to "-1"? sorry, but where do you get it?

Quoted 2 lines

   if (next > max)
      next = max;

again, ocfs2_find_next_bit will return a value larger than 'max'? I am afraid not. Otherwise, it will be nonsense to pass a 'max' to it.

Say we're handling the last group, and the 'start + len' was within a hole, then the 'max'
is 'first_bit + len', while the next none-zero bit we found may be larger than 'max', isn't
that possible?

Quoted 8 lines

Quoted 5 lines

fs/ocfs2/alloc.c out_commit: · line 7244

Quoted 4 lines

fs/ocfs2/alloc.c +4 -0

+int ocfs2_trim_fs(struct super_block *sb, struct fstrim_range *range)
+{
+    struct ocfs2_super *osb = OCFS2_SB(sb);
+    u64 start, len, minlen, trimmed, first_group, last_group, group;

why not using u32 start, len, minlen, trimmed;

we may use 64 bit clusters later I guess. And what's more, they will be set by the user later. and it may overflow. Say the user pass a u64 range->len, it will overflow with range->len >> osb->s_clustersize_bits.

I just found we were using u32 for counting clusters all around ocfs2 codes, e.g truncate/punching_hole
codes, also passing an u64 byte_offset from userspace, so my original intention is to keep an unification;-)

Overflow can theoretically happen anyway, however, it's not very likely to pass a 16TB+ byte_offset from userspace.

Quoted 76 lines · Tristan Ye

Quoted 75 lines · Tao Ma

Quoted 3 lines · Tristan Ye

Hi Tao,

Most of codes looks pretty neat to me, few comments inlined below:

Thanks for the review.

Quoted 63 lines

Quoted 44 lines · Tao Ma

From: Tao Ma <[email protected]>

Add ocfs2_trim_fs to support trimming freed clusters in the volume. A range will be given and all the freed clusters greater than minlen will be discarded to the block layer.

Signed-off-by Tao Ma <[email protected]>

---
 fs/ocfs2/alloc.c |  154
++++++++++++++++++++++++++++++++++++++++++++++++++++++
 fs/ocfs2/alloc.h |    1 +
 2 files changed, 155 insertions(+), 0 deletions(-)

fs/ocfs2/alloc.c +16 -0

index b27a0d8..6e1b3b5 100644
@@ -29,6 +29,7 @@
 #include <linux/highmem.h>
 #include <linux/swap.h>
 #include <linux/quotaops.h>
+#include <linux/blkdev.h>
 
 #include <cluster/masklog.h>
 

@@ -7184,3 +7185,156 @@ out_commit:
 out:
     return ret;
 }
+
+static int ocfs2_trim_extent(struct super_block *sb,
+                 struct ocfs2_group_desc *gd,
+                 int start, int count)
+{
+    u64 discard;
+
+    count = ocfs2_clusters_to_blocks(sb, count);
+    discard = le64_to_cpu(gd->bg_blkno) +
+            ocfs2_clusters_to_blocks(sb, start);
+
+    return sb_issue_discard(sb, discard, count, GFP_NOFS, 0);
+}
+
+static int ocfs2_trim_group(struct super_block *sb,

fs/ocfs2/alloc.c out_commit: · line 7212

Quoted 10 lines

fs/ocfs2/alloc.c +10 -0

+                struct ocfs2_group_desc *gd,
+                int start, int max, int minbits)
+{
+    int ret = 0, count = 0, next;
+    void *bitmap = gd->bg_bitmap;
+
+    while (start < max) {
+        start = ocfs2_find_next_zero_bit(bitmap, max, start);
+        if (start >= max)
+            break;

/* What if the 'start' stands within a hole */

   if (ocfs2_test_bit(...)) {
      start = ocfs2_find_next_zero_bit(...);
      if ((start == -1) || (start >= max))
         break;
   }

Quoted 4 lines

fs/ocfs2/alloc.c out_commit: · line 7213

Quoted 1 line

fs/ocfs2/alloc.c +1 -0

+        next = ocfs2_find_next_bit(bitmap, max, start);

     next = ocfs2_find_next_bit(...);
   if (next == -1)
      break;

next will be set to "-1"? sorry, but where do you get it?

Quoted 2 lines

   if (next > max)
      next = max;

Quoted 8 lines

Quoted 2 lines

again, ocfs2_find_next_bit will return a value larger than 'max'? I am afraid not. Otherwise, it will be nonsense to pass a 'max' to it.

Say we're handling the last group, and the 'start + len' was within a hole, then the 'max'
is 'first_bit + len', while the next none-zero bit we found may be larger than 'max', isn't
that possible?

ocfs2_find_next_bit(and ext2_find_next_bit) won't parse, check and return 'bit' after 'max'. otherwise there should be a problem of memory overflow(you read and check some memory which isn't owned and handled by you). So the same goes here. If it can return a value larger than 'max', every caller will have to check the overflow. That would be too painful.

Quoted 8 lines

Quoted 8 lines

Quoted 5 lines

fs/ocfs2/alloc.c out_commit: · line 7244

Quoted 4 lines

fs/ocfs2/alloc.c +4 -0

+int ocfs2_trim_fs(struct super_block *sb, struct fstrim_range *range)
+{
+    struct ocfs2_super *osb = OCFS2_SB(sb);
+    u64 start, len, minlen, trimmed, first_group, last_group, group;

why not using u32 start, len, minlen, trimmed;

we may use 64 bit clusters later I guess. And what's more, they will be set by the user later. and it may overflow. Say the user pass a u64 range->len, it will overflow with range->len >> osb->s_clustersize_bits.

Quoted 7 lines

I just found we were using u32 for counting clusters all around ocfs2 codes, e.g truncate/punching_hole
codes, also passing an u64 byte_offset from userspace, so my original intention is to keep an unification;-)

Overflow can theoretically happen anyway, however, it's not very likely to pass a 16TB+ byte_offset from userspace.

I am afraid it is very likely. So say you want to trim all the clusters within the volume, how could you set 'range->len'? Will you first fdisk to get the volume size and then set it accordingly?
Most guys will set it to ULLONG_MAX and let the file system handles it. This is not my personal view, please check this article:
http://lwn.net/Articles/417809/
Jonathan also suggests to set len to ULLONG_MAX so that you can trim the whole volume.

Regards,
Tao

Quoted 85 lines · Tao Ma

Quoted 76 lines · Tristan Ye

Quoted 75 lines · Tao Ma

Quoted 3 lines · Tristan Ye

Hi Tao,

Most of codes looks pretty neat to me, few comments inlined below:

Thanks for the review.

Quoted 63 lines

Quoted 44 lines · Tao Ma

From: Tao Ma <[email protected]>

Add ocfs2_trim_fs to support trimming freed clusters in the volume. A range will be given and all the freed clusters greater than minlen will be discarded to the block layer.

Signed-off-by Tao Ma <[email protected]>

---
 fs/ocfs2/alloc.c |  154
++++++++++++++++++++++++++++++++++++++++++++++++++++++
 fs/ocfs2/alloc.h |    1 +
 2 files changed, 155 insertions(+), 0 deletions(-)

fs/ocfs2/alloc.c +16 -0

index b27a0d8..6e1b3b5 100644
@@ -29,6 +29,7 @@
 #include <linux/highmem.h>
 #include <linux/swap.h>
 #include <linux/quotaops.h>
+#include <linux/blkdev.h>
 
 #include <cluster/masklog.h>
 

@@ -7184,3 +7185,156 @@ out_commit:
 out:
     return ret;
 }
+
+static int ocfs2_trim_extent(struct super_block *sb,
+                 struct ocfs2_group_desc *gd,
+                 int start, int count)
+{
+    u64 discard;
+
+    count = ocfs2_clusters_to_blocks(sb, count);
+    discard = le64_to_cpu(gd->bg_blkno) +
+            ocfs2_clusters_to_blocks(sb, start);
+
+    return sb_issue_discard(sb, discard, count, GFP_NOFS, 0);
+}
+
+static int ocfs2_trim_group(struct super_block *sb,

fs/ocfs2/alloc.c out_commit: · line 7212

Quoted 10 lines

fs/ocfs2/alloc.c +10 -0

+                struct ocfs2_group_desc *gd,
+                int start, int max, int minbits)
+{
+    int ret = 0, count = 0, next;
+    void *bitmap = gd->bg_bitmap;
+
+    while (start < max) {
+        start = ocfs2_find_next_zero_bit(bitmap, max, start);
+        if (start >= max)
+            break;

/* What if the 'start' stands within a hole */

   if (ocfs2_test_bit(...)) {
      start = ocfs2_find_next_zero_bit(...);
      if ((start == -1) || (start >= max))
         break;
   }

Quoted 4 lines

fs/ocfs2/alloc.c out_commit: · line 7213

Quoted 1 line

fs/ocfs2/alloc.c +1 -0

+        next = ocfs2_find_next_bit(bitmap, max, start);

     next = ocfs2_find_next_bit(...);
   if (next == -1)
      break;

next will be set to "-1"? sorry, but where do you get it?

Quoted 2 lines

   if (next > max)
      next = max;

Quoted 8 lines

Quoted 2 lines

again, ocfs2_find_next_bit will return a value larger than 'max'? I am afraid not. Otherwise, it will be nonsense to pass a 'max' to it.

Say we're handling the last group, and the 'start + len' was within a hole, then the 'max'
is 'first_bit + len', while the next none-zero bit we found may be larger than 'max', isn't
that possible?

Quoted 5 lines

ocfs2_find_next_bit(and ext2_find_next_bit) won't parse, check and return 'bit' after 'max'. otherwise there should be a problem of memory overflow(you read and check some memory which isn't owned and handled by you). So the same goes here. If it can return a value larger than 'max', every caller will have to check the overflow. That would be too painful.

Oh, you may misunderstood my words, the 'max' you passed to
ocfs2_find_next_bit()
may not be the ending-edge of the cluster group(bitmap), it may be the end of what user specified
for TRIMing, therefore the 'next'(ending-edge for a wanted hole) bit you found from ocfs2_find_next_bit()
might be larger than 'max', is that possible?

Quoted 18 lines

Quoted 11 lines

Quoted 11 lines

Quoted 7 lines

fs/ocfs2/alloc.c out_commit: · line 7244

Quoted 6 lines

fs/ocfs2/alloc.c +1 -0

+int ocfs2_trim_fs(struct super_block *sb, struct fstrim_range

*range)

fs/ocfs2/alloc.c +3 -0

+{
+    struct ocfs2_super *osb = OCFS2_SB(sb);
+    u64 start, len, minlen, trimmed, first_group, last_group, group;

why not using u32 start, len, minlen, trimmed;

we may use 64 bit clusters later I guess. And what's more, they will be set by the user later. and it may overflow. Say the user pass a u64 range->len, it will overflow with range->len >> osb->s_clustersize_bits.

Quoted 7 lines

I just found we were using u32 for counting clusters all around ocfs2 codes, e.g truncate/punching_hole
codes, also passing an u64 byte_offset from userspace, so my original intention is to keep an unification;-)

Overflow can theoretically happen anyway, however, it's not very likely to pass a 16TB+ byte_offset from userspace.

Quoted 8 lines

I am afraid it is very likely. So say you want to trim all the clusters within the volume, how could you set 'range->len'? Will you first fdisk to get the volume size and then set it accordingly?
Most guys will set it to ULLONG_MAX and let the file system handles it. This is not my personal view, please check this article:
http://lwn.net/Articles/417809/
Jonathan also suggests to set len to ULLONG_MAX so that you can trim the whole volume.

Nice self-defense;-), how about the overflow risk in
truncate/punching-hole
codes, where u32 were being used for cluster counting.

Hi Tao,

Most of codes looks pretty neat to me, few comments inlined below:

Quoted 17 lines · Tao Ma

From: Tao Ma <[email protected]>

Add ocfs2_trim_fs to support trimming freed clusters in the volume. A range will be given and all the freed clusters greater than minlen will be discarded to the block layer.

Signed-off-by Tao Ma <[email protected]>

---
 fs/ocfs2/alloc.c |  154
++++++++++++++++++++++++++++++++++++++++++++++++++++++
 fs/ocfs2/alloc.h |    1 +
 2 files changed, 155 insertions(+), 0 deletions(-)

fs/ocfs2/alloc.c +0 -0

index b27a0d8..6e1b3b5 100644

fs/ocfs2/alloc.c out_commit: · line 7212

Quoted 11 lines

fs/ocfs2/alloc.c +11 -0

+static int ocfs2_trim_group(struct super_block *sb,
+                struct ocfs2_group_desc *gd,
+                int start, int max, int minbits)
+{
+    int ret = 0, count = 0, next;
+    void *bitmap = gd->bg_bitmap;
+
+    while (start < max) {
+        start = ocfs2_find_next_zero_bit(bitmap, max, start);
+        if (start >= max)
+            break;

   /* What if the 'start' stands within a hole */

   if (ocfs2_test_bit(...)) {
      start = ocfs2_find_next_zero_bit(...);
      if ((start == -1) || (start >= max))
         break;
   }

Quoted 22 lines · Tristan Ye

Hi Tao,

Most of codes looks pretty neat to me, few comments inlined below:

Quoted 17 lines · Tao Ma

From: Tao Ma <[email protected]>

Add ocfs2_trim_fs to support trimming freed clusters in the volume. A range will be given and all the freed clusters greater than minlen will be discarded to the block layer.

Signed-off-by Tao Ma <[email protected]>

---
 fs/ocfs2/alloc.c |  154
++++++++++++++++++++++++++++++++++++++++++++++++++++++
 fs/ocfs2/alloc.h |    1 +
 2 files changed, 155 insertions(+), 0 deletions(-)

fs/ocfs2/alloc.c +0 -0

index b27a0d8..6e1b3b5 100644

<snip>

Quoted 13 lines

fs/ocfs2/alloc.c out_commit: · line 7212

Quoted 11 lines

fs/ocfs2/alloc.c +11 -0

+static int ocfs2_trim_group(struct super_block *sb,
+                struct ocfs2_group_desc *gd,
+                int start, int max, int minbits)
+{
+    int ret = 0, count = 0, next;
+    void *bitmap = gd->bg_bitmap;
+
+    while (start < max) {
+        start = ocfs2_find_next_zero_bit(bitmap, max, start);
+        if (start >= max)
+            break;

/* What if the 'start' stands within a hole */

Quoted 5 lines

   if (ocfs2_test_bit(...)) {
      start = ocfs2_find_next_zero_bit(...);
      if ((start == -1) || (start >= max))
         break;
   }

I just noticed that I forget to response to this. what do you define a hole? It is within the global bitmap, so it is either freed or allocated. I don't get your meaning of 'hole'. ocfs2_find_next_zero_bit will do as we expected.

Regards,
Tao