Re: [PATCH 5/5] scsi_debug: change store from vmalloc to sgl

[Bodo Stroesser <bostroesser@gmail.com>]

On 24.10.22 03:02, Douglas Gilbert wrote:
> A long time ago this driver's store was allocated by kmalloc() or
> alloc_pages(). When this was switched to vmalloc() the author
> noticed slower ramdisk access times and more variability in repeated
> tests. So try going back with sgl_alloc_order() to get uniformly
> sized allocations in a sometimes large scatter gather _array_. That
> array is the basis of maintaining O(1) access to the store.
> 
> Using sgl_alloc_order() and friends requires CONFIG_SGL_ALLOC
> so add a 'select' to the Kconfig file.
> 
> Remove kcalloc() in resp_verify() as sgl_s can now be compared
> directly without forming an intermediate buffer. This is a
> performance win for the SCSI VERIFY command implementation.
> 
> Make the SCSI COMPARE AND WRITE command yield the offset of the
> first miscompared byte when the compare fails (as required by
> T10).
> 
> Signed-off-by: Douglas Gilbert <dgilbert@interlog.com>
> ---
>   drivers/scsi/Kconfig      |   3 +-
>   drivers/scsi/scsi_debug.c | 478 +++++++++++++++++++++++++++-----------
>   2 files changed, 341 insertions(+), 140 deletions(-)
> 
> diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig
> index 03e71e3d5e5b..97edb4e17319 100644
> --- a/drivers/scsi/Kconfig
> +++ b/drivers/scsi/Kconfig
> @@ -1229,13 +1229,14 @@ config SCSI_DEBUG
>   	tristate "SCSI debugging host and device simulator"
>   	depends on SCSI
>   	select CRC_T10DIF
> +	select SGL_ALLOC
>   	help
>   	  This pseudo driver simulates one or more hosts (SCSI initiators),
>   	  each with one or more targets, each with one or more logical units.
>   	  Defaults to one of each, creating a small RAM disk device. Many
>   	  parameters found in the /sys/bus/pseudo/drivers/scsi_debug
>   	  directory can be tweaked at run time.
> -	  See <http://sg.danny.cz/sg/sdebug26.html> for more information.
> +	  See <https://sg.danny.cz/sg/scsi_debug.html> for more information.
>   	  Mainly used for testing and best as a module. If unsure, say N.
>   
>   config SCSI_MESH
> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
> index 697fc57bc711..0715521b2527 100644
> --- a/drivers/scsi/scsi_debug.c
> +++ b/drivers/scsi/scsi_debug.c
> @@ -221,6 +221,7 @@ static const char *sdebug_version_date = "20210520";
>   #define SDEBUG_CANQUEUE_WORDS  3	/* a WORD is bits in a long */
>   #define SDEBUG_CANQUEUE  (SDEBUG_CANQUEUE_WORDS * BITS_PER_LONG)
>   #define DEF_CMD_PER_LUN  SDEBUG_CANQUEUE
> +#define SDEB_ORDER_TOO_LARGE 4096
>   
>   /* UA - Unit Attention; SA - Service Action; SSU - Start Stop Unit */
>   #define F_D_IN			1	/* Data-in command (e.g. READ) */
> @@ -318,8 +319,11 @@ struct sdebug_host_info {
>   
>   /* There is an xarray of pointers to this struct's objects, one per host */
>   struct sdeb_store_info {
> +	unsigned int n_elem;    /* number of sgl elements */
> +	unsigned int order;	/* as used by alloc_pages() */
> +	unsigned int elem_pow2;	/* PAGE_SHIFT + order */
>   	rwlock_t macc_lck;	/* for atomic media access on this store */
> -	u8 *storep;		/* user data storage (ram) */
> +	struct scatterlist *sgl;  /* main store: n_elem array of same sized allocs */
>   	struct t10_pi_tuple *dif_storep; /* protection info */
>   	void *map_storep;	/* provisioning map */
>   };
> @@ -880,19 +884,6 @@ static inline bool scsi_debug_lbp(void)
>   		(sdebug_lbpu || sdebug_lbpws || sdebug_lbpws10);
>   }
>   
> -static void *lba2fake_store(struct sdeb_store_info *sip,
> -			    unsigned long long lba)
> -{
> -	struct sdeb_store_info *lsip = sip;
> -
> -	lba = do_div(lba, sdebug_store_sectors);
> -	if (!sip || !sip->storep) {
> -		WARN_ON_ONCE(true);
> -		lsip = xa_load(per_store_ap, 0);  /* should never be NULL */
> -	}
> -	return lsip->storep + lba * sdebug_sector_size;
> -}
> -
>   static struct t10_pi_tuple *dif_store(struct sdeb_store_info *sip,
>   				      sector_t sector)
>   {
> @@ -1001,7 +992,6 @@ static int scsi_debug_ioctl(struct scsi_device *dev, unsigned int cmd,
>   				    __func__, cmd);
>   	}
>   	return -EINVAL;
> -	/* return -ENOTTY; // correct return but upsets fdisk */
>   }
>   
>   static void config_cdb_len(struct scsi_device *sdev)
> @@ -1221,6 +1211,55 @@ static int fetch_to_dev_buffer(struct scsi_cmnd *scp, unsigned char *arr,
>   	return scsi_sg_copy_to_buffer(scp, arr, arr_len);
>   }
>   
> +static bool sdeb_sgl_cmp_buf(struct scatterlist *sgl, unsigned int nents,
> +			     const void *buf, size_t buflen, off_t skip)
> +{
> +	bool equ = true;
> +	size_t offset = 0;
> +	size_t len;
> +	struct sg_mapping_iter miter;
> +
> +	if (buflen == 0)
> +		return true;
> +	sg_miter_start(&miter, sgl, nents, SG_MITER_ATOMIC | SG_MITER_FROM_SG);
> +	if (!sg_miter_skip(&miter, skip))
> +		goto fini;
> +
> +	while (equ && (offset < buflen) && sg_miter_next(&miter)) {
> +		len = min(miter.length, buflen - offset);
> +		equ = memcmp(buf + offset, miter.addr, len) == 0;
> +		offset += len;
> +		miter.consumed = len;
> +		sg_miter_stop(&miter);

I think, the sg_miter_stop is obsolete here.
Also, I think there is no need to set miter.consumed. If len is less
than miter.length, the loop will end and not not call sg_miter_next
again.

Bodo

> +	}
> +fini:
> +	sg_miter_stop(&miter);
> +	return equ;
> +}
> +