Subject: Re: [PATCH v2] xen/scsifront: don't request a slot on the ring until
 request is ready
To: Juergen Gross <jgross@suse.com>, linux-kernel@vger.kernel.org,
        xen-devel@lists.xenproject.org
References: <20161125202650.GK6266@mwanda>
 <20161202061545.14614-1-jgross@suse.com>
Cc: lambert.quentin@gmail.com, linux-scsi@vger.kernel.org,
        dan.carpenter@oracle.com, jejb@linux.vnet.ibm.com,
        martin.petersen@oracle.com
From: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Message-ID: <6cbdced5-c4fe-8083-3a90-09c428f3ff8f@oracle.com>
Date: Mon, 5 Dec 2016 10:32:18 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101
 Thunderbird/45.4.0
MIME-Version: 1.0
In-Reply-To: <20161202061545.14614-1-jgross@suse.com>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8BIT
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1441
Lines: 51

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.)


> @@ -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?

-boris