Re: [PATCH 1/2] scsi: core: avoid to pre-allocate big chunk for protection meta data

[Bart Van Assche <bvanassche@acm.org>]

On Tue, 2019-04-23 at 18:32 +0800, Ming Lei wrote:
> +/*
> + * Size of integrity meta data size is usually small, 1 inline sg
                        ^^^^^^^^^^^^^^
Please change this into "metadata". Mentioning "size" twice is not
useful.
> + * should cover normal cases.
> + */
> +#define  SCSI_INLINE_PROT_SG_CNT  1
> +
>  static struct kmem_cache *scsi_sdb_cache;
>  static struct kmem_cache *scsi_sense_cache;
>  static struct kmem_cache *scsi_sense_isadma_cache;
> @@ -553,12 +559,21 @@ static void scsi_uninit_cmd(struct scsi_cmnd *cmd)
>         }
>  }
>  
> +static inline bool scsi_prot_use_inline_sg(struct scsi_cmnd *cmd)
> +{
> +       if (!scsi_prot_sglist(cmd))
> +               return false;

Since scsi_prot_use_inline_sg() is only called if scsi_prot_sg_count() > 0,
is the above test necessary?

> +       return cmd->prot_sdb->table.sgl ==
> +               (struct scatterlist *)(cmd->prot_sdb + 1);
> +}
> +
>  static void scsi_mq_free_sgtables(struct scsi_cmnd *cmd)
>  {
>         if (cmd->sdb.table.nents)
>                 sg_free_table_chained(&cmd->sdb.table, true);
> -       if (scsi_prot_sg_count(cmd))
> -               sg_free_table_chained(&cmd->prot_sdb->table, true);
> +       if (scsi_prot_sg_count(cmd) && !scsi_prot_use_inline_sg(cmd))
> +               sg_free_table_chained(&cmd->prot_sdb->table, false);
>  }
>  
>  static void scsi_mq_uninit_cmd(struct scsi_cmnd *cmd)
> @@ -1044,9 +1059,11 @@ blk_status_t scsi_init_io(struct scsi_cmnd *cmd)
>                 }
>  
>                 ivecs = blk_rq_count_integrity_sg(rq->q, rq->bio);
> -
> -               if (sg_alloc_table_chained(&prot_sdb->table, ivecs,
> -                               prot_sdb->table.sgl)) {
> +               if (ivecs <= SCSI_INLINE_PROT_SG_CNT)
> +                       prot_sdb->table.nents = prot_sdb->table.orig_nents =
> +                               SCSI_INLINE_PROT_SG_CNT;

Please call sg_init_table() instead of open-coding it.

> +               else if (sg_alloc_table_chained(&prot_sdb->table, ivecs,
> +                                       NULL)) {
>                         ret = BLK_STS_RESOURCE;
>                         goto out_free_sgtables;
>                 }

Bart.