openzfs icon indicating copy to clipboard operation
openzfs copied to clipboard

Change zvol type from "Image/file" to "internal/external"

Open JMoVS opened this issue 4 years ago • 4 comments

Previously, zvols were connected "files" according to the apple specification in https://opensource.apple.com/source/IOStorageFamily/IOStorageFamily-260.100.1/IOStorageProtocolCharacteristics.h.auto.html.

Changing it from kIOPropertyInterconnectFileKey to kIOPropertyInternalExternalKey makes most sense as Apple itself writes about

kIOPropertyInternalExternalKey

This key defines the value of Internal/External for the key
kIOPropertyPhysicalInterconnectLocationKey. If the device is connected
to a bus and it is indeterminate whether it is internal or external,
this key should be set.

This would likely solve #85

Motivation and Context

Description

How Has This Been Tested?

Types of changes

  • [x] 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:

  • [x] My code follows the OpenZFS code style requirements.
  • [ ] I have updated the documentation accordingly.
  • [ ] I have read the contributing document.
  • [ ] I have added tests to cover my changes.
  • [ ] I have run the ZFS Test Suite with this change applied.
  • [ ] All commit messages are properly formatted and contain Signed-off-by.

JMoVS avatar Jul 07 '21 07:07 JMoVS

What does this really do? I don't see a difference on Catalina. Is there a difference on Big Sur? Could you show it in this conversation (include a diff -u on the "before" and "after" of diskutil info or if you feel intrepid diskutil info -plist or the relevant bits of ioreg, mostly ioreg -n pool/volname -r -l -w0).

If you can actually show a difference, I'd be happy to hoist this Catalina test VM into Big Sur to see if various things like exporting (do all the filesystems still get unmounted & ejected?) still work, the volmode property is obeyed, and so forth.

with https://github.com/openzfsonosx/openzfs/pull/86/commits/209da17c0afbcc3ccd11b61dee928905d1a8e6ff

% diskutil info disk4
   Device Identifier:         disk4
   Device Node:               /dev/disk4
   Whole:                     Yes
   Part of Whole:             disk4
   Device / Media Name:       ZVOL vmdkpool/zvtst

   Volume Name:               Not applicable (no file system)
   Mounted:                   Not applicable (no file system)
   File System:               None

   Content (IOContent):       GUID_partition_scheme
   OS Can Be Installed:       No
   Media Type:                Generic
   Protocol:                  Disk Image
   SMART Status:              Not Supported

   Disk Size:                 85.9 GB (85899345920 Bytes) (exactly 167772160 512-Byte-Units)
   Device Block Size:         512 Bytes

   Read-Only Media:           No
   Read-Only Volume:          Not applicable (no file system)

   Device Location:           External
   Removable Media:           Fixed

   Solid State:               Yes
   Virtual:                   Yes

and without

   Device Identifier:         disk4
Device Node:               /dev/disk4
Whole:                     Yes
Part of Whole:             disk4
Device / Media Name:       ZVOL vmdkpool/zvtst

Volume Name:               Not applicable (no file system)
Mounted:                   Not applicable (no file system)
File System:               None

Content (IOContent):       GUID_partition_scheme
OS Can Be Installed:       No
Media Type:                Generic
Protocol:                  Disk Image
SMART Status:              Not Supported

Disk Size:                 85.9 GB (85899345920 Bytes) (exactly 167772160 512-Byte-Units)
Device Block Size:         512 Bytes

Read-Only Media:           No
Read-Only Volume:          Not applicable (no file system)

Device Location:           External
Removable Media:           Fixed

Solid State:               Yes
Virtual:                   Yes

(they are identical per diff and eyeball, 10.15.7)

Also, this very VM's files are stored in a zvol on the Big Sur host hardware, with code lagging about a month behind the tip of the development branch. The zvol's device looks very similar:

   Device Identifier:         disk7
   Device Node:               /dev/disk7
   Whole:                     Yes
   Part of Whole:             disk7
   Device / Media Name:       ZVOL Vpool/vvol

   Volume Name:               Not applicable (no file system)
   Mounted:                   Not applicable (no file system)
   File System:               None

   Content (IOContent):       GUID_partition_scheme
   OS Can Be Installed:       No
   Media Type:                Generic
   Protocol:                  Disk Image
   SMART Status:              Not Supported

   Disk Size:                 4.4 TB (4398046511104 Bytes) (exactly 8589934592 512-Byte-Units)
   Device Block Size:         512 Bytes

   Media OS Use Only:         No
   Media Read-Only:           No
   Volume Read-Only:          Not applicable (no file system)

   Device Location:           External
   Removable Media:           Fixed

   Solid State:               Yes
   Virtual:                   Yes

So this does not seem to affect what you were hoping for. On older Big Sur, Catalina-without-patch, Catalina-with-your-patch, it's all

<key>VirtualOrPhysical</key>
        <string>Virtual</string>

I also looked a bit for ioreg differences, and did not spot any, but gave up fairly quickly. The most obvious thing that hasn't changed:

  |   "Protocol Characteristics" = {"Physical Interconnect"="Virtual Interface","Physical Interconnect Location"="External"}

rottegift avatar Jul 07 '21 19:07 rottegift

Whle you're poking around in org_openzfsonosx_zfs_zvol_device::attach(), I think it would be useful to check whether the setProperty() calls work, and IOlog if they don't (or use an ASSERT macro).

https://developer.apple.com/documentation/kernel/ioregistryentry/1811442-setproperty says they return a true-on-success bool.

rottegift avatar Jul 07 '21 20:07 rottegift

@rottegift I am currently unsure why gthe package that lundman provided doesn't work but if it's easy for you to test the latest commit, I would greatly appreciate your help - thanks for looking into my PR!

JMoVS avatar Jul 07 '21 20:07 JMoVS

Thanks @rottegift - this is indeed a draft and needs thorough testing and potentially more work - now marked as draft!

JMoVS avatar Jul 07 '21 21:07 JMoVS

PR is abonduned

JMoVS avatar Aug 03 '23 09:08 JMoVS