Fix issues with raw sends and receive_write_byref() #7701

tcaputi · 2018-07-10T21:26:40Z

This patch fixes 2 issues with raw, deduplicated send streams. The
first is that datasets who had been completely received earlier in
the stream were not still marked as raw receives. This caused
problems when newly received datasets attempted to fetch raw data
from these datasets without this flag set.

The second problem was that the arc freeze checksum code was not
consistent about which locks needed to be held while performing
its asserts. The code now guarantees that the hdr lock is held
when iterating through the linked list of buffers and the
b_freeze_lock is held when attempting to read or modify the
b_freeze_cksum. This is not strictly a problem with the write_byref
code, but it seems to be the only consistent code path to trigger
the issue.

Signed-off-by: Tom Caputi tcaputi@datto.com

Motivation and Context

Without this patch raw, deduplicated send streams would cause crashes.

How Has This Been Tested?

send-wDR_encrypted_zvol.ksh has been added

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.

ahrens · 2018-07-10T22:34:18Z

module/zfs/dmu_send.c

@@ -2846,6 +2845,9 @@ receive_write_byref(struct receive_writer_arg *rwa,
 		}
 		if (dmu_objset_from_ds(gmep->gme_ds, &ref_os))
 			return (SET_ERROR(EINVAL));
+
+		if (gmep->raw)
+			ref_os->os_raw_receive = B_TRUE;


Why are we setting this here, rather than when we create the guid_map_entry_t? At a minimum, this deserves a comment explaining why we need to set this flag.

You're right we can move it there. This used to be a long hold, so the objset was technically eligible for eviction.

ahrens · 2018-07-10T22:40:17Z

module/zfs/arc.c

@@ -1574,15 +1577,16 @@ arc_cksum_verify(arc_buf_t *buf)
 	if (!(zfs_flags & ZFS_DEBUG_MODIFY))
 		return;

+	mutex_enter(&hdr->b_l1hdr.b_freeze_lock);
 	if (ARC_BUF_COMPRESSED(buf)) {
 		ASSERT(hdr->b_l1hdr.b_freeze_cksum == NULL ||
 		    arc_hdr_has_uncompressed_buf(hdr));


Perhaps we should add an assertion to arc_cksum_verify() to check that we already have the HDR_LOCK, since we aren't grabbing it but arc_hdr_has_uncompressed_buf() requires it.

I can add that. I see one spot where that isn't true that I will correct (arc_write_done())

behlendorf · 2018-07-27T20:18:17Z

module/zfs/dmu_send.c

+		 * os_raw_receive flag now.
+		 */
+		if (raw) {
+			if (dmu_objset_from_ds(snapds, &os)) {


I'd suggest preserving and returning the real error. It's currently unused by the caller so this doesn't really matter, but I don't see a compelling reason to convert it to EINVAL.

behlendorf · 2018-07-27T20:30:46Z

module/zfs/dmu_send.c

+		if (raw) {
+			if (dmu_objset_from_ds(snapds, &os)) {
+				dsl_pool_rele(dp, FTAG);
+				return (SET_ERROR(EINVAL));


It looks to me like gmep is leaked here and the the dataset is never disowned since it's not added (correctly) to the guid_map on error.

codecov · 2018-08-16T02:53:46Z

Codecov Report

Merging #7701 into master will increase coverage by 0.07%.
The diff coverage is 75%.

@@            Coverage Diff             @@
##           master    #7701      +/-   ##
==========================================
+ Coverage   78.26%   78.34%   +0.07%     
==========================================
  Files         373      373              
  Lines      112791   112791              
==========================================
+ Hits        88279    88361      +82     
+ Misses      24512    24430      -82

Flag	Coverage Δ
#kernel	`78.77% <80.76%> (+0.32%)`	⬆️
#user	`67.16% <32.14%> (-0.12%)`	⬇️

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 4338c5c...d747d86. Read the comment docs.

behlendorf

Looks good.

behlendorf · 2018-08-16T22:10:02Z

tests/zfs-tests/tests/functional/rsend/send-wDR_encrypted_zvol.ksh

+	datasetexists $TESTPOOL/recv && \
+		log_must zfs destroy -r $TESTPOOL/recv
+	datasetexists $TESTPOOL/$TESTVOL && \
+		log_must zfs destroy -r $TESTPOOL/$TESTVOL


I'd suggest using the destroy_dataset helper here since it handles the case where the dataset is open which can happen with volumes.

destroy_dataset $TESTPOOL/recv "-r" destroy_dataset $TESTPOOL/$TESTVOL "-r"

behlendorf · 2018-08-16T23:31:02Z

module/zfs/arc.c

-	}
-
-	hash_lock = HDR_LOCK(hdr);
-	mutex_enter(hash_lock);

 	ASSERT(HDR_HAS_L1HDR(hdr));


For non-debug builds. Let's drop the local variable which is only used in the ASSERT now.

../../module/zfs/arc.c: In function 'arc_buf_freeze': ../../module/zfs/arc.c:1759:17: warning: unused variable 'hdr' [-Wunused-variable]

This patch fixes 2 issues with raw, deduplicated send streams. The first is that datasets who had been completely received earlier in the stream were not still marked as raw receives. This caused problems when newly received datasets attempted to fetch raw data from these datasets without this flag set. The second problem was that the arc freeze checksum code was not consistent about which locks needed to be held while performing its asserts. The proper locking needed to run these asserts is actually fairly nuanced, since the asserts touch the linked list of buffers (requiring the header lock), the arc_state (requiring the b_evict_lock), and the b_freeze_cksum (requiring the b_freeze_lock). This seems like a large performance sacrifice and a lot of unneeded complexity to verify that this relatively small debug feature is working as intended, so this patch simply removes these asserts instead. Signed-off-by: Tom Caputi <tcaputi@datto.com>

rincebrain mentioned this pull request Jul 10, 2018

zfs send --dedup corrupts some files #7703

Closed

tcaputi force-pushed the write_byref_fix branch from d9fd289 to 855fd78 Compare July 11, 2018 15:39

ahrens reviewed Jul 11, 2018

View reviewed changes

tcaputi force-pushed the write_byref_fix branch from 855fd78 to e84f676 Compare July 11, 2018 20:52

behlendorf reviewed Jul 27, 2018

View reviewed changes

behlendorf added Reviewed Status: Work in Progress Not yet ready for general review labels Jul 27, 2018

tcaputi force-pushed the write_byref_fix branch 5 times, most recently from 0e39b32 to 709efcf Compare August 2, 2018 19:03

tcaputi force-pushed the write_byref_fix branch 2 times, most recently from 165c523 to 7c44f91 Compare August 15, 2018 17:08

behlendorf approved these changes Aug 16, 2018

View reviewed changes

behlendorf removed the Status: Work in Progress Not yet ready for general review label Aug 16, 2018

behlendorf reviewed Aug 16, 2018

View reviewed changes

tcaputi force-pushed the write_byref_fix branch from 7c44f91 to d747d86 Compare August 17, 2018 14:53

behlendorf added the Status: Accepted Ready to integrate (reviewed, tested) label Aug 17, 2018

behlendorf merged commit 149ce88 into openzfs:master Aug 20, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix issues with raw sends and receive_write_byref() #7701

Fix issues with raw sends and receive_write_byref() #7701

tcaputi commented Jul 10, 2018

ahrens Jul 10, 2018

tcaputi Jul 11, 2018

ahrens Jul 10, 2018

tcaputi Jul 11, 2018

behlendorf Jul 27, 2018

behlendorf Jul 27, 2018

codecov bot commented Aug 16, 2018 •

edited

Loading

behlendorf left a comment

behlendorf Aug 16, 2018

behlendorf Aug 16, 2018

Fix issues with raw sends and receive_write_byref() #7701

Fix issues with raw sends and receive_write_byref() #7701

Conversation

tcaputi commented Jul 10, 2018

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Aug 16, 2018 • edited Loading

Codecov Report

behlendorf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Aug 16, 2018 •

edited

Loading