On Tue, Jan 07, 2020 at 05:10:46PM +0100, David Sterba wrote:
> On Tue, Jan 07, 2020 at 11:10:58AM +0300, Dan Carpenter wrote:
> > 276 diff = diff * csum_size;
> > 277 count = min_t(int, nblocks, (item_last_offset - disk_bytenr) >>
> > 278 inode->i_sb->s_blocksize_bits);
> > 279 read_extent_buffer(path->nodes[0], csum,
> > 280 ((unsigned long)item) + diff,
> > 281 csum_size * count);
> > 282 found:
> > 283 csum += count * csum_size;
> > 284 nblocks -= count;
> > 285 next:
> > 286 while (count--) {
> > ^^^^^^^
> > This loop exits with count set to -1.
> >
> > 287 disk_bytenr += fs_info->sectorsize;
> > 288 offset += fs_info->sectorsize;
> > 289 page_bytes_left -= fs_info->sectorsize;
> > 290 if (!page_bytes_left)
> > 291 break; /* move to next bio */
> > 292 }
> > 293 }
> > 294
> > 295 WARN_ON_ONCE(count);
> > ^^^^^
> > Originally this warning was next to the line 291 so it should probably
> > be "WARN_ON_ONCE(count >= 0);" This WARN is two years old now and no
> > one has complained about it at run time. That's very surprising to me
> > because I would have expected count to -1 in the common case.
>
> Possible explanation I see is that the "if (!page_bytes_left)" does not
> let the count go from 0 -> -1 and exits just in time. I'm runing a test
> to see if it's true.
It is. It's not very clear from the context, count is set up so that it
matches page_bytes_left decrements. So using "count--" is not completely
wrong, but it is confusing and relying on other subtle behaviour. It
should be either --count or the decrement moved to out of the condition.
I can write the patch and add you as reporter or you can send the patch
as you did the analysis in the first place.
