Re: [PATCH 14/14] sd: Honor block layer integrity handling flags

[Sagi Grimberg <sagig@dev.mellanox.co.il>]

On 8/28/2014 10:31 PM, Martin K. Petersen wrote:
> A set of flags introduced in the block layer enable better control over
> how protection information is handled. These flags are useful for both
> error injection and data recovery purposes. Checking can be enabled and
> disabled for controller and disk, and the guard tag format is now a
> per-I/O property.
>

Hi Martin,

Not sure why I didn't post a review for this one...
Anyway sorry, I hope it's not too late...
just two comments below.

> Update sd_protect_op to communicate the relevant information to the
> low-level device driver via a set of flags in scsi_cmnd.
>
> Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
> ---
>   drivers/scsi/sd.c        | 73 ++++++++++++++++++++++++++++--------------------
>   drivers/scsi/sd.h        | 66 +++++++++++++++++++++++++++++++++++++++++--
>   drivers/scsi/sd_dif.c    | 23 ++++++---------
>   include/scsi/scsi_cmnd.h | 36 +++++++++++++++++-------
>   4 files changed, 142 insertions(+), 56 deletions(-)
>

<SNIP>

> +/*
> + * Returns a mask of the protection flags that are valid for a given DIX
> + * operation.
> + */
> +static inline unsigned int sd_prot_flag_mask(unsigned int prot_op)
> +{
> +	const unsigned int flag_mask[] = {
> +		[SCSI_PROT_NORMAL]		= 0,
> +
> +		[SCSI_PROT_READ_STRIP]		= SCSI_PROT_TRANSFER_PI |
> +						  SCSI_PROT_GUARD_CHECK |
> +						  SCSI_PROT_REF_CHECK |
> +						  SCSI_PROT_REF_INCREMENT,
> +
> +		[SCSI_PROT_READ_INSERT]		= SCSI_PROT_REF_INCREMENT |
> +						  SCSI_PROT_IP_CHECKSUM,
> +
> +		[SCSI_PROT_READ_PASS]		= SCSI_PROT_TRANSFER_PI |
> +						  SCSI_PROT_GUARD_CHECK |
> +						  SCSI_PROT_REF_CHECK |
> +						  SCSI_PROT_REF_INCREMENT |
> +						  SCSI_PROT_IP_CHECKSUM,
> +
> +		[SCSI_PROT_WRITE_INSERT]	= SCSI_PROT_TRANSFER_PI |
> +						  SCSI_PROT_REF_INCREMENT,
> +
> +		[SCSI_PROT_WRITE_STRIP]		= SCSI_PROT_GUARD_CHECK |
> +						  SCSI_PROT_REF_CHECK |
> +						  SCSI_PROT_REF_INCREMENT |
> +						  SCSI_PROT_IP_CHECKSUM,
> +
> +		[SCSI_PROT_WRITE_PASS]		= SCSI_PROT_TRANSFER_PI |
> +						  SCSI_PROT_GUARD_CHECK |
> +						  SCSI_PROT_REF_CHECK |
> +						  SCSI_PROT_REF_INCREMENT |
> +						  SCSI_PROT_IP_CHECKSUM,

A bit strange to me that you put REF_CHECK & REF_INCREMENT flag
depending on the prot_op while it really depends on the prot_type.
It is &'nd with preset scmnd->prot_flags anyways.. so no worries...

> +	};
> +
> +	return flag_mask[prot_op];
> +}
> +

<SNIP>

> +/*
> +enum scsi_prot_flags {
> +	SCSI_PROT_TRANSFER_PI		= 1 << 0,
> +	SCSI_PROT_GUARD_CHECK		= 1 << 1,
> +	SCSI_PROT_REF_CHECK		= 1 << 2,
> +	SCSI_PROT_REF_INCREMENT		= 1 << 3,
> +	SCSI_PROT_IP_CHECKSUM		= 1 << 4,

I have a question here,
What are your plans with explicit ref/app escape flags?
Were these just left behind on this series?


Other than that, looks good.

Reviewed-by: Sagi Grimberg <sagig@mellanox.com>

Sagi.