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
Attachment:
signature.asc
Description: OpenPGP digital signature
