On Tue, Jan 07, 2020 at 05:41:09PM +0100, David Sterba wrote:
> 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.
Could you add me as the reporter? I'd feel uncomfortable changing the
code here when I don't really understand it.
regards,
dan carpenter
