Re: [PATCH 04 of 10] scsi: Support devices with protection information (DIF)

[James Bottomley <James.Bottomley@HansenPartnership.com>]

On Wed, 2008-06-25 at 11:22 -0400, Martin K. Petersen wrote:
> - Add support for an extra scatter-gather list containing protection
>    information.
> 
>  - Remember devices with protection information turned on in INQUIRY.
> 
> Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
> 
> ---
> 4 files changed, 62 insertions(+)
> drivers/scsi/scsi_lib.c    |   38 ++++++++++++++++++++++++++++++++++++++
> drivers/scsi/scsi_scan.c   |    3 +++
> include/scsi/scsi_cmnd.h   |   20 ++++++++++++++++++++
> include/scsi/scsi_device.h |    1 +
> 
> 
> 
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -778,6 +778,11 @@ void scsi_release_buffers(struct scsi_cm
>  		kmem_cache_free(scsi_sdb_cache, bidi_sdb);
>  		cmd->request->next_rq->special = NULL;
>  	}
> +
> +	if (scsi_prot_sg_count(cmd)) {
> +		scsi_free_sgtable(cmd->prot_sdb);
> +		kmem_cache_free(scsi_sdb_cache, cmd->prot_sdb);
> +	}
>  }
>  EXPORT_SYMBOL(scsi_release_buffers);
>  
> @@ -1031,6 +1036,32 @@ static int scsi_init_sgtable(struct requ
>  	return BLKPREP_OK;
>  }
>  
> +static int scsi_protect_io(struct scsi_cmnd *cmd, gfp_t gfp_mask)
> +{
> +	struct request *req;
> +	struct scsi_data_buffer *pdb;
> +	int ivecs, count;
> +
> +	req = cmd->request;
> +
> +	pdb = kmem_cache_zalloc(scsi_sdb_cache, gfp_mask);
> +	if (unlikely(pdb == NULL))
> +		return BLKPREP_DEFER;

I'm afraid you can't do it like this because it will violate our forward
progress guarantees.  If this is the last spare command required for
writeout, you need to guarantee either this allocation will succeed, or
we can proceed without the DIF data.

I know you copied it from the bidirectional stuff, but they don't need
forward progress guarantees; something in the mainline does because we
could get into a vmwriteout failure case even for a DIF protected
device.

> +
> +	ivecs = blk_rq_count_integrity_sg(req);
> +
> +	if (unlikely(scsi_alloc_sgtable(pdb, ivecs, gfp_mask)))
> +		return BLKPREP_DEFER;
> +
> +	count = blk_rq_map_integrity_sg(req, pdb->table.sgl);
> +	BUG_ON(unlikely(count > ivecs));
> +
> +	cmd->prot_sdb = pdb;
> +	cmd->prot_sdb->table.nents = count;
> +
> +	return BLKPREP_OK;
> +}
> +
>  /*
>   * Function:    scsi_init_io()
>   *
> @@ -1060,6 +1091,13 @@ int scsi_init_io(struct scsi_cmnd *cmd, 
>  		error = scsi_init_sgtable(cmd->request->next_rq, bidi_sdb,
>  								    GFP_ATOMIC);
>  		if (error)
> +			goto err_exit;
> +	}
> +
> +	if (blk_integrity_rq(cmd->request)) {
> +		error = scsi_protect_io(cmd, gfp_mask);
> +
> +		if (error != BLKPREP_OK)
>  			goto err_exit;
>  	}
>  
> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
> --- a/drivers/scsi/scsi_scan.c
> +++ b/drivers/scsi/scsi_scan.c
> @@ -882,6 +882,9 @@ static int scsi_add_lun(struct scsi_devi
>  
>  	if (*bflags & BLIST_USE_10_BYTE_MS)
>  		sdev->use_10_for_ms = 1;
> +
> +	if (inq_result[5] & 0x1)
> +		sdev->protection = 1;

Is there a reason to have a separate flag here and not just do something
like the usual in scsi_device.h:

static inline int scsi_device_protection(struct scsi_device *sdev)
{
	return sdev->inquiry[5] & (1<<0);
}

James