As per the previous comments, here is the webrev for just 3889. Webrev: ---------- http://cr.opensolaris.org/~equach/webrev.3889
Defect: -------- 3889 be_create_snapshot fails when running pkg image-update http://defect.opensolaris.org/bz/show_bug.cgi?id=3889 thanks, -ethan Evan Layton wrote: > Ethan Quach wrote: >> >> >> Evan Layton wrote: >>> HI Ethan, >>> >>> This looks really good. I do have a question though. >>> >>> On line 876 it's setting be_created if we get a failure back from >>> be_clone_fs_callback. If this fails then we haven't created the BE >>> but we're still setting be_created, doing the goto done and >>> attempting to clean-up the BE. >> >> That's an incorrect assumption; if the call to be_clone_fs_callback() >> fails, >> that doesn't mean no datasets for the new BE have been created yet. For >> example it could have succeeded in creating the new root dataset, but >> failed >> in cloning one of the subordinates. The places where be_created is set >> are the places where datasets have potentially been created. > > Yes I realized after I sent the question that it was at best incorrect... > >> >> Though I just realized there could be a race condition. If another >> process >> created a BE name "foobar" just as this code was running, >> be_clone_fs_callback() >> would fail and this code would end up destroying the "foobar" BE just >> created >> by the other process. I think this is going to need some rework. > > Good catch! > >> >>> I can understand if we've at least >>> created one of a set of needed snapshots (or clones) we need to clean >>> up but what happens in the case where no snapshots (or clones) were >>> created? >> >> For cases where we're sure no datasets have been created, be_created >> doesn't >> get set. Those are 893, 921, 957. > > Ah ok I see what you're getting at there and it makes sense to me now. > Thanks for the explanation! > >> >> >> Anyway, as per my comment about, I'm going to separate 3844 and 3889 >> (the >> latter being a much simpler fix), so that 3889 can get in earlier >> than later. >> I'll resend 3889 in just a few... > > Sounds good! > > -evan > >> >> >> thanks, >> -ethan >> >> >>> I guess that gets covered in _be_destroy in that the first >>> zfs_open will fail? >>> Anyway the changes look fine, I just wanted to make sure this >>> scenario >>> was covered. >>> >>> Thanks, >>> -evan >>> >>> Ethan Quach wrote: >>> >>>> Two more fixes in this webrev. The first defect fixes the case where >>>> creating a BE will silently ignore failures in creating corresponding >>>> zones in the new BE. It will now stop the creation process and report >>>> error. be_copy() will now also cleanup a partially created boot >>>> environment upon any failure that happens during the creation >>>> process instead of leaving it lying around. >>>> >>>> The second defect is a regression due to 1197. The additional test >>>> case that caught this was doing a pkg image-update from a system >>>> where the pool is running less then the required pool version that >>>> supports snapshot user properties. >>>> >>>> >>>> >>>> Webrev: >>>> ---------- >>>> http://cr.opensolaris.org/~equach/webrev.3844.3889 >>>> >>>> Defects: >>>> ---------- >>>> 3884 be_copy_zones() silently ignores zones >>>> http://defect.opensolaris.org/bz/show_bug.cgi?id=3844 >>>> >>>> 3889 be_create_snapshot fails when running pkg image-update >>>> http://defect.opensolaris.org/bz/show_bug.cgi?id=3889 >>>> >>>> >>>> thanks, >>>> -ethan >>>> >>>> _______________________________________________ >>>> caiman-discuss mailing list >>>> caiman-discuss at opensolaris.org >>>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >>>> >>> >>> _______________________________________________ >>> caiman-discuss mailing list >>> caiman-discuss at opensolaris.org >>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >>> >
