Re: [PATCH v2] xen/scsifront: don't request a slot on the ring until request is ready

From: Juergen Gross
Date: Mon Dec 05 2016 - 10:35:24 EST


On 05/12/16 16:32, Boris Ostrovsky wrote:
> On 12/02/2016 01:15 AM, Juergen Gross wrote:
>>
>> -static struct vscsiif_request *scsifront_pre_req(struct vscsifrnt_info *info)
>> +static int scsifront_do_request(struct vscsifrnt_info *info,
>> + struct vscsifrnt_shadow *shadow)
>> {
>> struct vscsiif_front_ring *ring = &(info->ring);
>> struct vscsiif_request *ring_req;
>> + struct scsi_cmnd *sc = shadow->sc;
>> uint32_t id;
>> + int i, notify;
>> +
>> + if (RING_FULL(&info->ring))
>> + return -EBUSY;
>>
>> id = scsifront_get_rqid(info); /* use id in response */
>> if (id >= VSCSIIF_MAX_REQS)
>> - return NULL;
>> + return -EBUSY;
>> +
>> + info->shadow[id] = shadow;
>> + shadow->rqid = id;
>>
>> ring_req = RING_GET_REQUEST(&(info->ring), ring->req_prod_pvt);
>> -
>> ring->req_prod_pvt++;
>>
>> - ring_req->rqid = (uint16_t)id;
>> + ring_req->rqid = id;
>> + ring_req->act = shadow->act;
>> + ring_req->ref_rqid = shadow->ref_rqid;
>> + ring_req->nr_segments = shadow->nr_segments;
>
> Shouldn't req_prod_pvt be incremented after you've copied everything? (I
> realize that there is not error until everything has been copied but still.)

That doesn't really matter as it is private.

>> @@ -473,44 +496,14 @@ static int map_data_for_request(struct vscsifrnt_info *info,
>> }
>>
>> if (seg_grants)
>> - ring_req->nr_segments = VSCSIIF_SG_GRANT | seg_grants;
>> + shadow->nr_segments = VSCSIIF_SG_GRANT | seg_grants;
>
> Are we guaranteed that seg_grants is not going to have VSCSIIF_SG_GRANT
> bit set?

Absolutely, yes. Can't be larger than VSCSIIF_SG_TABLESIZE which is 26.


Juergen