Skip to content

Conversation

ixhamza
Copy link
Member

@ixhamza ixhamza commented Sep 29, 2025

Motivation and Context

If zfs_mount_and_share() fails, the error propagates to zfs create/clone commands despite successful operation. If create/clone operations were successful, there's no point in making zfs_mount_and_share() failures fatal.

Description

How Has This Been Tested?

  • CI to test.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Quality assurance (non-breaking change which makes the code more robust against bugs)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
  • Documentation (a change to man pages or other documentation)

Checklist:

@ixhamza ixhamza force-pushed the fix-create-clone-exit-code branch from e1f10e7 to 08d2caf Compare September 29, 2025 17:10
@amotin amotin added the Status: Code Review Needed Ready for review and testing label Sep 29, 2025
If zfs_mount_and_share() fails, the error propagates to zfs create/clone
commands despite successful operation. If create/clone operations were
successful, there's no point in making zfs_mount_and_share() failures
fatal.

Signed-off-by: Ameer Hamza <ahamza@ixsystems.com>
@ixhamza ixhamza force-pushed the fix-create-clone-exit-code branch from 08d2caf to b4352d1 Compare September 29, 2025 17:12
@tonyhutter
Copy link
Contributor

The error code seems valid here. The user is asking us to make a dataset/clone and then mount it, but we can't do the mount. If they had ZFS_PROP_CANMOUNT=off it would succeed.

Can you give a little more context for the change?

@ixhamza
Copy link
Member Author

ixhamza commented Sep 29, 2025

@tonyhutter - thank you for taking a look. For context, we are disabling NFS/SMB sharing at compile-time in TrueNAS, which makes zfs_mount_and_share() return an error, which is propogated to zfs create. After discussing internally with @amotin, unless we undo dataset creation, it seems wrong to return a non-zero exit code when the dataset was successfully created.

@amotin
Copy link
Member

amotin commented Sep 29, 2025

Also this is consistent with zfs set/inherit, where we don't report mount/share errors as long as the set/inherit themselves succeeded. Also note, that the mount/share errors are still reported to stderr, just not to return code.

@behlendorf
Copy link
Contributor

My concern would be for any scripts that depend on the existing behavior. Even if it's not ideal. Have you considered using zfs create -u which creates the dataset but doesn't attempt the mount? That would provide you more control for any orchestration. I suppose the problem there is zfs clone doesn't support it.

@amotin
Copy link
Member

amotin commented Oct 1, 2025

@behlendorf In this case it is primarily not about mounting, but about sharing. The fact that sharing has failed, does not mean creation has failed. From all other purposes the dataset is created. Though even mounting failure does not necessary mean creation failure, unless we roll it back.

Copy link
Contributor

@behlendorf behlendorf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be nice to have more granular zfs create|clone cli options for requesting mounting and sharing. But since we don't, I can see your point the return value should reflect result of the core operation requested.

@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Oct 1, 2025
@amotin amotin merged commit ac2d8c8 into openzfs:master Oct 2, 2025
25 of 26 checks passed
@amotin amotin deleted the fix-create-clone-exit-code branch October 2, 2025 15:25
ixhamza added a commit to truenas/zfs that referenced this pull request Oct 2, 2025
If zfs_mount_and_share() fails, the error propagates to zfs create/clone
commands despite successful operation. If create/clone operations were
successful, there's no point in making zfs_mount_and_share() failures
fatal.

Signed-off-by: Ameer Hamza <ahamza@ixsystems.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Alexander Motin <alexander.motin@TrueNAS.com>
Closes openzfs#17799
ixhamza added a commit to truenas/zfs that referenced this pull request Oct 7, 2025
If zfs_mount_and_share() fails, the error propagates to zfs create/clone
commands despite successful operation. If create/clone operations were
successful, there's no point in making zfs_mount_and_share() failures
fatal.

Signed-off-by: Ameer Hamza <ahamza@ixsystems.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Alexander Motin <alexander.motin@TrueNAS.com>
Closes openzfs#17799
ixhamza added a commit to truenas/zfs that referenced this pull request Oct 7, 2025
If zfs_mount_and_share() fails, the error propagates to zfs create/clone
commands despite successful operation. If create/clone operations were
successful, there's no point in making zfs_mount_and_share() failures
fatal.

Signed-off-by: Ameer Hamza <ahamza@ixsystems.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Alexander Motin <alexander.motin@TrueNAS.com>
Closes openzfs#17799
ixhamza added a commit to truenas/zfs that referenced this pull request Oct 7, 2025
If zfs_mount_and_share() fails, the error propagates to zfs create/clone
commands despite successful operation. If create/clone operations were
successful, there's no point in making zfs_mount_and_share() failures
fatal.

Signed-off-by: Ameer Hamza <ahamza@ixsystems.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Alexander Motin <alexander.motin@TrueNAS.com>
Closes openzfs#17799
ixhamza added a commit to truenas/zfs that referenced this pull request Oct 7, 2025
If zfs_mount_and_share() fails, the error propagates to zfs create/clone
commands despite successful operation. If create/clone operations were
successful, there's no point in making zfs_mount_and_share() failures
fatal.

Signed-off-by: Ameer Hamza <ahamza@ixsystems.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Alexander Motin <alexander.motin@TrueNAS.com>
Closes openzfs#17799
ixhamza added a commit to truenas/zfs that referenced this pull request Oct 8, 2025
If zfs_mount_and_share() fails, the error propagates to zfs create/clone
commands despite successful operation. If create/clone operations were
successful, there's no point in making zfs_mount_and_share() failures
fatal.

Signed-off-by: Ameer Hamza <ahamza@ixsystems.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Alexander Motin <alexander.motin@TrueNAS.com>
Closes openzfs#17799
ixhamza added a commit to truenas/zfs that referenced this pull request Oct 8, 2025
If zfs_mount_and_share() fails, the error propagates to zfs create/clone
commands despite successful operation. If create/clone operations were
successful, there's no point in making zfs_mount_and_share() failures
fatal.

Signed-off-by: Ameer Hamza <ahamza@ixsystems.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Alexander Motin <alexander.motin@TrueNAS.com>
Closes openzfs#17799
ixhamza added a commit to truenas/zfs that referenced this pull request Oct 10, 2025
If zfs_mount_and_share() fails, the error propagates to zfs create/clone
commands despite successful operation. If create/clone operations were
successful, there's no point in making zfs_mount_and_share() failures
fatal.

Signed-off-by: Ameer Hamza <ahamza@ixsystems.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Alexander Motin <alexander.motin@TrueNAS.com>
Closes openzfs#17799
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Status: Accepted Ready to integrate (reviewed, tested)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants