Fix zil replay panic when TX_REMOVE followed by TX_CREATE #9145

tuxoko · 2019-08-09T04:11:28Z

If TX_REMOVE is followed by TX_CREATE on the same object id, we need to
make sure the object removal is completely finished before creation. The
current implementation relies on dnode_hold_impl with
DNODE_MUST_BE_ALLOCATED returning ENOENT. While this check seems to work
fine before, in current version it does not guarantee the object removal
is completed.

We fix this by checking if DNODE_MUST_BE_FREE returns successful
instead. Also add test and remove dead code in dnode_hold_impl.

Closes #7151
Closes #8910
Closes #9123
Signed-off-by: Chunwei Chen david.chen@nutanix.com

Motivation and Context

Description

How Has This Been Tested?

#!/bin/bash

POOLNAME=pp
TESTDIR=/pp

if [ "$(ls $TESTDIR | wc -l)" -lt "1000" ]; then
	for i in $(seq 1000); do touch $TESTDIR/file.$i; done
	sleep 2
	zpool sync $POOLNAME
fi

touch $TESTDIR && sync
zpool sync $POOLNAME
zpool freeze $POOLNAME

rm -f $TESTDIR/*
for i in $(seq 1000); do touch $TESTDIR/file.$i; done
sync

zpool export $POOLNAME
zpool import $POOLNAME

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:

My code follows the ZFS on Linux code style requirements.
I have updated the documentation accordingly.
I have read the contributing document.
I have added tests to cover my changes.
All new and existing tests passed.
All commit messages are properly formatted and contain Signed-off-by.

behlendorf

Thanks! You've definitely identified the root cause, it looks like this was accidentally introduced with the large dnode change. A few comments inline.

module/zfs/zfs_replay.c

module/zfs/dnode.c

tests/zfs-tests/tests/functional/slog/slog_replay_fs.ksh

module/zfs/dnode.c

module/zfs/zfs_replay.c

codecov · 2019-08-22T01:45:03Z

Codecov Report

Merging #9145 into master will increase coverage by 0.06%.
The diff coverage is 71.42%.

@@            Coverage Diff             @@
##           master    #9145      +/-   ##
==========================================
+ Coverage   79.19%   79.26%   +0.06%     
==========================================
  Files         400      400              
  Lines      122002   122009       +7     
==========================================
+ Hits        96625    96714      +89     
+ Misses      25377    25295      -82

Flag	Coverage Δ
#kernel	`79.79% <71.42%> (-0.01%)`	⬇️
#user	`67.14% <41.17%> (+0.2%)`	⬆️

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 4302698...636a114. Read the comment docs.

module/zfs/dnode.c

If TX_REMOVE is followed by TX_CREATE on the same object id, we need to make sure the object removal is completely finished before creation. The current implementation relies on dnode_hold_impl with DNODE_MUST_BE_ALLOCATED returning ENOENT. While this check seems to work fine before, in current version it does not guarantee the object removal is completed. We fix this by checking if DNODE_MUST_BE_FREE returns successful instead. Also add test and remove dead code in dnode_hold_impl. Closes openzfs#7151 Closes openzfs#8910 Closes openzfs#9123 Signed-off-by: Chunwei Chen <david.chen@nutanix.com>

behlendorf · 2019-08-26T20:05:27Z

@prakashsurya @tcaputi if you get a chance it would be great to get a second reviewer for this fix.

tcaputi

LGTM

If TX_REMOVE is followed by TX_CREATE on the same object id, we need to make sure the object removal is completely finished before creation. The current implementation relies on dnode_hold_impl with DNODE_MUST_BE_ALLOCATED returning ENOENT. While this check seems to work fine before, in current version it does not guarantee the object removal is completed. We fix this by checking if DNODE_MUST_BE_FREE returns successful instead. Also add test and remove dead code in dnode_hold_impl. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Reviewed-by: Tom Caputi <tcaputi@datto.com> Signed-off-by: Chunwei Chen <david.chen@nutanix.com> Closes openzfs#7151 Closes openzfs#8910 Closes openzfs#9123 Closes openzfs#9145

If TX_REMOVE is followed by TX_CREATE on the same object id, we need to make sure the object removal is completely finished before creation. The current implementation relies on dnode_hold_impl with DNODE_MUST_BE_ALLOCATED returning ENOENT. While this check seems to work fine before, in current version it does not guarantee the object removal is completed. We fix this by checking if DNODE_MUST_BE_FREE returns successful instead. Also add test and remove dead code in dnode_hold_impl. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Reviewed-by: Tom Caputi <tcaputi@datto.com> Signed-off-by: Chunwei Chen <david.chen@nutanix.com> Closes #7151 Closes #8910 Closes #9123 Closes #9145

tuxoko force-pushed the ziltxg branch 3 times, most recently from a8fd598 to 698ba75 Compare August 10, 2019 00:22

behlendorf added the Status: Code Review Needed Ready for review and testing label Aug 12, 2019

behlendorf requested changes Aug 21, 2019

View reviewed changes

tuxoko force-pushed the ziltxg branch 2 times, most recently from 9d54188 to 15b839a Compare August 21, 2019 17:06

behlendorf reviewed Aug 21, 2019

View reviewed changes

module/zfs/dnode.c Show resolved Hide resolved

tuxoko force-pushed the ziltxg branch from 15b839a to 827980b Compare August 21, 2019 19:46

tuxoko requested a review from behlendorf August 21, 2019 19:46

tuxoko commented Aug 21, 2019

View reviewed changes

module/zfs/dnode.c Show resolved Hide resolved

behlendorf reviewed Aug 21, 2019

View reviewed changes

module/zfs/dnode.c Outdated Show resolved Hide resolved

module/zfs/dnode.c Show resolved Hide resolved

module/zfs/zfs_replay.c Outdated Show resolved Hide resolved

tuxoko force-pushed the ziltxg branch from 827980b to c48de75 Compare August 21, 2019 21:22

tuxoko requested a review from behlendorf August 21, 2019 21:23

behlendorf reviewed Aug 22, 2019

View reviewed changes

module/zfs/dnode.c Outdated Show resolved Hide resolved

module/zfs/dnode.c Outdated Show resolved Hide resolved

behlendorf requested a review from tcaputi August 22, 2019 23:31

tuxoko force-pushed the ziltxg branch from c48de75 to 72e6a18 Compare August 23, 2019 00:12

behlendorf approved these changes Aug 23, 2019

View reviewed changes

tuxoko force-pushed the ziltxg branch from 72e6a18 to 636a114 Compare August 23, 2019 02:46

behlendorf requested a review from prakashsurya August 26, 2019 20:04

tcaputi approved these changes Aug 27, 2019

View reviewed changes

behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Aug 27, 2019

behlendorf merged commit 035e961 into openzfs:master Aug 28, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix zil replay panic when TX_REMOVE followed by TX_CREATE #9145

Fix zil replay panic when TX_REMOVE followed by TX_CREATE #9145

tuxoko commented Aug 9, 2019 •

edited

Loading

behlendorf left a comment

codecov bot commented Aug 22, 2019 •

edited

Loading

behlendorf commented Aug 26, 2019

tcaputi left a comment

Fix zil replay panic when TX_REMOVE followed by TX_CREATE #9145

Fix zil replay panic when TX_REMOVE followed by TX_CREATE #9145

Conversation

tuxoko commented Aug 9, 2019 • edited Loading

Motivation and Context

Description

How Has This Been Tested?

Types of changes

Checklist:

behlendorf left a comment

Choose a reason for hiding this comment

codecov bot commented Aug 22, 2019 • edited Loading

Codecov Report

behlendorf commented Aug 26, 2019

tcaputi left a comment

Choose a reason for hiding this comment

tuxoko commented Aug 9, 2019 •

edited

Loading

codecov bot commented Aug 22, 2019 •

edited

Loading