On 3.01.19 г. 16:44 ч., David Sterba wrote: > On Thu, Jan 03, 2019 at 10:50:04AM +0200, Nikolay Borisov wrote: >> In a couple of places it's required to calculate the number of pages >> given a start and end offsets. Currently this is opencoded, unify the >> code base by replacing all such sites with the DIV_ROUND_UP macro. Also, >> current open-coded sites were buggy in that they were adding >> 'PAGE_SIZE', rather than 'PAGE_SIZE - 1'. > > Didn't you find it strange that it's so consistently wrong? After a > closer inspection, you'd find that the end of the range is inclusive. So > the math is correct and your patch introduces a bug. But since we are talking about number of pages, why does the range need to be inclusive. Indeed, let's take delalloc_to_write in writepage_delalloc. Say we have a 1mb range, 0-1m - that's 256 pages right so with DIV_ROUND_UP we'll have: (1048576 + 4096 - 1) / 4096 = 256 with the old version we'll have: 1048576 + 4096 / 4096 = 257 Since delalloc_to_write is used in a context where we care about the number of pages written I think using DIV_ROUND_UP is correct and it fixes an off-by-one bug, no ? Let's take extent_write_locked_range, it's called only from submit_compressed_extents with the range for the async (compressed) extent: Say we have an extent which is 2k in ram size and starts at 1m offset. We'll have: start = 1048576 end = 1048576 + 2048 - 1 = 1050623 nr_pages in this case should be 1, with DIV_ROUND_UP: nr_pages = 2047 + 4096 -1 / 4096 = 1 (1,4995 actually but due to integer math we don't care about floating point part) with old formula: nr_pages = (2047 + 4096) / 4096 = 1 (1,4998 but ditto as above). > > end - start + PAGE_SIZE = end + 1 - start + PAGE_SIZE - 1 > > Check eg. writepage_delalloc how it sets up page_end. > > The correct use in DIV_ROUND_UP needs +1 adjustment. >
