NAS-132959 / 25.04 / Mark extent read-only if underlying zvol is R/O (#…

…15195)

* Update extent ro prop if zvol readonly changes.

* Update zvol readonly prop if extent is ro changed

* Add unit test test__target_readonly_extent

---------

Co-authored-by: Caleb <[email protected]>

src/middlewared/middlewared/plugins/iscsi_/extents.py

@@ -90,8 +90,8 @@ async def do_create(self, data):
 
         `ro` when set to true prevents the initiator from writing to this LUN.
         """
+        await self.validate(data)
         verrors = ValidationErrors()
-        await self.middleware.call('iscsi.extent.validate', data)
         await self.clean(data, 'iscsi_extent_create', verrors)
         verrors.check()
 
@@ -100,7 +100,16 @@ async def do_create(self, data):
         # This change is being made in conjunction with threads_num being specified in scst.conf
         if data['type'] == 'DISK' and data['path'].startswith('zvol/'):
             zvolname = zvol_path_to_name(os.path.join('/dev', data['path']))
-            await self.middleware.call('zfs.dataset.update', zvolname, {'properties': {'volthreading': {'value': 'off'}}})
+            await self.middleware.call(
+                'zfs.dataset.update',
+                zvolname,
+                {
+                    'properties': {
+                        'volthreading': {'value': 'off'},
+                        'readonly': {'value': 'on' if data['ro'] else 'off'}
+                    }
+                }
+            )
 
         data['id'] = await self.middleware.call(
             'datastore.insert', self._config.datastore, {**data, 'vendor': 'TrueNAS'},
@@ -127,15 +136,26 @@ async def do_update(self, audit_callback, id_, data):
         new.update(data)
 
         await self.middleware.call('iscsi.extent.validate', new)
-        await self.clean(
-            new, 'iscsi_extent_update', verrors, old=old
-        )
+        await self.clean(new, 'iscsi_extent_update', verrors, old=old)
         verrors.check()
 
         await self.middleware.call('iscsi.extent.save', new, 'iscsi_extent_create', verrors, old)
         verrors.check()
         new.pop(self.locked_field)
 
+        zvolpath = new.get('path')
+        if zvolpath is not None and zvolpath.startswith('zvol/'):
+            zvolname = zvol_path_to_name(os.path.join('/dev', zvolpath))
+            await self.middleware.call(
+                'zfs.dataset.update',
+                zvolname,
+                {
+                    'properties': {
+                        'readonly': {'value': 'on' if new['ro'] else 'off'}
+                    }
+                }
+            )
+
         await self.middleware.call(
             'datastore.update',
             self._config.datastore,
@@ -146,6 +166,9 @@ async def do_update(self, audit_callback, id_, data):
 
         await self._service_change('iscsitarget', 'reload')
 
+        # scstadmin can have issues when modifying an existing extent, re-run
+        await self._service_change('iscsitarget', 'reload')
+
         return await self.get_instance(id_)
 
     @api_method(
@@ -200,8 +223,8 @@ async def do_delete(self, audit_callback, id_, remove, force):
                 await self.middleware.call('iscsi.alua.wait_for_alua_settled')
 
     @private
-    def validate(self, data):
-        data['serial'] = self.extent_serial(data['serial'])
+    async def validate(self, data):
+        data['serial'] = await self.extent_serial(data['serial'])
         data['naa'] = self.extent_naa(data.get('naa'))
 
     @private
@@ -367,10 +390,10 @@ def clean_size(self, data, schema_name, verrors):
         return data
 
     @private
-    def extent_serial(self, serial):
+    async def extent_serial(self, serial):
         if serial in [None, '']:
             used_serials = [i['serial'] for i in (
-                self.middleware.call_sync('iscsi.extent.query', [], {'select': ['serial']})
+                await self.middleware.call('iscsi.extent.query', [], {'select': ['serial']})
             )]
             tries = 5
             for i in range(tries):

src/middlewared/middlewared/plugins/iscsi_/global_linux.py

@@ -3,6 +3,7 @@
 
 from middlewared.schema import Bool, Dict, Int, Str
 from middlewared.service import filterable, filterable_returns, private, Service
+from middlewared.service_exception import MatchNotFound
 from middlewared.utils import filter_list, run
 
 
@@ -94,6 +95,20 @@ def sessions(self, filters, options):
                 sessions.append(session_dict)
         return filter_list(sessions, filters, options)
 
+    @private
+    def resync_readonly_property_for_zvol(self, id_, read_only_value):
+        try:
+            extent = self.middleware.call_sync(
+                'iscsi.extent.query',
+                [['enabled', '=', True], ['path', '=', f'zvol/{id_}']],
+                {'get': True}
+            )
+            ro = True if read_only_value.lower() == 'on' else False
+            if extent['ro'] != ro:
+                self.middleware.call_sync('iscsi.extent.update', extent['id'], {'ro': ro})
+        except MatchNotFound:
+            return
+
     @private
     def resync_lun_size_for_zvol(self, id_):
         if not self.middleware.call_sync('service.started', 'iscsitarget'):

src/middlewared/middlewared/plugins/pool_/dataset.py

@@ -928,10 +928,18 @@ async def do_update(self, audit_callback, id_, data):
             verrors.add_child('pool_dataset_update', self.__handle_zfs_set_property_error(e, properties_definitions))
             raise verrors
 
-        if data['type'] == 'VOLUME' and 'volsize' in data and data['volsize'] > dataset[0]['volsize']['parsed']:
-            # means the zvol size has increased so we need to check if this zvol is shared via SCST (iscsi)
-            # and if it is, resync it so the connected initiators can see the new size of the zvol
-            await self.middleware.call('iscsi.global.resync_lun_size_for_zvol', id_)
+        if data['type'] == 'VOLUME':
+            if 'volsize' in data and data['volsize'] > dataset[0]['volsize']['parsed']:
+                # means the zvol size has increased so we need to check if this zvol is shared via SCST (iscsi)
+                # and if it is, resync it so the connected initiators can see the new size of the zvol
+                await self.middleware.call('iscsi.global.resync_lun_size_for_zvol', id_)
+
+            if 'readonly' in data:
+                # depending on the iscsi client connected to us, if someone marks a zvol
+                # as R/O (or R/W), we need to be sure and update the associated extent so
+                # that we don't get into a scenario where the iscsi extent is R/W but the
+                # underlying zvol is R/O. Windows clients seem to not handle this very well.
+                await self.middleware.call('iscsi.global.resync_readonly_property_for_zvol', id_, data['readonly'])
 
         updated_ds = await self.get_instance(id_)
         self.middleware.send_event('pool.dataset.query', 'CHANGED', id=id_, fields=updated_ds)

tests/api2/test_261_iscsi_cmd.py

@@ -20,7 +20,7 @@
 from auto_config import ha, hostname, isns_ip, password, pool_name, user
 from functions import SSH_TEST
 from protocols import ISCSIDiscover, initiator_name_supported, iscsi_scsi_connection, isns_connection
-from pyscsi.pyscsi.scsi_sense import sense_ascq_dict
+from pyscsi.pyscsi.scsi_sense import sense_ascq_dict, sense_key_dict
 from pytest_dependency import depends
 
 from middlewared.service_exception import InstanceNotFound, ValidationError, ValidationErrors
@@ -218,8 +218,16 @@ def get_client_count():
     return call('iscsi.global.client_count')
 
 
+def get_zvol_property(zvolid, property_name):
+    return call('zfs.dataset.query', [['id', '=', zvolid]], {'get': True})['properties'][property_name]['value']
+
+
 def get_volthreading(zvolid):
-    return call('zfs.dataset.query', [['id', '=', zvolid]], {'get': True})['properties']['volthreading']['value']
+    return get_zvol_property(zvolid, 'volthreading')
+
+
+def get_readonly(zvolid):
+    return get_zvol_property(zvolid, 'readonly')
 
 
 def verify_client_count(count, retries=10):
@@ -388,6 +396,14 @@ def expect_check_condition(s, text=None, check_type=CheckType.CHECK_CONDITION):
             s.testunitready()
 
 
+@contextlib.contextmanager
+def raises_check_condition(sense_key, ascq):
+    with pytest.raises(Exception) as excinfo:
+        yield
+    e = excinfo.value
+    assert f"Check Condition: {sense_key_dict[sense_key]}(0x{sense_key:02X}) ASC+Q:{sense_ascq_dict[ascq]}(0x{ascq:04X})" == str(e)
+
+
 def _verify_inquiry(s):
     """
     Verify that the supplied SCSI has the expected INQUIRY response.
@@ -2844,3 +2860,93 @@ def test__delete_extent_no_dataset(extent_type):
                 with zvol_dataset(zvol, 100, None, True, True):
                     with zvol_extent(zvol, extent_name='zvolextent1'):
                         ssh(DESTROY_CMD)
+
+
+def test__target_readonly_extent(iscsi_running):
+    """Validate a target that is made RO - either by modifying the extent
+    setting, or the underlying ZVOL - behaves correctly."""
+    name1 = f"{target_name}x1"
+    iqn = f'{basename}:{name1}'
+
+    zeros = bytearray(512)
+    deadbeef = bytearray.fromhex('deadbeef') * 128
+
+    def lba_data(flipped):
+        if flipped:
+            return deadbeef, zeros
+        else:
+            return zeros, deadbeef
+
+    def write_lbas(s, flipped=False):
+        lba0, lba1 = lba_data(flipped)
+        s.write16(0, 1, lba0)
+        s.write16(1, 1, lba1)
+
+    def read_lbas(s, flipped=False):
+        lba0, lba1 = lba_data(flipped)
+        r = s.read16(0, 1)
+        assert r.datain == lba0, r.datain
+        r = s.read16(1, 1)
+        assert r.datain == lba1, r.datain
+
+    def check_readonly_state(zvolid, extentid, readonly):
+        if readonly:
+            assert get_readonly(zvolid) == 'on'
+            assert call('iscsi.extent.get_instance', extentid)['ro'] is True
+        else:
+            assert get_readonly(zvolid) == 'off'
+            assert call('iscsi.extent.get_instance', extentid)['ro'] is False
+
+    with initiator_portal() as config:
+        with configured_target(config, name1, 'VOLUME') as target_config:
+            with iscsi_scsi_connection(truenas_server.ip, iqn) as s:
+                zvolid = target_config['dataset']
+                extentid = target_config['extent']['id']
+
+                # Ensure that we can read and write
+                write_lbas(s)
+                read_lbas(s)
+                check_readonly_state(zvolid, extentid, False)
+
+                # Set RO by updating the extent
+                call('iscsi.extent.update', extentid, {'ro': True})
+                expect_check_condition(s, sense_ascq_dict[0x2900])  # "POWER ON, RESET, OR BUS DEVICE RESET OCCURRED"
+                check_readonly_state(zvolid, extentid, True)
+
+                # Ensure that we can only READ
+                read_lbas(s)
+                # Write => Check Condition Sense key = 7 for Data Protect, ASCQ == 0
+                with raises_check_condition(7, 0):
+                    write_lbas(s, True)
+                read_lbas(s)
+
+                # Set RW by updating the extent
+                call('iscsi.extent.update', extentid, {'ro': False})
+                expect_check_condition(s, sense_ascq_dict[0x2900])  # "POWER ON, RESET, OR BUS DEVICE RESET OCCURRED"
+                check_readonly_state(zvolid, extentid, False)
+
+                # Ensure that we can read and write
+                read_lbas(s)
+                write_lbas(s, True)
+                read_lbas(s, True)
+
+                # Set RO by updating the ZVOL
+                call('pool.dataset.update', zvolid, {'readonly': 'ON'})
+                expect_check_condition(s, sense_ascq_dict[0x2900])  # "POWER ON, RESET, OR BUS DEVICE RESET OCCURRED"
+                check_readonly_state(zvolid, extentid, True)
+
+                # Ensure that we can only READ
+                read_lbas(s, True)
+                with raises_check_condition(7, 0):
+                    write_lbas(s)
+                read_lbas(s, True)
+
+                # Set RW by updating the ZVOL
+                call('pool.dataset.update', zvolid, {'readonly': 'OFF'})
+                expect_check_condition(s, sense_ascq_dict[0x2900])  # "POWER ON, RESET, OR BUS DEVICE RESET OCCURRED"
+                check_readonly_state(zvolid, extentid, False)
+
+                # Ensure that we can read and write
+                read_lbas(s, True)
+                write_lbas(s)
+                read_lbas(s)