[developer] Re: [openzfs/openzfs] 8857 zio_remove_child() panic due to already destroyed parent zio (#505)
Closed #505 via 862ff6d99c6366f6258ffce1b7f45bb10ae42fa7. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/openzfs/openzfs/pull/505#event-1472496551 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/Td0adf1bfacf39ae0-Me4611fe29a37d7a879c757f2 Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 8857 zio_remove_child() panic due to already destroyed parent zio (#505)
Yes. The test failure is unrelated to these changes. I'll open the RTI today; thanks all. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/openzfs/openzfs/pull/505#issuecomment-365344721 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/Td0adf1bfacf39ae0-M74101772a184713304d9e603 Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 8857 zio_remove_child() panic due to already destroyed parent zio (#505)
Perfect! Please RTI as soon as you can. P.S. I do not understand the latest build failure, seems like an infra problem: http://jenkins.open-zfs.org/blue/organizations/jenkins/openzfs%2Fopenzfs/detail/PR-505/6/pipeline/179 -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/openzfs/openzfs/pull/505#issuecomment-365344318 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/Td0adf1bfacf39ae0-M6791b75296dd19f8791ee297 Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 8857 zio_remove_child() panic due to already destroyed parent zio (#505)
@grwilson pushed 1 commit. 1d7e2b0 all bits -- You are receiving this because you are subscribed to this thread. View it on GitHub: https://github.com/openzfs/openzfs/pull/505/files/445e55128ca0a1cf9b0a7013d996edd50da3a0f4..1d7e2b0d0cb2416d9838f0bd25fc174f31c4be09 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/Td0adf1bfacf39ae0-M94874194d6bc4b41eff311de Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 8857 zio_remove_child() panic due to already destroyed parent zio (#505)
avg-I commented on this pull request.
> @@ -3534,11 +3545,11 @@ zio_done(zio_t *zio)
* If our children haven't all completed,
* wait for them and then repeat this pipeline stage.
*/
- if (zio_wait_for_children(zio, ZIO_CHILD_VDEV, ZIO_WAIT_DONE) ||
- zio_wait_for_children(zio, ZIO_CHILD_GANG, ZIO_WAIT_DONE) ||
- zio_wait_for_children(zio, ZIO_CHILD_DDT, ZIO_WAIT_DONE) ||
- zio_wait_for_children(zio, ZIO_CHILD_LOGICAL, ZIO_WAIT_DONE))
+ if (zio_wait_for_children(zio, ZIO_CHILD_VDEV_BIT |
+ ZIO_CHILD_GANG_BIT | ZIO_CHILD_DDT_BIT | ZIO_CHILD_LOGICAL_BIT,
+ ZIO_WAIT_DONE)) {
One small and probably last nit-pick, would we benefit from defining
`ZIO_CHILD_ALL_BITS` and using it here?
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/openzfs/openzfs/pull/505#pullrequestreview-95624819
--
openzfs-developer
Archives:
https://openzfs.topicbox.com/groups/developer/discussions/Td0adf1bfacf39ae0-M769c82a642435d083fd6bfda
Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 8857 zio_remove_child() panic due to already destroyed parent zio (#505)
avg-I approved this pull request. LGTM -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/openzfs/openzfs/pull/505#pullrequestreview-95624832 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/Td0adf1bfacf39ae0-Md31dbd08b553202ae00674f9 Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 8857 zio_remove_child() panic due to already destroyed parent zio (#505)
@grwilson pushed 1 commit. 445e551 predefine child bits -- You are receiving this because you are subscribed to this thread. View it on GitHub: https://github.com/openzfs/openzfs/pull/505/files/b1342b226af1aa8a39cbcbe3b59a9270765c8ef2..445e55128ca0a1cf9b0a7013d996edd50da3a0f4 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/Td0adf1bfacf39ae0-M70d1f4a1e1285c1d085c2cc6 Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 8857 zio_remove_child() panic due to already destroyed parent zio (#505)
Maybe even `ZIO_GANG_BIT`, etc. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/openzfs/openzfs/pull/505#issuecomment-364269552 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/Td0adf1bfacf39ae0-M83a47cac272575e655a9a6fe Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 8857 zio_remove_child() panic due to already destroyed parent zio (#505)
@grwilson this looks good to me! But... But! I've wondered only __just now__ if we could have best of both approaches by pre-defining the child type bits. E.g. ``` #define ZIO_CHILD_BIT_GANGZIO_CHILD_BIT(ZIO_CHILD_GANG) ``` ... or something like that. Maybe that could help to make the code where the bits are used to be just a little bit more compact. This is a purely stylistic suggestion, of course. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/openzfs/openzfs/pull/505#issuecomment-364269244 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/Td0adf1bfacf39ae0-M0d91c623e9549814c80465ae Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 8857 zio_remove_child() panic due to already destroyed parent zio (#505)
youzhongyang commented on this pull request. It looks good to me. Thanks. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/openzfs/openzfs/pull/505#pullrequestreview-95227067 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/Td0adf1bfacf39ae0-M6d130cb035d3ef1c98d1b543 Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 8857 zio_remove_child() panic due to already destroyed parent zio (#505)
@youzhongyang @avg-I can you take another look to make sure I've address all of your concerns? -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/openzfs/openzfs/pull/505#issuecomment-364239237 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/Td0adf1bfacf39ae0-M53d2269b8fffa3144053b079 Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 8857 zio_remove_child() panic due to already destroyed parent zio (#505)
Your comments are considered and I will be pushing a new commit with the updated logic. I've tried several variations to the suggestions to find which looks the best and has the least amount of impact. The suggested `ZIO_CHILD_BIT` option looks the best so I've gone with that option. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/openzfs/openzfs/pull/505#issuecomment-364237275 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/Td0adf1bfacf39ae0-Me6489fa6afb526dca5007381 Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 8857 zio_remove_child() panic due to already destroyed parent zio (#505)
so our comments will not be considered? -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/openzfs/openzfs/pull/505#issuecomment-364225821 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/Td0adf1bfacf39ae0-M4b16282b9a28cc2cc3ab396a Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 8857 zio_remove_child() panic due to already destroyed parent zio (#505)
ahrens approved this pull request. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/openzfs/openzfs/pull/505#pullrequestreview-95183334 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/Td0adf1bfacf39ae0-Mc248ebec0fac616fae17af3b Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 8857 zio_remove_child() panic due to already destroyed parent zio (#505)
[ping] -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/openzfs/openzfs/pull/505#issuecomment-361855336 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/Td0adf1bfacf39ae0-M726c1a5115dac27c932bf470 Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 8857 zio_remove_child() panic due to already destroyed parent zio (#505)
grwilson commented on this pull request.
> @@ -209,12 +209,13 @@ enum zio_flag {
ZIO_FLAG_CANFAIL)
enum zio_child {
- ZIO_CHILD_VDEV = 0,
- ZIO_CHILD_GANG,
- ZIO_CHILD_DDT,
- ZIO_CHILD_LOGICAL,
- ZIO_CHILD_TYPES
+ ZIO_CHILD_VDEV = 1 << 0,
+ ZIO_CHILD_GANG = 1 << 1,
+ ZIO_CHILD_DDT = 1 << 2,
+ ZIO_CHILD_LOGICAL = 1 << 3
Yeah, I looked at this option but didn't like how it looked specifically for
this reason. I've looked at using variable arguments too but it has drawback
too. I'm going to consider this option again but using a variable to hold the
bits we plan to pass into `zio_wait_for_children`.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/openzfs/openzfs/pull/505#discussion_r158716835
--
openzfs-developer
Archives:
https://openzfs.topicbox.com/groups/developer/discussions/Td0adf1bfacf39ae0-Ma54010b2908fc3d5b0c3ea0a
Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 8857 zio_remove_child() panic due to already destroyed parent zio (#505)
Also, looks like the build error is actually a transient problem in the infrastructure... -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/openzfs/openzfs/pull/505#issuecomment-353045436 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/Td0adf1bfacf39ae0-Md8dc819615dd805016518df1 Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 8857 zio_remove_child() panic due to already destroyed parent zio (#505)
Thanks for the analysis and the fix.
I guess highbit64() is not cheap, so likely the fix can be simplified.
How about this:
static boolean_t
zio_wait_for_children(zio_t *zio, uint32_t children_bits, enum zio_wait_type
wait)
{
boolean_t waiting = B_FALSE;
mutex_enter(>io_lock);
for (int child = 0; child < ZIO_CHILD_TYPES; child++) {
if (!(children_bits & (1 << child)))
continue;
uint64_t *countp = >io_children[child][wait];
ASSERT(zio->io_stall == NULL);
if (*countp != 0) {
zio->io_stage >>= 1;
ASSERT3U(zio->io_stage, !=, ZIO_STAGE_OPEN);
zio->io_stall = countp;
waiting = B_TRUE;
break;
}
}
mutex_exit(>io_lock);
return (waiting);
}
and then call the function like:
zio_wait_for_children(zio, 1 << ZIO_CHILD_GANG | 1 << ZIO_CHILD_LOGICAL,
ZIO_WAIT_READY)
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/openzfs/openzfs/pull/505#issuecomment-352228820
--
openzfs-developer
Archives:
https://openzfs.topicbox.com/groups/developer/discussions/Td0adf1bfacf39ae0-M274963b52ef07f25e0e78b79
Powered by Topicbox: https://topicbox.com
