On Mon, 17 Feb 2014 11:34:16 +0100, Andreas Rohner wrote:
> On 2014-02-17 11:06, Ryusuke Konishi wrote:
>> On Mon, 17 Feb 2014 10:17:57 +0100, Andreas Rohner wrote:
>>> On 2014-02-17 04:00, Ryusuke Konishi wrote:
>>>>> + if (end > nilfs->ns_nsegments)
>>>>> + end = nilfs->ns_nsegments;
>>>>
>>>> Yes, this adjustment is what we should do here, but 'end' segnum was
>>>> rounded down to segment alighment before. So, it should be:
>>>>
>>>> if (end >= nilfs->ns_nsegments)
>>>> end = nilfs->ns_nsegments - 1;
>>>>
>>>>> + if (end == segnum)
>>>>> + goto out;
>>>>
>>>> and
>>>>
>>>> if (end < segnum)
>>>> goto out;
>>>>
>>>>> +
>>>>> + down_read(&NILFS_MDT(sufile)->mi_sem);
>>>>> +
>>>>> + while (segnum < end) {
>>>>
>>>> and
>>>>
>>>> while (segnum <= end) {
>>>>
>>>>> + n = min_t(unsigned long,
>>>>> + segusages_per_block -
>>>>> + nilfs_sufile_get_offset(sufile, segnum),
>>>>> + end - segnum);
>>>>
>>>> Then, we can reuse nilfs_sufile_segment_usages_in_block() to calculate
>>>> 'n'.
>>>
>>> Actually I don't think that is correct. What if range->start = 0 and
>>> range->end = 8MB. Then segnum = 0 and end = 1. Your code would discard
>>> segment 0 and segment 1, whereas my version would discard only segment
>>> 0, which seems to be more reasonable.
>>
>> The problem seems that 'end' is not calculated properly.
>> I think it should be
>>
>> end = nilfs_get_segnum_of_block(
>> nilfs,
>> (range->start + range->len + <segment-size-in-bytes> - 1)
>> >> nilfs->ns_blocksize_bits) - 1;
>>
>> or can be simplified to
>>
>> end = nilfs_get_segnum_of_block(
>> nilfs,
>> (range->start + range->len - 1) >> nilfs->ns_blocksize_bits);
>>
>> if range->len > 0 is guaranteed.
>>
>>
>> The calculation of segnum extends the range to be discarded since it
>> is rounded down to segment alignment, but that of the current
>> implementation for 'end' truncates the range. Both should be
>> extended.
>
> Then shouldn't both be truncated? The user expects that less bytes than
> range->len are discarded but not more. Maybe I am wrong and it is
> defined somewhere...
Uum, xfs looks to be truncating both. This seems to be authentic
when we think the original meaning of the fitrim range.
I first thought truncating the range can generate gaps between
consecutive calls of FITRIM (if we use FITRIM like that). But now I'm
tempted to take the xfs approach. Let's take this semantics.
So, we now have to round up range->start to calculate (start) segnum.
Regards,
Ryusuke Konishi
--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
