Change in reported values of some block integrity sysfs attributes

Change in reported values of some block integrity sysfs attributes

[M Nikhil]

Hi Everyone,

  * We have observed change in the values of some of the block integrity
    sysfs attributes for the block devices on the master branch. The
    sysfs attributes related to block device integrity , write_generate
    and read_verify are  enabled for the block device when the parameter
    device_is_integrity_capable is disabled. This behaviour is seen on
    the scsi disks irrespective of DIF protection enabled or disabled on
    the disks.

    *Logs of the block integrity sysfs attributes for one of the block
    device:*

a3560030:~ # cat /sys/block/sda/integrity/write_generate

1

a3560030:~ # cat /sys/block/sda/integrity/read_verify

1

a3560030:~ # cat /sys/block/sda/integrity/device_is_integrity_capable

0

  * Similarly unexpected values of block integrity sysfs attributes are
    seen for multipath devices as well. Multipath device reporting value
    1 for device_is_integrity_capable even though it is based on SCSI
    disk devices, which all have 0 for device_is_integrity_capable.

a3560030:~ # cat /sys/block/dm-0/integrity/device_is_integrity_capable

1

a3560030:~ # cat /sys/block/dm-0/integrity/read_verify

1

a3560030:~ # cat /sys/block/dm-0/integrity/write_generate

1

  * Earlier the block integrity sysfs parameters "write_generate" and
    "read_verify" reported value 0 when the sysfs attribute
    device_is_integrity_capable was not set. But when tested with a
    recent upstream kernel, there is a change in the block device
    integrity sysfs attributes.
  * In the process of finding the kernel changes which might have caused
    the change in functionality, we have identified the below commit
    which was leading to the change in the sysfs attributes.
    9f4aa46f2a74 ("block: invert the BLK_INTEGRITY_{GENERATE,VERIFY} flags")
  * By reverting the code changes which are part of above commit and
    when tested the values of attributes read_verify and write_generate
    were set to 0 which was the older functionality.
   * From the description in the patch related to above commit, what we
    understand is that the changes are meant to invert the block
    integrity flags(READ_VERIFY and WRITE_GENERATE) vs the values in
    sysfs for making the user values persistent.
  * We would like to know if the change in the values of sysfs
    attributes write_generate and read_verify is expected?
  * And some additional information on in which scenario the attributes
    will be disabled or set to 0 and the affect of other block integrity
    attribute device_is_integrity_capable on attributes read_verify and
    write_generate.


Regards,

*M Nikhil
*

Software Engineer

ISDL, Bangalore, India

Linux on IBM Z and LinuxONE

Re: Change in reported values of some block integrity sysfs attributes

[Christoph Hellwig]

Hi M,

On Fri, Dec 13, 2024 at 12:46:14PM +0530, M Nikhil wrote:
> Hi Everyone,
>
>  * We have observed change in the values of some of the block integrity
>    sysfs attributes for the block devices on the master branch. The
>    sysfs attributes related to block device integrity , write_generate
>    and read_verify are  enabled for the block device when the parameter
>    device_is_integrity_capable is disabled. This behaviour is seen on
>    the scsi disks irrespective of DIF protection enabled or disabled on
>    the disks.

As in after a "echo 1 > /sys/.../device_is_integrity_capable" ?

I'll look into it.

Re: Change in reported values of some block integrity sysfs attributes

[M Nikhil]

Hi Christoph,

We did not modify 'device_is_integrity_capable' attribute.

On 13/12/24 8:03 pm, Christoph Hellwig wrote:
> Hi M,
>
> On Fri, Dec 13, 2024 at 12:46:14PM +0530, M Nikhil wrote:
>> Hi Everyone,
>>
>>   * We have observed change in the values of some of the block integrity
>>     sysfs attributes for the block devices on the master branch. The
>>     sysfs attributes related to block device integrity , write_generate
>>     and read_verify are  enabled for the block device when the parameter
>>     device_is_integrity_capable is disabled. This behaviour is seen on
>>     the scsi disks irrespective of DIF protection enabled or disabled on
>>     the disks.
> As in after a "echo 1 > /sys/.../device_is_integrity_capable" ?
>
> I'll look into it.
>

Re: Change in reported values of some block integrity sysfs attributes

[Martin K. Petersen]

> The sysfs attributes related to block device integrity ,
> write_generate and read_verify are enabled for the block device when
> the parameter device_is_integrity_capable is disabled.

'device_is_integrity_capable' is set if storage device (media) is
formatted with PI.

That is completely orthogonal to 'write_generate' and 'read_verify'
which are enabled if the HBA supports DIX. If the HBA supports DIX Type
0, 'write_generate' and 'read_verify' are enabled even if the attached
disk is not formatted with PI.

I don't see any change in what's reported with block/for-next in a
regular SCSI HBA/disk setup. Will have to look at whether there is a
stacking issue wrt. multipathing.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: Change in reported values of some block integrity sysfs attributes

[M Nikhil]

Thanks for the inputs Martin.

We have observed that even though the HBA does not support DIX Type, 
still the integrity attributes 'write_generate' and 'read_verify' are 
set to 1 for the block devices.


a3560030:~ # cat /sys/class/scsi_host/host0/prot_capabilities
0
a3560030:~ # cat /sys/class/scsi_host/host1/prot_capabilities
0


a3560030:~ # cat /sys/block/sdc/integrity/write_generate
1
a3560030:~ # cat /sys/block/sdc/integrity/read_verify
1


Thanks and Regards,

Nikhil

On 14/12/24 4:00 am, Martin K. Petersen wrote:
>> The sysfs attributes related to block device integrity ,
>> write_generate and read_verify are enabled for the block device when
>> the parameter device_is_integrity_capable is disabled.
> 'device_is_integrity_capable' is set if storage device (media) is
> formatted with PI.
>
> That is completely orthogonal to 'write_generate' and 'read_verify'
> which are enabled if the HBA supports DIX. If the HBA supports DIX Type
> 0, 'write_generate' and 'read_verify' are enabled even if the attached
> disk is not formatted with PI.
>
> I don't see any change in what's reported with block/for-next in a
> regular SCSI HBA/disk setup. Will have to look at whether there is a
> stacking issue wrt. multipathing.
>

Re: Change in reported values of some block integrity sysfs attributes

[M Nikhil]

gentle ping!!!

Hopefully I haven't missed anything. I was wondering if you could find 
out something and if I can provide more information.

Re: Change in reported values of some block integrity sysfs attributes

[Anuj gupta]

> I don't see any change in what's reported with block/for-next in a
> regular SCSI HBA/disk setup. Will have to look at whether there is a
> stacking issue wrt. multipathing.

Hi Martin, Christoph,

It seems this change in behaviour is not limited to SCSI only. As Nikhil
mentioned an earlier commit
[9f4aa46f2a74 ("block: invert the BLK_INTEGRITY_{GENERATE,VERIFY} flags")]
causes this change in behaviour. On my setup with a NVMe drive not formatted
with PI, I see that:

Without this commit:
Value reported by read_verify and write_generate sysfs entries is 0.

With this commit:
Value reported by read_verify and write_generate sysfs entries is 1.

Diving a bit deeper, both these flags got inverted due to this commit.
But during init (in nvme_init_integrity) these values get initialized to 0,
inturn setting the sysfs entries to 1. In order to fix this, the driver has to
initialize both these flags to 1 in nvme_init_integrity if PI is not supported.
That way, the value in sysfs for these entries would become 0 again. Tried this
approach in my setup, and I am able to see the right values now. Then something
like this would also need to be done for SCSI too.

Re: Change in reported values of some block integrity sysfs attributes

[Anuj Gupta]

[-- Attachment #1: Type: text/plain, Size: 2886 bytes --]

On Fri, Feb 21, 2025 at 04:07:55PM +0530, Anuj gupta wrote:
> > I don't see any change in what's reported with block/for-next in a
> > regular SCSI HBA/disk setup. Will have to look at whether there is a
> > stacking issue wrt. multipathing.
> 
> Hi Martin, Christoph,
> 
> It seems this change in behaviour is not limited to SCSI only. As Nikhil
> mentioned an earlier commit
> [9f4aa46f2a74 ("block: invert the BLK_INTEGRITY_{GENERATE,VERIFY} flags")]
> causes this change in behaviour. On my setup with a NVMe drive not formatted
> with PI, I see that:
> 
> Without this commit:
> Value reported by read_verify and write_generate sysfs entries is 0.
> 
> With this commit:
> Value reported by read_verify and write_generate sysfs entries is 1.
> 
> Diving a bit deeper, both these flags got inverted due to this commit.
> But during init (in nvme_init_integrity) these values get initialized to 0,
> inturn setting the sysfs entries to 1. In order to fix this, the driver has to
> initialize both these flags to 1 in nvme_init_integrity if PI is not supported.
> That way, the value in sysfs for these entries would become 0 again. Tried this
> approach in my setup, and I am able to see the right values now. Then something
> like this would also need to be done for SCSI too.

This is the patch that I used:

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 818d4e49aab5..6cd9f57131cc 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1799,6 +1799,7 @@ static bool nvme_init_integrity(struct nvme_ns_head *head,
 
 	memset(bi, 0, sizeof(*bi));
 
+	bi->flags |= BLK_INTEGRITY_NOGENERATE | BLK_INTEGRITY_NOVERIFY;
 	if (!head->ms)
 		return true;
 
@@ -1817,11 +1818,15 @@ static bool nvme_init_integrity(struct nvme_ns_head *head,
 			bi->csum_type = BLK_INTEGRITY_CSUM_CRC;
 			bi->tag_size = sizeof(u16) + sizeof(u32);
 			bi->flags |= BLK_INTEGRITY_DEVICE_CAPABLE;
+			bi->flags &= ~(BLK_INTEGRITY_NOGENERATE |
+				       BLK_INTEGRITY_NOVERIFY);
 			break;
 		case NVME_NVM_NS_64B_GUARD:
 			bi->csum_type = BLK_INTEGRITY_CSUM_CRC64;
 			bi->tag_size = sizeof(u16) + 6;
 			bi->flags |= BLK_INTEGRITY_DEVICE_CAPABLE;
+			bi->flags &= ~(BLK_INTEGRITY_NOGENERATE |
+				       BLK_INTEGRITY_NOVERIFY);
 			break;
 		default:
 			break;
@@ -1835,12 +1840,16 @@ static bool nvme_init_integrity(struct nvme_ns_head *head,
 			bi->tag_size = sizeof(u16);
 			bi->flags |= BLK_INTEGRITY_DEVICE_CAPABLE |
 				     BLK_INTEGRITY_REF_TAG;
+			bi->flags &= ~(BLK_INTEGRITY_NOGENERATE |
+				       BLK_INTEGRITY_NOVERIFY);
 			break;
 		case NVME_NVM_NS_64B_GUARD:
 			bi->csum_type = BLK_INTEGRITY_CSUM_CRC64;
 			bi->tag_size = sizeof(u16);
 			bi->flags |= BLK_INTEGRITY_DEVICE_CAPABLE |
 				     BLK_INTEGRITY_REF_TAG;
+			bi->flags &= ~(BLK_INTEGRITY_NOGENERATE |
+				       BLK_INTEGRITY_NOVERIFY);
 			break;
 		default:
 			break;


[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: Change in reported values of some block integrity sysfs attributes

[Anuj Gupta]

[-- Attachment #1: Type: text/plain, Size: 2310 bytes --]

On Fri, Feb 21, 2025 at 04:07:55PM +0530, Anuj gupta wrote:
> > I don't see any change in what's reported with block/for-next in a
> > regular SCSI HBA/disk setup. Will have to look at whether there is a
> > stacking issue wrt. multipathing.
> 
> Hi Martin, Christoph,
> 
> It seems this change in behaviour is not limited to SCSI only. As Nikhil
> mentioned an earlier commit
> [9f4aa46f2a74 ("block: invert the BLK_INTEGRITY_{GENERATE,VERIFY} flags")]
> causes this change in behaviour. On my setup with a NVMe drive not formatted
> with PI, I see that:
> 
> Without this commit:
> Value reported by read_verify and write_generate sysfs entries is 0.
> 
> With this commit:
> Value reported by read_verify and write_generate sysfs entries is 1.
> 
> Diving a bit deeper, both these flags got inverted due to this commit.
> But during init (in nvme_init_integrity) these values get initialized to 0,
> inturn setting the sysfs entries to 1. In order to fix this, the driver has to
> initialize both these flags to 1 in nvme_init_integrity if PI is not supported.
> That way, the value in sysfs for these entries would become 0 again. Tried this
> approach in my setup, and I am able to see the right values now. Then something
> like this would also need to be done for SCSI too.
> 

I tried to make it work for SCSI too. However my testing is limited as I
don't have a SCSI device. With scsi_debug I see this currently:

# modprobe scsi_debug dev_size_mb=128 dix=0 dif=0
# cat /sys/block/sda/integrity/write_generate
1
# cat /sys/block/sda/integrity/read_verify
1
# cat /sys/class/scsi_host/host0/prot_capabilities
0

To fix this, I added this. Nikhil can you try below patch? Martin, can
you please take a look as well.

After this patch, with the same scsi_debug device, I see sysfs entries
populated as 0.

diff --git a/drivers/scsi/sd_dif.c b/drivers/scsi/sd_dif.c
index ae6ce6f5d622..be2cd06f500b 100644
--- a/drivers/scsi/sd_dif.c
+++ b/drivers/scsi/sd_dif.c
@@ -40,8 +40,10 @@ void sd_dif_config_host(struct scsi_disk *sdkp, struct queue_limits *lim)
 		dif = 0; dix = 1;
 	}
 
-	if (!dix)
+	if (!dix) {
+		bi->flags |= BLK_INTEGRITY_NOGENERATE | BLK_INTEGRITY_NOVERIFY;
 		return;
+	}
 
 	/* Enable DMA of protection information */
 	if (scsi_host_get_guard(sdkp->device->host) & SHOST_DIX_GUARD_IP)

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: Change in reported values of some block integrity sysfs attributes

[Martin K. Petersen]

Anuj,

> diff --git a/drivers/scsi/sd_dif.c b/drivers/scsi/sd_dif.c
> index ae6ce6f5d622..be2cd06f500b 100644
> --- a/drivers/scsi/sd_dif.c
> +++ b/drivers/scsi/sd_dif.c
> @@ -40,8 +40,10 @@ void sd_dif_config_host(struct scsi_disk *sdkp, struct queue_limits *lim)
>  		dif = 0; dix = 1;
>  	}
>  
> -	if (!dix)
> +	if (!dix) {
> +		bi->flags |= BLK_INTEGRITY_NOGENERATE | BLK_INTEGRITY_NOVERIFY;
>  		return;
> +	}
>  
>  	/* Enable DMA of protection information */
>  	if (scsi_host_get_guard(sdkp->device->host) & SHOST_DIX_GUARD_IP)
>

This looks good to me. Originally we didn't register an integrity
profile at all unless the device was capable.

-- 
Martin K. Petersen	Oracle Linux Engineering