Evan Layton wrote: > >> >>> >>>> 342 - Seems like you should be calling zfs_iter_filesystems() >>>> here instead, that way it doesn't end up iterating through >>>> snapshots of this pool's BE container dataset unnecessarily, >>>> and then you can remove that code that specifically checks >>>> for this at 373-379. >>>> >>> This and the two items below are things I had been talking about doing >>> as part of the fix for 3168, 3169 and 3170. If you feel they should be >>> done as part of this I can do that but I would prefer to keep them >>> separate to make these changes a bit easier to work with. > > As far as the changes you mentioned for lines 342 and 373-379 I had a > change of heart and I agree that this fix is probably the proper > place for this. I've changed the call to zfs_iter_filesystem so we're > no longer calling this for snapshots we don't need to.
I don't think you can make this change without the other. For the single BE case now, you don't ever process the root dataset's snapshots. Maybe just do a simple if/else to call either zfs_iter_children/filesystem for the corresponding need? > I added a call > to zfs_get_type to check for the snapshots as well and moved the snapshot > processing up so that we only have to check for snapshots once. > >> The changes I'm suggesting seem more appropriately related to 3608, >> but if you were planning to do these with the other bugs then >> I'm fine with that. > > For the rest of the comments below I'd still prefer to do them as part > of the bugs mentioned before. One of the reasons for this is that I'll > be changing the structure so that the snapshots get attached to the > dataset they are a snapshot of, not just attached to the BE node. When > this change is made all the functions will also need to change. For > these changes I wanted to iterate into a dataset and from the dataset > into it's snapshots. Based on this I'd like to handle the rest of your > comments with these later changes. That sounds reasonable. Additional comments on new changes: 387 - container -> parent 408-409 - This comparison and the comment at 416 scare me. If you're properly using the zfs_iter_*() functions to recurse the datasets, there's no reason why you'd ever process a snapshot twice. Or am I missing something, can you explain? thanks, -ethan > > I've updated the webrev with the changes mentioned above. > > Thanks! > -evan > > >> >> thanks, >> -ethan >> >>>> I think there can be some further reduction done here. >>>> Instead of using be_add_children_callback() as the iteration >>>> function that processes BE roots for the case where we're >>>> listing all BEs (line 342), use be_get_node_data() as that >>>> iteration. You'd have to modify be_get_node_data() to become >>>> a callback, and inturn have it call be_add_children_callback() >>>> to process children, but that shouldn't be too hard. >>> This is definitely something I plan to do but I had purposely not done >>> this here because I planned to redo these callback functions as part >>> of the changes mentioned above. >>> >>>> This would allow you to remove all the code in >>>> be_add_children_callback() that checks if the dataset its >>>> processing is a BE root. In addition, to determine if what >>>> you're dealing with is a file system or a snapshot, you can >>>> just use zfs_get_type() instead of those ugly string searches >>>> for the characters '/' or '@' to determine that difference. >>> Definitely! I do want to remove these very ugly string searches when I >>> redo the node structure. >>> >>> Thanks! >>> -evan >>> >>>> >>>> thanks, >>>> -ethan >>>> >>>> >>>> Evan Layton wrote: >>>>> Hi, >>>>> >>>>> I need to get a review the following fix. This is one in a series >>>>> of bugs related to code refactoring in libbe. >>>>> >>>>> 3608 libbe: Remove duplicate code from be_list >>>>> http://defect.opensolaris.org/bz/show_bug.cgi?id=3608 >>>>> >>>>> Webrev: >>>>> http://cr.opensolaris.org/~evanl/3608/ >>>>> >>>>> Thanks! >>>>> -evan >>>>> _______________________________________________ >>>>> 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
