On Wed, 19 Feb 2014 07:36:58 +0100, Andreas Rohner wrote: > On 2014-02-18 19:37, Ryusuke Konishi wrote: >> On Mon, 17 Feb 2014 23:39:51 +0100, Andreas Rohner wrote: >> >> Please try to compile this patch both for 32-bit kernel and 64-bit >> kernel to test if the patch is architecture independent. > > Ok. > > With all the proper division macros it gets very complicated. I think it > would simplify things if we just truncate to block size instead of > sector size. Then we could use simple bit shifts. It would look > something like this: > > if (range->len < nilfs->ns_blocksize || > range->start >= max_byte) > return -EINVAL; > /* sector_t could be 32 bit */ > if (range->len > max_byte) > range->len = max_byte; > > sects_per_block = (1 << nilfs->ns_blocksize_bits) / > bdev_logical_block_size(nilfs->ns_bdev); > > start_block = (range->start + nilfs->ns_blocksize - 1) >> > nilfs->ns_blocksize_bits; > end_block = start_block + (range->len >> > nilfs->ns_blocksize_bits); > > segnum = nilfs_get_segnum_of_block(nilfs, start_block); > end = nilfs_get_segnum_of_block(nilfs, end_block + > nilfs->ns_blocks_per_segment - 1); > > minlen = range->minlen >> nilfs->ns_blocksize_bits; > > > What do you think?
Yes, sector-based calculations looks a bit complicated. Your idea, adjusting positions to fs-block alignment, is reasonable. I agree with the approach. >> So, these will be as follows: >> >> u64 len = range->len; >> >> ... >> >> do_div(len, sect_size); >> if (!len) >> goto out; >> >> ... >> start_sect = DIV_ROUND_UP_ULL(range->start, sect_size); >> end_sect = start_sect + len - 1; /* this end_sect is inclusive */ > > I don't get why this has to be inclusive. To me this seems to be a > matter of taste. I think it is much easier to reason about this stuff > and more "natural", if start_sect is inclusive and end_sect is > exclusive. Then segnum is inclusive and end is exclusive. It is just > like with pointer arithmetic. Yes, it is implementation matter (taste). The above comment just notice that the result of the calculation is inclusive which differs from the original meaning. What I hope there, is that usage of variables (e.g. the edge is inclusive or exclusive) is taken care to avoid confusion. Regards, Ryusuke Konishi -- 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
