----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.csiden.org/r/184/#review630 -----------------------------------------------------------
General error handling question: I see that we hold references to snapshots that we need to release, but the same isn't true of bookmarks, for which we do lookups that hold references only for the duration of the lookups. It's not clear to me whether this means we get fundamentally different synchronization with bookmarks when we don't seem to hold references in the same way, and, not understanding this, I'm not clear on implications. I've read through the bookmark code, although I haven't followed that across from DSL to ZAP, and not been able to satisfy myself that this is without the potential for complication. Could you clarify as to expected mechanics? usr/src/lib/libzfs_core/common/libzfs_core.c (line 493) <https://reviews.csiden.org/r/184/#comment485> Nit: "This" needs a clearer antecedent, e.g. "Traversal for the bookmark case will result...". Otherwise it's slightly ambiguous that you mean to contrast the two to show that snapshots are more efficient than bookmarks, which is what I think is the meaning. usr/src/uts/common/fs/zfs/zfs_ioctl.c (line 5345) <https://reviews.csiden.org/r/184/#comment486> Nit: given that we're creating another path that exercises identical logic, would prefer that we add a label to the releases at the end of the function and use a goto. Centralizing this seems preferable for maintainability. usr/src/uts/common/fs/zfs/zfs_ioctl.c (line 5362) <https://reviews.csiden.org/r/184/#comment487> Same maintability nit about labelling the releases and using a goto--if what compiler optimization does is more straightforward in working with source, better to write it that way. (Fortuitous, isn't it, that all of these returns are propagating as opposed to generating errors and that none of them would therefore appear to need SET_ERROR.) - Bayard Bell On April 6, 2015, 8:12 p.m., Matthew Ahrens wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.csiden.org/r/184/ > ----------------------------------------------------------- > > (Updated April 6, 2015, 8:12 p.m.) > > > Review request for OpenZFS Developer Mailing List and Christopher Siden. > > > Bugs: 5765 > https://www.illumos.org/projects/illumos-gate//issues/5765 > > > Repository: illumos-gate > > > Description > ------- > > 5765 add support for estimating send stream size with lzc_send_space when > source is a bookmark > Reviewed by: Matthew Ahrens <[email protected]\> > Reviewed by: Christopher Siden <[email protected]\> > > Original author: Max Grossman > > Note: it would be great if someone wants to add support for this in the CLI, > via "zfs send -v -i <bookmark> <snapshot>" > > > Diffs > ----- > > usr/src/uts/common/fs/zfs/sys/dsl_dataset.h > 5bfc34ea6380f28b79ccd054957307a8ac057d0b > usr/src/uts/common/fs/zfs/sys/dmu_send.h > 3a8dc89abd4ab4ba7aa143f01abb0ffd0098a47a > usr/src/uts/common/fs/zfs/dmu_send.c > 59c6385826da515a0a9b86cecc531c95a84ec408 > usr/src/lib/libzfs_core/common/libzfs_core.c > 06221fab4ae640f88b0f155cd1ed7d8745be22bd > usr/src/uts/common/fs/zfs/zfs_ioctl.c > 20a64666a78e99b893900cf117dcd808cca3c24c > > Diff: https://reviews.csiden.org/r/184/diff/ > > > Testing > ------- > > ztest > zfs test suite > manual testing using separate c program > > http://jenkins/job/zfs-precommit/1964/ > > > Thanks, > > Matthew Ahrens > >
_______________________________________________ developer mailing list [email protected] http://lists.open-zfs.org/mailman/listinfo/developer
