Re: [RFC PATCH 12/13] scsi: dh: ufshpb: Add prep_fn handler

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

On 2020-05-15 03:30, Avri Altman wrote:
> diff --git a/drivers/scsi/device_handler/scsi_dh_ufshpb.c b/drivers/scsi/device_handler/scsi_dh_ufshpb.c
> index affc143..04e3d56 100644
> --- a/drivers/scsi/device_handler/scsi_dh_ufshpb.c
> +++ b/drivers/scsi/device_handler/scsi_dh_ufshpb.c
> @@ -15,6 +15,7 @@
>  #include <scsi/scsi_eh.h>
>  #include <scsi/scsi_dh.h>
>  #include <scsi/scsi_dh_ufshpb.h>
> +#include "../sd.h"

Please add a comment that explains why this include directive is necessary.

> +static void __update_read_counters(struct ufshpb_dh_lun *hpb,
> +				   struct ufshpb_region *r,
> +				   struct ufshpb_subregion *s, u64 nr_blocks)
> +{
> +	enum ufshpb_state s_state;
> +
> +	atomic64_add(nr_blocks, &s->reads);
> +	atomic64_add(nr_blocks, &r->reads);
> +
> +	/* normalize read counters if needed */
> +	if (atomic64_read(&r->reads) >= READ_NORMALIZATION * entries_per_region)
> +		queue_work(hpb->wq, &hpb->reads_normalization_work);
> +
> +	rcu_read_lock();
> +	s_state = s->state;
> +	rcu_read_unlock();

We don't use locking in the Linux kernel to read a scalar that can be
read with a single load instruction, even if it can be modified while it
is being read.

> +/* Call this on read from prep_fn */
> +static bool ufshpb_test_block_dirty(struct ufshpb_dh_data *h,
> +				    struct request *rq, u64 start_lba,
> +				    u32 nr_blocks)
> +{
> +	struct ufshpb_dh_lun *hpb = h->hpb;
> +	u64 end_lba = start_lba + nr_blocks;
> +	unsigned int region = ufshpb_lba_to_region(start_lba);
> +	unsigned int subregion = ufshpb_lba_to_subregion(start_lba);
> +	struct ufshpb_region *r = hpb->region_tbl + region;
> +	struct ufshpb_subregion *s = r->subregion_tbl + subregion;
> +	enum ufshpb_state s_state;
> +
> +	__update_rw_counters(hpb, start_lba, end_lba, REQ_OP_READ);
> +
> +	rcu_read_lock();
> +	s_state = s->state;
> +	rcu_read_unlock();
> +
> +	if (s_state != HPB_STATE_ACTIVE)
> +		return true;
> +
> +	return (atomic64_read(&s->writes) >= SET_AS_DIRTY);
> +}

No parentheses around returned values please.

>  /*
>   * ufshpb_dispatch - ufshpb state machine
>   * make the various decisions based on region/subregion state & events
> @@ -631,6 +875,9 @@ static void ufshpb_work_handler(struct work_struct *work)
>  	ufshpb_dispatch(s->hpb, s->r, s);
>  
>  	mutex_unlock(&s->state_lock);
> +
> +	/* the subregion state might has changed */
> +	synchronize_rcu();
>  }

What is the purpose of this synchronize_rcu() call? This is the first
time that I see a synchronize_rcu() call at the end of a work handler.

>  static int ufshpb_activate_pinned_regions(struct ufshpb_dh_data *h, bool init)
> @@ -679,6 +926,12 @@ static int ufshpb_activate_pinned_regions(struct ufshpb_dh_data *h, bool init)
>  		set_bit(start_idx + i, hpb->pinned_map);
>  	}
>  
> +	/*
> +	 * those subregions of the pinned regions changed their state - they
> +	 * are active now
> +	 */
> +	synchronize_rcu();
> +
>  	return ret;
>  }

Same question here: what is the purpose of this synchronize_rcu() call?

> @@ -713,6 +966,9 @@ static void ufshpb_lun_reset_work_handler(struct work_struct *work)
>  		__region_reset(hpb, r);
>  	}
>  
> +	/* update rcu post lun reset */
> +	synchronize_rcu();
> +

Also here: what is the purpose of this synchronize_rcu() call?

> +/*
> + * ufshpb_prep_fn - Construct HPB_READ when possible
> + */
> +static blk_status_t ufshpb_prep_fn(struct scsi_device *sdev, struct request *rq)
> +{
> +	struct ufshpb_dh_data *h = sdev->handler_data;
> +	struct ufshpb_dh_lun *hpb = h->hpb;
> +	u64 lba = sectors_to_logical(sdev, blk_rq_pos(rq));
> +	u32 nr_blocks = sectors_to_logical(sdev, blk_rq_sectors(rq));
> +
> +	if (op_is_write(req_op(rq)) || op_is_discard(req_op(rq))) {
> +		ufshpb_set_block_dirty(h, rq, lba, nr_blocks);
> +		goto out;
> +	}
> +
> +	if (req_op(rq) != REQ_OP_READ || nr_blocks > 255)
> +		goto out;
> +
> +	if (ufshpb_test_block_dirty(h, rq, lba, nr_blocks))
> +		goto out;
> +
> +	ufshpb_prepare_hpb_read_cmd(rq, hpb, lba, (u8)nr_blocks);
> +
> +out:
> +	return BLK_STS_OK;
> +}

So this prep function calls ufshpb_prepare_hpb_read_cmd(), and that
function does the following:

	memcpy(scsi_req(rq)->cmd, cmnd, sizeof(cmnd));

I'm not sure that such a construct would be acceptable in any SCSI LLD
or device handler. The SCSI CDB is overwritten without updating the
other members of the request structure, e.g. the page pointers in the
bvecs of the bio attached to a request structure. What will e.g. happen
if rq_for_each_segment() would be called? Will it iterate over the data
buffer of the original REQ_OP_READ or will it iterate over the data
buffer of the UFSHPB_READ command?

Bart.