5 ms·

This isn't just a rename. The structure dm_bio_details was changed to remove one field that points to a block_device structure, and add a field that points to

This isn't just a rename. The structure dm_bio_details was changed to remove one field that points to a block_device structure, and add a field that points to a gendisk structure, and an integer called bi_partno. This change copies the gendisk structure, but not bi_partno. That's the bug.

lomnakkus 9y ago
AFAIU the commits there was a rename and then a remove. Am I wrong in that assessment?
- mlyle 9y ago
  The important distinction is where the critical information on where the device offset is encoded from. Before it was specified by whether a partition or parent disk device was used; now it is always the actual disk and a partition number. It's not a bad change, but IMO it was made recklessly. From this patchset, there's now at least four different fixes: tags referring to it in Linus's tree. The patchset itself was broken and intermediate states didn't compile, making bisection annoying. And there's many other callers of this function that are potentially getting bad results, along with other cousins like bio_clone_bioset that are still unfixed. This was not a fun experience for a new maintainer. It turns out if you had created a fresh bcache on 4.14, like most of my tests do-- you're probably fine. And not all environments will experience problems. It went in a couple of weeks before I became maintainer, and isn't in the bcache code itself (and wasn't reviewed by anyone who worked on bcache, and was never sent to the linux-bcache mailing list.) I've learned some new things to add to my test suite, and I guess i need to read all the block diff to make sure people don't break things out from under me.
  - vog 9y ago
    > The patchset itself was broken and intermediate states didn't compile As a maintainer, isn't it then your right to refuse the patchset and demanding a clean patchset instead?
    - peterwaller 9y ago
      He's a maintainer of bcache, not of block layer, where the problematic patchset was introduced.
    - mlyle 9y ago
      As pointed out elsewhere, I never accepted this patchset. But: For stuff I accept, I try and make sure that intermediate states compile, but I can't promise I run a compile on every intermediate step or that my review eye is good enough to spot every wrongly-ordered-dependent-change. It would be nice if someone would throw hardware at building intermediate states of linux-next and sent nastygrams to maintainers/committers about it: might make people more careful.
      - vog 9y ago
        Can't you use GitHub's integrated TravisCI for that? At least that's what I know other Free Software projects are doing.
        mlyle 9y ago
        TravisCI generally tests the end result of a PR, not all intermediate states. I have a continuous integration tool for bcache in place, but it's more geared on end states-- compiling/checkpatch/test sequence.
  - benmmurphy 9y ago
    i ran coccinelle on the original code before the patch with this rule to see if it would spot any other missing changes. linux git:(c2ee070fb003) cat patch.coci @@ expression lhs; expression rhs; @@ - lhs->bi_bdev = rhs; + bio_set_dev(bio, rhs); so it spotted the original missing bi_partno = in the same file. diff -u -p a/block/bio.c b/block/bio.c --- a/block/bio.c +++ b/block/bio.c @@ -596,7 +596,7 @@ void __bio_clone_fast(struct bio *bio, s * most users will be overriding ->bi_bdev with a new target, * so we don't set nor calculate new physical/hw segment counts here */ - bio->bi_bdev = bio_src->bi_bdev; + bio_set_dev(bio, bio_src->bi_bdev); bio_set_flag(bio, BIO_CLONED); bio->bi_opf = bio_src->bi_opf; bio->bi_write_hint = bio_src->bi_write_hint; @@ -681,7 +681,7 @@ struct bio *bio_clone_bioset(struct bio bio = bio_alloc_bioset(gfp_mask, bio_segments(bio_src), bs); if (!bio) return NULL; - bio->bi_bdev = bio_src->bi_bdev; + bio_set_dev(bio, bio_src->bi_bdev); bio->bi_opf = bio_src->bi_opf; bio->bi_write_hint = bio_src->bi_write_hint; bio->bi_iter.bi_sector = bio_src->bi_iter.bi_sector; but it also spotted a missing assignment in the same file in the bio_clone_bioset function. i don't know enough about this functionality to know if the assignment is meant to be here as well. also, considering it is in the same file i would have thought if it was necessary it would have been noticed in the last patch. just thought i would let someone else who would have a better idea about how this works know. i also haven't checked the rest of the coccinelle output. i know it has generated a lot of _wrong_ code. like the actual patch looks like it has done something more intelligent than what coccinelle blindly did.
    - mlyle 9y ago
      Read. :P To quote what you replied to: "And there's many other callers of this function that are potentially getting bad results, along with other cousins like bio_clone_bioset that are still unfixed." bcache doesn't call bio_clone_bioset. I put in the minimum change to fix the bcache regression, for quick review and acceptance. You're free to submit this to linux-block if you want.
      - benmmurphy 9y ago
        thanks :) i actually didn't read the post at all. heh

lomnakkus 9y ago

AFAIU the commits there was a rename and then a remove. Am I wrong in that assessment?

mlyle 9y ago
The important distinction is where the critical information on where the device offset is encoded from. Before it was specified by whether a partition or parent disk device was used; now it is always the actual disk and a partition number. It's not a bad change, but IMO it was made recklessly. From this patchset, there's now at least four different fixes: tags referring to it in Linus's tree. The patchset itself was broken and intermediate states didn't compile, making bisection annoying. And there's many other callers of this function that are potentially getting bad results, along with other cousins like bio_clone_bioset that are still unfixed. This was not a fun experience for a new maintainer. It turns out if you had created a fresh bcache on 4.14, like most of my tests do-- you're probably fine. And not all environments will experience problems. It went in a couple of weeks before I became maintainer, and isn't in the bcache code itself (and wasn't reviewed by anyone who worked on bcache, and was never sent to the linux-bcache mailing list.) I've learned some new things to add to my test suite, and I guess i need to read all the block diff to make sure people don't break things out from under me.
- vog 9y ago
  > The patchset itself was broken and intermediate states didn't compile As a maintainer, isn't it then your right to refuse the patchset and demanding a clean patchset instead?
  - peterwaller 9y ago
    He's a maintainer of bcache, not of block layer, where the problematic patchset was introduced.
  - mlyle 9y ago
    As pointed out elsewhere, I never accepted this patchset. But: For stuff I accept, I try and make sure that intermediate states compile, but I can't promise I run a compile on every intermediate step or that my review eye is good enough to spot every wrongly-ordered-dependent-change. It would be nice if someone would throw hardware at building intermediate states of linux-next and sent nastygrams to maintainers/committers about it: might make people more careful.
    - vog 9y ago
      Can't you use GitHub's integrated TravisCI for that? At least that's what I know other Free Software projects are doing.
      - mlyle 9y ago
        TravisCI generally tests the end result of a PR, not all intermediate states. I have a continuous integration tool for bcache in place, but it's more geared on end states-- compiling/checkpatch/test sequence.
- benmmurphy 9y ago
  i ran coccinelle on the original code before the patch with this rule to see if it would spot any other missing changes. linux git:(c2ee070fb003) cat patch.coci @@ expression lhs; expression rhs; @@ - lhs->bi_bdev = rhs; + bio_set_dev(bio, rhs); so it spotted the original missing bi_partno = in the same file. diff -u -p a/block/bio.c b/block/bio.c --- a/block/bio.c +++ b/block/bio.c @@ -596,7 +596,7 @@ void __bio_clone_fast(struct bio *bio, s * most users will be overriding ->bi_bdev with a new target, * so we don't set nor calculate new physical/hw segment counts here */ - bio->bi_bdev = bio_src->bi_bdev; + bio_set_dev(bio, bio_src->bi_bdev); bio_set_flag(bio, BIO_CLONED); bio->bi_opf = bio_src->bi_opf; bio->bi_write_hint = bio_src->bi_write_hint; @@ -681,7 +681,7 @@ struct bio *bio_clone_bioset(struct bio bio = bio_alloc_bioset(gfp_mask, bio_segments(bio_src), bs); if (!bio) return NULL; - bio->bi_bdev = bio_src->bi_bdev; + bio_set_dev(bio, bio_src->bi_bdev); bio->bi_opf = bio_src->bi_opf; bio->bi_write_hint = bio_src->bi_write_hint; bio->bi_iter.bi_sector = bio_src->bi_iter.bi_sector; but it also spotted a missing assignment in the same file in the bio_clone_bioset function. i don't know enough about this functionality to know if the assignment is meant to be here as well. also, considering it is in the same file i would have thought if it was necessary it would have been noticed in the last patch. just thought i would let someone else who would have a better idea about how this works know. i also haven't checked the rest of the coccinelle output. i know it has generated a lot of _wrong_ code. like the actual patch looks like it has done something more intelligent than what coccinelle blindly did.
  - mlyle 9y ago
    Read. :P To quote what you replied to: "And there's many other callers of this function that are potentially getting bad results, along with other cousins like bio_clone_bioset that are still unfixed." bcache doesn't call bio_clone_bioset. I put in the minimum change to fix the bcache regression, for quick review and acceptance. You're free to submit this to linux-block if you want.
    - benmmurphy 9y ago
      thanks :) i actually didn't read the post at all. heh