[pbs-devel] [PATCH proxmox-backup 1/4] tape: use correct MAM attribute definitions

[pbs-devel] [PATCH proxmox-backup 1/4] tape: use correct MAM attribute definitions

[Dominik Csapak]

080Ch: it's binary not ascii, and the length is not specified
    (hp says 23-n bytes)
0820h and 0821h are also binary and not ascii

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 pbs-tape/src/sg_tape/mam.rs | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/pbs-tape/src/sg_tape/mam.rs b/pbs-tape/src/sg_tape/mam.rs
index 634b5a29..b1b4d59a 100644
--- a/pbs-tape/src/sg_tape/mam.rs
+++ b/pbs-tape/src/sg_tape/mam.rs
@@ -133,20 +133,20 @@ static MAM_ATTRIBUTES: &[(u16, u16, MamFormat, &str)] = &[
     (0x08_0B, 16, MamFormat::ASCII, "Application Format Version"),
     (
         0x08_0C,
-        50,
-        MamFormat::ASCII,
+        0, // length is not specified for IBM, and HP says 23-n
+        MamFormat::BINARY,
         "Volume Coherency Information",
     ),
     (
         0x08_20,
         36,
-        MamFormat::ASCII,
+        MamFormat::BINARY,
         "Medium Globally Unique Identifier",
     ),
     (
         0x08_21,
         36,
-        MamFormat::ASCII,
+        MamFormat::BINARY,
         "Media Pool Globally Unique Identifier",
     ),
     (
@@ -238,7 +238,7 @@ fn decode_mam_attributes(data: &[u8]) -> Result<Vec<MamAttribute>, Error> {
         };
 
         if let Some(info) = MAM_ATTRIBUTE_NAMES.get(&head_id) {
-            if info.1 == head.len {
+            if info.1 == 0 || info.1 == head.len {
                 let value = match info.2 {
                     MamFormat::ASCII => String::from_utf8_lossy(&data).to_string(),
                     MamFormat::DEC => {
-- 
2.30.2

[pbs-devel] [PATCH proxmox-backup 2/4] tape: continue without MAM media_usage page

[Dominik Csapak]

sometimes we cannot parse these pages (e.g. vendor quirks that implement
the field differently)

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 pbs-tape/src/sg_tape.rs | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/pbs-tape/src/sg_tape.rs b/pbs-tape/src/sg_tape.rs
index 7ad94d9d..6a5569ac 100644
--- a/pbs-tape/src/sg_tape.rs
+++ b/pbs-tape/src/sg_tape.rs
@@ -911,11 +911,16 @@ impl SgTape {
             status.block_number = Some(position.logical_object_number);
 
             if let Ok(mam) = self.cartridge_memory() {
-                let usage = mam_extract_media_usage(&mam)?;
-
-                status.manufactured = Some(usage.manufactured);
-                status.bytes_read = Some(usage.bytes_read);
-                status.bytes_written = Some(usage.bytes_written);
+                match mam_extract_media_usage(&mam) {
+                    Ok(usage) => {
+                        status.manufactured = Some(usage.manufactured);
+                        status.bytes_read = Some(usage.bytes_read);
+                        status.bytes_written = Some(usage.bytes_written);
+                    }
+                    Err(err) => {
+                        log::warn!("unable to get MAM media usage: {err}");
+                    }
+                }
 
                 if let Ok(volume_stats) = self.volume_statistics() {
                     let passes = std::cmp::max(
-- 
2.30.2

[pbs-devel] [PATCH proxmox-backup 3/4] tape: implement 6 byte fallback for MODE SENSE/SELECT

[Dominik Csapak]

there are tape drives (esp. virtual ones) that don't implement the
10-byte variants of MODE SENSE/SELECT. Since the pages we set/request
are never bigger than 255 bytes anyway, we can implement a fallback
with the 6 byte variant here.

Implementing this as a fallback to make sure that existing working
drives keep the existing implementation.

Tested with Starwind VTL.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 pbs-tape/src/sg_tape.rs  |  44 ++++++++-
 pbs-tape/src/sgutils2.rs | 197 ++++++++++++++++++++++++++++++++-------
 2 files changed, 202 insertions(+), 39 deletions(-)

diff --git a/pbs-tape/src/sg_tape.rs b/pbs-tape/src/sg_tape.rs
index 6a5569ac..97d90201 100644
--- a/pbs-tape/src/sg_tape.rs
+++ b/pbs-tape/src/sg_tape.rs
@@ -30,8 +30,9 @@ use pbs_api_types::{Lp17VolumeStatistics, LtoDriveAndMediaStatus, MamAttribute};
 
 use crate::{
     sgutils2::{
-        alloc_page_aligned_buffer, scsi_inquiry, scsi_mode_sense, scsi_request_sense, InquiryInfo,
-        ModeBlockDescriptor, ModeParameterHeader, ScsiError, SenseInfo, SgRaw,
+        alloc_page_aligned_buffer, scsi_inquiry, scsi_mode_sense, scsi_request_sense,
+        sense_err_is_invalid_command, InquiryInfo, ModeBlockDescriptor, ModeParameterHeader,
+        ModeParameterHeader10, ModeParameterHeader6, ScsiError, SenseInfo, SgRaw,
     },
     BlockRead, BlockReadError, BlockWrite, BlockedReader, BlockedWriter,
 };
@@ -764,6 +765,7 @@ impl SgTape {
 
         let mut data = Vec::new();
         unsafe {
+            let head: ModeParameterHeader10 = head.clone().into();
             data.write_be_value(head)?;
             data.write_be_value(block_descriptor)?;
             data.write_be_value(page)?;
@@ -782,9 +784,41 @@ impl SgTape {
 
         buffer[..data.len()].copy_from_slice(&data[..]);
 
-        sg_raw
-            .do_out_command(&cmd, &buffer[..data.len()])
-            .map_err(|err| format_err!("set drive options failed - {}", err))?;
+        match sg_raw.do_out_command(&cmd, &buffer[..data.len()]) {
+            Ok(()) => {}
+            Err(ScsiError::Sense(err)) if sense_err_is_invalid_command(&err) => {
+                let mut data = Vec::new();
+                unsafe {
+                    let head: ModeParameterHeader6 = head.try_into()?;
+                    data.write_be_value(head)?;
+                    data.write_be_value(block_descriptor)?;
+                    data.write_be_value(page)?;
+                }
+
+                let mut cmd = Vec::new();
+                cmd.push(0x15); // MODE SELECT(6)
+                cmd.push(0b0001_0000); // PF=1
+                cmd.extend([0, 0]); //reserved
+
+                if data.len() > u8::MAX as usize {
+                    bail!("set drive options (mode select(6)) failed - parameters too long")
+                }
+
+                cmd.push(data.len() as u8);
+                cmd.push(0); // control
+
+                let mut buffer = alloc_page_aligned_buffer(4096)?;
+
+                buffer[..data.len()].copy_from_slice(&data[..]);
+
+                sg_raw
+                    .do_out_command(&cmd, &buffer[..data.len()])
+                    .map_err(|err| {
+                        format_err!("set drive options (mode select(6)) failed - {err}")
+                    })?;
+            }
+            Err(err) => bail!("set drive options (mode select(10)) failed - {err}"),
+        }
 
         Ok(())
     }
diff --git a/pbs-tape/src/sgutils2.rs b/pbs-tape/src/sgutils2.rs
index dd1aa1b4..91444cec 100644
--- a/pbs-tape/src/sgutils2.rs
+++ b/pbs-tape/src/sgutils2.rs
@@ -224,7 +224,7 @@ pub struct InquiryInfo {
 
 #[repr(C, packed)]
 #[derive(Endian, Debug, Copy, Clone)]
-pub struct ModeParameterHeader {
+pub struct ModeParameterHeader10 {
     pub mode_data_len: u16,
     // Note: medium_type and density_code are not the same. On HP
     // drives, medium_type provides very limited information and is
@@ -232,27 +232,106 @@ pub struct ModeParameterHeader {
     pub medium_type: u8,
     pub flags3: u8,
     reserved4: [u8; 2],
-    pub block_descriptior_len: u16,
+    pub block_descriptor_len: u16,
+}
+
+// header for the short variant of MODE SENSE/SELECT
+#[repr(C, packed)]
+#[derive(Endian, Debug, Copy, Clone)]
+pub struct ModeParameterHeader6 {
+    pub mode_data_len: u8,
+    // Note: medium_type and density_code are not the same. On HP
+    // drives, medium_type provides very limited information and is
+    // not compatible with IBM.
+    pub medium_type: u8,
+    pub flags2: u8,
+    pub block_descriptor_len: u8,
+}
+
+#[derive(Clone)]
+pub struct ModeParameterHeader {
+    pub mode_data_len: u16,
+    // Note: medium_type and density_code are not the same. On HP
+    // drives, medium_type provides very limited information and is
+    // not compatible with IBM.
+    pub medium_type: u8,
+    pub flags: u8,
+    pub block_descriptor_len: u16,
+}
+
+impl TryFrom<ModeParameterHeader> for ModeParameterHeader6 {
+    type Error = Error;
+
+    fn try_from(value: ModeParameterHeader) -> Result<Self, Self::Error> {
+        if value.mode_data_len > u8::MAX as u16 {
+            bail!("mode_data_len too big for 6 byte mode parameter header")
+        }
+
+        if value.block_descriptor_len > u8::MAX as u16 {
+            bail!("block_descriptor_len too big for 6 byte mode parameter header")
+        }
+
+        Ok(Self {
+            mode_data_len: value.mode_data_len as u8,
+            medium_type: value.medium_type,
+            flags2: value.flags,
+            block_descriptor_len: value.block_descriptor_len as u8,
+        })
+    }
+}
+
+impl From<ModeParameterHeader> for ModeParameterHeader10 {
+    fn from(value: ModeParameterHeader) -> Self {
+        Self {
+            mode_data_len: value.mode_data_len,
+            medium_type: value.medium_type,
+            flags3: value.flags,
+            block_descriptor_len: value.block_descriptor_len,
+            reserved4: [0, 0],
+        }
+    }
+}
+
+impl From<ModeParameterHeader10> for ModeParameterHeader {
+    fn from(val: ModeParameterHeader10) -> Self {
+        Self {
+            mode_data_len: val.mode_data_len,
+            medium_type: val.medium_type,
+            flags: val.flags3,
+            block_descriptor_len: val.block_descriptor_len,
+        }
+    }
+}
+
+impl From<ModeParameterHeader6> for ModeParameterHeader {
+    fn from(val: ModeParameterHeader6) -> Self {
+        Self {
+            mode_data_len: val.mode_data_len as u16,
+            medium_type: val.medium_type,
+            flags: val.flags2,
+            block_descriptor_len: val.block_descriptor_len as u16,
+        }
+    }
 }
 
 impl ModeParameterHeader {
     #[allow(clippy::unusual_byte_groupings)]
     pub fn buffer_mode(&self) -> u8 {
-        (self.flags3 & 0b0_111_0000) >> 4
+        (self.flags & 0b0_111_0000) >> 4
     }
 
     #[allow(clippy::unusual_byte_groupings)]
     pub fn set_buffer_mode(&mut self, buffer_mode: bool) {
-        let mut mode = self.flags3 & 0b1_000_1111;
+        let mut mode = self.flags & 0b1_000_1111;
         if buffer_mode {
             mode |= 0b0_001_0000;
         }
-        self.flags3 = mode;
+        self.flags = mode;
     }
 
     #[allow(clippy::unusual_byte_groupings)]
     pub fn write_protect(&self) -> bool {
-        (self.flags3 & 0b1_000_0000) != 0
+        (self.flags & 0b1_000_0000) != 0
     }
 }
 
@@ -670,6 +749,13 @@ pub fn scsi_inquiry<F: AsRawFd>(file: &mut F) -> Result<InquiryInfo, Error> {
     .map_err(|err: Error| format_err!("decode inquiry page failed - {}", err))
 }
 
+/// True if the given sense info is INVALID COMMAND OPERATION CODE
+/// means that the device does not know/support the command
+/// https://www.t10.org/lists/asc-num.htm#ASC_20
+pub fn sense_err_is_invalid_command(err: &SenseInfo) -> bool {
+    err.sense_key == SENSE_KEY_ILLEGAL_REQUEST && err.asc == 0x20 && err.ascq == 0x00
+}
+
 /// Run SCSI Mode Sense
 ///
 /// Warning: P needs to be repr(C, packed)]
@@ -695,43 +781,86 @@ pub fn scsi_mode_sense<F: AsRawFd, P: Endian>(
     cmd.extend(allocation_len.to_be_bytes()); // allocation len
     cmd.push(0); //control
 
-    let data = sg_raw
-        .do_command(&cmd)
-        .map_err(|err| format_err!("mode sense failed - {}", err))?;
-
-    proxmox_lang::try_block!({
-        let mut reader = data;
+    let (head, mut reader): (ModeParameterHeader, &[u8]) = match sg_raw.do_command(&cmd) {
+        Ok(data) => {
+            let mut reader = data;
+
+            let head: ModeParameterHeader10 = unsafe { reader.read_be_value()? };
+            let expected_len = head.mode_data_len as usize + 2;
+
+            use std::cmp::Ordering;
+            match data.len().cmp(&expected_len) {
+                Ordering::Less => bail!(
+                    "wrong mode_data_len: got {}, expected {}",
+                    data.len(),
+                    expected_len
+                ),
+                Ordering::Greater => {
+                    // Note: Some hh7 drives returns the allocation_length
+                    // instead of real data_len
+                    let header_size = std::mem::size_of::<ModeParameterHeader10>();
+                    reader = &data[header_size..expected_len];
+                }
+                _ => (),
+            }
 
-        let head: ModeParameterHeader = unsafe { reader.read_be_value()? };
-        let expected_len = head.mode_data_len as usize + 2;
-
-        use std::cmp::Ordering;
-        match data.len().cmp(&expected_len) {
-            Ordering::Less => bail!(
-                "wrong mode_data_len: got {}, expected {}",
-                data.len(),
-                expected_len
-            ),
-            Ordering::Greater => {
-                // Note: Some hh7 drives returns the allocation_length
-                // instead of real data_len
-                let header_size = std::mem::size_of::<ModeParameterHeader>();
-                reader = &data[header_size..expected_len];
+            (head.into(), reader)
+        }
+        Err(ScsiError::Sense(err)) if sense_err_is_invalid_command(&err) => {
+            // fall back to small mode sense
+            let mut cmd = vec![0x1A]; // MODE SENSE(6)
+            if disable_block_descriptor {
+                cmd.push(8); // DBD=1 (Disable Block Descriptors)
+            } else {
+                cmd.push(0); // DBD=0 (Include Block Descriptors)
+            }
+            cmd.push(page_code & 63); // report current values for page_code
+            cmd.push(sub_page_code);
+
+            cmd.push(0xFF); // allocation len
+            cmd.push(0); //control
+            let data = sg_raw
+                .do_command(&cmd)
+                .map_err(|err| format_err!("mode sense(6) failed - {err}"))?;
+
+            let mut reader = data;
+
+            let head: ModeParameterHeader6 = unsafe { reader.read_be_value()? };
+            let expected_len = head.mode_data_len as usize + 1;
+
+            use std::cmp::Ordering;
+            match data.len().cmp(&expected_len) {
+                Ordering::Less => bail!(
+                    "wrong mode_data_len: got {}, expected {}",
+                    data.len(),
+                    expected_len
+                ),
+                Ordering::Greater => {
+                    // Note: Some hh7 drives returns the allocation_length
+                    // instead of real data_len
+                    let header_size = std::mem::size_of::<ModeParameterHeader6>();
+                    reader = &data[header_size..expected_len];
+                }
+                _ => (),
             }
-            _ => (),
+
+            (head.into(), reader)
         }
+        Err(err) => bail!("mode sense(10) failed - {err}"),
+    };
 
-        if disable_block_descriptor && head.block_descriptior_len != 0 {
-            let len = head.block_descriptior_len;
-            bail!("wrong block_descriptior_len: {}, expected 0", len);
+    proxmox_lang::try_block!({
+        if disable_block_descriptor && head.block_descriptor_len != 0 {
+            let len = head.block_descriptor_len;
+            bail!("wrong block_descriptor_len: {}, expected 0", len);
         }
 
         let mut block_descriptor: Option<ModeBlockDescriptor> = None;
 
         if !disable_block_descriptor {
-            if head.block_descriptior_len != 8 {
-                let len = head.block_descriptior_len;
-                bail!("wrong block_descriptior_len: {}, expected 8", len);
+            if head.block_descriptor_len != 8 {
+                let len = head.block_descriptor_len;
+                bail!("wrong block_descriptor_len: {}, expected 8", len);
             }
 
             block_descriptor = Some(unsafe { reader.read_be_value()? });
-- 
2.30.2

[pbs-devel] [PATCH proxmox-backup 4/4] tape: typo fixes

[Dominik Csapak]

the field is not currently used anywhere

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 pbs-tape/src/sgutils2.rs | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/pbs-tape/src/sgutils2.rs b/pbs-tape/src/sgutils2.rs
index 91444cec..0b894717 100644
--- a/pbs-tape/src/sgutils2.rs
+++ b/pbs-tape/src/sgutils2.rs
@@ -192,7 +192,7 @@ pub struct RequestSenseFixed {
     pub command_specific_information: [u8; 4],
     pub additional_sense_code: u8,
     pub additional_sense_code_qualifier: u8,
-    pub field_replacable_unit_code: u8,
+    pub field_replaceable_unit_code: u8,
     pub sense_key_specific: [u8; 3],
 }
 
@@ -873,7 +873,7 @@ pub fn scsi_mode_sense<F: AsRawFd, P: Endian>(
     .map_err(|err: Error| format_err!("decode mode sense failed - {}", err))
 }
 
-/// Resuqest Sense
+/// Request Sense
 pub fn scsi_request_sense<F: AsRawFd>(file: &mut F) -> Result<RequestSenseFixed, ScsiError> {
     // request 252 bytes, as mentioned in the Seagate SCSI reference
     let allocation_len: u8 = 252;
-- 
2.30.2

Re: [pbs-devel] [PATCH proxmox-backup 3/4] tape: implement 6 byte fallback for MODE SENSE/SELECT

[Dominik Csapak]

On 3/30/23 13:28, Dominik Csapak wrote:
> there are tape drives (esp. virtual ones) that don't implement the
> 10-byte variants of MODE SENSE/SELECT. Since the pages we set/request
> are never bigger than 255 bytes anyway, we can implement a fallback
> with the 6 byte variant here.
> 
> Implementing this as a fallback to make sure that existing working
> drives keep the existing implementation.
> 
> Tested with Starwind VTL.
> 

note because if forgot: this patch does not fix pbs tape usage
with starwinds vtl completely, there are some things that still
don't work, but this fixes these specific commands there and
may increase compatibility with other drives too

[pbs-devel] applied [PATCH proxmox-backup 1/4] tape: use correct MAM attribute definitions

[Dietmar Maurer]

applied, thanks.

[pbs-devel] applied [PATCH proxmox-backup 2/4] tape: continue without MAM media_usage page

[Dietmar Maurer]

applied