[Ocfs2-devel] [PATCH 1/3] ocfs2: Add ocfs2_trim_fs for SSD trim support.
Tristan Ye
tristan.ye at oracle.com
Mon Mar 7 22:53:53 PST 2011
Tao Ma wrote:
> On 03/08/2011 02:23 PM, Tristan Ye wrote:
>> Tao Ma wrote:
>>> On 03/08/2011 12:55 PM, Tristan Ye wrote:
>>>> Hi Tao,
>>>>
>>>> Most of codes looks pretty neat to me, few comments inlined below:
>>> Thanks for the review.
>>>> Tao Ma wrote:
>>>>> From: Tao Ma <boyu.mt at taobao.com>
>>>>>
>>>>> 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 <boyu.mt at taobao.com>
>>>>> ---
>>>>> 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;
>>> next will be set to "-1"? sorry, but where do you get it?
>>>> 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?
> 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?
>>>>> +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.
> 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.
>
> Regards,
> Tao
More information about the Ocfs2-devel
mailing list