Hi Tao,
Most of codes looks pretty neat to me, few comments inlined below:
Tao Ma wrote:
> 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(-)
>
> diff --git a/fs/ocfs2/alloc.c b/fs/ocfs2/alloc.c
> index b27a0d8..6e1b3b5 100644
> --- a/fs/ocfs2/alloc.c
> +++ b/fs/ocfs2/alloc.c
> @@ -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,
> + 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;
}
> + next = ocfs2_find_next_bit(bitmap, max, start);
next = ocfs2_find_next_bit(...);
if (next == -1)
break;
if (next > max)
next = max;
> +
> + 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;
> +
> + 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;
> + 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;
> + 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.
> + 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);
> + if (first_group == osb->first_cluster_group_blkno)
> + 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'
> + else
> + last_bit = start + len;
why above case is not 'last_bit = first_bit + len';
> +
> + 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;
> +}
> diff --git a/fs/ocfs2/alloc.h b/fs/ocfs2/alloc.h
> index 3bd08a0..ca381c5 100644
> --- a/fs/ocfs2/alloc.h
> +++ b/fs/ocfs2/alloc.h
> @@ -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.
> */
_______________________________________________
Ocfs2-devel mailing list
[email protected]
http://oss.oracle.com/mailman/listinfo/ocfs2-devel
