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.
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
