Re: [PATCH v3 1/3] block: centralize PI remapping logic to the block layer

[Minwoo Im <minwoo.im.dev@gmail.com>]

On 19-09-05 19:43:54, Max Gurtovoy wrote:
> Currently dif_prepare/dif_complete functions are called during the

Maybe nicer if:
	s/dif_prepare\/dif_complete/t10_pi_prepare\/t10_pi_complete

> NVMe and SCSi layers command preparetion/completion, but their actual
> place should be the block layer since T10-PI is a general data integrity
> feature that is used by block storage protocols.
> 
> Suggested-by: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Max Gurtovoy <maxg@mellanox.com>
> ---
> 
> changes from v2:
>  - remove local variable for protection_type
>  - remove remapping between NVMe T10 definition to blk definition
>  - added patches 2/3 and 3/3
>  - remove pi_type from ns structure
> 
> changes from v1:
>  - seperate from nvme_cleanup command patches
>  - introduce blk_integrity_interval_shift to avoid div in fast path
> 
> ---
>  block/blk-core.c         |  5 +++++
>  block/blk-mq.c           |  4 ++++
>  block/blk-settings.c     |  1 +
>  block/t10-pi.c           | 16 ++++++----------
>  drivers/nvme/host/core.c | 27 ++++++++++-----------------
>  drivers/nvme/host/nvme.h |  1 -
>  drivers/scsi/sd.c        | 28 ++++++++++------------------
>  drivers/scsi/sd.h        |  1 -
>  drivers/scsi/sd_dif.c    |  2 +-
>  include/linux/blkdev.h   | 12 ++++++++++++
>  include/linux/genhd.h    |  1 +
>  include/linux/t10-pi.h   | 10 ++++------
>  12 files changed, 54 insertions(+), 54 deletions(-)
> 
> diff --git a/block/blk-core.c b/block/blk-core.c
> index d0cc6e1..eda33f9 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -34,6 +34,7 @@
>  #include <linux/ratelimit.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/blk-cgroup.h>
> +#include <linux/t10-pi.h>
>  #include <linux/debugfs.h>
>  #include <linux/bpf.h>
>  
> @@ -1405,6 +1406,10 @@ bool blk_update_request(struct request *req, blk_status_t error,
>  	if (!req->bio)
>  		return false;
>  
> +	if (blk_integrity_rq(req) && req_op(req) == REQ_OP_READ &&
> +	    error == BLK_STS_OK)
> +		t10_pi_complete(req, nr_bytes);
> +
>  	if (unlikely(error && !blk_rq_is_passthrough(req) &&
>  		     !(req->rq_flags & RQF_QUIET)))
>  		print_req_error(req, error, __func__);
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index 0835f4d..30ec078 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -30,6 +30,7 @@
>  #include <trace/events/block.h>
>  
>  #include <linux/blk-mq.h>
> +#include <linux/t10-pi.h>
>  #include "blk.h"
>  #include "blk-mq.h"
>  #include "blk-mq-debugfs.h"
> @@ -693,6 +694,9 @@ void blk_mq_start_request(struct request *rq)
>  		 */
>  		rq->nr_phys_segments++;
>  	}
> +
> +	if (blk_integrity_rq(rq) && req_op(rq) == REQ_OP_WRITE)
> +		t10_pi_prepare(rq);
>  }
>  EXPORT_SYMBOL(blk_mq_start_request);
>  
> diff --git a/block/blk-settings.c b/block/blk-settings.c
> index 2c18312..8183ffc 100644
> --- a/block/blk-settings.c
> +++ b/block/blk-settings.c
> @@ -330,6 +330,7 @@ void blk_queue_max_segment_size(struct request_queue *q, unsigned int max_size)
>  void blk_queue_logical_block_size(struct request_queue *q, unsigned short size)
>  {
>  	q->limits.logical_block_size = size;
> +	q->limits.logical_block_shift = ilog2(size);
>  
>  	if (q->limits.physical_block_size < size)
>  		q->limits.physical_block_size = size;
> diff --git a/block/t10-pi.c b/block/t10-pi.c
> index 0c00946..7d9a151 100644
> --- a/block/t10-pi.c
> +++ b/block/t10-pi.c
> @@ -171,7 +171,6 @@ static blk_status_t t10_pi_type3_verify_ip(struct blk_integrity_iter *iter)
>  /**
>   * t10_pi_prepare - prepare PI prior submitting request to device
>   * @rq:              request with PI that should be prepared
> - * @protection_type: PI type (Type 1/Type 2/Type 3)
>   *
>   * For Type 1/Type 2, the virtual start sector is the one that was
>   * originally submitted by the block layer for the ref_tag usage. Due to
> @@ -181,13 +180,13 @@ static blk_status_t t10_pi_type3_verify_ip(struct blk_integrity_iter *iter)
>   *
>   * Type 3 does not have a reference tag so no remapping is required.
>   */
> -void t10_pi_prepare(struct request *rq, u8 protection_type)
> +void t10_pi_prepare(struct request *rq)
>  {
>  	const int tuple_sz = rq->q->integrity.tuple_size;
>  	u32 ref_tag = t10_pi_ref_tag(rq);
>  	struct bio *bio;
>  
> -	if (protection_type == T10_PI_TYPE3_PROTECTION)
> +	if (rq->rq_disk->protection_type == T10_PI_TYPE3_PROTECTION)
>  		return;

What do you think about making rq->rq_disk->protection_type with a
helper?  Would it be no need or more confusing if helper provided?

Thanks!