Re: [bug report] btrfs: get rid of trivial __btrfs_lookup_bio_sums() wrappers

[Dan Carpenter <dan.carpenter@oracle.com>]

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