Ethan Quach wrote: > > 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? >
Actually this does work because the zfs_iter_filesystem is only used for the initial root dataset. All subsequent datasets and snapshots are processed inside be_add_children which calls zfs_iter_children. Because of this the snapshots for the single BE case are processed as well. > >> 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 fixed > > 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? This is a case of my paranoia. We should never get into a situation where we're already processed a snapshot and need to skip it but just in case I added this to make sure in the very unlikely event that we did already have this snapshot that it didn't get added twice. I can either remove this code or add to the comment the fact that we should never get into this situation. Thanks! -evan > > > 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
