Bug 249438

Summary: [ZFS] Regression in recv_incremental_replication() after MFC r354116, r354120
Product: Base System Reporter: Dmitry Wagin <dmitry.wagin>
Component: kernAssignee: Alan Somers <asomers>
Status: Open ---    
Severity: Affects Some People CC: andriys, asomers, emaste, frans-jan, freebsd.org, kimmo, mmacy, so, terry-freebsd, tsuroerusu, vsasjason
Priority: --- Keywords: regression
Version: 12.1-STABLE   
Hardware: Any   
OS: Any   
Attachments:
Description Flags
libzfs_sendrecv.c.diff
none
Fix error merging r354116 from OpenZFS
none
Partially complete errata notice none

Description Dmitry Wagin 2020-09-18 15:33:39 UTC
Created attachment 218061 [details]
libzfs_sendrecv.c.diff

After https://svnweb.freebsd.org/base?view=revision&revision=354568

> zfs send -R -I pool/test1@snap | zfs recv -F pool/test2

works incorrectly.

Snapshots deleted on pool/test1 are not deleted on pool/test2.
Comment 1 Dmitry Wagin 2020-10-17 10:22:09 UTC
ping
Comment 2 Alan Somers freebsd_committer 2020-10-17 13:40:30 UTC
"Works incorrectly" is not a bug report.  When reporting a bug to FreeBSD or any other project, you should provide a complete description of the problem, including exact steps to reproduce it, the problem's symptoms, and what you expect to happen instead.
Comment 3 Dmitry Wagin 2020-10-19 13:05:33 UTC
(In reply to Alan Somers from comment #2)
>Snapshots deleted on pool/test1 are not deleted on pool/test2.


ALL steps:

> zfs create pool/test1
> zfs snap pool/test1@snap1
> zfs send -R pool/test1@snap1 | zfs recv -F pool/test2
> zfs snap pool/test1@snap2
> zfs send -R -I pool/test1@snap1 pool/test1@snap2 | zfs recv -F pool/test2
> zfs destroy pool/test1@snap1
> zfs snap pool/test1@snap3
> zfs send -R -I pool/test1@snap2 pool/test1@snap3 | zfs recv -F pool/test2
^^^ after last step,
@snap1 should also be deleted on pool/test2.
Comment 4 Alan Somers freebsd_committer 2020-10-19 15:55:24 UTC
Reproduced on stable/12 at r366189M.  Cannot reproduce on head.
Comment 5 Dmitry Wagin 2020-10-19 16:01:30 UTC
(In reply to Alan Somers from comment #4)

HEAD doesn't have this typo.
Comment 6 Alan Somers freebsd_committer 2020-10-21 20:36:25 UTC
Created attachment 218957 [details]
Fix error merging r354116 from OpenZFS

When we merged 4c0883fb4af0d5565459099b98fcf90ecbfa1ca1 from OpenZFS (svn r354116), there were some merge conflicts.  One of those was resolved incorrectly, causing "zfs receive" to fail to delete snapshots that a "zfs send -R" stream has deleted.

This change corrects that merge conflict, and also reduces some harmless diffs vis-a-vis OpenZFS that were also introduced by r354116.  Direct commit to stable/12 because head has moved on to OpenZFS.
Comment 7 andriys 2020-11-15 10:24:24 UTC
Is this going to be fixed, and in case it is, will it be merged to 12.2? I am asking because 12.1 is approaching its EoL, but this issue seems to be serious enough for me to refrain from upgrading to 12.2.
Comment 8 Terry Kennedy 2020-11-15 21:08:08 UTC
(In reply to andriys from comment #7)

It doesn't seem to have made it into 12.2-RELEASE as the last code commit to the affected file in the release/12.2.0 tree was on October 6th. I don't know what the plans are to include it in 12.2-p<something>, if any.

You might either need to hand merge it yourself or use 12-STABLE instead of 12.2-RELEASE. Note that if you choose this path, freebsd-update will no longer work correctly on your system(s) as it only supports -RELEASE and -RELEASE-p<something> systems, except when it says it specifically supports beta releases during the beta period.
Comment 9 commit-hook freebsd_committer 2020-12-01 15:16:09 UTC
A commit references this bug:

Author: asomers
Date: Tue Dec  1 15:15:19 UTC 2020
New revision: 368233
URL: https://svnweb.freebsd.org/changeset/base/368233

Log:
  Fix error merging r354116 from OpenZFS

  When we merged 4c0883fb4af0d5565459099b98fcf90ecbfa1ca1 from OpenZFS (svn
  r354116), there were some merge conflicts.  One of those was resolved
  incorrectly, causing "zfs receive" to fail to delete snapshots that a "zfs
  send -R" stream has deleted.

  This change corrects that merge conflict, and also reduces some harmless
  diffs vis-a-vis OpenZFS that were also introduced by the same revision.
  Direct commit to stable/12 because head has moved on to OpenZFS.

  PR:		249438
  Reported by:	Dmitry Wagin <dmitry.wagin@ya.ru>
  Reviewed by:	mmacy
  Sponsored by:	Axcient

Changes:
  stable/12/cddl/contrib/opensolaris/lib/libzfs/common/libzfs_sendrecv.c
  stable/12/sys/cddl/contrib/opensolaris/uts/common/sys/fs/zfs.h
Comment 10 tsuroerusu 2020-12-02 00:19:02 UTC
Is there any ETA for this fix being issued as an errata for 12.2-RELEASE now that has been committed to stable/12 ?
Comment 11 Alan Somers freebsd_committer 2020-12-02 00:42:42 UTC
Only the security officer, so@FreeBSD.org, can approve an errata.  Dear Security Officer (CC'd), do you consider this bug serious enough to warrant an errata?  The bad stuff is that ZFS snapshots could be wrongly left around after a zfs recv.
Comment 12 tsuroerusu 2020-12-02 00:52:59 UTC
(In reply to Alan Somers from comment #11)
Just to add my two cents, I find this to be a pretty big issue because if somebody has an entire setup with a bunch of automated scripts set up to log and/or handle any errors, like snapshots not being deleted, this would mess that up.

Frankly, as I understand this issue, it could even be said that this is, de facto, a new behaviour of "zfs recv" in specific situations in the -STABLE branch which goes against the classic FreeBSD ethos of being predictable and not introducing breaking changes and behaviours overnight.

By the way, thanks for your efforts in resolving the issue.
Comment 13 Ed Maste freebsd_committer 2020-12-02 01:45:06 UTC
(In reply to Alan Somers from comment #11)
The errata template is available at https://www.freebsd.org/security/errata-template.txt; if you can fill out as much of that as possible (in particular the background, impact etc. sections) secteam can take care of including it in the next errata batch.
Comment 14 Alan Somers freebsd_committer 2020-12-02 02:38:03 UTC
Created attachment 220151 [details]
Partially complete errata notice

Here is a partially completed errata notice that secteam can use, if they decide to issue an errata for this.