On 07/14/2017 04:03 PM, David Sterba wrote:

On Fri, Jul 14, 2017 at 09:47:30PM +0800, Ming Lei wrote:

quoted

On Fri, Jul 14, 2017 at 9:40 PM, David Sterba [off-list ref] wrote:

quoted

We've switched to cloned bios in btrfs and hit a nasty bug leading to
corruptions, when cloned bios are iterated by bio_for_each_segment_all.

No, you simply can't use bio_for_each_segment_all on cloned bio, and the
reason is obviously.

This was not obvious to us, speaking for the btrfs developers trying to
make more use of the of the bio API, so we had to find out the hard way.

Yep, it might be obvious to those familiar with the block layer's
internals, but for those not so familiar, it's not. There's no mention
in bio_clone_fast() that the cloned bio's bi_vcnt shouldn't be used, and
after finding that, one has to check which bio APIs use it and which
don't. In this specific btrfs issue, it lead to silent write
corruptions, making it harder to find (as opposed to crashes or other
immediate failures).

The proposed WARN_ON, possibly more sanity checks or documentation would
help us not to trip over similar problems in the future. I try to take
great care when dealing with code changing bios on our side so I read
the headers, and partially the implementation, but still can miss
something.