From 4cfc086e4ddde787f99489dcfa3cfb0d6127d64c Mon Sep 17 00:00:00 2001 From: Ned Bass Date: Mon, 2 Oct 2017 15:36:04 -0700 Subject: [PATCH] receive_freeobjects() skips freeing some objects 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 Reviewed-by: Giuseppe Di Natale Reviewed-by: Brian Behlendorf Signed-off-by: Ned Bass Closes #6694 Closes #6695 --- module/zfs/dmu_send.c | 6 +- tests/runfiles/linux.run | 2 +- .../tests/functional/rsend/Makefile.am | 3 +- .../functional/rsend/send_freeobjects.ksh | 81 +++++++++++++++++++ 4 files changed, 86 insertions(+), 6 deletions(-) create mode 100755 tests/zfs-tests/tests/functional/rsend/send_freeobjects.ksh diff --git a/module/zfs/dmu_send.c b/module/zfs/dmu_send.c index a2ace9be51..0c53ced0ff 100644 --- a/module/zfs/dmu_send.c +++ b/module/zfs/dmu_send.c @@ -2233,12 +2233,10 @@ receive_freeobjects(struct receive_writer_arg *rwa, int err; err = dmu_object_info(rwa->os, obj, &doi); - if (err == ENOENT) { - obj++; + if (err == ENOENT) continue; - } else if (err != 0) { + else if (err != 0) return (err); - } err = dmu_free_long_object(rwa->os, obj); if (err != 0) diff --git a/tests/runfiles/linux.run b/tests/runfiles/linux.run index a12fc2d43f..5583a25541 100644 --- a/tests/runfiles/linux.run +++ b/tests/runfiles/linux.run @@ -501,7 +501,7 @@ tests = ['rsend_001_pos', 'rsend_002_pos', 'rsend_003_pos', 'rsend_004_pos', 'send-c_lz4_disabled', 'send-c_recv_lz4_disabled', 'send-c_mixed_compression', 'send-c_stream_size_estimate', 'send-cD', 'send-c_embedded_blocks', 'send-c_resume', 'send-cpL_varied_recsize', - 'send-c_recv_dedup'] + 'send-c_recv_dedup', 'send_freeobjects'] [tests/functional/scrub_mirror] tests = ['scrub_mirror_001_pos', 'scrub_mirror_002_pos', diff --git a/tests/zfs-tests/tests/functional/rsend/Makefile.am b/tests/zfs-tests/tests/functional/rsend/Makefile.am index b9f8dba658..60ca776ba0 100644 --- a/tests/zfs-tests/tests/functional/rsend/Makefile.am +++ b/tests/zfs-tests/tests/functional/rsend/Makefile.am @@ -37,4 +37,5 @@ dist_pkgdata_SCRIPTS = \ send-c_verify_ratio.ksh \ send-c_volume.ksh \ send-c_zstreamdump.ksh \ - send-cpL_varied_recsize.ksh + send-cpL_varied_recsize.ksh \ + send_freeobjects.ksh diff --git a/tests/zfs-tests/tests/functional/rsend/send_freeobjects.ksh b/tests/zfs-tests/tests/functional/rsend/send_freeobjects.ksh new file mode 100755 index 0000000000..6533352a9a --- /dev/null +++ b/tests/zfs-tests/tests/functional/rsend/send_freeobjects.ksh @@ -0,0 +1,81 @@ +#!/bin/ksh + +# +# This file and its contents are supplied under the terms of the +# Common Development and Distribution License ("CDDL"), version 1.0. +# You may only use this file in accordance with the terms of version +# 1.0 of the CDDL. +# +# A full copy of the text of the CDDL should have accompanied this +# source. A copy of the CDDL is also available via the Internet at +# http://www.illumos.org/license/CDDL. +# + +# +# Copyright (c) 2017 by Lawrence Livermore National Security, LLC. +# + +. $STF_SUITE/include/libtest.shlib +. $STF_SUITE/tests/functional/rsend/rsend.kshlib + +# +# Description: +# Verify FREEOBJECTS record frees sequential objects (See +# https://github.com/zfsonlinux/zfs/issues/6694) +# +# Strategy: +# 1. Create three files with sequential object numbers, f1 f2 and f3 +# 2. Delete f2 +# 3. Take snapshot A +# 4. Delete f3 +# 5. Take snapshot B +# 6. Receive a full send of A +# 7. Receive an incremental send of B +# 8. Fail test if f3 exists on received snapshot B +# + +verify_runnable "both" + +log_assert "Verify FREEOBJECTS record frees sequential objects" + +sendds=sendfo +recvds=recvfo +f1=/$POOL/$sendds/f1 +f2=/$POOL/$sendds/f2 +f3=/$POOL/$sendds/f3 + +# +# We need to set xattr=sa and dnodesize=legacy to guarantee sequential +# object numbers for this test. Otherwise, if we used directory-based +# xattrs, SELinux extended attributes might consume intervening object +# numbers. +# +log_must zfs create -o xattr=sa -o dnodesize=legacy $POOL/$sendds + +tries=100 +for ((i=0; i<$tries; i++)); do + 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}') + + if [[ $o2 -ne $(( $o1 + 1 )) ]] || [[ $o3 -ne $(( $o2 + 1 )) ]]; then + rm -f $f1 $f2 $f3 + else + break + fi +done + +if [[ $i -eq $tries ]]; then + log_fail "Failed to create three sequential objects" +fi + +log_must rm $f2 +log_must zfs snap $POOL/$sendds@A +log_must rm $f3 +log_must zfs snap $POOL/$sendds@B +log_must eval "zfs send $POOL/$sendds@A | zfs recv $POOL/$recvds" +log_must eval "zfs send -i $POOL/$sendds@A $POOL/$sendds@B |" \ + "zfs recv $POOL/$recvds" +log_mustnot zdb $POOL/$recvds@B $o3 +log_pass "Verify FREEOBJECTS record frees sequential objects"