On 2014-02-17 12:21, Ryusuke Konishi wrote:
> 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).
Hmm good point I didn't think of that. Then by extending both sides the
overlapping segments would get discarded twice with consecutive calls,
which shouldn't be a problem.
> 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.
Ok. I think in the end it doesn't really matter, because most users will
use the default values:
range->start = 0
range->minlen = 0
range->len = ULLONG_MAX
Regards,
Andreas Rohner
--
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
