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

Allow pool names that look like Solaris disk names #11813

Merged
merged 1 commit into from Apr 1, 2021
Merged

Allow pool names that look like Solaris disk names #11813

merged 1 commit into from Apr 1, 2021

Conversation

ghost
Copy link

@ghost ghost commented Mar 29, 2021

Motivation and Context

Nothing bad happens if a prefix of your pool name matches a disk name.
This is a bit of a silly restriction at this point.

Fixes #11781

Description

Remove pool name check for Solaris disk-like prefix.

How Has This Been Tested?

Tested by reporter of #11781

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)
  • 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:

@ghost ghost added the Status: Code Review Needed Ready for review and testing label Mar 29, 2021
@behlendorf
Copy link
Contributor

Wouldn't this still lead to problems if someone tried to import the pool on illumos?

Copy link
Contributor

@ikozhukhov ikozhukhov left a comment

Choose a reason for hiding this comment

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

i do not approve it because it can lead issue on DilOS
can we discuss it first for compatibility and transparent import between platforms ?

@ghost
Copy link
Author

ghost commented Mar 29, 2021

@ikozhukhov could you explain what is the issue it causes? From what I can tell it is only enforced for cosmetics.

Nothing bad happens if a prefix of your pool name matches a disk name.
This is a bit of a silly restriction at this point.

Fixes #11781

Signed-off-by: Ryan Moeller <freqlabs@FreeBSD.org>
@ikozhukhov
Copy link
Contributor

easy explain by example where you can find VDEV name format.

root@zfs-t1:~# diskinfo
TYPE    DISK                                   VID      PID              SIZE            RMV SSD
SCSI    c0t600144F08844C0490000605C7CF20001d0  XDSK     XLUN             8G              no  no
SCSI    c0t600144F08844C0490000605C7D000002d0  XDSK     XLUN             8G              no  no
SCSI    c0t600144F08844C0490000605C7D150003d0  XDSK     XLUN             8G              no  no
SCSI    c0t600144F08844C0490000605C7D6C0004d0  XDSK     XLUN             8G              no  no
SCSI    c0t600144F08844C0490000605C7D7A0005d0  XDSK     XLUN             8G              no  no
SCSI    c0t600144F08844C0490000605C7D840006d0  XDSK     XLUN             8G              no  no
SCSI    c0t600144F08844C0490000605C7D950007d0  XDSK     XLUN             8G              no  no
SCSI    c0t600144F08844C0490000605C7DA10008d0  XDSK     XLUN             8G              no  no
SCSI    c0t600144F08844C0490000605C7DAB0009d0  XDSK     XLUN             8G              no  no
SCSI    c0t600144F08844C0490000605C7DB6000Ad0  XDSK     XLUN             8G              no  no
SCSI    c0t600144F08844C0490000605C7DC0000Bd0  XDSK     XLUN             8G              no  no
SCSI    c0t600144F08844C0490000605C7DC9000Cd0  XDSK     XLUN             8G              no  no
SATA    c1t1d0                                 VMware   Virtual SATA Ha> 40G             no  no
SATA    c1t2d0                                 VMware   Virtual SATA Ha> 8G              no  no
SATA    c1t3d0                                 VMware   Virtual SATA Ha> 8G              no  no
SATA    c1t4d0                                 VMware   Virtual SATA Ha> 8G              no  no
SATA    c1t5d0                                 VMware   Virtual SATA Ha> 10G             no  no

root@zfs-t1:~# zpool create -f c1t4d0 c1t4d0
cannot create 'c1t4d0': pool name is reserved
pool name may have been omitted

@ghost
Copy link
Author

ghost commented Mar 29, 2021

I can see it being a way to help prevent a user from accidentally creating a pool without specifying a pool name. If you patch out the check on DilOS and create a pool with a disklike name, does anything actually break?

@ikozhukhov
Copy link
Contributor

it will be really confusion of users where you want offline/add/remove/online vdev:
zpool add c1t4d0 c1t4d0
how you think it can work and described in docs ?

@ghost
Copy link
Author

ghost commented Mar 29, 2021

It wouldn't be any more confusing than if someone named their pool md0 or ada0 or da0 on FreeBSD or sda1 on Linux, which is currently possible. The issue is really that this check is overly simplistic and fails to serve its purpose on the majority of platforms. A proper device name check could be put here for each platform to implement appropriately, but as currently implemented this check is preventing perfectly reasonable pool names from being used.

@ikozhukhov
Copy link
Contributor

ikozhukhov commented Mar 29, 2021

will be much more better do not allow these pool names on all platforms where we are using OpenZFS for reduce confusions between imports of pools.
you can extend checks and propagate it to know FreeBSD and Linux platforms too.
it's my opinion

@ghost ghost closed this Mar 30, 2021
@gmelikov
Copy link
Member

@freqlabs sorry if I confused you with my thumbs up, I wanted to agree that we don't restrict any Linux/FreeBSD names and don't have any problem now, so I'm all for this PR.

@ghost
Copy link
Author

ghost commented Mar 30, 2021

I prefer this approach.

@ghost ghost reopened this Mar 30, 2021
@behlendorf
Copy link
Contributor

This seems preferable to me as well. Particularly because on Linux with the /dev/disk/by-vdev/ paths there are no restrictions on what you can name a disk. I could see an argument for issuing a warning at create time when creating a pool with one of these names, but I don't think it should be fatal. This kind of thing has always been allowed on Linux and to my knowledge it's never really been an issue.

Copy link
Member

@gmelikov gmelikov left a comment

Choose a reason for hiding this comment

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

LGTM, we may clean up other NAME_ERR_DISKLIKE parts later.

@behlendorf behlendorf added Status: Design Review Needed Architecture or design is under discussion and removed Status: Code Review Needed Ready for review and testing labels Mar 31, 2021
@behlendorf
Copy link
Contributor

There's some value in keeping the NAME_ERR_DISKLIKE part if we were to instead treat this as a warning at pool creation time and not an error.

@ghost
Copy link
Author

ghost commented Mar 31, 2021

Keeping NAME_ERR_DISKLIKE in libzfs also ensures userland will understand it on an older module, but my main motivation for leaving it in was to avoid breaking the libzfs ABI.

@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Design Review Needed Architecture or design is under discussion labels Apr 1, 2021
@behlendorf behlendorf merged commit c05eec3 into openzfs:master Apr 1, 2021
@ghost ghost deleted the disklike branch April 1, 2021 15:57
behlendorf pushed a commit that referenced this pull request Apr 7, 2021
Nothing bad happens if a prefix of your pool name matches a disk name.
This is a bit of a silly restriction at this point.

Reviewed-by: Richard Laager <rlaager@wiktel.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: George Melikov <mail@gmelikov.ru>
Signed-off-by: Ryan Moeller <freqlabs@FreeBSD.org>
Closes #11781 
Closes #11813
adamdmoss pushed a commit to adamdmoss/zfs that referenced this pull request Apr 10, 2021
Nothing bad happens if a prefix of your pool name matches a disk name.
This is a bit of a silly restriction at this point.

Reviewed-by: Richard Laager <rlaager@wiktel.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: George Melikov <mail@gmelikov.ru>
Signed-off-by: Ryan Moeller <freqlabs@FreeBSD.org>
Closes openzfs#11781 
Closes openzfs#11813
ghost pushed a commit to truenas/zfs that referenced this pull request May 6, 2021
Nothing bad happens if a prefix of your pool name matches a disk name.
This is a bit of a silly restriction at this point.

Reviewed-by: Richard Laager <rlaager@wiktel.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: George Melikov <mail@gmelikov.ru>
Signed-off-by: Ryan Moeller <freqlabs@FreeBSD.org>
Closes openzfs#11781 
Closes openzfs#11813
ghost pushed a commit to truenas/zfs that referenced this pull request May 6, 2021
Nothing bad happens if a prefix of your pool name matches a disk name.
This is a bit of a silly restriction at this point.

Reviewed-by: Richard Laager <rlaager@wiktel.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: George Melikov <mail@gmelikov.ru>
Signed-off-by: Ryan Moeller <freqlabs@FreeBSD.org>
Closes openzfs#11781 
Closes openzfs#11813
ghost pushed a commit to truenas/zfs that referenced this pull request May 6, 2021
Nothing bad happens if a prefix of your pool name matches a disk name.
This is a bit of a silly restriction at this point.

Reviewed-by: Richard Laager <rlaager@wiktel.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: George Melikov <mail@gmelikov.ru>
Signed-off-by: Ryan Moeller <freqlabs@FreeBSD.org>
Closes openzfs#11781 
Closes openzfs#11813
ghost pushed a commit to truenas/zfs that referenced this pull request May 7, 2021
Nothing bad happens if a prefix of your pool name matches a disk name.
This is a bit of a silly restriction at this point.

Reviewed-by: Richard Laager <rlaager@wiktel.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: George Melikov <mail@gmelikov.ru>
Signed-off-by: Ryan Moeller <freqlabs@FreeBSD.org>
Closes openzfs#11781 
Closes openzfs#11813
ghost pushed a commit to truenas/zfs that referenced this pull request May 10, 2021
Nothing bad happens if a prefix of your pool name matches a disk name.
This is a bit of a silly restriction at this point.

Reviewed-by: Richard Laager <rlaager@wiktel.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: George Melikov <mail@gmelikov.ru>
Signed-off-by: Ryan Moeller <freqlabs@FreeBSD.org>
Closes openzfs#11781 
Closes openzfs#11813
ghost pushed a commit to truenas/zfs that referenced this pull request May 10, 2021
Nothing bad happens if a prefix of your pool name matches a disk name.
This is a bit of a silly restriction at this point.

Reviewed-by: Richard Laager <rlaager@wiktel.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: George Melikov <mail@gmelikov.ru>
Signed-off-by: Ryan Moeller <freqlabs@FreeBSD.org>
Closes openzfs#11781 
Closes openzfs#11813
ghost pushed a commit to truenas/zfs that referenced this pull request May 10, 2021
Nothing bad happens if a prefix of your pool name matches a disk name.
This is a bit of a silly restriction at this point.

Reviewed-by: Richard Laager <rlaager@wiktel.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: George Melikov <mail@gmelikov.ru>
Signed-off-by: Ryan Moeller <freqlabs@FreeBSD.org>
Closes openzfs#11781 
Closes openzfs#11813
ghost pushed a commit to truenas/zfs that referenced this pull request May 13, 2021
Nothing bad happens if a prefix of your pool name matches a disk name.
This is a bit of a silly restriction at this point.

Reviewed-by: Richard Laager <rlaager@wiktel.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: George Melikov <mail@gmelikov.ru>
Signed-off-by: Ryan Moeller <freqlabs@FreeBSD.org>
Closes openzfs#11781 
Closes openzfs#11813
behlendorf pushed a commit that referenced this pull request May 20, 2021
Nothing bad happens if a prefix of your pool name matches a disk name.
This is a bit of a silly restriction at this point.

Reviewed-by: Richard Laager <rlaager@wiktel.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: George Melikov <mail@gmelikov.ru>
Signed-off-by: Ryan Moeller <freqlabs@FreeBSD.org>
Closes #11781 
Closes #11813
sempervictus pushed a commit to sempervictus/zfs that referenced this pull request May 31, 2021
Nothing bad happens if a prefix of your pool name matches a disk name.
This is a bit of a silly restriction at this point.

Reviewed-by: Richard Laager <rlaager@wiktel.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: George Melikov <mail@gmelikov.ru>
Signed-off-by: Ryan Moeller <freqlabs@FreeBSD.org>
Closes openzfs#11781 
Closes openzfs#11813
tonyhutter pushed a commit that referenced this pull request Jun 23, 2021
Nothing bad happens if a prefix of your pool name matches a disk name.
This is a bit of a silly restriction at this point.

Reviewed-by: Richard Laager <rlaager@wiktel.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: George Melikov <mail@gmelikov.ru>
Signed-off-by: Ryan Moeller <freqlabs@FreeBSD.org>
Closes #11781 
Closes #11813
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.

NAME_ERR_DISKLIKE is Solaris specific in module/zcommon/zfs_namecheck.c
4 participants