On 2019/4/6 上午10:01, Al Viro wrote:
> On Sat, Apr 06, 2019 at 09:53:07AM +0800, Qu Wenruo wrote:
>> Hi,
>>
>> I'm looking into a strange behavior that we can't break
>> bio_for_each_segment_all() after commit 6dc4f100c175 ("block: allow
>> bio_for_each_segment_all() to iterate over multi-page bvec").
>>
>> It's screwing up all bio_for_each_segment_all() call with error out branch.
> 
>>         bio_for_each_segment_all(bvec, bio, i, iter_all) {
>>                 root = BTRFS_I(bvec->bv_page->mapping->host)->root;
>>                 ret = csum_dirty_buffer(root->fs_info, bvec->bv_page);
>> -               if (ret)
>> +               if (ret) {
>> +                       err = ret;
>> +                       pr_info("breaking out with ret=%d\n", ret);
>>                         break;
>> +               }
>>         }
>>
>> +       if (err)
>> +               pr_info("err=%d out, but ret=%d\n",err, ret);
>>         return errno_to_blk_status(ret);
>>  }
>>
>> Straightforward, if we break, we should have err == ret.
>>
>> Then run fstests btrfs/151, which will trigger a false alert in
>> tree-checker:
>>
>>   BTRFS critical (device dm-1): corrupt leaf: root=3 block=570572800
>> slot=1 devid=1 invalid total bytes: have 0
>>   BTRFS error (device dm-1): block=570572800 write time tree block
>> corruption detected
>>   breaking out with ret=-117
>>   err=-117 out, but ret=0
>>
>> So it looks like the break line doens't really break, but continue
>> executing.
> 
> It expands to for-inside-for since that commit, so break only takes you
> out of the inner loop...
> 
> No comments on desirability of such macros - personally, I prefer to
> avoid those, but I'm not stepping into that holy war...

But it's a regression at least, not only for btrfs, but at least another
caller:
  https://elixir.bootlin.com/linux/v5.1-rc3/source/block/bio.c#L1134

At least it's a surprise for some old code.

Anyway, I'll fix the problem in btrfs.

Thanks,
Qu