Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix panic on debug build #8736

Closed
wants to merge 1 commit into from
Closed

Conversation

ikozhukhov
Copy link
Contributor

i have panic on DEBUG build on DilOS with test send_mixed_raw

panic[cpu3]/thread=fffffe0bf62f6c40:
assertion failed: (!copied) is equivalent to (hdr->b_l1hdr.b_freeze_cksum == NULL), file: ../../common/fs/zfs/arc.c, line: 1839


fffffe0012014700 genunix:process_type+1946ed ()
fffffe0012014730 zfs:arc_buf_try_copy_decompressed_data+b7 ()
fffffe00120147c0 zfs:arc_buf_fill+2e3 ()
fffffe0012014810 zfs:arc_untransform+25 ()
fffffe00120148a0 zfs:dbuf_read+4a1 ()
fffffe0012014910 zfs:dmu_buf_hold+80 ()
fffffe00120149f0 zfs:zap_lockdir+5e ()
fffffe0012014aa0 zfs:zap_lookup_norm+6c ()
fffffe0012014b00 zfs:zap_lookup+36 ()
fffffe0012014b60 zfs:zfs_get_zplprop+fc ()
fffffe0012014bc0 zfs:nvl_add_zplprop+36 ()
fffffe0012014c10 zfs:zfs_ioc_objset_zplprops+f5 ()
fffffe0012014cc0 zfs:zfsdev_ioctl+524 ()
fffffe0012014d00 genunix:cdev_ioctl+25 ()
fffffe0012014d50 specfs:spec_ioctl+4d ()
fffffe0012014de0 genunix:fop_ioctl+55 ()
fffffe0012014ef0 genunix:ioctl+a4 ()
fffffe0012014f00 unix:brand_sys_syscall+320 ()

Motivation and Context

Description

based on research and comments, we should to check b_freeze_cksum in loop

How Has This Been Tested?

test send_mixed_raw PASS after changes

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation (a change to man pages or other documentation)

Checklist:

Signed-off-by: Igor Kozhukhov <[email protected]>
@codecov
Copy link

codecov bot commented May 11, 2019

Codecov Report

Merging #8736 into master will decrease coverage by 0.05%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8736      +/-   ##
==========================================
- Coverage   78.97%   78.92%   -0.06%     
==========================================
  Files         381      381              
  Lines      117797   117802       +5     
==========================================
- Hits        93034    92978      -56     
- Misses      24763    24824      +61
Flag Coverage Δ
#kernel 79.4% <25%> (-0.02%) ⬇️
#user 67.9% <50%> (+0.14%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f378f42...919a496. Read the comment docs.

@ahrens ahrens added the Status: Code Review Needed Ready for review and testing label May 14, 2019
@behlendorf behlendorf requested a review from grwilson May 23, 2019 22:43
}

/*
* There were no decompressed bufs, so there should not be a
* checksum on the hdr either.
*/
EQUIV(!copied, hdr->b_l1hdr.b_freeze_cksum == NULL);
if (cnt > 0)
EQUIV(!copied, b_freeze_cksum == NULL);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ikozhukhov given that b_l1hdr.b_freeze_cksum can only be set when ZFS_DEBUG_MODIFY is enabled (see arc_cksum_compute(). I think this EQUIV is only valid when ZFS_DEBUG_MODIFY is set, and we should instead wrap it with a conditional that checks for the flag. For the same reason, the ASSERT after the call to arc_buf_try_copy_decompressed_data() in arc_buf_fill() looks wrong to me as well.

My suspicion is we've never seen this in testing because by default for debug builds we enable ZFS_DEBUG_MODIFY. It would be great to get @grwilson's thoughts on this, since I may be misunderstanding the intent of b_freeze_cksum here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, b_freeze_cksum should only have data when ZFS_DEBUG_MODIFY is enabled. So if I understand the original problem then wrapping the EQUIV with if (zfs_flags & ZFS_DEBUG_MODIFY) would fix the problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Code Review Needed Ready for review and testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants