receive_freeobjects() skips freeing some objects #6695

nedbass · 2017-09-29T01:32:48Z

Description

When receiving a FREEOBJECTS record, receive_freeobjects()
incorrectly skips a freed object in some cases. This leaves an object
allocated on disk on the receiving side which is unallocated on the
sending side, which may cause receiving subsequent incremental
streams to fail.

The bug was caused by an incorrect increment of the object index
variable when current object being freed doesn't exist. The
increment is incorrect because incrementing the object index is
handled by a call to dmu_object_next() in the increment portion of
the for loop statement.

Motivation and Context

Receive backup streams correctly.

How Has This Been Tested?

Added test case rsend/send_freeobjects.ksh.

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.
Change has been approved by a ZFS on Linux member.

gmelikov · 2017-09-29T07:02:24Z

I think it would be better to add this test.

codecov · 2017-09-29T11:01:49Z

Codecov Report

❗ No coverage uploaded for pull request base (master@01ff0d7). Click here to learn what that means.
The diff coverage is 100%.

@@            Coverage Diff            @@
##             master    #6695   +/-   ##
=========================================
  Coverage          ?   74.02%           
=========================================
  Files             ?      295           
  Lines             ?    93878           
  Branches          ?        0           
=========================================
  Hits              ?    69495           
  Misses            ?    24383           
  Partials          ?        0

Flag	Coverage Δ
#kernel	`74.3% <100%> (?)`
#user	`63.31% <0%> (?)`

Impacted Files	Coverage Δ
module/zfs/dmu_send.c	`75.17% <100%> (ø)`

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 01ff0d7...d79b4b3. Read the comment docs.

nedbass · 2017-09-29T17:12:24Z

Added test case.

gmelikov

Thanks!

behlendorf

Makes sense and looks good. Just a few nits.

behlendorf · 2017-09-29T20:21:20Z

tests/zfs-tests/tests/functional/rsend/send_freeobjects.ksh

+
+log_must zfs create $POOL/$sendds
+
+while :; do


What does the : here do? Why not while true; do? Or better yet put large limit on this so can can't spin forever.

behlendorf · 2017-09-29T20:24:30Z

tests/zfs-tests/tests/functional/rsend/send_freeobjects.ksh

+	touch $f1 $f2 $f3
+	o1=`ls -li $f1 | awk '{print $1}'`
+	o2=`ls -li $f2 | awk '{print $1}'`
+	o3=`ls -li $f3 | awk '{print $1}'`


Can you switch the legacy backticks to the preferred $() syntax.

behlendorf · 2017-09-29T20:25:40Z

tests/zfs-tests/tests/functional/rsend/send_freeobjects.ksh

+	o3=`ls -li $f3 | awk '{print $1}'`
+
+	if [[ $o2 -ne $(( $o1 + 1 )) ]] || [[ $o3 -ne $(( $o2 + 1 )) ]]; then
+		rm $f1 $f2 $f3


Maybe add -f to handle the unlike case where one of these files failed to be created and is missing.

behlendorf · 2017-09-29T21:21:49Z

tests/zfs-tests/tests/functional/rsend/send_freeobjects.ksh

+log_must zfs create $POOL/$sendds
+
+tries=100
+for ((i=0; i<$tries; i++)); do


Actually, it looks like this did happen in the last test run.

http://build.zfsonlinux.org/builders/CentOS%206%20x86_64%20%28TEST%29/builds/489/steps/shell_8/logs/stdio

Test: /usr/share/zfs/zfs-tests/tests/functional/rsend/send_freeobjects (run as root) [10:00] [KILLED] 19:19:53.38 ASSERTION: Verify FREEOBJECTS record frees sequential objects 19:19:53.42 SUCCESS: zfs create testpool/sendfo

Sigh. Of course it almost always exits the loop on the first or second iteration in manual testing. I may have to push a more verbose version to debug the builders.

Turns out SELinux xattrs were consuming the intervening object numbers. Updated test case to force xattr=sa to work around this.

When receiving a FREEOBJECTS record, receive_freeobjects() incorrectly skips a freed object in some cases. Specifically, this happens when the first object in the range to be freed doesn't exist, but the second object does. This leaves an object allocated on disk on the receiving side which is unallocated on the sending side, which may cause receiving subsequent incremental streams to fail. The bug was caused by an incorrect increment of the object index variable when current object being freed doesn't exist. The increment is incorrect because incrementing the object index is handled by a call to dmu_object_next() in the increment portion of the for loop statement. Add test case that exposes this bug. Fixes openzfs#6694 Signed-off-by: Ned Bass <bass6@llnl.gov>

When receiving a FREEOBJECTS record, receive_freeobjects() incorrectly skips a freed object in some cases. Specifically, this happens when the first object in the range to be freed doesn't exist, but the second object does. This leaves an object allocated on disk on the receiving side which is unallocated on the sending side, which may cause receiving subsequent incremental streams to fail. The bug was caused by an incorrect increment of the object index variable when current object being freed doesn't exist. The increment is incorrect because incrementing the object index is handled by a call to dmu_object_next() in the increment portion of the for loop statement. Add test case that exposes this bug. Reviewed-by: George Melikov <mail@gmelikov.ru> Reviewed-by: Giuseppe Di Natale <dinatale2@llnl.gov> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Ned Bass <bass6@llnl.gov> Closes openzfs#6694 Closes openzfs#6695

When receiving a FREEOBJECTS record, receive_freeobjects() incorrectly skips a freed object in some cases. Specifically, this happens when the first object in the range to be freed doesn't exist, but the second object does. This leaves an object allocated on disk on the receiving side which is unallocated on the sending side, which may cause receiving subsequent incremental streams to fail. The bug was caused by an incorrect increment of the object index variable when current object being freed doesn't exist. The increment is incorrect because incrementing the object index is handled by a call to dmu_object_next() in the increment portion of the for loop statement. Add test case that exposes this bug. Reviewed-by: George Melikov <mail@gmelikov.ru> Reviewed-by: Giuseppe Di Natale <dinatale2@llnl.gov> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Ned Bass <bass6@llnl.gov> Closes #6694 Closes #6695

When receiving a FREEOBJECTS record, receive_freeobjects() incorrectly skips a freed object in some cases. Specifically, this happens when the first object in the range to be freed doesn't exist, but the second object does. This leaves an object allocated on disk on the receiving side which is unallocated on the sending side, which may cause receiving subsequent incremental streams to fail. The bug was caused by an incorrect increment of the object index variable when current object being freed doesn't exist. The increment is incorrect because incrementing the object index is handled by a call to dmu_object_next() in the increment portion of the for loop statement. Add test case that exposes this bug. Reviewed-by: George Melikov <mail@gmelikov.ru> Reviewed-by: Giuseppe Di Natale <dinatale2@llnl.gov> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Ned Bass <bass6@llnl.gov> Closes openzfs#6694 Closes openzfs#6695

nedbass force-pushed the b_6694 branch 3 times, most recently from 904de02 to 2a0b774 Compare September 29, 2017 17:11

gmelikov approved these changes Sep 29, 2017

View reviewed changes

behlendorf requested changes Sep 29, 2017

View reviewed changes

nedbass force-pushed the b_6694 branch from 2a0b774 to 6bd6ba5 Compare September 29, 2017 21:14

behlendorf reviewed Sep 29, 2017

View reviewed changes

nedbass force-pushed the b_6694 branch from 6bd6ba5 to d79b4b3 Compare September 29, 2017 23:05

behlendorf approved these changes Sep 30, 2017

View reviewed changes

dinatale2 approved these changes Oct 2, 2017

View reviewed changes

behlendorf merged commit 39f5662 into openzfs:master Oct 2, 2017

receive_freeobjects() skips freeing some objects #6695

receive_freeobjects() skips freeing some objects #6695

Uh oh!

Conversation

nedbass commented Sep 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

Uh oh!

gmelikov commented Sep 29, 2017

Uh oh!

codecov bot commented Sep 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

nedbass commented Sep 29, 2017

Uh oh!

gmelikov left a comment

Choose a reason for hiding this comment

Uh oh!

behlendorf left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

nedbass commented Sep 29, 2017 •

edited

Loading

codecov bot commented Sep 29, 2017 •

edited

Loading