[developer] Re: [openzfs/openzfs] 8414 Implemented zpool scrub pause/resume (#407)
@rottegift yes, I've tried this. The paused scrub will just resume on import (instead of staying paused) on older systems. -- 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/407#issuecomment-324113936 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/T693650e65dc896fc-M64ed311ba026884c263512a8 Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 8414 Implemented zpool scrub pause/resume (#407)
Sorry I'm late to the party but has anyone tried pausing a scrub, shutting the system down, and booting a kernel that does not understand paused scrubs (the older and less related to the normal kernel the better; ideally this could be cross-platform testable with an image of a pool which is in the middle of a suspended scrub) ? -- 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/407#issuecomment-324077689 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/T693650e65dc896fc-Me1bfa49ca613838e434663be Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 8414 Implemented zpool scrub pause/resume (#407)
Closed #407 via c296160769c460efd21c8619cb23f91525e1e9d5. -- 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/407#event-1215143893 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/T693650e65dc896fc-M43f6911bf304009694df598d Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 8414 Implemented zpool scrub pause/resume (#407)
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/407#pullrequestreview-55611397 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/T693650e65dc896fc-M572e1fb51a7632059065c924 Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 8414 Implemented zpool scrub pause/resume (#407)
alek-p commented on this pull request.
> + } else if (*cmd != POOL_SCRUB_NORMAL) {
+ return (SET_ERROR(ENOTSUP));
+ }
+
+ return (0);
+}
+
+static void
+dsl_scrub_pause_resume_sync(void *arg, dmu_tx_t *tx)
+{
+ pool_scrub_cmd_t *cmd = arg;
+ dsl_pool_t *dp = dmu_tx_pool(tx);
+ spa_t *spa = dp->dp_spa;
+ dsl_scan_t *scn = dp->dp_scan;
+
+
looks like I have an extra blank line here, I'll remove that in a min
--
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/407#pullrequestreview-49300907
--
openzfs-developer
Archives:
https://openzfs.topicbox.com/groups/developer/discussions/T693650e65dc896fc-M57b16702199bc78c5d6b8640
Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 8414 Implemented zpool scrub pause/resume (#407)
brad-lewis 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/407#pullrequestreview-49254397 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/T693650e65dc896fc-M66d9fdb6f42734cb8c61d036 Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 8414 Implemented zpool scrub pause/resume (#407)
sdimitro 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/407#pullrequestreview-48710702 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/T693650e65dc896fc-Ma649308d80df3629bd77a84e Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 8414 Implemented zpool scrub pause/resume (#407)
sdimitro commented on this pull request. - if (zfs_ioctl(hdl, ZFS_IOC_POOL_SCAN, ) == 0 || - (errno == ENOENT && func != POOL_SCAN_NONE)) + /* ECANCELED on a scrub means we resumed a paused scrub */ Yeah I figured that was the case. I just wanted to ask first. Thank you Alek! -- 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/407#discussion_r125983445 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/T693650e65dc896fc-M832ea552432231578d273bae Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 8414 Implemented zpool scrub pause/resume (#407)
alek-p commented on this pull request. > .\" -.Dd Oct 2, 2016 +.Dd June 21, 2016 Thanks, I'll fix that in a couple min. -- 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/407#discussion_r125970940 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/T693650e65dc896fc-Mfc741c56e135d81e44a1c5d4 Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 8414 Implemented zpool scrub pause/resume (#407)
alek-p commented on this pull request. - if (zfs_ioctl(hdl, ZFS_IOC_POOL_SCAN, ) == 0 || - (errno == ENOENT && func != POOL_SCAN_NONE)) + /* ECANCELED on a scrub means we resumed a paused scrub */ I agree with you that using errno here isn't the cleanest way to do this. ECANCELED doesn't quite fit but seems to be the best value aside from ERESTART which is off the table for the reason I've mentioned above. Looking at the way zc_nvlist_dst is used I'm not convinced it would make things cleaner, however. It also seems to be a bit of an overkill. This is why I'm inclined to follow the precedent set in the existing code and continuing using errno to pass results back. -- 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/407#discussion_r125970949 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/T693650e65dc896fc-Mf70e0969442a1af2c6c13636 Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 8414 Implemented zpool scrub pause/resume (#407)
sdimitro commented on this pull request. LGTM overall. I just have one question and a small nit. > .\" -.Dd Oct 2, 2016 +.Dd June 21, 2016 [nit] 2017 - if (zfs_ioctl(hdl, ZFS_IOC_POOL_SCAN, ) == 0 || - (errno == ENOENT && func != POOL_SCAN_NONE)) + /* ECANCELED on a scrub means we resumed a paused scrub */ Even though this comment makes everything clear with respect to the rest of the code, I found it a bit confusing that ECANCELED means resume a paused scrub. Skimming through other errnos the best alternative I could find is EADV, that I read as EADVANCE but apparently is EADVERTISE which is obscure. That said, I wonder if we could make use of zc.zc_nvlist_dst in the kernel and back to the userland to deal with both the ECANCELED and the ENOENT case later in this code path, in a cleaner way. Did we consider that? -- 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/407#pullrequestreview-48383427 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/T693650e65dc896fc-Ma394423fbf836b08aea75c70 Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 8414 Implemented zpool scrub pause/resume (#407)
@avg-I yes it is -- 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/407#issuecomment-311765827 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/T693650e65dc896fc-Maf73b078a6d385db63fc4e29 Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 8414 Implemented zpool scrub pause/resume (#407)
I hear that there is some work on optimizing the scrub performance in the pipeline. I am curious if this change is compatible with that work... -- 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/407#issuecomment-311764457 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/T693650e65dc896fc-M3712c27cb9dd39b6e51292a4 Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 8414 Implemented zpool scrub pause/resume (#407)
Thanks for the link @loli10K! The problem was something else: ``` 23:06:48.76 SUCCESS: is_pool_scrub_paused testpool 23:06:50.01 cannot scrub testpool: currently scrubbing; use 'zpool scrub -s' to cancel current scrub ``` So it's the scrub resume that "wasn't working". In fact the resume was working but it was issuing two resume IOCTLs per one scrub pause cmd. The second one would see the restarted scrub and bail with an error. Looks like illumos will re-issue a syscall when it returns ERESTART (probably what the OS should do). I've changed the scrub resume to use ECANCELED instead of ERESTART and it looks to be working as expected now. If the tests look good I'll make the same change in ZoL to keep the code consistent. -- 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/407#issuecomment-310740392 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/T693650e65dc896fc-M1c2d438b811de624fe6066f1 Powered by Topicbox: https://topicbox.com
