Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

destroying a snapshot does not attempt unmount #1210

Closed
nedbass opened this issue Jan 16, 2013 · 5 comments
Closed

destroying a snapshot does not attempt unmount #1210

nedbass opened this issue Jan 16, 2013 · 5 comments
Milestone

Comments

@nedbass
Copy link
Contributor

nedbass commented Jan 16, 2013

The 'zfs destroy' command fails to destroy a snapshot if the snapshot is mounted, but still returns a successful error code. I believe it should attempt to unmount the snapshot and return a 'dataset busy' or similar error if the unmount fails.

To reproduce:

$ zpool create tank /tmp/tank
$ zfs snap tank@a
$ mount -t zfs tank@a /mnt
$ zfs destroy tank@a # returns success
$ zfs list -t snapshot
NAME     USED  AVAIL  REFER  MOUNTPOINT
tank@a      0      -    30K  -

Similarly, recursively destroying a dataset or pool fails if it has a mounted snapshot.

nedbass added a commit to nedbass/zfs that referenced this issue Jan 16, 2013
A misplaced single quote caused the umount command to fail with a
syntax error when unmounting snapshots under the .zfs/snapshot
control directory.

Closes openzfs#1210
@behlendorf
Copy link
Contributor

Nice find. That cleanly explains why it would fail but not why no error was being logged. It looks like the error code should be returned by the ioctl().

@nedbass
Copy link
Contributor Author

nedbass commented Jan 16, 2013

I found that I got a 'dataset busy' error when destroying a mounted snapshot with the latest master, but not with rc13. Not sure which commit improved the error reporting. At first I suspected it was 761394b but I tried reverting it and still got the (expected) error.

@behlendorf
Copy link
Contributor

Hmm, it might have been addressed in the feature flags patches in particular the async destroy feature. Anyway, your fix is clearly right I'll get it merged.

@behlendorf
Copy link
Contributor

94a9bb4 Fix quoting error in unmount command

behlendorf pushed a commit to behlendorf/zfs that referenced this issue Jan 16, 2013
A misplaced single quote caused the umount command to fail with a
syntax error when unmounting snapshots under the .zfs/snapshot
control directory.

Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Closes openzfs#1210
@nedbass
Copy link
Contributor Author

nedbass commented Jan 16, 2013

Yes is was fixed by 9ae529e, in particular this line

9ae529#L51L941

removed a redeclaration of err inside the for loop, effectively discarding the return value of dsl_dataset_own() which is where the EBUSY error originates.

While investigating this I noticed that zfs_ioc_destroy_snaps_nvl() discards the return value of zfs_unmount_snap(), which is why the umount syntax error wasn't propagated back to the ioctl() call.

unya pushed a commit to unya/zfs that referenced this issue Dec 13, 2013
A misplaced single quote caused the umount command to fail with a
syntax error when unmounting snapshots under the .zfs/snapshot
control directory.

Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Closes openzfs#1210
pcd1193182 pushed a commit to pcd1193182/zfs that referenced this issue Sep 26, 2023
…ates (openzfs#1210)

build(deps): bump the patch group

Bumps the patch group in /cmd/zfs_object_agent with 5 updates:

| Package | From | To |
| --- | --- | --- |
| [chrono](https://github.com/chronotope/chrono) | `0.4.30` | `0.4.31` |
| [libc](https://github.com/rust-lang/libc) | `0.2.147` | `0.2.148` |
| [serde_json](https://github.com/serde-rs/json) | `1.0.106` | `1.0.107` |
| [proc-macro2](https://github.com/dtolnay/proc-macro2) | `1.0.66` | `1.0.67` |
| [unicode-ident](https://github.com/dtolnay/unicode-ident) | `1.0.11` | `1.0.12` |


Updates `chrono` from 0.4.30 to 0.4.31
- [Release notes](https://github.com/chronotope/chrono/releases)
- [Changelog](https://github.com/chronotope/chrono/blob/main/CHANGELOG.md)
- [Commits](chronotope/chrono@v0.4.30...v0.4.31)

Updates `libc` from 0.2.147 to 0.2.148
- [Release notes](https://github.com/rust-lang/libc/releases)
- [Commits](rust-lang/libc@0.2.147...0.2.148)

Updates `serde_json` from 1.0.106 to 1.0.107
- [Release notes](https://github.com/serde-rs/json/releases)
- [Commits](serde-rs/json@v1.0.106...v1.0.107)

Updates `proc-macro2` from 1.0.66 to 1.0.67
- [Release notes](https://github.com/dtolnay/proc-macro2/releases)
- [Commits](dtolnay/proc-macro2@1.0.66...1.0.67)

Updates `unicode-ident` from 1.0.11 to 1.0.12
- [Release notes](https://github.com/dtolnay/unicode-ident/releases)
- [Commits](dtolnay/unicode-ident@1.0.11...1.0.12)

---
updated-dependencies:
- dependency-name: chrono
  dependency-type: direct:production
  update-type: version-update:semver-patch
  dependency-group: patch
- dependency-name: libc
  dependency-type: direct:production
  update-type: version-update:semver-patch
  dependency-group: patch
- dependency-name: serde_json
  dependency-type: direct:production
  update-type: version-update:semver-patch
  dependency-group: patch
- dependency-name: proc-macro2
  dependency-type: indirect
  update-type: version-update:semver-patch
  dependency-group: patch
- dependency-name: unicode-ident
  dependency-type: indirect
  update-type: version-update:semver-patch
  dependency-group: patch
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants