> On Feb. 15, 2015, 2:29 p.m., Andriy Gapon wrote: > > usr/src/lib/libzfs/common/libzfs_sendrecv.c, line 628 > > <https://reviews.csiden.org/r/141/diff/1/?file=11927#file11927line628> > > > > As @dweeezil correctly noted this simple solution breaks several > > usecases. > > First, zfs recv -F is broken because to implement it the receiving side > > needs to know about all snpashots that exist on the sending side. > > Another bug is that the properties are not included into a full stream > > produced with zfs send -p pool/ds@snap. > > Most likely the original performance issue better be fixed at the > > receiving end.
Coming back to this issue after a while. I think that the biggest problem with my original patch was that it changed the content of the backup stream and that required that the receiving side is upgraded before it can properly receive the new streams. That's besides the unintentional bug. That was a very dangerous change as Tim has correctly pointed out. So now I'm returning to my alternative idea of not changing the stream content at all and instead making changes only in the receiving code. That way the policy of least astonishment should not be violated. Looking at the code I see that `recv_incremental_replication()` is called if `drr_payloadlen != 0` (and the stream is incremental), that is, if there is a serialized nvlist included with the stream begin record. That is the case even if just `-p` option (as opposed to `-R`) is used on the sending side. But, as far as I can see, the job of `recv_incremental_replication` is to handle situations like removed or renamed datasets, etc. None of those things are applicable if the stream has been generated with `-p [-i|-I]`. That would be an incremental stream for a single dataset just with the snapshot properties included. So, IMO, there is no need to apply all the `recv_incremental_replication` machinery in that case. And it is that function that (re-)sets properties of all the already existing snapshots. Finally, it should be noted that `zfs_receive_one()` already has the code to set the received properties, if any, for a newly received dataset. - Andriy ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.csiden.org/r/141/#review522 ----------------------------------------------------------- On Dec. 2, 2014, 2:11 p.m., Justin Gibbs wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.csiden.org/r/141/ > ----------------------------------------------------------- > > (Updated Dec. 2, 2014, 2:11 p.m.) > > > Review request for OpenZFS Developer Mailing List. > > > Bugs: 5380 > https://www.illumos.org/issues/5380 > > > Repository: illumos-gate > > > Description > ------- > > ... as opposed to sending properties of all snapshots of the relevant > filesystem. The previous behavior results in properties being set on > all snapshots on the receiving side, which is quite slow. > > Behavior of zfs send -R is not changed. > > Note that send_iterate_fs() now has to iterate snapshots in their > creation order. > > > Diffs > ----- > > usr/src/lib/libzfs/common/libzfs_sendrecv.c > c4944438aa2cdcb751ae04a73be52ce9d7b2cef4 > > Diff: https://reviews.csiden.org/r/141/diff/ > > > Testing > ------- > > Only FreeBSD and ZoL. None for illumos. > > > Thanks, > > Justin Gibbs > >
_______________________________________________ developer mailing list [email protected] http://lists.open-zfs.org/mailman/listinfo/developer
