> On April 7, 2015, 1:20 a.m., Bayard Bell wrote: > > 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?
I assume you're looking at zfs_ioc_send_space(), where this is most obvious. Given that snapshots and bookmarks are both immutable, I don't see how there could be a problem. For example, the bookmark could be deleted after we look it up but before fully calculating the used space. You could even create another bookmark with the same name that represents a different point in time. The space estimate will still be accurate (for *some* bookmark that existed during the time we were doing the estimate). Adding more locks in the kernel can't change that (e.g. you could destroy & recreate the bookmark after the kernel returns to userland). > On April 7, 2015, 1:20 a.m., Bayard Bell wrote: > > usr/src/uts/common/fs/zfs/zfs_ioctl.c, line 5346 > > <https://reviews.csiden.org/r/184/diff/2/?file=14866#file14866line5346> > > > > 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. Sure. Taking another look at this, there's a bug where we do the return (SET_ERROR(EINVAL)) -- it doesn't drop the holds. I'll fix that too. > On April 7, 2015, 1:20 a.m., Bayard Bell wrote: > > usr/src/lib/libzfs_core/common/libzfs_core.c, line 493 > > <https://reviews.csiden.org/r/184/diff/1/?file=14508#file14508line493> > > > > 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. That's correct. Added some whitespace to hopefully clarify. - Matthew ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.csiden.org/r/184/#review630 ----------------------------------------------------------- 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
