[developer] Re: [openzfs/openzfs] 8520 lzc_rollback_to should support rolling back to origin (#434)
ahrens commented on this pull request.
looks good aside from the comment updates I noted
> + * See if the snapshot is a snapshot of the filesystem
+* or the snapshot is an origin of the filesystem.
+*/
+ if (snapds->ds_dir == ds->ds_dir ||
+ (dsl_dir_is_clone(ds->ds_dir) &&
+ dsl_dir_phys(ds->ds_dir)->dd_origin_obj ==
+ snapds->ds_object)) {
+ error = SET_ERROR(EEXIST);
+ } else {
+ error = SET_ERROR(ESRCH);
+ }
+ dsl_dataset_rele(snapds, FTAG);
+ dsl_dataset_rele(ds, FTAG);
+ return (error);
+ }
+ dsl_dataset_rele(snapds, FTAG);
}
/* must not have any bookmarks after the most recent snapshot */
while you're here, can you fix line 2545, it needs to release `ds` as well when
returning the error.
> + /* Check if the target snapshot exists at all. */
+ error = dsl_dataset_hold(dp, ddra->ddra_tosnap, FTAG, );
+ if (error != 0) {
+ /* Distinguish between missing dataset and snapshot. */
+ if (error == ENOENT || error == EXDEV)
+ error = SET_ERROR(ESRCH);
+ dsl_dataset_rele(ds, FTAG);
+ return (error);
+ }
+ ASSERT(snapds->ds_is_snapshot);
+
+ /* Check if the snapshot is the latest snapshot indeed. */
+ if (snapds != ds->ds_prev) {
+ /*
+* See if the snapshot is a snapshot of the filesystem
+* or the snapshot is an origin of the filesystem.
nit: `is *the* origin`
you might say something like, `distinguish between the case where the only
problem is intervening snapshots (EEXIST) vs the snapshot can't possibly be a
valid target for rollback (or doesn't exist) (ESRCH).`
- dsl_dataset_name(ds->ds_prev, namebuf);
- if (strcmp(namebuf, ddra->ddra_tosnap) != 0)
- return (SET_ERROR(EXDEV));
+ /* Check if the target snapshot exists at all. */
+ error = dsl_dataset_hold(dp, ddra->ddra_tosnap, FTAG, );
+ if (error != 0) {
+ /* Distinguish between missing dataset and snapshot. */
I think this is squashing `missing dataset and missing snapshot` to the same
error, not distinguishing them.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/openzfs/openzfs/pull/434#pullrequestreview-61313581
--
openzfs-developer
Archives:
https://openzfs.topicbox.com/groups/developer/discussions/T5032507ba3c52b88-M9986a7c719f13ac1bcf683b9
Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 8372 sync and async zio-s share the same task queues (#438)
ahrens approved this pull request. > @@ -1417,6 +1417,13 @@ zio_taskq_dispatch(zio_t *zio, zio_taskq_type_t q, > boolean_t cutinline) spa_t *spa = zio->io_spa; zio_type_t t = zio->io_type; int flags = (cutinline ? TQ_FRONT : 0); + zio_priority_t p = zio->io_priority; + + ASSERT(q == ZIO_TASKQ_ISSUE || q == ZIO_TASKQ_INTERRUPT); + if (p == ZIO_PRIORITY_SYNC_WRITE) + ASSERT3U(t, ==, ZIO_TYPE_WRITE); FYI, another way to do this is: `IMPLY(p == SYNC_WRITE, t == TYPE_WRITE)` -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/openzfs/openzfs/pull/438#pullrequestreview-61311784 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/T2b9fdebe0c16de6b-Mde5a1b94742088a61ef59d07 Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 640 number_to_scaled_string is duplicated in several commands (#435)
Closed #435. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/openzfs/openzfs/pull/435#event-1239343195 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/T1bb1d6d8ecb1936c-Md982f985cad73ab16e0cc7c3 Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 8081 Compiler warnings in zdb (#354)
ahrens approved this pull request. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/openzfs/openzfs/pull/354#pullrequestreview-61309811 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/T95949071eb220b14-M7aa97f0854411af31a86858e Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 8081 Compiler warnings in zdb (#354)
Squashed and rebased -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/openzfs/openzfs/pull/354#issuecomment-327882759 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/T95949071eb220b14-Ma2a2e9df5bc9968f54928fe8 Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 7584 Improve 'zpool labelclear' command (#424)
martymac commented on this pull request. > @@ -47,6 +48,7 @@ #include #include #include +#include Yes, this is needed just for VDEV_LABELS. You mean moving VDEV_LABELS definition to sys/fs/zfs.h ? I don't know what that would imply for other parts of the code :/ Help would be welcome here :) -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/openzfs/openzfs/pull/424#discussion_r137500550 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/T92ce69bb6bd4a9c6-M4fe57a2f20c29e2d371687f3 Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 7584 Improve 'zpool labelclear' command (#424)
Hi Matthew, I've pushed fixes following your recommendations, thanks :) Only remains warnings about abd stuff. Any hint ? -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/openzfs/openzfs/pull/424#issuecomment-327757235 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/T92ce69bb6bd4a9c6-M76a27d61f16e62cb3b0fb458 Powered by Topicbox: https://topicbox.com
