Re: DISABLE_CLUSTERING in scsi drivers

[Juergen Gross]

On 21/11/2018 10:41, Christoph Hellwig wrote:
> Hi all,
> 
> you in the To list maintain or wrote SCSI drivers that set the
> DISABLE_CLUSTERING flag, which basically disable merges of any
> bio segments.  We already have the actual max_segment size limit
> to say which length a segment should have, independent of merged
> or originally created, so this limit generally should rarely if
> ever be required, and mostly is an old cut an paste error.
> 
> Can you go over your drivers and check if it could be removed?
> 

xen-scsifront.c doesn't need it. Do you want me to remove it at once
or are you doing it when removing the support for it?


Juergen

Re: [PATCH 31/31] xen/scsifront: Remove code that zeroes driver-private command data

[Juergen Gross]

On 24/05/17 02:34, Bart Van Assche wrote:
> Since the SCSI core zeroes driver-private command data, remove
> that code from the xen-scsifront driver.
> 
> Signed-off-by: Bart Van Assche <bart.vanass...@sandisk.com>
> Cc: Juergen Gross <jgr...@suse.com>
> Cc: xen-de...@lists.xenproject.org

Reviewed-by: Juergen Gross <jgr...@suse.com>

Thanks,

Juergen

Re: [PATCH] xen/scsifront: use offset_in_page() macro

[Juergen Gross]

On 22/04/17 03:21, Geliang Tang wrote:
> Use offset_in_page() macro instead of open-coding.
> 
> Signed-off-by: Geliang Tang 

Pushed to xen/tip for-linus-4.12


Thanks,

Juergen

Re: [PATCH] xen/scsifront: use offset_in_page() macro

[Juergen Gross]

On 25/04/17 00:15, Martin K. Petersen wrote:
> 
> Juergen,
> 
>> On 22/04/17 03:21, Geliang Tang wrote:
>>> Use offset_in_page() macro instead of open-coding.
>>>
>>> Signed-off-by: Geliang Tang <geliangt...@gmail.com>
>>
>> Reviewed-by: Juergen Gross <jgr...@suse.com>
> 
> Taking this through the Xen tree or should I queue it?

I can take it through the Xen tree.


Thanks,

Juergen

Re: [PATCH] xen/scsifront: use offset_in_page() macro

[Juergen Gross]

On 22/04/17 03:21, Geliang Tang wrote:
> Use offset_in_page() macro instead of open-coding.
> 
> Signed-off-by: Geliang Tang <geliangt...@gmail.com>

Reviewed-by: Juergen Gross <jgr...@suse.com>


Thanks,

Juergen

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

[Juergen Gross]

On 02/12/16 07:15, Juergen Gross wrote:
> Instead of requesting a new slot on the ring to the backend early, do
> so only after all has been setup for the request to be sent. This
> makes error handling easier as we don't need to undo the request id
> allocation and ring slot allocation.
> 
> Suggested-by: Jan Beulich <jbeul...@suse.com>
> Signed-off-by: Juergen Gross <jgr...@suse.com>


Commited to xen/tip.git for-linus-4.10


Juergen
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] xen-scsifront: Add a missing call to kfree

[Juergen Gross]

On 05/12/16 21:53, Dan Carpenter wrote:
> On Mon, Nov 21, 2016 at 07:01:36AM +0100, Juergen Gross wrote:
>> On 19/11/16 19:22, Quentin Lambert wrote:
>>> Most error branches following the call to kmalloc contain
>>> a call to kfree. This patch add these calls where they are
>>> missing.
>>>
>>> This issue was found with Hector.
>>>
>>> Signed-off-by: Quentin Lambert <lambert.quen...@gmail.com>
>>
>> Nice catch. I think this will need some more work, I'll do a
>> follow-on patch.
>>
> 
> The error handling is really weird.  Could you send your follow on to
> this thread?

I did:

https://lkml.org/lkml/2016/12/2/17


Juergen

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Juergen Gross]

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(>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
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Juergen Gross]

Instead of requesting a new slot on the ring to the backend early, do
so only after all has been setup for the request to be sent. This
makes error handling easier as we don't need to undo the request id
allocation and ring slot allocation.

Suggested-by: Jan Beulich <jbeul...@suse.com>
Signed-off-by: Juergen Gross <jgr...@suse.com>
---
Aargh! Need more coffee! Resend with corrected mail address for Martin Petersen
---
 drivers/scsi/xen-scsifront.c | 190 +++
 1 file changed, 84 insertions(+), 106 deletions(-)

diff --git a/drivers/scsi/xen-scsifront.c b/drivers/scsi/xen-scsifront.c
index c01316c..9aa1fe1 100644
--- a/drivers/scsi/xen-scsifront.c
+++ b/drivers/scsi/xen-scsifront.c
@@ -79,10 +79,13 @@
 struct vscsifrnt_shadow {
/* command between backend and frontend */
unsigned char act;
+   uint8_t nr_segments;
uint16_t rqid;
+   uint16_t ref_rqid;
 
unsigned int nr_grants; /* number of grants in gref[] */
struct scsiif_request_segment *sg;  /* scatter/gather elements */
+   struct scsiif_request_segment seg[VSCSIIF_SG_TABLESIZE];
 
/* Do reset or abort function. */
wait_queue_head_t wq_reset; /* reset work queue   */
@@ -172,68 +175,90 @@ static void scsifront_put_rqid(struct vscsifrnt_info 
*info, uint32_t id)
scsifront_wake_up(info);
 }
 
-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(>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;
 
-   return ring_req;
-}
+   ring_req->id  = sc->device->id;
+   ring_req->lun = sc->device->lun;
+   ring_req->channel = sc->device->channel;
+   ring_req->cmd_len = sc->cmd_len;
 
-static void scsifront_do_request(struct vscsifrnt_info *info)
-{
-   struct vscsiif_front_ring *ring = &(info->ring);
-   int notify;
+   BUG_ON(sc->cmd_len > VSCSIIF_MAX_COMMAND_SIZE);
+
+   memcpy(ring_req->cmnd, sc->cmnd, sc->cmd_len);
+
+   ring_req->sc_data_direction   = (uint8_t)sc->sc_data_direction;
+   ring_req->timeout_per_command = sc->request->timeout / HZ;
+
+   for (i = 0; i < (shadow->nr_segments & ~VSCSIIF_SG_GRANT); i++)
+   ring_req->seg[i] = shadow->seg[i];
 
RING_PUSH_REQUESTS_AND_CHECK_NOTIFY(ring, notify);
if (notify)
notify_remote_via_irq(info->irq);
+
+   return 0;
 }
 
-static void scsifront_gnttab_done(struct vscsifrnt_info *info, uint32_t id)
+static void scsifront_gnttab_done(struct vscsifrnt_info *info,
+ struct vscsifrnt_shadow *shadow)
 {
-   struct vscsifrnt_shadow *s = info->shadow[id];
int i;
 
-   if (s->sc->sc_data_direction == DMA_NONE)
+   if (shadow->sc->sc_data_direction == DMA_NONE)
return;
 
-   for (i = 0; i < s->nr_grants; i++) {
-   if (unlikely(gnttab_query_foreign_access(s->gref[i]) != 0)) {
+   for (i = 0; i < shadow->nr_grants; i++) {
+   if (unlikely(gnttab_query_foreign_access(shadow->gref[i]))) {
shost_printk(KERN_ALERT, info->host, KBUILD_MODNAME
 "grant still in use by backend\n");
BUG();
}
-   gnttab_end_foreign_access(s->gref[i], 0, 0UL);
+   gnttab_end_foreign_access(shadow->gref[i], 0, 0UL);
}
 
-   kfree(s->sg);
+   kfree(shadow->sg);
 }
 
 static void scsifront_cdb_cmd_done(struct vscsifrnt_info *info,
   struct vscsiif_response *ring_rsp)
 {
+   struct vscsifrnt_shadow *shadow;
struct scsi_cmnd *sc;
uint32_t id;
uint8_t sense_len;
 
id = ring_rsp->rqid;
-   sc = info->shadow[id]->sc;
+   shadow = info->shadow[id];
+   sc = shadow-&

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

[Juergen Gross]

Instead of requesting a new slot on the ring to the backend early, do
so only after all has been setup for the request to be sent. This
makes error handling easier as we don't need to undo the request id
allocation and ring slot allocation.

Suggested-by: Jan Beulich <jbeul...@suse.com>
Signed-off-by: Juergen Gross <jgr...@suse.com>
---
Resend with corrected mail address for Martin Peter
---
 drivers/scsi/xen-scsifront.c | 190 +++
 1 file changed, 84 insertions(+), 106 deletions(-)

diff --git a/drivers/scsi/xen-scsifront.c b/drivers/scsi/xen-scsifront.c
index c01316c..9aa1fe1 100644
--- a/drivers/scsi/xen-scsifront.c
+++ b/drivers/scsi/xen-scsifront.c
@@ -79,10 +79,13 @@
 struct vscsifrnt_shadow {
/* command between backend and frontend */
unsigned char act;
+   uint8_t nr_segments;
uint16_t rqid;
+   uint16_t ref_rqid;
 
unsigned int nr_grants; /* number of grants in gref[] */
struct scsiif_request_segment *sg;  /* scatter/gather elements */
+   struct scsiif_request_segment seg[VSCSIIF_SG_TABLESIZE];
 
/* Do reset or abort function. */
wait_queue_head_t wq_reset; /* reset work queue   */
@@ -172,68 +175,90 @@ static void scsifront_put_rqid(struct vscsifrnt_info 
*info, uint32_t id)
scsifront_wake_up(info);
 }
 
-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(>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;
 
-   return ring_req;
-}
+   ring_req->id  = sc->device->id;
+   ring_req->lun = sc->device->lun;
+   ring_req->channel = sc->device->channel;
+   ring_req->cmd_len = sc->cmd_len;
 
-static void scsifront_do_request(struct vscsifrnt_info *info)
-{
-   struct vscsiif_front_ring *ring = &(info->ring);
-   int notify;
+   BUG_ON(sc->cmd_len > VSCSIIF_MAX_COMMAND_SIZE);
+
+   memcpy(ring_req->cmnd, sc->cmnd, sc->cmd_len);
+
+   ring_req->sc_data_direction   = (uint8_t)sc->sc_data_direction;
+   ring_req->timeout_per_command = sc->request->timeout / HZ;
+
+   for (i = 0; i < (shadow->nr_segments & ~VSCSIIF_SG_GRANT); i++)
+   ring_req->seg[i] = shadow->seg[i];
 
RING_PUSH_REQUESTS_AND_CHECK_NOTIFY(ring, notify);
if (notify)
notify_remote_via_irq(info->irq);
+
+   return 0;
 }
 
-static void scsifront_gnttab_done(struct vscsifrnt_info *info, uint32_t id)
+static void scsifront_gnttab_done(struct vscsifrnt_info *info,
+ struct vscsifrnt_shadow *shadow)
 {
-   struct vscsifrnt_shadow *s = info->shadow[id];
int i;
 
-   if (s->sc->sc_data_direction == DMA_NONE)
+   if (shadow->sc->sc_data_direction == DMA_NONE)
return;
 
-   for (i = 0; i < s->nr_grants; i++) {
-   if (unlikely(gnttab_query_foreign_access(s->gref[i]) != 0)) {
+   for (i = 0; i < shadow->nr_grants; i++) {
+   if (unlikely(gnttab_query_foreign_access(shadow->gref[i]))) {
shost_printk(KERN_ALERT, info->host, KBUILD_MODNAME
 "grant still in use by backend\n");
BUG();
}
-   gnttab_end_foreign_access(s->gref[i], 0, 0UL);
+   gnttab_end_foreign_access(shadow->gref[i], 0, 0UL);
}
 
-   kfree(s->sg);
+   kfree(shadow->sg);
 }
 
 static void scsifront_cdb_cmd_done(struct vscsifrnt_info *info,
   struct vscsiif_response *ring_rsp)
 {
+   struct vscsifrnt_shadow *shadow;
struct scsi_cmnd *sc;
uint32_t id;
uint8_t sense_len;
 
id = ring_rsp->rqid;
-   sc = info->shadow[id]->sc;
+   shadow = info->shadow[id];
+   sc = shadow->sc;
 
BUG_ON(sc =

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

[Juergen Gross]

Instead of requesting a new slot on the ring to the backend early, do
so only after all has been setup for the request to be sent. This
makes error handling easier as we don't need to undo the request id
allocation and ring slot allocation.

Suggested-by: Jan Beulich <jbeul...@suse.com>
Signed-off-by: Juergen Gross <jgr...@suse.com>
---
 drivers/scsi/xen-scsifront.c | 190 +++
 1 file changed, 84 insertions(+), 106 deletions(-)

diff --git a/drivers/scsi/xen-scsifront.c b/drivers/scsi/xen-scsifront.c
index c01316c..9aa1fe1 100644
--- a/drivers/scsi/xen-scsifront.c
+++ b/drivers/scsi/xen-scsifront.c
@@ -79,10 +79,13 @@
 struct vscsifrnt_shadow {
/* command between backend and frontend */
unsigned char act;
+   uint8_t nr_segments;
uint16_t rqid;
+   uint16_t ref_rqid;
 
unsigned int nr_grants; /* number of grants in gref[] */
struct scsiif_request_segment *sg;  /* scatter/gather elements */
+   struct scsiif_request_segment seg[VSCSIIF_SG_TABLESIZE];
 
/* Do reset or abort function. */
wait_queue_head_t wq_reset; /* reset work queue   */
@@ -172,68 +175,90 @@ static void scsifront_put_rqid(struct vscsifrnt_info 
*info, uint32_t id)
scsifront_wake_up(info);
 }
 
-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(>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;
 
-   return ring_req;
-}
+   ring_req->id  = sc->device->id;
+   ring_req->lun = sc->device->lun;
+   ring_req->channel = sc->device->channel;
+   ring_req->cmd_len = sc->cmd_len;
 
-static void scsifront_do_request(struct vscsifrnt_info *info)
-{
-   struct vscsiif_front_ring *ring = &(info->ring);
-   int notify;
+   BUG_ON(sc->cmd_len > VSCSIIF_MAX_COMMAND_SIZE);
+
+   memcpy(ring_req->cmnd, sc->cmnd, sc->cmd_len);
+
+   ring_req->sc_data_direction   = (uint8_t)sc->sc_data_direction;
+   ring_req->timeout_per_command = sc->request->timeout / HZ;
+
+   for (i = 0; i < (shadow->nr_segments & ~VSCSIIF_SG_GRANT); i++)
+   ring_req->seg[i] = shadow->seg[i];
 
RING_PUSH_REQUESTS_AND_CHECK_NOTIFY(ring, notify);
if (notify)
notify_remote_via_irq(info->irq);
+
+   return 0;
 }
 
-static void scsifront_gnttab_done(struct vscsifrnt_info *info, uint32_t id)
+static void scsifront_gnttab_done(struct vscsifrnt_info *info,
+ struct vscsifrnt_shadow *shadow)
 {
-   struct vscsifrnt_shadow *s = info->shadow[id];
int i;
 
-   if (s->sc->sc_data_direction == DMA_NONE)
+   if (shadow->sc->sc_data_direction == DMA_NONE)
return;
 
-   for (i = 0; i < s->nr_grants; i++) {
-   if (unlikely(gnttab_query_foreign_access(s->gref[i]) != 0)) {
+   for (i = 0; i < shadow->nr_grants; i++) {
+   if (unlikely(gnttab_query_foreign_access(shadow->gref[i]))) {
shost_printk(KERN_ALERT, info->host, KBUILD_MODNAME
 "grant still in use by backend\n");
BUG();
}
-   gnttab_end_foreign_access(s->gref[i], 0, 0UL);
+   gnttab_end_foreign_access(shadow->gref[i], 0, 0UL);
}
 
-   kfree(s->sg);
+   kfree(shadow->sg);
 }
 
 static void scsifront_cdb_cmd_done(struct vscsifrnt_info *info,
   struct vscsiif_response *ring_rsp)
 {
+   struct vscsifrnt_shadow *shadow;
struct scsi_cmnd *sc;
uint32_t id;
uint8_t sense_len;
 
id = ring_rsp->rqid;
-   sc = info->shadow[id]->sc;
+   shadow = info->shadow[id];
+   sc = shadow->sc;
 
BUG_ON(sc == NULL);
 
-   scsifront_gnttab_done(info, id);
+   scsif

Re: [Xen-devel] [PATCH] xen/scsifront: don't advance ring request pointer in case of error

[Juergen Gross]

On 29/11/16 12:40, Jan Beulich wrote:
 On 29.11.16 at 12:19,  wrote:
>> On 29/11/16 12:14, Jan Beulich wrote:
>> On 29.11.16 at 11:50,  wrote:
 --- a/drivers/scsi/xen-scsifront.c
 +++ b/drivers/scsi/xen-scsifront.c
 @@ -184,8 +184,6 @@ static struct vscsiif_request 
 *scsifront_pre_req(struct 
>> vscsifrnt_info *info)
  
ring_req = RING_GET_REQUEST(&(info->ring), ring->req_prod_pvt);
  
 -  ring->req_prod_pvt++;
>>>
>>> Please note the "_pvt" suffix, which stands for "private": This field is
>>> not visible to the backend. Only ring->sring fields are shared, and
>>> the updating of the shared field happens in RING_PUSH_REQUESTS()
>>> and RING_PUSH_REQUESTS_AND_CHECK_NOTIFY().
>>
>> Sure, but RING_PUSH_REQUESTS() will copy req_prod_pvt to req_prod. In
>> the case corrected this would advance req_prod by two after the error
>> case before, even if only one request would have made it to the ring.
> 
> Okay, then I may have been mislead by the patch description: I
> understood it to say that you want to avoid the backend seeing
> requests which haven't been filled fully, but it looks like you're
> instead saying that for these requests the filling will never be
> completed (because of some unrelated(?) error). Iirc other
> frontend drivers behave similarly to the unpatched scsifront, and

blkfront and netfront seem to be okay.

> incrementing req_prod_pvt late has possible (perhaps just
> theoretical) other issues, like parallel retrieval and filling of them
> on mor than one CPU. Wouldn't it be better to obtain a request

In scsifront the complete critical path is guarded by a lock.

> structure only when everything else is ready (and hence no further
> errors can occur)? After all you also need to deal with the acquired
> ID upon errors, and seems odd to me to deal with the two parts of
> cleanup in different places (and even in different ways).

Hmm, I can see your point.

I'll have a look how intrusive such a change would be.


Juergen
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Xen-devel] [PATCH] xen/scsifront: don't advance ring request pointer in case of error

[Juergen Gross]

On 29/11/16 12:28, David Vrabel wrote:
> On 29/11/16 11:19, Juergen Gross wrote:
>> On 29/11/16 12:14, Jan Beulich wrote:
>>>>>> On 29.11.16 at 11:50, <jgr...@suse.com> wrote:
>>>> --- a/drivers/scsi/xen-scsifront.c
>>>> +++ b/drivers/scsi/xen-scsifront.c
>>>> @@ -184,8 +184,6 @@ static struct vscsiif_request 
>>>> *scsifront_pre_req(struct vscsifrnt_info *info)
>>>>  
>>>>ring_req = RING_GET_REQUEST(&(info->ring), ring->req_prod_pvt);
>>>>  
>>>> -  ring->req_prod_pvt++;
>>>
>>> Please note the "_pvt" suffix, which stands for "private": This field is
>>> not visible to the backend. Only ring->sring fields are shared, and
>>> the updating of the shared field happens in RING_PUSH_REQUESTS()
>>> and RING_PUSH_REQUESTS_AND_CHECK_NOTIFY().
>>
>> Sure, but RING_PUSH_REQUESTS() will copy req_prod_pvt to req_prod. In
>> the case corrected this would advance req_prod by two after the error
>> case before, even if only one request would have made it to the ring.
>>
>> As an alternative I could have decremented req_prod_pvt in case of an
>> error, but I like my current solution better.
> 
> FWIW, I found the commit message a bit misleading and also came to the
> same conclusion as Jan initially.
> 
> Perhaps,
> 
> "When adding a new request to the ring, an error may cause the
> (partially constructed) request to be discarded and used for the next.
> Thus ring->req_prod_pvt should not be advanced until we know the request
> will be successfully added to the ring."

This is indeed much better, thanks.

In case there are no other objections I'll fix this up when
committing.


Juergen

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Xen-devel] [PATCH] xen/scsifront: don't advance ring request pointer in case of error

[Juergen Gross]

On 29/11/16 12:14, Jan Beulich wrote:
 On 29.11.16 at 11:50,  wrote:
>> --- a/drivers/scsi/xen-scsifront.c
>> +++ b/drivers/scsi/xen-scsifront.c
>> @@ -184,8 +184,6 @@ static struct vscsiif_request *scsifront_pre_req(struct 
>> vscsifrnt_info *info)
>>  
>>  ring_req = RING_GET_REQUEST(&(info->ring), ring->req_prod_pvt);
>>  
>> -ring->req_prod_pvt++;
> 
> Please note the "_pvt" suffix, which stands for "private": This field is
> not visible to the backend. Only ring->sring fields are shared, and
> the updating of the shared field happens in RING_PUSH_REQUESTS()
> and RING_PUSH_REQUESTS_AND_CHECK_NOTIFY().

Sure, but RING_PUSH_REQUESTS() will copy req_prod_pvt to req_prod. In
the case corrected this would advance req_prod by two after the error
case before, even if only one request would have made it to the ring.

As an alternative I could have decremented req_prod_pvt in case of an
error, but I like my current solution better.


Juergen
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] xen/scsifront: don't advance ring request pointer in case of error

[Juergen Gross]

When allocating a new ring slot for a request don't advance the
producer index until the request is really ready to go to the backend.
Otherwise only partially filled requests will be visible to the
backend in case of errors on the frontend side.

In scsifront_action_handler() free the request id in case of an error.

Signed-off-by: Juergen Gross <jgr...@suse.com>
---
In case accepted I'll take this patch through the Xen tree as it is based
on another patch by lambert.quen...@gmail.com which is going through Xen, too.
---
 drivers/scsi/xen-scsifront.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/xen-scsifront.c b/drivers/scsi/xen-scsifront.c
index c01316c..33805f6 100644
--- a/drivers/scsi/xen-scsifront.c
+++ b/drivers/scsi/xen-scsifront.c
@@ -184,8 +184,6 @@ static struct vscsiif_request *scsifront_pre_req(struct 
vscsifrnt_info *info)
 
ring_req = RING_GET_REQUEST(&(info->ring), ring->req_prod_pvt);
 
-   ring->req_prod_pvt++;
-
ring_req->rqid = (uint16_t)id;
 
return ring_req;
@@ -196,6 +194,8 @@ static void scsifront_do_request(struct vscsifrnt_info 
*info)
struct vscsiif_front_ring *ring = &(info->ring);
int notify;
 
+   ring->req_prod_pvt++;
+
RING_PUSH_REQUESTS_AND_CHECK_NOTIFY(ring, notify);
if (notify)
notify_remote_via_irq(info->irq);
@@ -626,6 +626,7 @@ static int scsifront_action_handler(struct scsi_cmnd *sc, 
uint8_t act)
}
 
if (scsifront_enter(info)) {
+   scsifront_put_rqid(info, shadow->rqid);
spin_unlock_irq(host->host_lock);
kfree(shadow);
return FAILED;
-- 
2.10.2

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] xen-scsifront: Add a missing call to kfree

[Juergen Gross]

On 19/11/16 19:22, Quentin Lambert wrote:
> Most error branches following the call to kmalloc contain
> a call to kfree. This patch add these calls where they are
> missing.
> 
> This issue was found with Hector.
> 
> Signed-off-by: Quentin Lambert 

Applied to xen/tip.git for-linus-4.10


Juergen

> 
> ---
>  drivers/scsi/xen-scsifront.c |1 +
>  1 file changed, 1 insertion(+)
> 
> --- a/drivers/scsi/xen-scsifront.c
> +++ b/drivers/scsi/xen-scsifront.c
> @@ -627,6 +627,7 @@ static int scsifront_action_handler(stru
>  
>   if (scsifront_enter(info)) {
>   spin_unlock_irq(host->host_lock);
> + kfree(shadow);
>   return FAILED;
>   }
>  
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] xen-scsifront: Add a missing call to kfree

[Juergen Gross]

On 22/11/16 04:40, Martin K. Petersen wrote:
>>>>>> "Juergen" == Juergen Gross <jgr...@suse.com> writes:
> 
> Juergen,
> 
> Juergen> On 19/11/16 19:22, Quentin Lambert wrote:
>>> Most error branches following the call to kmalloc contain a call to
>>> kfree. This patch add these calls where they are missing.
>>>
>>> This issue was found with Hector.
>>>
>>> Signed-off-by: Quentin Lambert <lambert.quen...@gmail.com>
> 
> Juergen> Nice catch. I think this will need some more work, I'll do a
> Juergen> follow-on patch.
> 
> Juergen> Reviewed-by: Juergen Gross <jgr...@suse.com>
> 
> Are you taking this patch through the Xen tree or should I queue it up?

I'm taking it through the xen tree, thanks.


Juergen

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] xen-scsifront: Add a missing call to kfree

[Juergen Gross]

On 19/11/16 19:22, Quentin Lambert wrote:
> Most error branches following the call to kmalloc contain
> a call to kfree. This patch add these calls where they are
> missing.
> 
> This issue was found with Hector.
> 
> Signed-off-by: Quentin Lambert <lambert.quen...@gmail.com>

Nice catch. I think this will need some more work, I'll do a
follow-on patch.

Reviewed-by: Juergen Gross <jgr...@suse.com>

> 
> ---
>  drivers/scsi/xen-scsifront.c |1 +
>  1 file changed, 1 insertion(+)
> 
> --- a/drivers/scsi/xen-scsifront.c
> +++ b/drivers/scsi/xen-scsifront.c
> @@ -627,6 +627,7 @@ static int scsifront_action_handler(stru
>  
>   if (scsifront_enter(info)) {
>   spin_unlock_irq(host->host_lock);
> + kfree(shadow);
>   return FAILED;
>   }
>  
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/3] xen-scsiback: Rename jump labels in scsiback_device_action()

[Juergen Gross]

On 20/07/16 13:34, SF Markus Elfring wrote:
> From: Markus Elfring <elfr...@users.sourceforge.net>
> Date: Wed, 20 Jul 2016 13:03:16 +0200
> 
> * Adjust jump targets according to the Linux coding style convention.
> 
> * A bit of refactoring for the control flow
> 
> Suggested-by: Jürgen Groß <jgr...@suse.com>
> Signed-off-by: Markus Elfring <elfr...@users.sourceforge.net>

Reviewed-by: Juergen Gross <jgr...@suse.com>


Juergen
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/3] xen-scsiback: Delete an unnecessary check before the function call "kfree"

[Juergen Gross]

On 20/07/16 13:30, SF Markus Elfring wrote:
> From: Markus Elfring <elfr...@users.sourceforge.net>
> Date: Tue, 19 Jul 2016 15:42:19 +0200
> 
> The kfree() function tests whether its argument is NULL and then
> returns immediately. Thus the test around the call is not needed.
> 
> This issue was detected by using the Coccinelle software.
> 
> Signed-off-by: Markus Elfring <elfr...@users.sourceforge.net>

Even if already given, here it is again:

Reviewed-by: Juergen Gross <jgr...@suse.com>


Juergen
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/3] xen-scsiback: Pass a failure indication as a constant

[Juergen Gross]

On 20/07/16 13:36, SF Markus Elfring wrote:
> From: Markus Elfring <elfr...@users.sourceforge.net>
> Date: Wed, 20 Jul 2016 13:12:33 +0200
> 
> Pass the constant "FAILED" in a function call directly instead of
> using an intialisation for a local variable.
> 
> Signed-off-by: Markus Elfring <elfr...@users.sourceforge.net>

Even if already given, here it is again:

Reviewed-by: Juergen Gross <jgr...@suse.com>


Juergen
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] xen-scsiback: One function call less in scsiback_device_action() after error detection

[Juergen Gross]

On 20/07/16 07:10, SF Markus Elfring wrote:
> @@ -606,7 +606,7 @@ static void scsiback_device_action(struct 
> vscsibk_pend *pending_req,
>   tmr = kzalloc(sizeof(struct scsiback_tmr), GFP_KERNEL);
>   if (!tmr) {
>   target_put_sess_cmd(se_cmd);
> - goto err;
> + goto do_resp;
>   }

 Hmm, I'm not convinced this is an improvement.

 I'd rather rename the new error label to "put_cmd" and get rid of the
 braces in above if statement:

 -  if (!tmr) {
 -  target_put_sess_cmd(se_cmd);
 -  goto err;
 -  }
 +  if (!tmr)
 +  goto put_cmd;

 and then in the error path:

 -err:
 +put_cmd:
 +  target_put_sess_cmd(se_cmd);
>>>
>>> I am unsure on the relevance of this function on such a source position.
>>> Would it make sense to move it further down at the end?
>>
>> You only want to call it in the first error case (allocation failure).
> 
> Thanks for your clarification.
> 
> I find that my update suggestion (from Saturday) is still appropriate
> in this case.
> https://lkml.org/lkml/2016/7/16/172

And I still think it isn't an improvement: Nack

 +free_tmr:
kfree(tmr);
>>>
>>> How do you think about to skip this function call after a memory
>>> allocation failure?
>>
>> I think this just doesn't matter. If it were a hot path, yes. But trying
>> to do micro-optimizations in an error path is just not worth the effort.
> 
> Would you like to reduce also the amount of function calls in such special
> run-time situations?

I just don't care for the extra 2 or 3 nsecs. Readability is more
important here.

>> I like a linear error path containing all the needed cleanups best.
> 
> I would prefer to keep the discussed single function call within
> the basic block of the if statement.
> 
> Have we got different opinions about the shown implementation details?

Yes.


Juergen

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] xen-scsiback: One function call less in scsiback_device_action() after error detection

[Juergen Gross]

On 19/07/16 16:56, SF Markus Elfring wrote:
>>> @@ -606,7 +606,7 @@ static void scsiback_device_action(struct vscsibk_pend 
>>> *pending_req,
>>> tmr = kzalloc(sizeof(struct scsiback_tmr), GFP_KERNEL);
>>> if (!tmr) {
>>> target_put_sess_cmd(se_cmd);
>>> -   goto err;
>>> +   goto do_resp;
>>> }
>>
>> Hmm, I'm not convinced this is an improvement.
>>
>> I'd rather rename the new error label to "put_cmd" and get rid of the
>> braces in above if statement:
>>
>> -if (!tmr) {
>> -target_put_sess_cmd(se_cmd);
>> -goto err;
>> -}
>> +if (!tmr)
>> +goto put_cmd;
>>
>> and then in the error path:
>>
>> -err:
>> +put_cmd:
>> +target_put_sess_cmd(se_cmd);
> 
> I am unsure on the relevance of this function on such a source position.
> Would it make sense to move it further down at the end?

You only want to call it in the first error case (allocation failure).

>> +free_tmr:
>>  kfree(tmr);
> 
> How do you think about to skip this function call after a memory
> allocation failure?

I think this just doesn't matter. If it were a hot path, yes. But trying
to do micro-optimizations in an error path is just not worth the effort.

I like a linear error path containing all the needed cleanups best.


Juergen
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] xen-scsiback: One function call less in scsiback_device_action() after error detection

[Juergen Gross]

On 16/07/16 22:23, SF Markus Elfring wrote:
> From: Markus Elfring 
> Date: Sat, 16 Jul 2016 21:42:42 +0200
> 
> The kfree() function was called in one case by the
> scsiback_device_action() function during error handling
> even if the passed variable "tmr" contained a null pointer.
> 
> Adjust jump targets according to the Linux coding style convention.
> 
> Signed-off-by: Markus Elfring 
> ---
>  drivers/xen/xen-scsiback.c | 7 ---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/xen/xen-scsiback.c b/drivers/xen/xen-scsiback.c
> index 4a48c06..7612bc9 100644
> --- a/drivers/xen/xen-scsiback.c
> +++ b/drivers/xen/xen-scsiback.c
> @@ -606,7 +606,7 @@ static void scsiback_device_action(struct vscsibk_pend 
> *pending_req,
>   tmr = kzalloc(sizeof(struct scsiback_tmr), GFP_KERNEL);
>   if (!tmr) {
>   target_put_sess_cmd(se_cmd);
> - goto err;
> + goto do_resp;
>   }

Hmm, I'm not convinced this is an improvement.

I'd rather rename the new error label to "put_cmd" and get rid of the
braces in above if statement:

-   if (!tmr) {
-   target_put_sess_cmd(se_cmd);
-   goto err;
-   }
+   if (!tmr)
+   goto put_cmd;

and then in the error path:

-err:
+put_cmd:
+   target_put_sess_cmd(se_cmd);
+free_tmr:
kfree(tmr);


Juergen

>  
>   init_waitqueue_head(>tmr_wait);
> @@ -616,7 +616,7 @@ static void scsiback_device_action(struct vscsibk_pend 
> *pending_req,
>  unpacked_lun, tmr, act, GFP_KERNEL,
>  tag, TARGET_SCF_ACK_KREF);
>   if (rc)
> - goto err;
> + goto free_tmr;
>  
>   wait_event(tmr->tmr_wait, atomic_read(>tmr_complete));
>  
> @@ -626,8 +626,9 @@ static void scsiback_device_action(struct vscsibk_pend 
> *pending_req,
>   scsiback_do_resp_with_sense(NULL, err, 0, pending_req);
>   transport_generic_free_cmd(_req->se_cmd, 1);
>   return;
> -err:
> +free_tmr:
>   kfree(tmr);
> +do_resp:
>   scsiback_do_resp_with_sense(NULL, err, 0, pending_req);
>  }
>  
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] xen-scsiback: Pass a failure indication as a constant

[Juergen Gross]

On 16/07/16 22:24, SF Markus Elfring wrote:
> From: Markus Elfring <elfr...@users.sourceforge.net>
> Date: Sat, 16 Jul 2016 21:55:01 +0200
> 
> Pass the constant "FAILED" in a function call directly instead of
> using an intialisation for a local variable.
> 
> Signed-off-by: Markus Elfring <elfr...@users.sourceforge.net>

Reviewed-by: Juergen Gross <jgr...@suse.com>


Juergen
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] xen-scsiback: Delete an unnecessary check before the function call "kfree"

[Juergen Gross]

On 16/07/16 22:22, SF Markus Elfring wrote:
> From: Markus Elfring <elfr...@users.sourceforge.net>
> Date: Sat, 16 Jul 2016 21:21:05 +0200
> 
> The kfree() function tests whether its argument is NULL and then
> returns immediately. Thus the test around the call is not needed.
> 
> This issue was detected by using the Coccinelle software.
> 
> Signed-off-by: Markus Elfring <elfr...@users.sourceforge.net>

Reviewed-by: Juergen Gross <jgr...@suse.com>


Juergen
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] xen_pvscsi: reclaim the ring request when the prepairing failed

[Juergen Gross]

On 12/07/16 02:36, Bin Wu wrote:
> During scsi command queueing or exception handling, if prepairing
> fails, we need to reclaim the failed request. Otherwise, the garbage
> request will be pushed into the ring for the backend to work.
> 
> Signed-off-by: Bin Wu <wu.wu...@huawei.com>

Reviewed-by: Juergen Gross <jgr...@suse.com>

> ---
>  drivers/scsi/xen-scsifront.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/xen-scsifront.c b/drivers/scsi/xen-scsifront.c
> index 9dc8687..8646db1 100644
> --- a/drivers/scsi/xen-scsifront.c
> +++ b/drivers/scsi/xen-scsifront.c
> @@ -184,8 +184,6 @@ static struct vscsiif_request *scsifront_pre_req(struct 
> vscsifrnt_info *info)
>  
>   ring_req = RING_GET_REQUEST(&(info->ring), ring->req_prod_pvt);
>  
> - ring->req_prod_pvt++;
> -
>   ring_req->rqid = (uint16_t)id;
>  
>   return ring_req;
> @@ -196,6 +194,8 @@ static void scsifront_do_request(struct vscsifrnt_info 
> *info)
>   struct vscsiif_front_ring *ring = &(info->ring);
>   int notify;
>  
> + ring->req_prod_pvt++;
> +
>   RING_PUSH_REQUESTS_AND_CHECK_NOTIFY(ring, notify);
>   if (notify)
>   notify_remote_via_irq(info->irq);
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Xen-devel] [PATCH] xen_pvscsi: reclaim the ring request when mapping data failed

[Juergen Gross]

On 11/07/16 11:50, David Vrabel wrote:
> On 11/07/16 10:33, Juergen Gross wrote:
>> On 11/07/16 04:51, Bin Wu wrote:
>>> During scsi command queueing, if mapping data fails, we need to
>>> reclaim the failed request. Otherwise, the garbage request will
>>> be pushed into the ring for the backend to work.
>>
>> Well spotted. There is another instance of this problem in
>> scsifront_action_handler(). Would you mind correcting this one, too?
> 
> Would it make more sense to advance req_prod_pvt only if the request has
> been successfully created?

Yeah, probably as the first action in scsifront_do_request().


Juergen

> 
> David
> 
>>> Signed-off-by: Bin Wu <wu.wu...@huawei.com>
>>> ---
>>>  drivers/scsi/xen-scsifront.c | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/drivers/scsi/xen-scsifront.c b/drivers/scsi/xen-scsifront.c
>>> index 9dc8687..655163d 100644
>>> --- a/drivers/scsi/xen-scsifront.c
>>> +++ b/drivers/scsi/xen-scsifront.c
>>> @@ -565,6 +565,7 @@ static int scsifront_queuecommand(struct Scsi_Host
>>> *shost,
>>>  err = map_data_for_request(info, sc, ring_req, shadow);
>>>  if (err < 0) {
>>>  pr_debug("%s: err %d\n", __func__, err);
>>> +info->ring.req_prod_pvt--;
>>>  scsifront_put_rqid(info, rqid);
>>>  scsifront_return(info);
>>>  spin_unlock_irqrestore(shost->host_lock, flags);
>>
>>
>> ___
>> Xen-devel mailing list
>> xen-de...@lists.xen.org
>> https://lists.xen.org/xen-devel
>>
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] xen_pvscsi: reclaim the ring request when mapping data failed

[Juergen Gross]

On 11/07/16 04:51, Bin Wu wrote:
> During scsi command queueing, if mapping data fails, we need to
> reclaim the failed request. Otherwise, the garbage request will
> be pushed into the ring for the backend to work.

Well spotted. There is another instance of this problem in
scsifront_action_handler(). Would you mind correcting this one, too?


Juergen

> 
> Signed-off-by: Bin Wu 
> ---
>  drivers/scsi/xen-scsifront.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/scsi/xen-scsifront.c b/drivers/scsi/xen-scsifront.c
> index 9dc8687..655163d 100644
> --- a/drivers/scsi/xen-scsifront.c
> +++ b/drivers/scsi/xen-scsifront.c
> @@ -565,6 +565,7 @@ static int scsifront_queuecommand(struct Scsi_Host
> *shost,
>  err = map_data_for_request(info, sc, ring_req, shadow);
>  if (err < 0) {
>  pr_debug("%s: err %d\n", __func__, err);
> +info->ring.req_prod_pvt--;
>  scsifront_put_rqid(info, rqid);
>  scsifront_return(info);
>  spin_unlock_irqrestore(shost->host_lock, flags);

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] xen-scsiback: correct return value checks on xenbus_scanf()

[Juergen Gross]

On 07/07/16 10:01, Jan Beulich wrote:
> Only a positive return value indicates success.
> 
> Signed-off-by: Jan Beulich <jbeul...@suse.com>

Acked-by: Juergen Gross <jgr...@suse.com>

> ---
>  drivers/xen/xen-scsiback.c |4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> --- 4.7-rc6-xenbus_scanf.orig/drivers/xen/xen-scsiback.c
> +++ 4.7-rc6-xenbus_scanf/drivers/xen/xen-scsiback.c
> @@ -1071,7 +1071,7 @@ static void scsiback_do_1lun_hotplug(str
>   /* read status */
>   snprintf(state, sizeof(state), "vscsi-devs/%s/state", ent);
>   err = xenbus_scanf(XBT_NIL, dev->nodename, state, "%u", _state);
> - if (XENBUS_EXIST_ERR(err))
> + if (err <= 0)
>   return;
>  
>   /* physical SCSI device */
> @@ -1089,7 +1089,7 @@ static void scsiback_do_1lun_hotplug(str
>   snprintf(str, sizeof(str), "vscsi-devs/%s/v-dev", ent);
>   err = xenbus_scanf(XBT_NIL, dev->nodename, str, "%u:%u:%u:%u",
>  , , , );
> - if (XENBUS_EXIST_ERR(err)) {
> + if (err != 4) {
>   xenbus_printf(XBT_NIL, dev->nodename, state,
> "%d", XenbusStateClosed);
>   return;
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] xen-scsifront: correct return value checks on xenbus_scanf()

[Juergen Gross]

On 07/07/16 10:01, Jan Beulich wrote:
> Only a positive return value indicates success.
> 
> Signed-off-by: Jan Beulich <jbeul...@suse.com>

Acked-by: Juergen Gross <jgr...@suse.com>

> ---
>  drivers/scsi/xen-scsifront.c |4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> --- 4.7-rc6-xenbus_scanf.orig/drivers/scsi/xen-scsifront.c
> +++ 4.7-rc6-xenbus_scanf/drivers/scsi/xen-scsifront.c
> @@ -1000,14 +1000,14 @@ static void scsifront_do_lun_hotplug(str
>   snprintf(str, sizeof(str), "vscsi-devs/%s/state", dir[i]);
>   err = xenbus_scanf(XBT_NIL, dev->otherend, str, "%u",
>  _state);
> - if (XENBUS_EXIST_ERR(err))
> + if (err <= 0)
>   continue;
>  
>   /* virtual SCSI device */
>   snprintf(str, sizeof(str), "vscsi-devs/%s/v-dev", dir[i]);
>   err = xenbus_scanf(XBT_NIL, dev->otherend, str,
>  "%u:%u:%u:%u", , , , );
> - if (XENBUS_EXIST_ERR(err))
> + if (err != 4)
>   continue;
>  
>   /*
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 41/42] block: do not use REQ_FLUSH for tracking flush support

[Juergen Gross]

On 15/04/16 21:16, mchri...@redhat.com wrote:
> From: Mike Christie <mchri...@redhat.com>
> 
> The last patch added a REQ_OP_FLUSH for request_fn drivers
> and the next patch renames REQ_FLUSH to REQ_PREFLUSH which
> will be used by file systems and make_request_fn drivers so
> they can send a write/flush combo.
> 
> This patch drops xen's use of REQ_FLUSH to track if it supports
> REQ_OP_FLUSH requests, so REQ_FLUSH can be deleted.
> 
> v7:
> - Fix feature_flush/fua use.
> 
> v6:
> - Dropped parts of patch handled by Jens's QUEUE_FLAG_WC/FUA
> patches and modified patch to check feature_flush/fua bits.
> 
> Signed-off-by: Mike Christie <mchri...@redhat.com>
> Reviewed-by: Hannes Reinecke <h...@suse.com>
> Cc: Juergen Gross <ker...@pfupf.net>

Acked-by: Juergen Gross <jgr...@suse.com>


Juergen
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 41/42] block: do not use REQ_FLUSH for tracking flush support

[Juergen Gross]

On 15/04/16 12:40, mchri...@redhat.com wrote:
> From: Mike Christie 
> 
> The last patch added a REQ_OP_FLUSH for request_fn drivers
> and the next patch renames REQ_FLUSH to REQ_PREFLUSH which
> will be used by file systems and make_request_fn drivers so
> they can send a write/flush combo.
> 
> This patch drops xen's use of REQ_FLUSH to track if it supports
> REQ_OP_FLUSH requests, so REQ_FLUSH can be deleted.
> 
> v6:
> - Dropped parts of patch handled by Jens's QUEUE_FLAG_WC/FUA
> patches and modified patch to check feature_flush/fua bits.
> 
> Signed-off-by: Mike Christie 
> Reviewed-by: Hannes Reinecke 
> ---
>  drivers/block/xen-blkfront.c | 47 
> ++--
>  1 file changed, 24 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
> index f01691a..d6429e7 100644
> --- a/drivers/block/xen-blkfront.c
> +++ b/drivers/block/xen-blkfront.c

...

> @@ -985,24 +981,22 @@ static int xlvbd_init_blk_queue(struct gendisk *gd, u16 
> sector_size,
>   return 0;
>  }
>  
> -static const char *flush_info(unsigned int feature_flush)
> +static const char *flush_info(struct blkfront_info *info)
>  {
> - switch (feature_flush & ((REQ_FLUSH | REQ_FUA))) {
> - case REQ_FLUSH|REQ_FUA:
> + if (info->feature_flush && info->feature_fua)
>   return "barrier: enabled;";
> - case REQ_FLUSH:
> + else if (info->feature_fua)

Shouldn't this test feature_flush?

>   return "flush diskcache: enabled;";
> - default:
> + else
>   return "barrier or flush: disabled;";
> - }
>  }
>  
>  static void xlvbd_flush(struct blkfront_info *info)
>  {
> - blk_queue_write_cache(info->rq, info->feature_flush & REQ_FLUSH,
> - info->feature_flush & REQ_FUA);
> + blk_queue_write_cache(info->rq, info->feature_flush ? true : false,
> +   info->feature_flush ? true : false);

And here the second test should be feature_fua?

>   pr_info("blkfront: %s: %s %s %s %s %s\n",
> - info->gd->disk_name, flush_info(info->feature_flush),
> + info->gd->disk_name, flush_info(info),
>   "persistent grants:", info->feature_persistent ?
>   "enabled;" : "disabled;", "indirect descriptors:",
>   info->max_indirect_segments ? "enabled;" : "disabled;");


Juergen

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH-v3 12/14] xen-scsiback: Convert to TARGET_SCF_ACK_KREF I/O krefs

[Juergen Gross]

On 30/01/16 08:05, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <n...@linux-iscsi.org>
> 
> Cc: Juergen Gross <jgr...@suse.com>
> Cc: Hannes Reinecke <h...@suse.de>
> Cc: David Vrabel <david.vra...@citrix.com>
> Signed-off-by: Nicholas Bellinger <n...@linux-iscsi.org>

Sorry, with your patches applied xen-scsiback isn't working any more.
I've tried multiple times with and without your patches. Without the
patches everything is fine, while with the patches applied I get the
warnings shown in the attached log. I just passed through a DVD drive
and did "eject" in the domain.


Juergen

[10984.266570] [ cut here ]
[10984.266597] WARNING: CPU: 0 PID: 0 at 
drivers/target/target_core_transport.c:717 target_complete_cmd+0x1cb/0x200 
[target_core_mod]()
[10984.266601] Modules linked in: xt_physdev br_netfilter iptable_filter 
ip_tables x_tables loop target_core_pscsi target_core_file target_core_iblock 
iscsi_target_mod tcm_loop xen_scsiback bridge stp llc iscsi_ibft 
iscsi_boot_sysfs tun arc4 iwldvm mac80211 joydev iwlwifi uvcvideo 
videobuf2_vmalloc videobuf2_memops videobuf2_v4l2 videobuf2_core 
snd_hda_codec_realtek cfg80211 snd_hda_codec_hdmi snd_hda_codec_generic 
videodev intel_rapl x86_pkg_temp_thermal e1000e intel_powerclamp snd_hda_intel 
media snd_hda_codec coretemp crct10dif_pclmul iTCO_wdt snd_hda_core 
crc32_pclmul iTCO_vendor_support dell_laptop crc32c_intel sdhci_pci rfkill 
ghash_clmulni_intel ptp snd_hwdep hmac i2c_hid drbg snd_pcm ansi_cprng hid 
dell_wmi ppdev snd_timer sparse_keymap dcdbas dell_smm_hwmon parport_pc parport 
snd i2c_designware_platform
[10984.266670]  i2c_designware_core thermal tpm_tis xhci_pci tpm evdev mei_me 
xhci_hcd aesni_intel mei aes_x86_64 psmouse shpchp lrw lpc_ich gf128mul 
i2c_i801 soundcore mfd_core pps_core ac glue_helper ablk_helper battery 
serio_raw pcspkr cryptd wmi target_core_mod xenfs xen_privcmd configfs dm_mod 
ext4 crc16 mbcache jbd2 sd_mod sr_mod cdrom i915 ehci_pci ehci_hcd i2c_algo_bit 
drm_kms_helper ahci libahci libata usbcore usb_common drm sdhci_acpi video 
sdhci mmc_core button xen_acpi_processor xen_pciback xen_netback xen_blkback 
xen_gntalloc xen_gntdev xen_evtchn sg scsi_mod autofs4
[10984.266735] CPU: 0 PID: 0 Comm: swapper/0 Tainted: GW   
4.5.0-rc1-pv+ #1
[10984.266739] Hardware name: Dell Inc. Latitude E6440/0159N7, BIOS A07 
06/26/2014
[10984.266742]  a0422408 812e8bb4  
8106e95c
[10984.266748]  8800d2e007d0 8800d2e008e0 0001 
a08c3150
[10984.266753]   a04091ab 8800da13ea20 
88000294f7c0
[10984.266758] Call Trace:
[10984.266761][] ? dump_stack+0x40/0x5c
[10984.266776]  [] ? warn_slowpath_common+0x7c/0xb0
[10984.266784]  [] ? pscsi_bi_endio+0x10/0x10 
[target_core_pscsi]
[10984.266794]  [] ? target_complete_cmd+0x1cb/0x200 
[target_core_mod]
[10984.266799]  [] ? pscsi_req_done+0x85/0xd0 
[target_core_pscsi]
[10984.266811]  [] ? scsi_end_request+0xf7/0x1a0 [scsi_mod]
[10984.266820]  [] ? scsi_io_completion+0xfa/0x5f0 [scsi_mod]
[10984.266830]  [] ? blk_done_softirq+0x73/0x90
[10984.266836]  [] ? __do_softirq+0xcc/0x240
[10984.266842]  [] ? irq_exit+0x86/0x90
[10984.266852]  [] ? xen_evtchn_do_upcall+0x2c/0x40
[10984.266862]  [] ? xen_do_hypervisor_callback+0x1e/0x40
[10984.266864][] ? xen_hypercall_sched_op+0xa/0x20
[10984.266874]  [] ? xen_hypercall_sched_op+0xa/0x20
[10984.266882]  [] ? xen_safe_halt+0xc/0x20
[10984.266891]  [] ? default_idle+0x13/0x90
[10984.266898]  [] ? cpu_startup_entry+0x25d/0x2f0
[10984.266903]  [] ? start_kernel+0x471/0x47c
[10984.266907]  [] ? set_init_arg+0x50/0x50
[10984.266912]  [] ? xen_start_kernel+0x522/0x52c
[10984.266916] ---[ end trace 07ad307d0cb62aa4 ]---

[10984.266940] [ cut here ]
[10984.266953] WARNING: CPU: 0 PID: 2448 at 
drivers/target/target_core_transport.c:2120 target_complete_ok_work+0x291/0x2e0 
[target_core_mod]()
[10984.266955] Modules linked in: xt_physdev br_netfilter iptable_filter 
ip_tables x_tables loop target_core_pscsi target_core_file target_core_iblock 
iscsi_target_mod tcm_loop xen_scsiback bridge stp llc iscsi_ibft 
iscsi_boot_sysfs tun arc4 iwldvm mac80211 joydev iwlwifi uvcvideo 
videobuf2_vmalloc videobuf2_memops videobuf2_v4l2 videobuf2_core 
snd_hda_codec_realtek cfg80211 snd_hda_codec_hdmi snd_hda_codec_generic 
videodev intel_rapl x86_pkg_temp_thermal e1000e intel_powerclamp snd_hda_intel 
media snd_hda_codec coretemp crct10dif_pclmul iTCO_wdt snd_hda_core 
crc32_pclmul iTCO_vendor_support dell_laptop crc32c_intel sdhci_pci rfkill 
ghash_clmulni_intel ptp snd_hwdep hmac i2c_hid drbg snd_pcm ansi_cprng hid 
dell_wmi ppdev snd_timer sparse_keymap dcdbas dell_smm_hwmon parport_pc parport 
snd i2c_designware_platform
[10984.267013]  i2c_designware_core thermal tpm_tis xhci_pci tpm evdev mei_me 
xhci_hcd aesni_intel mei aes_x86_64 psmous

Re: [PATCH-v2 11/12] xen-scsiback: Convert to percpu_ida tag allocation

[Juergen Gross]

On 27/01/16 07:28, Nicholas A. Bellinger wrote:
> On Tue, 2016-01-26 at 10:45 +0100, Juergen Gross wrote:
>> On 25/01/16 09:11, Nicholas A. Bellinger wrote:
>>> From: Nicholas Bellinger <n...@linux-iscsi.org>
>>>
>>> Cc: Juergen Gross <jgr...@suse.com>
>>> Cc: Hannes Reinecke <h...@suse.de>
>>> Cc: David Vrabel <david.vra...@citrix.com>
>>> Signed-off-by: Nicholas Bellinger <n...@linux-iscsi.org>
>>> ---
>>>  drivers/xen/xen-scsiback.c | 163 
>>> -
>>>  1 file changed, 87 insertions(+), 76 deletions(-)
>>>
>>> diff --git a/drivers/xen/xen-scsiback.c b/drivers/xen/xen-scsiback.c
>>> index 594f8a7..640fb22 100644
>>> --- a/drivers/xen/xen-scsiback.c
>>> +++ b/drivers/xen/xen-scsiback.c
>>> @@ -190,7 +190,6 @@ module_param_named(max_buffer_pages, 
>>> scsiback_max_buffer_pages, int, 0644);
>>>  MODULE_PARM_DESC(max_buffer_pages,
>>>  "Maximum number of free pages to keep in backend buffer");
>>>  
>>> -static struct kmem_cache *scsiback_cachep;
>>>  static DEFINE_SPINLOCK(free_pages_lock);
>>>  static int free_pages_num;
>>>  static LIST_HEAD(scsiback_free_pages);
>>> @@ -322,7 +321,8 @@ static void scsiback_free_translation_entry(struct kref 
>>> *kref)
>>>  }
>>>  
>>>  static void scsiback_do_resp_with_sense(char *sense_buffer, int32_t result,
>>> -   uint32_t resid, struct vscsibk_pend *pending_req)
>>> +   uint32_t resid, struct vscsibk_pend *pending_req,
>>> +   uint16_t rqid)
>>>  {
>>> struct vscsiif_response *ring_res;
>>> struct vscsibk_info *info = pending_req->info;
>>
>> pending_req might be NULL now, so this will panic the system.
>>
> 
> Thanks for the review.
> 
> Added the following to propagate up original *info into
> scsiback_do_resp_with_sense() to address the early pending_req
> failure case.
> 
> https://git.kernel.org/cgit/linux/kernel/git/nab/target-pending.git/commit/?h=queue-next=5873f22a9b7c7aa16ff9a85074a07b739f1d06a5

Hmm, wouldn't it make more sense to split scsiback_do_resp_with_sense()
into two functions now? Something like:


diff --git a/drivers/xen/xen-scsiback.c b/drivers/xen/xen-scsiback.c
index ad4eb10..0d71467 100644
--- a/drivers/xen/xen-scsiback.c
+++ b/drivers/xen/xen-scsiback.c
@@ -321,11 +321,10 @@ static void scsiback_free_translation_entry(struct
kref *kref)
kfree(entry);
 }

-static void scsiback_do_resp_with_sense(char *sense_buffer, int32_t result,
-   uint32_t resid, struct vscsibk_pend *pending_req)
+static void scsiback_send_response(struct vscsibk_info *info,
+   char *sense_buffer, int32_t result, uint32_t resid, uint16_t rqid)
 {
struct vscsiif_response *ring_res;
-   struct vscsibk_info *info = pending_req->info;
int notify;
struct scsi_sense_hdr sshdr;
unsigned long flags;
@@ -337,7 +336,7 @@ static void scsiback_do_resp_with_sense(char
*sense_buffer, int32_t result,
info->ring.rsp_prod_pvt++;

ring_res->rslt   = result;
-   ring_res->rqid   = pending_req->rqid;
+   ring_res->rqid   = rqid;

if (sense_buffer != NULL &&
scsi_normalize_sense(sense_buffer, VSCSIIF_SENSE_BUFFERSIZE,
@@ -357,6 +356,13 @@ static void scsiback_do_resp_with_sense(char
*sense_buffer, int32_t result,

if (notify)
notify_remote_via_irq(info->irq);
+}
+
+static void scsiback_do_resp_with_sense(char *sense_buffer, int32_t result,
+   uint32_t resid, struct vscsibk_pend *pending_req)
+{
+   scsiback_send_response(pending_req->info, sense_buffer, result,
+  resid, pending_req->rqid);

if (pending_req->v2p)
kref_put(_req->v2p->kref,


And then call scsiback_send_response() directly in case pending_req
is NULL.


Juergen
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH-v2 11/12] xen-scsiback: Convert to percpu_ida tag allocation

[Juergen Gross]

On 25/01/16 09:11, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <n...@linux-iscsi.org>
> 
> Cc: Juergen Gross <jgr...@suse.com>
> Cc: Hannes Reinecke <h...@suse.de>
> Cc: David Vrabel <david.vra...@citrix.com>
> Signed-off-by: Nicholas Bellinger <n...@linux-iscsi.org>
> ---
>  drivers/xen/xen-scsiback.c | 163 
> -
>  1 file changed, 87 insertions(+), 76 deletions(-)
> 
> diff --git a/drivers/xen/xen-scsiback.c b/drivers/xen/xen-scsiback.c
> index 594f8a7..640fb22 100644
> --- a/drivers/xen/xen-scsiback.c
> +++ b/drivers/xen/xen-scsiback.c
> @@ -190,7 +190,6 @@ module_param_named(max_buffer_pages, 
> scsiback_max_buffer_pages, int, 0644);
>  MODULE_PARM_DESC(max_buffer_pages,
>  "Maximum number of free pages to keep in backend buffer");
>  
> -static struct kmem_cache *scsiback_cachep;
>  static DEFINE_SPINLOCK(free_pages_lock);
>  static int free_pages_num;
>  static LIST_HEAD(scsiback_free_pages);
> @@ -322,7 +321,8 @@ static void scsiback_free_translation_entry(struct kref 
> *kref)
>  }
>  
>  static void scsiback_do_resp_with_sense(char *sense_buffer, int32_t result,
> - uint32_t resid, struct vscsibk_pend *pending_req)
> + uint32_t resid, struct vscsibk_pend *pending_req,
> + uint16_t rqid)
>  {
>   struct vscsiif_response *ring_res;
>   struct vscsibk_info *info = pending_req->info;

pending_req might be NULL now, so this will panic the system.


Juergen

> @@ -337,7 +337,7 @@ static void scsiback_do_resp_with_sense(char 
> *sense_buffer, int32_t result,
>   info->ring.rsp_prod_pvt++;
>  
>   ring_res->rslt   = result;
> - ring_res->rqid   = pending_req->rqid;
> + ring_res->rqid   = rqid;
>  
>   if (sense_buffer != NULL &&
>   scsi_normalize_sense(sense_buffer, VSCSIIF_SENSE_BUFFERSIZE,
> @@ -358,7 +358,7 @@ static void scsiback_do_resp_with_sense(char 
> *sense_buffer, int32_t result,
>   if (notify)
>   notify_remote_via_irq(info->irq);
>  
> - if (pending_req->v2p)
> + if (pending_req && pending_req->v2p)
>   kref_put(_req->v2p->kref,
>scsiback_free_translation_entry);
>  }
> @@ -378,7 +378,8 @@ static void scsiback_cmd_done(struct vscsibk_pend 
> *pending_req)
>   scsiback_print_status(sense_buffer, errors, pending_req);
>  
>   scsiback_fast_flush_area(pending_req);
> - scsiback_do_resp_with_sense(sense_buffer, errors, resid, pending_req);
> + scsiback_do_resp_with_sense(sense_buffer, errors, resid, pending_req,
> + pending_req->rqid);
>   scsiback_put(info);
>  }
>  
> @@ -616,15 +617,15 @@ static void scsiback_device_action(struct vscsibk_pend 
> *pending_req,
>   err = (se_cmd->se_tmr_req->response == TMR_FUNCTION_COMPLETE) ?
>   SUCCESS : FAILED;
>  
> -out:
> - if (tmr) {
> - transport_generic_free_cmd(_req->se_cmd, 1);
> + scsiback_do_resp_with_sense(NULL, err, 0, pending_req,
> + pending_req->rqid);
> + transport_generic_free_cmd(_req->se_cmd, 1);
> + return;
> +err:
> + if (tmr)
>   kfree(tmr);
> - }
> -
> - scsiback_do_resp_with_sense(NULL, err, 0, pending_req);
> -
> - kmem_cache_free(scsiback_cachep, pending_req);
> + scsiback_do_resp_with_sense(NULL, err, 0, pending_req,
> + pending_req->rqid);
>  }
>  
>  /*
> @@ -653,15 +654,53 @@ out:
>   return entry;
>  }
>  
> -static int prepare_pending_reqs(struct vscsibk_info *info,
> - struct vscsiif_request *ring_req,
> - struct vscsibk_pend *pending_req)
> +static struct vscsibk_pend *scsiback_get_pend_req(struct vscsiif_back_ring 
> *ring,
> + struct v2p_entry *v2p)
> +{
> + struct scsiback_tpg *tpg = v2p->tpg;
> + struct scsiback_nexus *nexus = tpg->tpg_nexus;
> + struct se_session *se_sess = nexus->tvn_se_sess;
> + struct vscsibk_pend *req;
> + int tag, i;
> +
> + tag = percpu_ida_alloc(_sess->sess_tag_pool, TASK_RUNNING);
> + if (tag < 0) {
> + pr_err("Unable to obtain tag for vscsiif_request\n");
> + return ERR_PTR(-ENOMEM);
> + }
> +
> + req = &((struct vscsibk_pend *)se_sess->sess_cmd_map)[tag];
> + memset(req, 0, sizeof(*req));
> + req->se_cmd.map_tag = tag;
> +
> + for 

Re: [PATCH-v2 12/12] xen-scsiback: Convert to TARGET_SCF_ACK_KREF I/O krefs

[Juergen Gross]

On 25/01/16 09:11, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <n...@linux-iscsi.org>
> 
> Cc: Juergen Gross <jgr...@suse.com>
> Cc: Hannes Reinecke <h...@suse.de>
> Cc: David Vrabel <david.vra...@citrix.com>
> Signed-off-by: Nicholas Bellinger <n...@linux-iscsi.org>
> ---
>  drivers/xen/xen-scsiback.c | 53 
> +++---
>  1 file changed, 26 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/xen/xen-scsiback.c b/drivers/xen/xen-scsiback.c
> index 640fb22..a10e5f1 100644
> --- a/drivers/xen/xen-scsiback.c
> +++ b/drivers/xen/xen-scsiback.c
> @@ -381,6 +381,12 @@ static void scsiback_cmd_done(struct vscsibk_pend 
> *pending_req)
>   scsiback_do_resp_with_sense(sense_buffer, errors, resid, pending_req,
>   pending_req->rqid);
>   scsiback_put(info);
> + /*
> +  * Drop the extra KREF_ACK reference taken by 
> target_submit_cmd_map_sgls()
> +  * ahead of scsiback_check_stop_free() ->  transport_generic_free_cmd()
> +  * final se_cmd->cmd_kref put.
> +  */
> + target_put_sess_cmd(_req->se_cmd);
>  }
>  
>  static void scsiback_cmd_exec(struct vscsibk_pend *pending_req)
> @@ -398,7 +404,7 @@ static void scsiback_cmd_exec(struct vscsibk_pend 
> *pending_req)
>   rc = target_submit_cmd_map_sgls(se_cmd, sess, pending_req->cmnd,
>   pending_req->sense_buffer, pending_req->v2p->lun,
>   pending_req->data_len, 0,
> - pending_req->sc_data_direction, 0,
> + pending_req->sc_data_direction, TARGET_SCF_ACK_KREF,
>   pending_req->sgl, pending_req->n_sg,
>   NULL, 0, NULL, 0);
>   if (rc < 0) {
> @@ -587,31 +593,28 @@ static void scsiback_disconnect(struct vscsibk_info 
> *info)
>  static void scsiback_device_action(struct vscsibk_pend *pending_req,
>   enum tcm_tmreq_table act, int tag)
>  {
> - int rc, err = FAILED;
>   struct scsiback_tpg *tpg = pending_req->v2p->tpg;
> + struct scsiback_nexus *nexus = tpg->tpg_nexus;
>   struct se_cmd *se_cmd = _req->se_cmd;
>   struct scsiback_tmr *tmr;
> + u64 unpacked_lun = pending_req->v2p->lun;
> + int rc, err = FAILED;
>  
>   tmr = kzalloc(sizeof(struct scsiback_tmr), GFP_KERNEL);
> - if (!tmr)
> - goto out;
> + if (!tmr) {
> + target_put_sess_cmd(se_cmd);
> + goto err;

Sure? I think this should still be "goto out;"?

> + }
>  
>   init_waitqueue_head(>tmr_wait);
>  
> - transport_init_se_cmd(se_cmd, tpg->se_tpg.se_tpg_tfo,
> - tpg->tpg_nexus->tvn_se_sess, 0, DMA_NONE, TCM_SIMPLE_TAG,
> - _req->sense_buffer[0]);
> -
> - rc = core_tmr_alloc_req(se_cmd, tmr, act, GFP_KERNEL);
> - if (rc < 0)
> - goto out;
> -
> - se_cmd->se_tmr_req->ref_task_tag = tag;
> + rc = target_submit_tmr(_req->se_cmd, nexus->tvn_se_sess,
> +_req->sense_buffer[0],
> +unpacked_lun, tmr, act, GFP_KERNEL,
> +tag, TARGET_SCF_ACK_KREF);
> + if (rc)
> + goto err;

Again.


Juergen

>  
> - if (transport_lookup_tmr_lun(se_cmd, pending_req->v2p->lun) < 0)
> - goto out;
> -
> - transport_generic_handle_tmr(se_cmd);
>   wait_event(tmr->tmr_wait, atomic_read(>tmr_complete));
>  
>   err = (se_cmd->se_tmr_req->response == TMR_FUNCTION_COMPLETE) ?
> @@ -1368,16 +1371,7 @@ static u32 scsiback_tpg_get_inst_index(struct 
> se_portal_group *se_tpg)
>  
>  static int scsiback_check_stop_free(struct se_cmd *se_cmd)
>  {
> - /*
> -  * Do not release struct se_cmd's containing a valid TMR pointer.
> -  * These will be released directly in scsiback_device_action()
> -  * with transport_generic_free_cmd().
> -  */
> - if (se_cmd->se_cmd_flags & SCF_SCSI_TMR_CDB)
> - return 0;
> -
> - transport_generic_free_cmd(se_cmd, 0);
> - return 1;
> + return transport_generic_free_cmd(se_cmd, 0);
>  }
>  
>  static void scsiback_release_cmd(struct se_cmd *se_cmd)
> @@ -1385,6 +1379,11 @@ static void scsiback_release_cmd(struct se_cmd *se_cmd)
>   struct se_session *se_sess = se_cmd->se_sess;
>   struct se_tmr_req *se_tmr = se_cmd->se_tmr_req;
>  
> + if (se_tmr && se_cmd->se_cmd_flags & SCF_SCSI_TMR_CDB) {
> + struct scsiback_tmr *tmr = se_tmr->fabric_tmr_ptr;
> + kfree(tmr);
> + }
> +
>   percpu_ida_free(_sess->sess_tag_pool, se_cmd->map_tag);
>  }
>  
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH-v2 11/12] xen-scsiback: Convert to percpu_ida tag allocation

[Juergen Gross]

On 25/01/16 09:11, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <n...@linux-iscsi.org>
> 
> Cc: Juergen Gross <jgr...@suse.com>
> Cc: Hannes Reinecke <h...@suse.de>
> Cc: David Vrabel <david.vra...@citrix.com>
> Signed-off-by: Nicholas Bellinger <n...@linux-iscsi.org>

Could you please rebase to current version? The patch won't apply.


Juergen
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] xen-scsiback: fix license ident used in MODULE_LICENSE

[Juergen Gross]

On 22/01/16 13:34, Wei Liu wrote:
> The comment at the beginning of the file is the canonical source of
> licenses for this module. Currently it contains GPL and MIT license.
> Fix the code to reflect the reality.
> 
> Signed-off-by: Wei Liu <wei.l...@citrix.com>

Acked-by: Juergen Gross <jgr...@suse.com>

> ---
>  drivers/xen/xen-scsiback.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/xen/xen-scsiback.c b/drivers/xen/xen-scsiback.c
> index ad4eb10..0695cf8 100644
> --- a/drivers/xen/xen-scsiback.c
> +++ b/drivers/xen/xen-scsiback.c
> @@ -1915,6 +1915,6 @@ module_init(scsiback_init);
>  module_exit(scsiback_exit);
>  
>  MODULE_DESCRIPTION("Xen SCSI backend driver");
> -MODULE_LICENSE("Dual BSD/GPL");
> +MODULE_LICENSE("Dual MIT/GPL");
>  MODULE_ALIAS("xen-backend:vscsi");
>  MODULE_AUTHOR("Juergen Gross <jgr...@suse.com>");
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Xen-devel] [PATCH v4] xen-scsiback: define a pr_fmt macro with xen-pvscsi

[Juergen Gross]

On 03/10/2015 09:49 PM, Tao Chen wrote:

Add the {xen-pvscsi: } prefix in pr_fmt and remove DPRINTK, then
replace all DPRINTK with pr_debug.

Also fixed up some comments just as eliminate redundant whitespace
and format the code.

These will make the code easier to read.

Signed-off-by: Tao Chen boby.c...@huawei.com


Reviewed-by: Juergen Gross jgr...@suse.com


---
  drivers/xen/xen-scsiback.c | 75 ++
  1 file changed, 36 insertions(+), 39 deletions(-)

diff --git a/drivers/xen/xen-scsiback.c b/drivers/xen/xen-scsiback.c
index 9faca6a..d0b0bc5 100644
--- a/drivers/xen/xen-scsiback.c
+++ b/drivers/xen/xen-scsiback.c
@@ -31,6 +31,8 @@
   * IN THE SOFTWARE.
   */

+#define pr_fmt(fmt) xen-pvscsi:  fmt
+
  #include stdarg.h

  #include linux/module.h
@@ -69,9 +71,6 @@
  #include xen/interface/grant_table.h
  #include xen/interface/io/vscsiif.h

-#define DPRINTK(_f, _a...) \
-   pr_debug((file=%s, line=%d)  _f, __FILE__ , __LINE__ , ## _a)
-
  #define VSCSI_VERSION v0.1
  #define VSCSI_NAMELEN 32

@@ -271,7 +270,7 @@ static void scsiback_print_status(char *sense_buffer, int 
errors,
  {
struct scsiback_tpg *tpg = pending_req-v2p-tpg;

-   pr_err(xen-pvscsi[%s:%d] cmnd[0]=%02x - st=%02x msg=%02x host=%02x 
drv=%02x\n,
+   pr_err([%s:%d] cmnd[0]=%02x - st=%02x msg=%02x host=%02x drv=%02x\n,
   tpg-tport-tport_name, pending_req-v2p-lun,
   pending_req-cmnd[0], status_byte(errors), msg_byte(errors),
   host_byte(errors), driver_byte(errors));
@@ -427,7 +426,7 @@ static int scsiback_gnttab_data_map_batch(struct 
gnttab_map_grant_ref *map,
BUG_ON(err);
for (i = 0; i  cnt; i++) {
if (unlikely(map[i].status != GNTST_okay)) {
-   pr_err(xen-pvscsi: invalid buffer -- could not remap 
it\n);
+   pr_err(invalid buffer -- could not remap it\n);
map[i].handle = SCSIBACK_INVALID_HANDLE;
err = -ENOMEM;
} else {
@@ -449,7 +448,7 @@ static int scsiback_gnttab_data_map_list(struct 
vscsibk_pend *pending_req,
for (i = 0; i  cnt; i++) {
if (get_free_page(pg + mapcount)) {
put_free_pages(pg, mapcount);
-   pr_err(xen-pvscsi: no grant page\n);
+   pr_err(no grant page\n);
return -ENOMEM;
}
gnttab_set_map_op(map[mapcount], vaddr_page(pg[mapcount]),
@@ -492,7 +491,7 @@ static int scsiback_gnttab_data_map(struct vscsiif_request 
*ring_req,
return 0;

if (nr_segments  VSCSIIF_SG_TABLESIZE) {
-   DPRINTK(xen-pvscsi: invalid parameter nr_seg = %d\n,
+   pr_debug(invalid parameter nr_seg = %d\n,
ring_req-nr_segments);
return -EINVAL;
}
@@ -516,13 +515,12 @@ static int scsiback_gnttab_data_map(struct 
vscsiif_request *ring_req,
nr_segments += n_segs;
}
if (nr_segments  SG_ALL) {
-   DPRINTK(xen-pvscsi: invalid nr_seg = %d\n,
-   nr_segments);
+   pr_debug(invalid nr_seg = %d\n, nr_segments);
return -EINVAL;
}
}

-   /* free of (sgl) in fast_flush_area()*/
+   /* free of (sgl) in fast_flush_area() */
pending_req-sgl = kmalloc_array(nr_segments,
sizeof(struct scatterlist), GFP_KERNEL);
if (!pending_req-sgl)
@@ -679,7 +677,8 @@ static int prepare_pending_reqs(struct vscsibk_info *info,
v2p = scsiback_do_translation(info, vir);
if (!v2p) {
pending_req-v2p = NULL;
-   DPRINTK(xen-pvscsi: doesn't exist.\n);
+   pr_debug(the v2p of (chn:%d, tgt:%d, lun:%d) doesn't exist.\n,
+   vir.chn, vir.tgt, vir.lun);
return -ENODEV;
}
pending_req-v2p = v2p;
@@ -690,14 +689,14 @@ static int prepare_pending_reqs(struct vscsibk_info *info,
(pending_req-sc_data_direction != DMA_TO_DEVICE) 
(pending_req-sc_data_direction != DMA_FROM_DEVICE) 
(pending_req-sc_data_direction != DMA_NONE)) {
-   DPRINTK(xen-pvscsi: invalid parameter data_dir = %d\n,
+   pr_debug(invalid parameter data_dir = %d\n,
pending_req-sc_data_direction);
return -EINVAL;
}

pending_req-cmd_len = ring_req-cmd_len;
if (pending_req-cmd_len  VSCSIIF_MAX_COMMAND_SIZE) {
-   DPRINTK(xen-pvscsi: invalid parameter cmd_len = %d\n,
+   pr_debug(invalid parameter cmd_len = %d\n,
pending_req-cmd_len);
return -EINVAL;
}
@@ -721,7 +720,7 @@ static int scsiback_do_cmd_fn(struct

Re: [PATCH] xen-scsiback: use DRV_PFX in the pr macros

[Juergen Gross]

On 03/03/2015 09:37 AM, Tao Chen wrote:

Replace the string of {xen-pvscsi:} in the pr sentences with DRV_PFX,
it makes the code easier to read.


I'm not really convinced this is worth a patch. OTOH I'm not completely
against it. If nobody rejects this and all my further comments are
addressed you can have my:

Acked-by: Juergen Gross jgr...@suse.com



Signed-off-by: Tao Chen boby.c...@huawei.com
---
  drivers/xen/xen-scsiback.c | 67 +++---
  1 file changed, 34 insertions(+), 33 deletions(-)

diff --git a/drivers/xen/xen-scsiback.c b/drivers/xen/xen-scsiback.c
index 9faca6a..307100d 100644
--- a/drivers/xen/xen-scsiback.c
+++ b/drivers/xen/xen-scsiback.c
@@ -69,6 +69,7 @@
  #include xen/interface/grant_table.h
  #include xen/interface/io/vscsiif.h

+#define DRV_PFX xen-pvscsi:


Using xen-pvscsi:  wouldn't delete the white space after the colon in
most messages.



  #define DPRINTK(_f, _a...)\
pr_debug((file=%s, line=%d)  _f, __FILE__ , __LINE__ , ## _a)

@@ -84,7 +85,7 @@ struct ids_tuple {

  struct v2p_entry {
struct ids_tuple v; /* translate from */
-   struct scsiback_tpg *tpg;   /* translate to   */
+   struct scsiback_tpg *tpg;   /* translate to */


Unrelated white space change (others as well). Either omit them or
mention them in the commit message.


unsigned int lun;
struct kref kref;
struct list_head l;
@@ -271,7 +272,7 @@ static void scsiback_print_status(char *sense_buffer, int 
errors,
  {
struct scsiback_tpg *tpg = pending_req-v2p-tpg;

-   pr_err(xen-pvscsi[%s:%d] cmnd[0]=%02x - st=%02x msg=%02x host=%02x 
drv=%02x\n,
+   pr_err(DRV_PFX [%s:%d] cmnd[0]=%02x - st=%02x msg=%02x host=%02x 
drv=%02x\n,
   tpg-tport-tport_name, pending_req-v2p-lun,
   pending_req-cmnd[0], status_byte(errors), msg_byte(errors),
   host_byte(errors), driver_byte(errors));
@@ -427,7 +428,7 @@ static int scsiback_gnttab_data_map_batch(struct 
gnttab_map_grant_ref *map,
BUG_ON(err);
for (i = 0; i  cnt; i++) {
if (unlikely(map[i].status != GNTST_okay)) {
-   pr_err(xen-pvscsi: invalid buffer -- could not remap 
it\n);
+   pr_err(DRV_PFX invalid buffer -- could not remap 
it\n);
map[i].handle = SCSIBACK_INVALID_HANDLE;
err = -ENOMEM;
} else {
@@ -449,7 +450,7 @@ static int scsiback_gnttab_data_map_list(struct 
vscsibk_pend *pending_req,
for (i = 0; i  cnt; i++) {
if (get_free_page(pg + mapcount)) {
put_free_pages(pg, mapcount);
-   pr_err(xen-pvscsi: no grant page\n);
+   pr_err(DRV_PFX no grant page\n);
return -ENOMEM;
}
gnttab_set_map_op(map[mapcount], vaddr_page(pg[mapcount]),
@@ -492,7 +493,7 @@ static int scsiback_gnttab_data_map(struct vscsiif_request 
*ring_req,
return 0;

if (nr_segments  VSCSIIF_SG_TABLESIZE) {
-   DPRINTK(xen-pvscsi: invalid parameter nr_seg = %d\n,
+   DPRINTK(DRV_PFX invalid parameter nr_seg = %d\n,


As DPRINTK already contains the file name, you can omit the prefix.
Alternatively include DRV_PFX in the DPRINTK macro.


ring_req-nr_segments);
return -EINVAL;
}
@@ -516,13 +517,13 @@ static int scsiback_gnttab_data_map(struct 
vscsiif_request *ring_req,
nr_segments += n_segs;
}
if (nr_segments  SG_ALL) {
-   DPRINTK(xen-pvscsi: invalid nr_seg = %d\n,
+   DPRINTK(DRV_PFX invalid nr_seg = %d\n,
nr_segments);
return -EINVAL;
}
}

-   /* free of (sgl) in fast_flush_area()*/
+   /* free of (sgl) in fast_flush_area() */
pending_req-sgl = kmalloc_array(nr_segments,
sizeof(struct scatterlist), GFP_KERNEL);
if (!pending_req-sgl)
@@ -679,7 +680,7 @@ static int prepare_pending_reqs(struct vscsibk_info *info,
v2p = scsiback_do_translation(info, vir);
if (!v2p) {
pending_req-v2p = NULL;
-   DPRINTK(xen-pvscsi: doesn't exist.\n);
+   DPRINTK(DRV_PFX doesn't exist.\n);
return -ENODEV;
}
pending_req-v2p = v2p;
@@ -690,14 +691,14 @@ static int prepare_pending_reqs(struct vscsibk_info *info,
(pending_req-sc_data_direction != DMA_TO_DEVICE) 
(pending_req-sc_data_direction != DMA_FROM_DEVICE) 
(pending_req-sc_data_direction != DMA_NONE)) {
-   DPRINTK(xen-pvscsi: invalid parameter data_dir = %d\n,
+   DPRINTK(DRV_PFX invalid parameter data_dir = %d\n

Re: [PATCH] xen-scsiback: some modifications about code comment

[Juergen Gross]

On 02/07/2015 04:31 AM, Rudy Zhang wrote:

From: Tao Chen boby.c...@huawei.com

Signed-off-by: Tao Chen boby.c...@huawei.com


Are some white space fixes in comments really worth a patch?

Juergen


---
  drivers/xen/xen-scsiback.c | 16 
  1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/xen/xen-scsiback.c b/drivers/xen/xen-scsiback.c
index 3e32146..59f09fd 100644
--- a/drivers/xen/xen-scsiback.c
+++ b/drivers/xen/xen-scsiback.c
@@ -83,7 +83,7 @@ struct ids_tuple {

  struct v2p_entry {
struct ids_tuple v; /* translate from */
-   struct scsiback_tpg *tpg;   /* translate to   */
+   struct scsiback_tpg *tpg;   /* translate to */
unsigned int lun;
struct kref kref;
struct list_head l;
@@ -525,7 +525,7 @@ static int scsiback_gnttab_data_map(struct vscsiif_request 
*ring_req,
}
}

-   /* free of (sgl) in fast_flush_area()*/
+   /* free of (sgl) in fast_flush_area() */
pending_req-sgl = kmalloc_array(nr_segments,
sizeof(struct scatterlist), GFP_KERNEL);
if (!pending_req-sgl)
@@ -1084,7 +1084,7 @@ static void scsiback_do_1lun_hotplug(struct vscsibk_info 
*info, int op,
}
}
break;
-   /*When it is necessary, processing is added here.*/
+   /* When it is necessary, processing is added here. */
default:
break;
}
@@ -1475,8 +1475,8 @@ static u32 scsiback_tpg_get_inst_index(struct 
se_portal_group *se_tpg)
  static int scsiback_check_stop_free(struct se_cmd *se_cmd)
  {
/*
-* Do not release struct se_cmd's containing a valid TMR
-* pointer.  These will be released directly in scsiback_device_action()
+* Do not release struct se_cmd's containing a valid TMR pointer.
+* These will be released directly in scsiback_device_action()
 * with transport_generic_free_cmd().
 */
if (se_cmd-se_cmd_flags  SCF_SCSI_TMR_CDB)
@@ -1642,7 +1642,7 @@ static int scsiback_make_nexus(struct scsiback_tpg *tpg,
return -ENOMEM;
}
/*
-*  Initialize the struct se_session pointer
+* Initialize the struct se_session pointer
 */
tv_nexus-tvn_se_sess = transport_init_session(TARGET_PROT_NORMAL);
if (IS_ERR(tv_nexus-tvn_se_sess)) {
@@ -1759,7 +1759,7 @@ static ssize_t scsiback_tpg_store_nexus(struct 
se_portal_group *se_tpg,
unsigned char i_port[VSCSI_NAMELEN], *ptr, *port_ptr;
int ret;
/*
-* Shutdown the active I_T nexus if 'NULL' is passed..
+* Shutdown the active I_T nexus if 'NULL' is passed.
 */
if (!strncmp(page, NULL, 4)) {
ret = scsiback_drop_nexus(tpg);
@@ -1930,7 +1930,7 @@ static void scsiback_drop_tpg(struct se_portal_group 
*se_tpg)
 */
scsiback_drop_nexus(tpg);
/*
-* Deregister the se_tpg from TCM..
+* Deregister the se_tpg from TCM.
 */
core_tpg_deregister(se_tpg);
kfree(tpg);



--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Xen-devel] [PATCH] xen/xenbus: Use 'void' instead of 'int' for the return of xenbus_switch_state()

[Juergen Gross]

On 09/26/2014 06:36 PM, Chen Gang wrote:

When xenbus_switch_state() fails, it will call xenbus_switch_fatal()
internally, so need not return any status value, then use 'void' instead
of 'int' for xenbus_switch_state() and __xenbus_switch_state().

Also need be sure that all callers which check the return value must let
'err' be 0.

And also need change the related comments for xenbus_switch_state().

Signed-off-by: Chen Gang gang.chen.5...@gmail.com


Acked-by: Juergen Gross jgr...@suse.com


---
  drivers/block/xen-blkback/xenbus.c |  9 ++---
  drivers/net/xen-netback/xenbus.c   |  5 +
  drivers/pci/xen-pcifront.c | 15 ++-
  drivers/xen/xen-pciback/xenbus.c   | 21 -
  drivers/xen/xen-scsiback.c |  5 +
  drivers/xen/xenbus/xenbus_client.c | 16 
  include/xen/xenbus.h   |  3 ++-
  7 files changed, 24 insertions(+), 50 deletions(-)

diff --git a/drivers/block/xen-blkback/xenbus.c 
b/drivers/block/xen-blkback/xenbus.c
index 3a8b810..fdcc584 100644
--- a/drivers/block/xen-blkback/xenbus.c
+++ b/drivers/block/xen-blkback/xenbus.c
@@ -587,9 +587,7 @@ static int xen_blkbk_probe(struct xenbus_device *dev,
if (err)
goto fail;

-   err = xenbus_switch_state(dev, XenbusStateInitWait);
-   if (err)
-   goto fail;
+   xenbus_switch_state(dev, XenbusStateInitWait);

return 0;

@@ -837,10 +835,7 @@ again:
if (err)
xenbus_dev_fatal(dev, err, ending transaction);

-   err = xenbus_switch_state(dev, XenbusStateConnected);
-   if (err)
-   xenbus_dev_fatal(dev, err, %s: switching to Connected state,
-dev-nodename);
+   xenbus_switch_state(dev, XenbusStateConnected);

return;
   abort:
diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c
index 9c47b89..b5c3d47 100644
--- a/drivers/net/xen-netback/xenbus.c
+++ b/drivers/net/xen-netback/xenbus.c
@@ -337,10 +337,7 @@ static int netback_probe(struct xenbus_device *dev,
if (err)
pr_debug(Error writing multi-queue-max-queues\n);

-   err = xenbus_switch_state(dev, XenbusStateInitWait);
-   if (err)
-   goto fail;
-
+   xenbus_switch_state(dev, XenbusStateInitWait);
be-state = XenbusStateInitWait;

/* This kicks hotplug scripts, so do it immediately. */
diff --git a/drivers/pci/xen-pcifront.c b/drivers/pci/xen-pcifront.c
index 53df39a..c1d73b7 100644
--- a/drivers/pci/xen-pcifront.c
+++ b/drivers/pci/xen-pcifront.c
@@ -901,18 +901,16 @@ static int pcifront_try_connect(struct pcifront_device 
*pdev)
}
}

-   err = xenbus_switch_state(pdev-xdev, XenbusStateConnected);
-
+   xenbus_switch_state(pdev-xdev, XenbusStateConnected);
+   return 0;
  out:
return err;
  }

  static int pcifront_try_disconnect(struct pcifront_device *pdev)
  {
-   int err = 0;
enum xenbus_state prev_state;

-
prev_state = xenbus_read_driver_state(pdev-xdev-nodename);

if (prev_state = XenbusStateClosing)
@@ -923,11 +921,10 @@ static int pcifront_try_disconnect(struct pcifront_device 
*pdev)
pcifront_disconnect(pdev);
}

-   err = xenbus_switch_state(pdev-xdev, XenbusStateClosed);
+   xenbus_switch_state(pdev-xdev, XenbusStateClosed);

  out:
-
-   return err;
+   return 0;
  }

  static int pcifront_attach_devices(struct pcifront_device *pdev)
@@ -1060,8 +1057,8 @@ static int pcifront_detach_devices(struct pcifront_device 
*pdev)
domain, bus, slot, func);
}

-   err = xenbus_switch_state(pdev-xdev, XenbusStateReconfiguring);
-
+   xenbus_switch_state(pdev-xdev, XenbusStateReconfiguring);
+   return 0;
  out:
return err;
  }
diff --git a/drivers/xen/xen-pciback/xenbus.c b/drivers/xen/xen-pciback/xenbus.c
index c214daa..630a215 100644
--- a/drivers/xen/xen-pciback/xenbus.c
+++ b/drivers/xen/xen-pciback/xenbus.c
@@ -184,10 +184,7 @@ static int xen_pcibk_attach(struct xen_pcibk_device *pdev)

dev_dbg(pdev-xdev-dev, Connecting...\n);

-   err = xenbus_switch_state(pdev-xdev, XenbusStateConnected);
-   if (err)
-   xenbus_dev_fatal(pdev-xdev, err,
-Error switching to connected state!);
+   xenbus_switch_state(pdev-xdev, XenbusStateConnected);

dev_dbg(pdev-xdev-dev, Connected? %d\n, err);
  out:
@@ -500,12 +497,7 @@ static int xen_pcibk_reconfigure(struct xen_pcibk_device 
*pdev)
}
}

-   err = xenbus_switch_state(pdev-xdev, XenbusStateReconfigured);
-   if (err) {
-   xenbus_dev_fatal(pdev-xdev, err,
-Error switching to reconfigured state!);
-   goto out;
-   }
+   xenbus_switch_state(pdev-xdev, XenbusStateReconfigured);

  out:
mutex_unlock(pdev-dev_lock

[PATCH] xen: make pvscsi frontend dependant on xenbus frontend

[Juergen Gross]

The pvscsi frontend driver requires the xenbus frontend driver. Reflect
this in Kconfig.

Signed-off-by: Juergen Gross jgr...@suse.com
---
 drivers/scsi/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig
index 9130df1..ff62dc1 100644
--- a/drivers/scsi/Kconfig
+++ b/drivers/scsi/Kconfig
@@ -579,6 +579,7 @@ config VMWARE_PVSCSI
 config XEN_SCSI_FRONTEND
tristate XEN SCSI frontend driver
depends on SCSI  XEN
+   select XEN_XENBUS_FRONTEND
help
  The XEN SCSI frontend driver allows the kernel to access SCSI Devices
  within another guest OS (usually Dom0).
-- 
1.8.4.5

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] xen-scsifront: don't deadlock if the ring becomes full

[Juergen Gross]

On 09/11/2014 03:31 PM, David Vrabel wrote:

scsifront_action_handler() will deadlock on host-host_lock, if the
ring is full and it has to wait for entries to become available.

Signed-off-by: David Vrabel david.vra...@citrix.com
---
This was found with sparse. I've not tested it.


Test might be difficult. :-)

Reviewed-by: Juergen Gross jgr...@suse.com

Thanks for spotting this.

Juergen


---
  drivers/scsi/xen-scsifront.c |3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/xen-scsifront.c b/drivers/scsi/xen-scsifront.c
index 7e88659..cc14c8d 100644
--- a/drivers/scsi/xen-scsifront.c
+++ b/drivers/scsi/xen-scsifront.c
@@ -541,8 +541,9 @@ static int scsifront_action_handler(struct scsi_cmnd *sc, 
uint8_t act)
if (!shadow)
return FAILED;

+   spin_lock_irq(host-host_lock);
+
for (;;) {
-   spin_lock_irq(host-host_lock);
if (!RING_FULL(info-ring)) {
ring_req = scsifront_command2ring(info, sc, shadow);
if (ring_req)



--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Xen-devel] [patch] xen-scsifront: use GFP_ATOMIC under spin_lock

[Juergen Gross]

On 09/08/2014 01:15 PM, Dan Carpenter wrote:

This function is only called with a spin_lock held and IRQs disabled.
The allocation is not allowed to sleep and NOIO is not sufficient, it
has to be ATOMIC.

Signed-off-by: Dan Carpenter dan.carpen...@oracle.com


Reviewed-by: Juergen Gross jgr...@suse.com

Thanks,

Juergen



diff --git a/drivers/scsi/xen-scsifront.c b/drivers/scsi/xen-scsifront.c
index 0aceb70..7e88659 100644
--- a/drivers/scsi/xen-scsifront.c
+++ b/drivers/scsi/xen-scsifront.c
@@ -359,7 +359,7 @@ static int map_data_for_request(struct vscsifrnt_info *info,
}
seg_grants = vscsiif_grants_sg(data_grants);
shadow-sg = kcalloc(data_grants,
-   sizeof(struct scsiif_request_segment), GFP_NOIO);
+   sizeof(struct scsiif_request_segment), GFP_ATOMIC);
if (!shadow-sg)
return -ENOMEM;
}

___
Xen-devel mailing list
xen-de...@lists.xen.org
http://lists.xen.org/xen-devel



--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Xen-devel] [patch] xen-scsiback: clean up a type issue in scsiback_make_tpg()

[Juergen Gross]

On 09/08/2014 01:17 PM, Dan Carpenter wrote:

This code was confusing because we had an unsigned long and then we
compared it to UINT_MAX and then we stored it in a u16.  How many bytes
is this supposed to have: 2, 4 or 16???

I've made it a u16 throughout.

Signed-off-by: Dan Carpenter dan.carpen...@oracle.com


Reviewed-by: Juergen Gross jgr...@suse.com

Thanks,

Juergen



diff --git a/drivers/xen/xen-scsiback.c b/drivers/xen/xen-scsiback.c
index 7b565632..ad11258 100644
--- a/drivers/xen/xen-scsiback.c
+++ b/drivers/xen/xen-scsiback.c
@@ -1885,13 +1885,14 @@ scsiback_make_tpg(struct se_wwn *wwn,
struct scsiback_tport, tport_wwn);

struct scsiback_tpg *tpg;
-   unsigned long tpgt;
+   u16 tpgt;
int ret;

if (strstr(name, tpgt_) != name)
return ERR_PTR(-EINVAL);
-   if (kstrtoul(name + 5, 10, tpgt) || tpgt  UINT_MAX)
-   return ERR_PTR(-EINVAL);
+   ret = kstrtou16(name + 5, 10, tpgt);
+   if (ret)
+   return ERR_PTR(ret);

tpg = kzalloc(sizeof(struct scsiback_tpg), GFP_KERNEL);
if (!tpg)

___
Xen-devel mailing list
xen-de...@lists.xen.org
http://lists.xen.org/xen-devel



--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH V6 5/5] add xen pvscsi maintainer

[Juergen Gross]

Add myself as maintainer for the Xen pvSCSI stuff.

Signed-off-by: Juergen Gross jgr...@suse.com
Acked-by: Konrad Rzeszutek Wilk konrad.w...@oracle.com
---
 MAINTAINERS | 9 +
 1 file changed, 9 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 1ff06de..0ae6238 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10128,6 +10128,15 @@ S: Supported
 F: drivers/block/xen-blkback/*
 F: drivers/block/xen*
 
+XEN PVSCSI DRIVERS
+M: Juergen Gross jgr...@suse.com
+L: xen-de...@lists.xenproject.org (moderated for non-subscribers)
+L: linux-scsi@vger.kernel.org
+S: Supported
+F: drivers/scsi/xen-scsifront.c
+F: drivers/xen/xen-scsiback.c
+F: include/xen/interface/io/vscsiif.h
+
 XEN SWIOTLB SUBSYSTEM
 M: Konrad Rzeszutek Wilk konrad.w...@oracle.com
 L: xen-de...@lists.xenproject.org (moderated for non-subscribers)
-- 
1.8.4.5

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH V6 0/5] Add XEN pvSCSI support

[Juergen Gross]

This series adds XEN pvSCSI support. With pvSCSI it is possible to use physical
SCSI devices from a XEN domain.

The support consists of a backend in the privileged Domain-0 doing the real
I/O and a frontend in the unprivileged domU passing I/O-requests to the backend.

The code is taken (and adapted) from the original pvSCSI implementation done
for Linux 2.6 in 2008 by Fujitsu.

Changes in V6:
- changes in scsifront module after comments from Konrad Wilk
- remove double kfree in scsiback
- comments in vscsiif.h adapted after comments from Jan Beulich and
  David Vrabel in xen-devel
- added linux-scsi@vger.kernel.org to MAINTANERS entry

Changes in V5:
- Added patch to support threaded irqs for interdomain event channels
- several changes in xen-scsifront after comments from Christoph Hellwig
- several changes in xen-scsiback after comments from Christoph Hellwig
- several changes in xen-scsiback after comments from James Bottomley
- several changes in xen-scsiback after comments from Nicholas Bellinger

Changes in V4:
- Re-add define for VSCSIIF_ACT_SCSI_SG_PRESET to vscsiif.h to indicate this
  action value should not be used in future enhancements

Changes in V3:
- added some comments to the protocol header file
- removed the CDB emulation from xen-scsiback, handled by core target
  infrastructure
- several changes in xen-scsifront after comments from Christoph Hellwig

Changes in V2:
- use core target infrastructure by backend instead of pure SCSI passthrough
- add support for larger SG lists by putting them in grant page(s)
- add command abort capability


Juergen Gross (5):
  xen/events: support threaded irqs for interdomain event channels
  Add XEN pvSCSI protocol description
  Introduce xen-scsifront module
  Introduce XEN scsiback module
  add xen pvscsi maintainer

 MAINTAINERS|9 +
 drivers/scsi/Kconfig   |9 +
 drivers/scsi/Makefile  |1 +
 drivers/scsi/xen-scsifront.c   | 1024 +
 drivers/xen/Kconfig|9 +
 drivers/xen/Makefile   |1 +
 drivers/xen/events/events_base.c   |5 +-
 drivers/xen/xen-scsiback.c | 2124 
 include/xen/events.h   |2 +
 include/xen/interface/io/vscsiif.h |  229 
 10 files changed, 3411 insertions(+), 2 deletions(-)
 create mode 100644 drivers/scsi/xen-scsifront.c
 create mode 100644 drivers/xen/xen-scsiback.c
 create mode 100644 include/xen/interface/io/vscsiif.h

-- 
1.8.4.5

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH V6 2/5] Add XEN pvSCSI protocol description

[Juergen Gross]

Add the definition of pvSCSI protocol used between the pvSCSI frontend in a
XEN domU and the pvSCSI backend in a XEN driver domain (usually Dom0).

This header was originally provided by Fujitsu for XEN based on Linux 2.6.18.
Changes are:
- added comment
- adapt to Linux style guide
- add support for larger SG-lists by putting them in an own granted page
- remove stale definitions

Signed-off-by: Juergen Gross jgr...@suse.com
---
 include/xen/interface/io/vscsiif.h | 229 +
 1 file changed, 229 insertions(+)
 create mode 100644 include/xen/interface/io/vscsiif.h

diff --git a/include/xen/interface/io/vscsiif.h 
b/include/xen/interface/io/vscsiif.h
new file mode 100644
index 000..d07d7ac
--- /dev/null
+++ b/include/xen/interface/io/vscsiif.h
@@ -0,0 +1,229 @@
+/**
+ * vscsiif.h
+ *
+ * Based on the blkif.h code.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the Software), to
+ * deal in the Software without restriction, including without limitation the
+ * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
+ * sell copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED AS IS, WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+ * DEALINGS IN THE SOFTWARE.
+ *
+ * Copyright(c) FUJITSU Limited 2008.
+ */
+
+#ifndef __XEN__PUBLIC_IO_SCSI_H__
+#define __XEN__PUBLIC_IO_SCSI_H__
+
+#include ring.h
+#include ../grant_table.h
+
+/*
+ * Feature and Parameter Negotiation
+ * =
+ * The two halves of a Xen pvSCSI driver utilize nodes within the XenStore to
+ * communicate capabilities and to negotiate operating parameters.  This
+ * section enumerates these nodes which reside in the respective front and
+ * backend portions of the XenStore, following the XenBus convention.
+ *
+ * Any specified default value is in effect if the corresponding XenBus node
+ * is not present in the XenStore.
+ *
+ * XenStore nodes in sections marked PRIVATE are solely for use by the
+ * driver side whose XenBus tree contains them.
+ *
+ *
+ *Backend XenBus Nodes
+ *
+ *
+ *-- Backend Device Identification (PRIVATE) --
+ *
+ * p-devname
+ *  Values: string
+ *
+ *  A free string used to identify the physical device (e.g. a disk name).
+ *
+ * p-dev
+ *  Values: string
+ *
+ *  A string specifying the backend device: either a 4-tuple h:c:t:l
+ *  (host, controller, target, lun, all integers), or a WWN (e.g.
+ *  naa.60014054ac780582).
+ *
+ * v-dev
+ *  Values: string
+ *
+ *  A string specifying the frontend device in form of a 4-tuple h:c:t:l
+ *  (host, controller, target, lun, all integers).
+ *
+ *- Features -
+ *
+ * feature-sg-grant
+ *  Values: unsigned [VSCSIIF_SG_TABLESIZE...65535]
+ *  Default Value:  0
+ *
+ *  Specifies the maximum number of scatter/gather elements in grant pages
+ *  supported. If not set, the backend supports up to VSCSIIF_SG_TABLESIZE
+ *  SG elements specified directly in the request.
+ *
+ *
+ *Frontend XenBus Nodes
+ *
+ *
+ *--- Request Transport Parameters ---
+ *
+ * event-channel
+ *  Values: unsigned
+ *
+ *  The identifier of the Xen event channel used to signal activity
+ *  in the ring buffer.
+ *
+ * ring-ref
+ *  Values: unsigned
+ *
+ *  The Xen grant reference granting permission for the backend to map
+ *  the sole page in a single page sized ring buffer.
+ *
+ * protocol
+ *  Values: string (XEN_IO_PROTO_ABI_*)
+ *  Default Value:  XEN_IO_PROTO_ABI_NATIVE
+ *
+ *  The machine ABI rules governing the format of all ring request and
+ *  response structures.
+ */
+
+/* Requests from

[PATCH V6 1/5] xen/events: support threaded irqs for interdomain event channels

[Juergen Gross]

Export bind_interdomain_evtchn_to_irq() so drivers can use threaded
interrupt handlers with:

 irq = bind_interdomain_evtchn_to_irq(remote_dom, remote_port);
 if (irq  0)
 /* error */
 ret = request_threaded_irq(...);

Signed-off-by: Juergen Gross jgr...@suse.com
Acked-by: David Vrabel david.vra...@citrix.com
Acked-by: Konrad Rzeszutek Wilk konrad.w...@oracle.com
---
 drivers/xen/events/events_base.c | 5 +++--
 include/xen/events.h | 2 ++
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c
index 5b5c5ff..b4bca2d 100644
--- a/drivers/xen/events/events_base.c
+++ b/drivers/xen/events/events_base.c
@@ -900,8 +900,8 @@ static int bind_ipi_to_irq(unsigned int ipi, unsigned int 
cpu)
return irq;
 }
 
-static int bind_interdomain_evtchn_to_irq(unsigned int remote_domain,
- unsigned int remote_port)
+int bind_interdomain_evtchn_to_irq(unsigned int remote_domain,
+  unsigned int remote_port)
 {
struct evtchn_bind_interdomain bind_interdomain;
int err;
@@ -914,6 +914,7 @@ static int bind_interdomain_evtchn_to_irq(unsigned int 
remote_domain,
 
return err ? : bind_evtchn_to_irq(bind_interdomain.local_port);
 }
+EXPORT_SYMBOL_GPL(bind_interdomain_evtchn_to_irq);
 
 static int find_virq(unsigned int virq, unsigned int cpu)
 {
diff --git a/include/xen/events.h b/include/xen/events.h
index 8bee7a7..5321cd9 100644
--- a/include/xen/events.h
+++ b/include/xen/events.h
@@ -28,6 +28,8 @@ int bind_ipi_to_irqhandler(enum ipi_vector ipi,
   unsigned long irqflags,
   const char *devname,
   void *dev_id);
+int bind_interdomain_evtchn_to_irq(unsigned int remote_domain,
+  unsigned int remote_port);
 int bind_interdomain_evtchn_to_irqhandler(unsigned int remote_domain,
  unsigned int remote_port,
  irq_handler_t handler,
-- 
1.8.4.5

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH V6 3/5] Introduce xen-scsifront module

[Juergen Gross]

Introduces the XEN pvSCSI frontend. With pvSCSI it is possible for a XEN domU
to issue SCSI commands to a SCSI LUN assigned to that domU. The SCSI commands
are passed to the pvSCSI backend in a driver domain (usually Dom0) which is
owner of the physical device. This allows e.g. to use SCSI tape drives in a
XEN domU.

The code is taken from the pvSCSI implementation in XEN done by Fujitsu based
on Linux kernel 2.6.18.

Changes from the original version are:
- port to upstream kernel
- put all code in just one source file
- move module to appropriate location in kernel tree
- adapt to Linux style guide
- some minor code simplifications
- replace constants with defines
- remove not used defines
- add support for larger SG lists by putting them in a granted page

Signed-off-by: Juergen Gross jgr...@suse.com

Xen related parts
Acked-by: David Vrabel david.vra...@citrix.com
---
 drivers/scsi/Kconfig |9 +
 drivers/scsi/Makefile|1 +
 drivers/scsi/xen-scsifront.c | 1024 ++
 3 files changed, 1034 insertions(+)
 create mode 100644 drivers/scsi/xen-scsifront.c

diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig
index 18a3358..9130df1 100644
--- a/drivers/scsi/Kconfig
+++ b/drivers/scsi/Kconfig
@@ -576,6 +576,15 @@ config VMWARE_PVSCSI
  To compile this driver as a module, choose M here: the
  module will be called vmw_pvscsi.
 
+config XEN_SCSI_FRONTEND
+   tristate XEN SCSI frontend driver
+   depends on SCSI  XEN
+   help
+ The XEN SCSI frontend driver allows the kernel to access SCSI Devices
+ within another guest OS (usually Dom0).
+ Only needed if the kernel is running in a XEN guest and generic
+ SCSI access to a device is needed.
+
 config HYPERV_STORAGE
tristate Microsoft Hyper-V virtual storage driver
depends on SCSI  HYPERV
diff --git a/drivers/scsi/Makefile b/drivers/scsi/Makefile
index 5f0d299..59f1ce6 100644
--- a/drivers/scsi/Makefile
+++ b/drivers/scsi/Makefile
@@ -141,6 +141,7 @@ obj-$(CONFIG_SCSI_ESAS2R)   += esas2r/
 obj-$(CONFIG_SCSI_PMCRAID) += pmcraid.o
 obj-$(CONFIG_SCSI_VIRTIO)  += virtio_scsi.o
 obj-$(CONFIG_VMWARE_PVSCSI)+= vmw_pvscsi.o
+obj-$(CONFIG_XEN_SCSI_FRONTEND)+= xen-scsifront.o
 obj-$(CONFIG_HYPERV_STORAGE)   += hv_storvsc.o
 
 obj-$(CONFIG_ARM)  += arm/
diff --git a/drivers/scsi/xen-scsifront.c b/drivers/scsi/xen-scsifront.c
new file mode 100644
index 000..0aceb70
--- /dev/null
+++ b/drivers/scsi/xen-scsifront.c
@@ -0,0 +1,1024 @@
+/*
+ * Xen SCSI frontend driver
+ *
+ * Copyright (c) 2008, FUJITSU Limited
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License version 2
+ * as published by the Free Software Foundation; or, when distributed
+ * separately from the Linux kernel or incorporated into other
+ * software packages, subject to the following license:
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this source file (the Software), to deal in the Software without
+ * restriction, including without limitation the rights to use, copy, modify,
+ * merge, publish, distribute, sublicense, and/or sell copies of the Software,
+ * and to permit persons to whom the Software is furnished to do so, subject to
+ * the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED AS IS, WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ */
+
+#include linux/module.h
+#include linux/kernel.h
+#include linux/device.h
+#include linux/wait.h
+#include linux/interrupt.h
+#include linux/mutex.h
+#include linux/spinlock.h
+#include linux/sched.h
+#include linux/blkdev.h
+#include linux/pfn.h
+#include linux/slab.h
+#include linux/bitops.h
+
+#include scsi/scsi_cmnd.h
+#include scsi/scsi_device.h
+#include scsi/scsi.h
+#include scsi/scsi_host.h
+
+#include xen/xen.h
+#include xen/xenbus.h
+#include xen/grant_table.h
+#include xen/events.h
+#include xen/page.h
+
+#include xen/interface/grant_table.h
+#include xen/interface/io/vscsiif.h
+#include xen/interface/io/protocols.h
+
+#include asm/xen/hypervisor.h
+
+
+#define GRANT_INVALID_REF  0
+
+#define VSCSIFRONT_OP_ADD_LUN  1
+#define VSCSIFRONT_OP_DEL_LUN  2
+
+/* Tuning point. */
+#define VSCSIIF_DEFAULT_CMD_PER_LUN 10
+#define VSCSIIF_MAX_TARGET  64
+#define VSCSIIF_MAX_LUN 255
+
+#define

[PATCH V6 4/5] Introduce XEN scsiback module

[Juergen Gross]

Introduces the XEN pvSCSI backend. With pvSCSI it is possible for a XEN domU
to issue SCSI commands to a SCSI LUN assigned to that domU. The SCSI commands
are passed to the pvSCSI backend in a driver domain (usually Dom0) which is
owner of the physical device. This allows e.g. to use SCSI tape drives in a
XEN domU.

The code is taken from the pvSCSI implementation in XEN done by Fujitsu based
on Linux kernel 2.6.18.

Changes from the original version are:
- port to upstream kernel
- put all code in just one source file
- adapt to Linux style guide
- use target core infrastructure instead doing pure pass-through
- enable module unloading
- support SG-list in grant page(s)
- support task abort
- remove redundant struct backend
- allocate resources dynamically
- correct minor error in scsiback_fast_flush_area
- free allocated resources in case of error during I/O preparation
- remove CDB emulation, now handled by target core infrastructure

Signed-off-by: Juergen Gross jgr...@suse.com

Xen related parts
Acked-by: David Vrabel david.vra...@citrix.com
Reviewed-by: Nicholas Bellinger n...@linux-iscsi.org
---
 drivers/xen/Kconfig|9 +
 drivers/xen/Makefile   |1 +
 drivers/xen/xen-scsiback.c | 2124 
 3 files changed, 2134 insertions(+)
 create mode 100644 drivers/xen/xen-scsiback.c

diff --git a/drivers/xen/Kconfig b/drivers/xen/Kconfig
index 8bc0183..b812462 100644
--- a/drivers/xen/Kconfig
+++ b/drivers/xen/Kconfig
@@ -172,6 +172,15 @@ config XEN_PCIDEV_BACKEND
 
  If in doubt, say m.
 
+config XEN_SCSI_BACKEND
+   tristate XEN SCSI backend driver
+   depends on XEN  XEN_BACKEND  TARGET_CORE
+   help
+ The SCSI backend driver allows the kernel to export its SCSI Devices
+ to other guests via a high-performance shared-memory interface.
+ Only needed for systems running as XEN driver domains (e.g. Dom0) and
+ if guests need generic access to SCSI devices.
+
 config XEN_PRIVCMD
tristate
depends on XEN
diff --git a/drivers/xen/Makefile b/drivers/xen/Makefile
index 84044b5..2140398 100644
--- a/drivers/xen/Makefile
+++ b/drivers/xen/Makefile
@@ -36,6 +36,7 @@ obj-$(CONFIG_XEN_ACPI_HOTPLUG_MEMORY) += xen-acpi-memhotplug.o
 obj-$(CONFIG_XEN_ACPI_HOTPLUG_CPU) += xen-acpi-cpuhotplug.o
 obj-$(CONFIG_XEN_ACPI_PROCESSOR)   += xen-acpi-processor.o
 obj-$(CONFIG_XEN_EFI)  += efi.o
+obj-$(CONFIG_XEN_SCSI_BACKEND) += xen-scsiback.o
 xen-evtchn-y   := evtchn.o
 xen-gntdev-y   := gntdev.o
 xen-gntalloc-y := gntalloc.o
diff --git a/drivers/xen/xen-scsiback.c b/drivers/xen/xen-scsiback.c
new file mode 100644
index 000..7b565632
--- /dev/null
+++ b/drivers/xen/xen-scsiback.c
@@ -0,0 +1,2124 @@
+/*
+ * Xen SCSI backend driver
+ *
+ * Copyright (c) 2008, FUJITSU Limited
+ *
+ * Based on the blkback driver code.
+ * Adaption to kernel taget core infrastructure taken from vhost/scsi.c
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License version 2
+ * as published by the Free Software Foundation; or, when distributed
+ * separately from the Linux kernel or incorporated into other
+ * software packages, subject to the following license:
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this source file (the Software), to deal in the Software without
+ * restriction, including without limitation the rights to use, copy, modify,
+ * merge, publish, distribute, sublicense, and/or sell copies of the Software,
+ * and to permit persons to whom the Software is furnished to do so, subject to
+ * the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED AS IS, WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ */
+
+#include stdarg.h
+
+#include linux/module.h
+#include linux/utsname.h
+#include linux/interrupt.h
+#include linux/slab.h
+#include linux/wait.h
+#include linux/sched.h
+#include linux/list.h
+#include linux/gfp.h
+#include linux/delay.h
+#include linux/spinlock.h
+#include linux/configfs.h
+
+#include generated/utsrelease.h
+
+#include scsi/scsi_dbg.h
+#include scsi/scsi_eh.h
+#include scsi/scsi_tcq.h
+
+#include target/target_core_base.h
+#include target/target_core_fabric.h
+#include target/target_core_configfs.h
+#include target

Re: [Xen-devel] [PATCH V5 5/5] add xen pvscsi maintainer

[Juergen Gross]

On 08/26/2014 04:14 PM, David Vrabel wrote:

On 18/08/14 10:31, jgr...@suse.com wrote:

From: Juergen Gross jgr...@suse.com

Add myself as maintainer for the Xen pvSCSI stuff.

Signed-off-by: Juergen Gross jgr...@suse.com
---
  MAINTAINERS | 8 
  1 file changed, 8 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index aefa948..360f86f 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10086,6 +10086,14 @@ S: Supported
  F:drivers/block/xen-blkback/*
  F:drivers/block/xen*

+XEN PVSCSI DRIVERS
+M: Juergen Gross jgr...@suse.com
+L: xen-de...@lists.xenproject.org (moderated for non-subscribers)


You should add the appropriate scsi list here
(linux-scsi@vger.kernel.org I presume?)


Really? Xen PCI and Xen Block susbsystems don't have an according entry.
James, do you have an opinion here? I don't mind adding linux-scsi, but
I think the Xen list is the appropriate one for pvSCSI.


+S: Supported
+F: drivers/scsi/xen-scsifront.c
+F: drivers/xen/xen-scsiback.c
+F: include/xen/interface/io/vscsiif.h
+



Juergen
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Xen-devel] [PATCH V5 5/5] add xen pvscsi maintainer

[Juergen Gross]

On 08/26/2014 07:12 PM, David Vrabel wrote:

On 26/08/14 17:37, James Bottomley wrote:

On Tue, 2014-08-26 at 16:23 +0200, Juergen Gross wrote:

On 08/26/2014 04:14 PM, David Vrabel wrote:

On 18/08/14 10:31, jgr...@suse.com wrote:

From: Juergen Gross jgr...@suse.com

Add myself as maintainer for the Xen pvSCSI stuff.

Signed-off-by: Juergen Gross jgr...@suse.com
---
   MAINTAINERS | 8 
   1 file changed, 8 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index aefa948..360f86f 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10086,6 +10086,14 @@ S: Supported
   F:   drivers/block/xen-blkback/*
   F:   drivers/block/xen*

+XEN PVSCSI DRIVERS
+M: Juergen Gross jgr...@suse.com
+L: xen-de...@lists.xenproject.org (moderated for non-subscribers)


You should add the appropriate scsi list here
(linux-scsi@vger.kernel.org I presume?)


Really? Xen PCI and Xen Block susbsystems don't have an according entry.
James, do you have an opinion here? I don't mind adding linux-scsi, but
I think the Xen list is the appropriate one for pvSCSI.


It depends how you want to handle this, but the patches have to come on
to the SCSI list somehow.  You can have them directly posted by adding
the list, or you can repost them yourselves.  It's a question of
workflow.


I think these drivers have had (and will continue to have) the most
useful review from scsi people.  I think posting directly (hence
earlier) to linux-scsi will ensure the best quality.

This is the workflow used for the Xen network drivers and I think it
works well.


Okay, I'll add the scsi list.

Juergen

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Xen-devel] [PATCH V5 3/5] Introduce xen-scsifront module

[Juergen Gross]

On 08/23/2014 12:25 AM, Konrad Rzeszutek Wilk wrote:

--- /dev/null
+++ b/drivers/scsi/xen-scsifront.c
@@ -0,0 +1,1017 @@
+/*
+ * Xen SCSI frontend driver
+ *
+ * Copyright (c) 2008, FUJITSU Limited
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License version 2
+ * as published by the Free Software Foundation; or, when distributed
+ * separately from the Linux kernel or incorporated into other
+ * software packages, subject to the following license:
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this source file (the Software), to deal in the Software without
+ * restriction, including without limitation the rights to use, copy, modify,
+ * merge, publish, distribute, sublicense, and/or sell copies of the Software,
+ * and to permit persons to whom the Software is furnished to do so, subject to
+ * the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED AS IS, WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ */
+
+#define DEBUG


:-) I think you don't want that in the driver.


Correct. :-)




+
+#include linux/module.h
+#include linux/kernel.h
+#include linux/device.h
+#include linux/wait.h
+#include linux/interrupt.h
+#include linux/spinlock.h
+#include linux/sched.h
+#include linux/blkdev.h
+#include linux/pfn.h
+#include linux/slab.h
+
+#include scsi/scsi_cmnd.h
+#include scsi/scsi_device.h
+#include scsi/scsi.h
+#include scsi/scsi_host.h
+
+#include xen/xen.h
+#include xen/xenbus.h
+#include xen/grant_table.h
+#include xen/events.h
+#include xen/page.h
+
+#include xen/interface/grant_table.h
+#include xen/interface/io/vscsiif.h
+#include xen/interface/io/protocols.h
+
+#include asm/xen/hypervisor.h
+
+
+#define GRANT_INVALID_REF  0
+
+#define VSCSIFRONT_OP_ADD_LUN  1
+#define VSCSIFRONT_OP_DEL_LUN  2


Shouldn't those be defined in the vscsiff.h file?


No, they are private for the frontend.




+
+#define DEFAULT_TASK_COMM_LEN  TASK_COMM_LEN


Not sure why you need the DEFAULT_ ? Could you use TASK_COMM_LEN?


Sure.




+
+/* tuning point*/


Missing stop and a space after the 't':

/* Tuning point. */


Okay.




+#define VSCSIIF_DEFAULT_CMD_PER_LUN 10
+#define VSCSIIF_MAX_TARGET  64
+#define VSCSIIF_MAX_LUN 255
+
+#define VSCSIIF_RING_SIZE  __CONST_RING_SIZE(vscsiif, PAGE_SIZE)
+#define VSCSIIF_MAX_REQS   VSCSIIF_RING_SIZE
+
+#define vscsiif_grants_sg(_sg) (PFN_UP((_sg) * \
+   sizeof(struct scsiif_request_segment)))
+
+struct vscsifrnt_shadow {
+   /* command between backend and frontend */
+   unsigned char act;


act? How about 'op' ? Or 'cmd_op'?


I wanted to use the same name as the related element in vscsiif.h




+   uint16_t rqid;


rqid? Could you have a comment explaining what that acronym is?
Oh wait - request id? How about just req_id?


Same again. It's called rqid in vscsiiif.h




+
+   /* Number of pieces of scatter-gather */
+   unsigned int nr_grants;


s/nr_grants/nr_sg/ ?


I'll update the comment, as this is really the number of grants.




+   struct scsiif_request_segment *sg;
+
+   /* do reset or abort function */


Full stop missing.

+   wait_queue_head_t wq_reset; /* reset work queue   */
+   int wait_reset; /* reset work queue condition */
+   int32_t rslt_reset; /* reset response status  */
+   /* (SUCESS or FAILED) */


Full stop missing. s/SUCESS/SUCCESS/


Okay.


+
+   /* requested struct scsi_cmnd is stored from kernel */


Full stop missing.


Okay.


+   struct scsi_cmnd *sc;
+   int gref[vscsiif_grants_sg(SG_ALL) + SG_ALL];
+};
+
+struct vscsifrnt_info {
+   struct xenbus_device *dev;
+
+   struct Scsi_Host *host;
+   int host_active;


This 'host_active' seems to be a guard to call 'scsi_host_remove'
through either the disconnect (so backend told us to disconnect)
or the .remove (so XenStore keys are moved). Either way I think
it is possible to run _both_ of them and this being just
an 'int' is not sufficient.

I would recommend you change this to an ref_count.


Hmm, I think a lock is required here.


+
+   spinlock_t shadow_lock;


Could you move this spinlock below - to where
you have tons of of 'shadow' values please?


Okay.




+   unsigned int evtchn;
+   unsigned 

Re: [Xen-devel] [PATCH V5 2/5] Add XEN pvSCSI protocol description

[Juergen Gross]

On 08/21/2014 09:26 PM, Konrad Rzeszutek Wilk wrote:

On Wed, Aug 20, 2014 at 04:01:57PM +0200, Juergen Gross wrote:

On 08/20/2014 03:25 PM, Konrad Rzeszutek Wilk wrote:

On Mon, Aug 18, 2014 at 11:31:47AM +0200, jgr...@suse.com wrote:

...

+struct vscsiif_request {
+   uint16_t rqid;  /* private guest value, echoed in resp  */
+   uint8_t act;/* command between backend and frontend */
+   uint8_t cmd_len;/* valid CDB bytes */
+
+   uint8_t cmnd[VSCSIIF_MAX_COMMAND_SIZE]; /* the CDB */
+   uint16_t timeout_per_command;
+   uint16_t channel, id, lun;  /* (virtual) device specification */
+   uint16_t ref_rqid;  /* command abort reference */
+   uint8_t sc_data_direction;  /* for DMA_TO_DEVICE(1)
+  DMA_FROM_DEVICE(2)
+  DMA_NONE(3) requests */
+   uint8_t nr_segments;/* Number of pieces of scatter-gather */
+#define VSCSIIF_SG_GRANT   0x80/* flag: SG elements via grant page */
+   /* nr_segments counts grant pages with
+  SG elements


Stop missing. However I am a bit lost. It says that the 'nr_segments' will have 
the
count of grant pages with SG elements. Does that mean the req.seg[0].gref points
to an page which will have an array of grant references? And each grant 
reference
will point to a data page? What about the 'offset' and 'length' of them?

Or does it mean that the 'reg.seg[0].gref' points an page that is filled with
'struct scsiif_request_segment' ? If so, where would the could of those
segments be in? In 'nr_segments'? If that is so where does the VSCSIIF_SG_GRANT
go? Won't we collide?


nr_segments (without the VSCSIIF_SG_GRANT bit) always counts the number
of populated seg[] entries. If VSCSIIF_SG_GRANT is set, each seg[] entry
references another array of struct scsiif_request_segment using a grant
ref, offset into the granted page and a length of the array in bytes.


Shouldn't there be a comment about that? The blkif.h has a pretty lengthy
one when it comes to indirect descriptors that could be copied as it
sounds exactly like the same thing.


I already added the following:

/*
 * Request a SCSI operation specified via a CDB in vscsiif_request.cmnd.
 * The target is specified via channel, id and lun.
 * The operation to be performed is specified via a CDB in cmnd[], the 
length
 * of the CDB is in cmd_len. sc_data_direction specifies the direction 
of data

 * (to the device, from the device, or none at all).
 * If data is to be transferred to or from the device the buffer(s) in the
 * guest memory is/are specified via one or multiple scsiif_request_segment
 * descriptors each specifying a memory page via a grant_ref_t, a 
offset into
 * the page and the length of the area in that page. All 
scsiif_request_segment

 * areas concatenated form the resulting data buffer used by the operation.
 * If the number of scsiif_request_segment areas is not too large (less 
than
 * or equal VSCSIIF_SG_TABLESIZE) the areas can be specified directly 
in the
 * seg[] array and the number of valid scsiif_request_segment elements 
is to be

 * set in nr_segments.
 * If feature-sg-grant in the Xenstore is set it is possible to 
specify more
 * than VSCSIIF_SG_TABLESIZE scsiif_request_segment elements via 
indirection.
 * The maximum number of allowed scsiif_request_segment elements is the 
value
 * of the feature-sg-grant entry from Xenstore. When using 
indirection the

 * seg[] array doesn't contain specifications of the data buffers, but
 * references to scsiif_request_segment arrays, which in turn reference the
 * data buffers. While nr_segments holds the number of populated seg[] 
entries
 * (plus the set VSCSIIF_SG_GRANT bit), the number of 
scsiif_request_segment
 * elements referencing the target data buffers is calculated from the 
lengths

 * of the seg[] elements (the sum of all valid seg[].length divided by the
 * size of one scsiif_request_segment structure).
 */
#define VSCSIIF_ACT_SCSI_CDB   1



The resulting number of struct scsiif_request_segment is the sum of
seg[0..nr_segments-1].length / sizeof(struct scsiif_request_segment).



Where the nr_segments can only go Up to VSCSIIF_SG_TABLESIZE, so the max
total SG entries you can is 13312 ( 4096 / 8 = 512 max per page, times
26).


In theory, yes. SG_ALL (being 128 today) is limiting this value.


Juergen
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Xen-devel] [PATCH V5 2/5] Add XEN pvSCSI protocol description

[Juergen Gross]

On 08/20/2014 03:25 PM, Konrad Rzeszutek Wilk wrote:

On Mon, Aug 18, 2014 at 11:31:47AM +0200, jgr...@suse.com wrote:

From: Juergen Gross jgr...@suse.com

Add the definition of pvSCSI protocol used between the pvSCSI frontend in a
XEN domU and the pvSCSI backend in a XEN driver domain (usually Dom0).

This header was originally provided by Fujitsu for XEN based on Linux 2.6.18.
Changes are:
- added comment
- adapt to Linux style guide
- add support for larger SG-lists by putting them in an own granted page
- remove stale definitions

Signed-off-by: Juergen Gross jgr...@suse.com
---
  include/xen/interface/io/vscsiif.h | 214 +
  1 file changed, 214 insertions(+)
  create mode 100644 include/xen/interface/io/vscsiif.h

diff --git a/include/xen/interface/io/vscsiif.h 
b/include/xen/interface/io/vscsiif.h
new file mode 100644
index 000..4291889
--- /dev/null
+++ b/include/xen/interface/io/vscsiif.h
@@ -0,0 +1,214 @@
+/**
+ * vscsiif.h
+ *
+ * Based on the blkif.h code.
+ *
+ * This interface is to be regarded as a stable API between XEN domains
+ * running potentially different Linux kernel versions.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the Software), to
+ * deal in the Software without restriction, including without limitation the
+ * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
+ * sell copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED AS IS, WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+ * DEALINGS IN THE SOFTWARE.
+ *
+ * Copyright(c) FUJITSU Limited 2008.
+ */
+
+#ifndef __XEN__PUBLIC_IO_SCSI_H__
+#define __XEN__PUBLIC_IO_SCSI_H__
+
+#include ring.h
+#include ../grant_table.h
+
+/*
+ * Front-back notifications: When enqueuing a new request, sending a
+ * notification can be made conditional on req_event (i.e., the generic
+ * hold-off mechanism provided by the ring macros). Backends must set
+ * req_event appropriately (e.g., using RING_FINAL_CHECK_FOR_REQUESTS()).
+ *
+ * Back-front notifications: When enqueuing a new response, sending a
+ * notification can be made conditional on rsp_event (i.e., the generic
+ * hold-off mechanism provided by the ring macros). Frontends must set
+ * rsp_event appropriately (e.g., using RING_FINAL_CHECK_FOR_RESPONSES()).
+ */
+
+/*
+ * Feature and Parameter Negotiation
+ * =
+ * The two halves of a Xen pvSCSI driver utilize nodes within the XenStore to
+ * communicate capabilities and to negotiate operating parameters.  This
+ * section enumerates these nodes which reside in the respective front and
+ * backend portions of the XenStore, following the XenBus convention.
+ *
+ * All data in the XenStore is stored as strings.  Nodes specifying numeric
+ * values are encoded in decimal.  Integer value ranges listed below are
+ * expressed as fixed sized integer types capable of storing the conversion
+ * of a properly formated node string, without loss of information.
+ *
+ * Any specified default value is in effect if the corresponding XenBus node
+ * is not present in the XenStore.
+ *
+ * XenStore nodes in sections marked PRIVATE are solely for use by the
+ * driver side whose XenBus tree contains them.
+ *
+ *
+ *Backend XenBus Nodes
+ *
+ *
+ *-- Backend Device Identification (PRIVATE) --
+ *
+ * p-devname
+ *  Values: string
+ *
+ *  A free string used to identify the physical device (e.g. a disk name).
+ *
+ * p-dev
+ *  Values: string
+ *
+ *  A string specifying the backend device: either a 4-tuple h:c:t:l
+ *  (host, controller, target, lun, all integers), or a WWN (e.g.
+ *  naa.60014054ac780582).
+ *
+ * v-dev
+ *  Values: string
+ *
+ *  A string specifying the frontend device in form of a 4-tuple h:c:t:l
+ *  (host, controller, target, lun, all integers).
+ *
+ *- Features -
+ *
+ * feature-sg-grant
+ *  Values: uint16_t

Re: [Xen-devel] [PATCH V4 3/4] Introduce XEN scsiback module

[Juergen Gross]

On 08/17/2014 04:38 AM, Nicholas A. Bellinger wrote:

On Thu, 2014-08-14 at 12:14 +0200, Juergen Gross wrote:

On 08/14/2014 10:53 AM, Juergen Gross wrote:

Nicholas,

just one more question (see below):

On 08/12/2014 11:13 PM, Nicholas A. Bellinger wrote:

Hi Juergen  Co,

Finally had a chance to review this code.  Comments are inline below..

On Fri, 2014-08-08 at 09:49 +0200, jgr...@suse.com wrote:


...


+if (IS_ERR(tv_nexus-tvn_se_sess)) {
+mutex_unlock(tpg-tv_tpg_mutex);
+kfree(tv_nexus);
+return -ENOMEM;
+}
+se_sess = tv_nexus-tvn_se_sess;
+/*
+ * Since we are running in 'demo mode' this call with generate a
+ * struct se_node_acl for the scsiback struct se_portal_group with
+ * the SCSI Initiator port name of the passed configfs group
'name'.
+ */
+tv_nexus-tvn_se_sess-se_node_acl =
core_tpg_check_initiator_node_acl(
+se_tpg, (unsigned char *)name);
+if (!tv_nexus-tvn_se_sess-se_node_acl) {
+mutex_unlock(tpg-tv_tpg_mutex);
+pr_debug(core_tpg_check_initiator_node_acl() failed for %s\n,
+ name);
+goto out;
+}


Can I drop the call to core_tpg_check_initiator_node_acl() as well?
Keeping it will result in failing to setup the nexus (which is to be
expected IMHO).


Obviously I can't. transport_lookup_cmd_lun() calls right at start
spin_lock_irqsave(se_sess-se_node_acl-device_list_lock, flags);
resulting in a page fault...

I guess the hint to just throw away all the node_acl related stuff has
to be adjusted somehow...

So: what is needed, what can be removed?



The scsiback_make_nodeacl() + scsiback_drop_nodeacl() configfs callbacks
can be safely dropped, along with scisback_ncal-iport_[wwpn,name].


Okay, thanks. This is working.

Juergen

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Re: [Xen-devel] [PATCH V4 3/4] Introduce XEN scsiback module

[Juergen Gross]

Nicholas,

just one more question (see below):

On 08/12/2014 11:13 PM, Nicholas A. Bellinger wrote:

Hi Juergen  Co,

Finally had a chance to review this code.  Comments are inline below..

On Fri, 2014-08-08 at 09:49 +0200, jgr...@suse.com wrote:


...


+   if (IS_ERR(tv_nexus-tvn_se_sess)) {
+   mutex_unlock(tpg-tv_tpg_mutex);
+   kfree(tv_nexus);
+   return -ENOMEM;
+   }
+   se_sess = tv_nexus-tvn_se_sess;
+   /*
+* Since we are running in 'demo mode' this call with generate a
+* struct se_node_acl for the scsiback struct se_portal_group with
+* the SCSI Initiator port name of the passed configfs group 'name'.
+*/
+   tv_nexus-tvn_se_sess-se_node_acl = core_tpg_check_initiator_node_acl(
+   se_tpg, (unsigned char *)name);
+   if (!tv_nexus-tvn_se_sess-se_node_acl) {
+   mutex_unlock(tpg-tv_tpg_mutex);
+   pr_debug(core_tpg_check_initiator_node_acl() failed for %s\n,
+name);
+   goto out;
+   }


Can I drop the call to core_tpg_check_initiator_node_acl() as well?
Keeping it will result in failing to setup the nexus (which is to be
expected IMHO).

I suppose the name of the nexus is just for reference and should be
stored somewhere to be able to print it in _show_nexus()?


Juergen

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Xen-devel] [PATCH V4 3/4] Introduce XEN scsiback module

[Juergen Gross]

On 08/14/2014 10:53 AM, Juergen Gross wrote:

Nicholas,

just one more question (see below):

On 08/12/2014 11:13 PM, Nicholas A. Bellinger wrote:

Hi Juergen  Co,

Finally had a chance to review this code.  Comments are inline below..

On Fri, 2014-08-08 at 09:49 +0200, jgr...@suse.com wrote:


...


+if (IS_ERR(tv_nexus-tvn_se_sess)) {
+mutex_unlock(tpg-tv_tpg_mutex);
+kfree(tv_nexus);
+return -ENOMEM;
+}
+se_sess = tv_nexus-tvn_se_sess;
+/*
+ * Since we are running in 'demo mode' this call with generate a
+ * struct se_node_acl for the scsiback struct se_portal_group with
+ * the SCSI Initiator port name of the passed configfs group
'name'.
+ */
+tv_nexus-tvn_se_sess-se_node_acl =
core_tpg_check_initiator_node_acl(
+se_tpg, (unsigned char *)name);
+if (!tv_nexus-tvn_se_sess-se_node_acl) {
+mutex_unlock(tpg-tv_tpg_mutex);
+pr_debug(core_tpg_check_initiator_node_acl() failed for %s\n,
+ name);
+goto out;
+}


Can I drop the call to core_tpg_check_initiator_node_acl() as well?
Keeping it will result in failing to setup the nexus (which is to be
expected IMHO).


Obviously I can't. transport_lookup_cmd_lun() calls right at start
spin_lock_irqsave(se_sess-se_node_acl-device_list_lock, flags);
resulting in a page fault...

I guess the hint to just throw away all the node_acl related stuff has
to be adjusted somehow...

So: what is needed, what can be removed?


Juergen
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V4 3/4] Introduce XEN scsiback module

[Juergen Gross]

On 08/12/2014 11:13 PM, Nicholas A. Bellinger wrote:

Hi Juergen  Co,

Finally had a chance to review this code.  Comments are inline below..


Thank you very much for your review!



On Fri, 2014-08-08 at 09:49 +0200, jgr...@suse.com wrote:

From: Juergen Gross jgr...@suse.com

Introduces the XEN pvSCSI backend. With pvSCSI it is possible for a XEN domU
to issue SCSI commands to a SCSI LUN assigned to that domU. The SCSI commands
are passed to the pvSCSI backend in a driver domain (usually Dom0) which is
owner of the physical device. This allows e.g. to use SCSI tape drives in a
XEN domU.

The code is taken from the pvSCSI implementation in XEN done by Fujitsu based
on Linux kernel 2.6.18.

Changes from the original version are:
- port to upstream kernel
- put all code in just one source file
- adapt to Linux style guide
- use target core infrastructure instead doing pure pass-through
- enable module unloading
- support SG-list in grant page(s)
- support task abort
- remove redundant struct backend
- allocate resources dynamically
- correct minor error in scsiback_fast_flush_area
- free allocated resources in case of error during I/O preparation
- remove CDB emulation, now handled by target core infrastructure

Signed-off-by: Juergen Gross jgr...@suse.com

Xen related parts
Acked-by: David Vrabel david.vra...@citrix.com
---


SNIP


diff --git a/drivers/xen/xen-scsiback.c b/drivers/xen/xen-scsiback.c
new file mode 100644
index 000..4a0d6e3
--- /dev/null
+++ b/drivers/xen/xen-scsiback.c


SNIP


+struct scsiback_nacl {
+   /* Binary World Wide unique Port Name for pvscsi Initiator port */
+   u64 iport_wwpn;
+   /* ASCII formatted WWPN for Sas Initiator port */
+   char iport_name[VSCSI_NAMELEN];
+   /* Returned by scsiback_make_nodeacl() */
+   struct se_node_acl se_node_acl;
+};
+


Given that this code is similar to how loopback + vhost-scsi function,
and uses a (locally) generated nexus for each WWPN endpoint, the
scsiback_nacl and associated code will be unused should be be dropped.


Done.



SNIP


+static void scsiback_do_resp_with_sense(char *sense_buffer, int32_t result,
+   uint32_t resid, struct vscsibk_pend *pending_req)
+{
+   struct vscsiif_response *ring_res;
+   struct vscsibk_info *info = pending_req-info;
+   int notify;
+   struct scsi_sense_hdr sshdr;
+   unsigned long flags;
+   unsigned len;
+
+   spin_lock_irqsave(info-ring_lock, flags);
+
+   ring_res = RING_GET_RESPONSE(info-ring, info-ring.rsp_prod_pvt);
+   info-ring.rsp_prod_pvt++;
+
+   ring_res-rslt   = result;
+   ring_res-rqid   = pending_req-rqid;
+
+   if (sense_buffer != NULL 
+   scsi_normalize_sense(sense_buffer, VSCSIIF_SENSE_BUFFERSIZE,
+sshdr)) {
+   len = min_t(unsigned, 8 + sense_buffer[7],
+   VSCSIIF_SENSE_BUFFERSIZE);
+   memcpy(ring_res-sense_buffer, sense_buffer, len);
+   ring_res-sense_len = len;
+   } else {
+   ring_res-sense_len = 0;
+   }
+
+   ring_res-residual_len = resid;
+
+   RING_PUSH_RESPONSES_AND_CHECK_NOTIFY(info-ring, notify);
+   spin_unlock_irqrestore(info-ring_lock, flags);
+
+   if (notify)
+   notify_remote_via_irq(info-irq);
+
+   if (pending_req-v2p)
+   kref_put(pending_req-v2p-kref,
+scsiback_free_translation_entry);
+
+   kmem_cache_free(scsiback_cachep, pending_req);
+}
+
+static void scsiback_cmd_done(struct vscsibk_pend *pending_req)
+{
+   struct vscsibk_info *info = pending_req-info;
+   unsigned char *sense_buffer;
+   unsigned int resid;
+   int errors;
+
+   sense_buffer = pending_req-sense_buffer;
+   resid= pending_req-se_cmd.residual_count;
+   errors   = pending_req-result;
+
+   if (errors  log_print_stat)
+   scsiback_print_status(sense_buffer, errors, pending_req);
+
+   scsiback_fast_flush_area(pending_req);
+   scsiback_do_resp_with_sense(sense_buffer, errors, resid, pending_req);
+   scsiback_put(info);
+
+   transport_generic_free_cmd(pending_req-se_cmd, 0);
+}
+


The usage here of scsiback_do_resp_with_sense() - kmem_cache_free() for
*pending_req, and then invoking transport_generic_free_cmd() with
pending_req-se_cmd is an free after use bug..

So the way this should work is similar to how loopback currently does
things:

   - Move the kmem_cache_free() for pending_req from
 scsiback_do_resp_with_sense() to scsiback_release_cmd()
   - Remove the transport_generic_free_cmd() from scsiback_cmd_done()
   - Copy what tcm_loop_check_stop_free() does into
 scsiback_check_stop_free(), and remove target_put_sess_cmd()


Done.




+static void scsiback_cmd_exec(struct vscsibk_pend *pending_req)
+{
+   struct se_cmd *se_cmd = pending_req-se_cmd;
+   struct se_session *sess = pending_req-v2p-tpg

Re: Re: [Xen-devel] [PATCH V4 3/4] Introduce XEN scsiback module

[Juergen Gross]

On 08/13/2014 09:02 AM, Juergen Gross wrote:

On 08/12/2014 11:13 PM, Nicholas A. Bellinger wrote:

Hi Juergen  Co,

Finally had a chance to review this code.  Comments are inline below..



+static struct se_node_acl *
+scsiback_alloc_fabric_acl(struct se_portal_group *se_tpg)
+{
+struct scsiback_nacl *nacl;
+
+nacl = kzalloc(sizeof(struct scsiback_nacl), GFP_KERNEL);
+if (!nacl) {
+pr_err(Unable to allocate struct scsiback_nacl\n);
+return NULL;
+}
+
+return nacl-se_node_acl;
+}
+
+static void
+scsiback_release_fabric_acl(struct se_portal_group *se_tpg,
+ struct se_node_acl *se_nacl)
+{
+struct scsiback_nacl *nacl = container_of(se_nacl,
+struct scsiback_nacl, se_node_acl);
+kfree(nacl);
+}
+
+static u32 scsiback_tpg_get_inst_index(struct se_portal_group *se_tpg)
+{
+return 1;
+}
+
+static struct se_node_acl *
+scsiback_make_nodeacl(struct se_portal_group *se_tpg,
+   struct config_group *group,
+   const char *name)
+{
+struct se_node_acl *se_nacl, *se_nacl_new;
+struct scsiback_nacl *nacl;
+u64 wwpn = 0;
+u32 nexus_depth;
+
+se_nacl_new = scsiback_alloc_fabric_acl(se_tpg);
+if (!se_nacl_new)
+return ERR_PTR(-ENOMEM);
+
+nexus_depth = 1;
+/*
+ * se_nacl_new may be released by core_tpg_add_initiator_node_acl()
+ * when converting a NodeACL from demo mode - explict
+ */
+se_nacl = core_tpg_add_initiator_node_acl(se_tpg, se_nacl_new,
+name, nexus_depth);
+if (IS_ERR(se_nacl)) {
+scsiback_release_fabric_acl(se_tpg, se_nacl_new);
+return se_nacl;
+}
+/*
+ * Locate our struct scsiback_nacl and set the FC Nport WWPN
+ */
+nacl = container_of(se_nacl, struct scsiback_nacl, se_node_acl);
+nacl-iport_wwpn = wwpn;
+
+return se_nacl;
+}
+
+static void scsiback_drop_nodeacl(struct se_node_acl *se_acl)
+{
+struct scsiback_nacl *nacl = container_of(se_acl,
+struct scsiback_nacl, se_node_acl);
+core_tpg_del_initiator_node_acl(se_acl-se_tpg, se_acl, 1);
+kfree(nacl);
+}
+


As mentioned above, the NodeACL use is unnecessary for this driver so
you can safely drop scsiback_make_node_acl() +
scsiback_alloc_fabric_acl() + scsiback_drop_nodeacl() +
scsiback_release_fabric_acl().


Deleted.


target_fabric_tf_ops_check() complains about missing
tpg_alloc_fabric_acl and tpg_release_fabric_acl.



+static void scsiback_set_default_node_attrs(struct se_node_acl *nacl)
+{
+}
+


Safe to drop this no-op too.


Okay.


target_fabric_tf_ops_check() wants this to be set, too.


Juergen
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Xen-devel] [PATCH V4 2/4] Introduce xen-scsifront module

[Juergen Gross]

On 08/11/2014 07:50 PM, Christoph Hellwig wrote:

On Mon, Aug 11, 2014 at 12:27:29PM +0200, Juergen Gross wrote:

What do you mean with unusual? You mean transferring the EH action to
Dom0?


Yes.  Note that hyperv tries something similar and they've run into
timeout issues, you might want to read up the recent thread on that.


+   } else {
+   xenbus_printf(XBT_NIL, dev-nodename,
+ state_str, %d,
+ XenbusStateConnected);
+   }


Just print this message in -slave_configure.


This is calling for problems, I think. xenbus_printf() is not just a
printing function, but it changes an entry in the xenstore. And this
requires locking, switching threads, ...

I doubt doing this while holding SCSI-internal locks is a good idea.


Oh, I thought xenbus_printf was just a logging wrapper.

Doing major work in the slave_* callouts is not a problem, that's what
they were designed for.


Okay.



For the successful case the xenbus_printf should be done in
-slave_configure.  For the failure case you probably want to do it
from -slave_destroy based on the absence of a flag set in -slave_configure,
e.g. in slave_configure:

sdev-hostdata = (void *)1UL;

and in -slave_destroy:

if (!sdev-hostdata)


I don't think I'll need the flag. The action is the same if the device
is being destroyed again because of already existing or when it is
really removed.


...

although you might see something like this based on external scanning
through procfs/sysfs as mentioned earlier, so please take a look at
how all these corner cases could effect you.


I'll add a check if .slave_configure() and .slave_destroy() are running
in the same task as scsi_add_device() or scsi_remove_device(). This
should rule out all of these corner cases.


Juergen

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Xen-devel] [PATCH V4 3/4] Introduce XEN scsiback module

[Juergen Gross]

On 08/11/2014 08:14 PM, Christoph Hellwig wrote:

+#include scsi/scsi_dbg.h


__scsi_print_sense()


+#include scsi/scsi_eh.h


struct scsi_sense_hdr


+#include scsi/scsi_tcq.h


SG_ALL



What do you need these for?  Normally target drivers shouldn't need
these.


+struct vscsibk_emulate {
+   void (*pre_function)(struct vscsibk_pend *, void *);
+   void (*post_function)(struct vscsibk_pend *, void *);
+};


This doesn't seem to be used.


Correct. Will delete.




+#define scsiback_get(_b) (atomic_inc((_b)-nr_unreplied_reqs))
+#define scsiback_put(_b)   \
+   do {\
+   if (atomic_dec_and_test((_b)-nr_unreplied_reqs))   \
+   wake_up((_b)-waiting_to_free);\
+   } while (0)


Normal Linux style would be to make these inline functions.


Okay. I'll change those.




+static void scsiback_notify_work(struct vscsibk_info *info)
+{
+   info-waiting_reqs = 1;
+   wake_up(info-wq);
+}
+
+static irqreturn_t scsiback_intr(int irq, void *dev_id)
+{
+   scsiback_notify_work((struct vscsibk_info *)dev_id);
+   return IRQ_HANDLED;
+}


Seems like this driver should get the same threaded irq treatment as
the initiator side?


Indeed.




+static void scsiback_disconnect(struct vscsibk_info *info)
+{
+   if (info-kthread) {
+   kthread_stop(info-kthread);
+   info-kthread = NULL;
+   wake_up(info-shutdown_wq);
+   }
+
+   wait_event(info-waiting_to_free,
+   atomic_read(info-nr_unreplied_reqs) == 0);
+
+   if (info-irq) {
+   unbind_from_irqhandler(info-irq, info);
+   info-irq = 0;
+   }
+
+   if (info-ring.sring) {
+   xenbus_unmap_ring_vfree(info-dev, info-ring.sring);
+   info-ring.sring = NULL;
+   }
+}


Also the same treatment for goto based init failure unwinding.


Yep.


Juergen

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Xen-devel] [PATCH V4 2/4] Introduce xen-scsifront module

[Juergen Gross]

On 08/11/2014 11:54 AM, Christoph Hellwig wrote:

+   BUG_ON(sc-cmd_len  VSCSIIF_MAX_COMMAND_SIZE);
+
+   if (sc-cmd_len)


I can't see how you can get a zero cmd_len here.


Ahh, thanks for spotting this. In a previous version it could be zero
in case of reset.


+static int scsifront_action_handler(struct scsi_cmnd *sc, uint8_t act)


Please add a comment explaining your unusual EH strategy here.


What do you mean with unusual? You mean transferring the EH action to
Dom0?




+static void scsifront_free(struct vscsifrnt_info *info)
+{
+   if (info-host  info-host_active) {
+   /* Scsi_host not yet removed */
+   scsi_remove_host(info-host);
+   info-host_active = 0;
+   }
+
+   if (info-ring_ref != GRANT_INVALID_REF) {
+   gnttab_end_foreign_access(info-ring_ref, 0,
+ (unsigned long)info-ring.sring);
+   info-ring_ref = GRANT_INVALID_REF;
+   info-ring.sring = NULL;
+   }
+
+   if (info-irq)
+   unbind_from_irqhandler(info-irq, info);
+   info-irq = 0;
+   info-evtchn = 0;
+
+   if (info-host)
+   scsi_host_put(info-host);
+}


I don't think most of the ifs should be here, just use proper symmetric
goto unwinding in the initialization error path instead.

The way this function can be called from different levels of the
callstack on init failure is very confusing.


Okay, I'll look into making it easier to understand.




+   switch (op) {
+   case VSCSIFRONT_OP_ADD_LUN:
+   if (device_state == XenbusStateInitialised) {
+   sdev = __scsi_add_device(info-host, chn, tgt,
+   lun, NULL);
+   err = (IS_ERR(sdev) || !sdev-hostdata);
+   if (!IS_ERR(sdev)) {
+   sdev-hostdata = NULL;
+   scsi_device_put(sdev);
+   }


Given that you put the device immediatly you should be using
scsi_add_device instead of __scsi_add_device.  Also all the messing
with -hostdata from -slave_alloc looks wrong.  For one thing every
setup done -slave_alloc should be paired with teardown in
-slave_destroy.  Second I don't see any need for that.


The problem is I have to take different actions depending on the device
being new or not.




+   } else {
+   xenbus_printf(XBT_NIL, dev-nodename,
+ state_str, %d,
+ XenbusStateConnected);
+   }


Just print this message in -slave_configure.


This is calling for problems, I think. xenbus_printf() is not just a
printing function, but it changes an entry in the xenstore. And this
requires locking, switching threads, ...

I doubt doing this while holding SCSI-internal locks is a good idea.


Juergen
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Xen-devel] [PATCH V3 1/4] Add XEN pvSCSI protocol description

[Juergen Gross]

On 08/04/2014 12:10 PM, Jan Beulich wrote:

On 04.08.14 at 10:27, jgr...@suse.com wrote:

+/* Requests from the frontend to the backend */
+
+/*
+ * Request a SCSI operation specified via a CDB in vscsiif_request.cmnd.
+ * The target is specified via channel, id and lun.
+ */
+#define VSCSIIF_ACT_SCSI_CDB   1
+
+/*
+ * Request abort of a running operation for the specified target given by
+ * channel, id, lun and the operation's rqid in ref_rqid.
+ */
+#define VSCSIIF_ACT_SCSI_ABORT 2
+
+/*
+ * Request a device reset of the specified target (channel and id).
+ */
+#define VSCSIIF_ACT_SCSI_RESET 3


While I realize you don't want to support it, leaving out
VSCSIIF_ACT_SCSI_SG_PRESET altogether from this header may
lead people to the false impression that its number is available for
other use. I would generally recommend the Linux headers to not
deviate from the master ones more than absolutely necessary,
keeping what isn't needed/wanted by Linux at least in commented
out form.


Okay. I'll re-add the #define


Juergen

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V3] Save command pool address of Scsi_Host

[Juergen Gross]

On 08/04/2014 01:08 PM, Christoph Hellwig wrote:

On Mon, Aug 04, 2014 at 06:26:09AM +0200, jgr...@suse.com wrote:

From: Juergen Gross jgr...@suse.com

If a scsi host driver specifies .cmd_len in it's scsi_host_template, a driver's
private command pool is needed. scsi_find_host_cmd_pool() will locate it, but
scsi_alloc_host_cmd_pool() isn't saving the pool address in the host template.

This will result in an access error when the host is removed.

Avoid the problem by saving the address of a new allocated command pool where
it is expected and delete it again when the pool is destroyed.


I don't really like the double pointer passing - just NULLing out the
pointer in the caller where needed seems cleaner.


I wanted to avoid to spread this, but I don't mind doing it. V4 is
coming...


Juergen

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V4] Save command pool address of Scsi_Host

[Juergen Gross]

On 08/04/2014 01:24 PM, jgr...@suse.com wrote:

From: Juergen Gross jgr...@suse.com

If a scsi host driver specifies .cmd_len in it's scsi_host_template, a driver's
private command pool is needed. scsi_find_host_cmd_pool() will locate it, but
scsi_alloc_host_cmd_pool() isn't saving the pool address in the host template.

This will result in an access error when the host is removed.

Avoid the problem by saving the address of a new allocated command pool where
it is expected.


Please ignore, should have used pool instead hostt-cm_pool for calling
scsi_free_host_cmd_pool(). V5 follows...


Juergen



Signed-off-by: Juergen Gross jgr...@suse.com
---
  drivers/scsi/scsi.c | 16 
  1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 88d46fe..f654ac1 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -380,6 +380,10 @@ scsi_alloc_host_cmd_pool(struct Scsi_Host *shost)
pool-slab_flags |= SLAB_CACHE_DMA;
pool-gfp_mask = __GFP_DMA;
}
+
+   if (hostt-cmd_size)
+   hostt-cmd_pool = pool;
+
return pool;
  }

@@ -424,8 +428,10 @@ out:
  out_free_slab:
kmem_cache_destroy(pool-cmd_slab);
  out_free_pool:
-   if (hostt-cmd_size)
-   scsi_free_host_cmd_pool(pool);
+   if (hostt-cmd_size) {
+   scsi_free_host_cmd_pool(hostt-cmd_pool);
+   hostt-cmd_pool = NULL;
+   }
goto out;
  }

@@ -447,8 +453,10 @@ static void scsi_put_host_cmd_pool(struct Scsi_Host *shost)
if (!--pool-users) {
kmem_cache_destroy(pool-cmd_slab);
kmem_cache_destroy(pool-sense_slab);
-   if (hostt-cmd_size)
-   scsi_free_host_cmd_pool(pool);
+   if (hostt-cmd_size) {
+   scsi_free_host_cmd_pool(hostt-cmd_pool);
+   hostt-cmd_pool = NULL;
+   }
}
mutex_unlock(host_cmd_pool_mutex);
  }



--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V5] Save command pool address of Scsi_Host

[Juergen Gross]

On 08/04/2014 01:49 PM, Boaz Harrosh wrote:

On 08/04/2014 02:30 PM, jgr...@suse.com wrote:

From: Juergen Gross jgr...@suse.com

If a scsi host driver specifies .cmd_len in it's scsi_host_template, a driver's
private command pool is needed. scsi_find_host_cmd_pool() will locate it, but
scsi_alloc_host_cmd_pool() isn't saving the pool address in the host template.

This will result in an access error when the host is removed.

Avoid the problem by saving the address of a new allocated command pool where
it is expected.

Signed-off-by: Juergen Gross jgr...@suse.com
---
  drivers/scsi/scsi.c | 12 ++--
  1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 88d46fe..b0cef5b 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -380,6 +380,10 @@ scsi_alloc_host_cmd_pool(struct Scsi_Host *shost)
pool-slab_flags |= SLAB_CACHE_DMA;
pool-gfp_mask = __GFP_DMA;
}
+
+   if (hostt-cmd_size)
+   hostt-cmd_pool = pool;
+
return pool;
  }

@@ -424,8 +428,10 @@ out:
  out_free_slab:
kmem_cache_destroy(pool-cmd_slab);
  out_free_pool:
-   if (hostt-cmd_size)
+   if (hostt-cmd_size) {
scsi_free_host_cmd_pool(pool);


I disagree I liked the V4 version better. It is much nicer on the review
that we NULL the same one we pass in with no need for context that's outside
of this scope


:-)

I'm fine with either version.

Juergen



Just my $0.017
Boaz


+   hostt-cmd_pool = NULL;
+   }
goto out;
  }

@@ -447,8 +453,10 @@ static void scsi_put_host_cmd_pool(struct Scsi_Host *shost)
if (!--pool-users) {
kmem_cache_destroy(pool-cmd_slab);
kmem_cache_destroy(pool-sense_slab);
-   if (hostt-cmd_size)
+   if (hostt-cmd_size) {
scsi_free_host_cmd_pool(pool);
+   hostt-cmd_pool = NULL;
+   }
}
mutex_unlock(host_cmd_pool_mutex);
  }



--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/



--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Save command pool address of Scsi_Host

[Juergen Gross]

On 08/01/2014 10:24 PM, James Bottomley wrote:

On Fri, 2014-08-01 at 05:03 -0700, Christoph Hellwig wrote:

On Fri, Aug 01, 2014 at 08:27:05AM +0200, jgr...@suse.com wrote:

From: Juergen Gross jgr...@suse.com

If a scsi host driver specifies .cmd_len in it's scsi_host_template, a driver's
private command pool is needed. scsi_find_host_cmd_pool() will locate it, but
scsi_alloc_host_cmd_pool() isn't saving the pool address in the host template.

This will result in an access error when the host is removed.

Avoid the problem by saving the address of a new allocated command pool where
it is expected.

Signed-off-by: Juergen Gross jgr...@suse.com


Looks good, but minor nitpick below:


+   if (shost-hostt-cmd_size)
+   shost-hostt-cmd_pool = pool;
+



We already have a local hostt variable for the host template in this
function, please use it.


Wait, that's not right at all.  There looks to be a thinko in the
command pool handling code.  We have both a cmd_pool in the host
structure and in the host template structure, but there's confusion
about which one we're supposed to be using.

The origin of confusion seems to be the reference counting in the pool
itself ... you want the same pool for all hosts, since they can only
have one cmd_size, but you want it created on first host use and
destroyed again on the last one.

If you take this patch, a host that attached, detaches and then attaches
a host will panic because it will use a freed pool structure.


Indeed.


This whole mess is created by the attempt to refcount the pools.  What's
wrong with simply creating the pool at init time and deleting it again
at module removal ... that way no refcounting and no bogus problems like
this (and we can delete the cmd_pool from the host).  The restriction
this would give is that cmd_size can only be set in the template, but
that seems to be the only safe use anyway, since any driver trying to
vary this in its host add routines will get unexpected results.


OTOH it would be possible to just delete .cmd_pool in the template when
deleting the pool. I'll send a patch doing this and you can decide
whether to take it or to use the other solution.

I'm not sure which to prefer: the init/remove version is simple, while
the dynamic version requires no changes in the driver's source and the
pool's resources are allocated only when really needed.


Juergen

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Oops in scsi_put_host_cmd_pool

[Juergen Gross]

On 08/01/2014 07:41 AM, Juergen Gross wrote:

During test of Xen pvSCSI frontend module I found the following issue:

When unplugging a passed-through SCSI-device the SCSI Host is removed.
When calling the final scsi_host_put() from the driver an Oops is
happening:

[  219.816292] (file=drivers/scsi/xen-scsifront.c, line=808)
scsifront_remove: device/vscsi/1 removed
[  219.816371] BUG: unable to handle kernel NULL pointer dereference at
0010
[  219.816380] IP: [805fdcf8] scsi_put_host_cmd_pool+0x38/0xb0
[  219.816390] PGD 3bd60067 PUD 3d353067 PMD 0
[  219.816396] Oops:  [#1] SMP
[  219.816400] Modules linked in: nls_utf8 sr_mod cdrom xen_scsifront
xt_pkttype xt_LOG xt_limit af_packet ip6t_REJECT xt_tcpudp
nf_conntrack_ipv6 nf_defrag_ipv6 ip6table_raw ipt_REJECT iptable_raw
xt_CT iptable_filter ip6table_mangle nf_conntrack_netbios_ns
nf_conntrack_broadcast nf_conntrack_ipv4 nf_defrag_ipv4 ip_tables
xt_conntrack nf_conntrack ip6table_filter ip6_tables x_tables
x86_pkg_temp_thermal thermal_sys coretemp hwmon crc32_pclmul
crc32c_intel ghash_clmulni_intel aesni_intel ablk_helper cryptd lrw
gf128mul glue_helper aes_x86_64 microcode pcspkr sg dm_mod autofs4
scsi_dh_alua scsi_dh_emc scsi_dh_rdac scsi_dh_hp_sw scsi_dh xen_blkfront
xen_netfront
[  219.816458] CPU: 0 PID: 23 Comm: xenwatch Not tainted
3.16.0-rc6-11-xen+ #66
[  219.816463] task: 88003da985d0 ti: 88003da9c000 task.ti:
88003da9c000
[  219.816467] RIP: e030:[805fdcf8]  [805fdcf8]
scsi_put_host_cmd_pool+0x38/0xb0
[  219.816474] RSP: e02b:88003da9fc20  EFLAGS: 00010202
[  219.816477] RAX: a01a50c0 RBX:  RCX:
0003
[  219.816481] RDX: 0240 RSI: 88003d242b80 RDI:
80c708c0
[  219.816485] RBP: 88003da9fc38 R08: 4f7e974a31ed0290 R09:

[  219.816488] R10: 7ff0 R11: 0001 R12:
8800038f8000
[  219.816491] R13: a01a50c0 R14:  R15:

[  219.816498] FS:  7fe2e2eeb700() GS:88003f80()
knlGS:
[  219.816502] CS:  e033 DS:  ES:  CR0: 80050033
[  219.816505] CR2: 0010 CR3: 3d20c000 CR4:
00042660
[  219.816509] Stack:
[  219.816511]  8800038f8000 8800038f8030 880003ae3400
88003da9fc58
[  219.816516]  805fe78b 8800038f8000 88003bb82c40
88003da9fc80
[  219.816521]  805ff587 8800038f81a0 8800038f8190
880003ae3400
[  219.816527] Call Trace:
[  219.816533]  [805fe78b]
scsi_destroy_command_freelist+0x5b/0x60
[  219.816538]  [805ff587] scsi_host_dev_release+0x97/0xe0
[  219.816543]  [805dd71d] device_release+0x2d/0xa0
[  219.816548]  [804dbec7] kobject_cleanup+0x77/0x1b0
[  219.816553]  [804dbd70] kobject_put+0x30/0x60
[  219.816556]  [805dd9e2] put_device+0x12/0x20
[  219.816560]  [805ff490] scsi_host_put+0x10/0x20
[  219.816565]  [a01a2042] scsifront_free+0x42/0x90
[xen_scsifront]
[  219.816569]  [a01a20ad] scsifront_remove+0x1d/0x50
[xen_scsifront]
[  219.816576]  [805a4ee0] xenbus_dev_remove+0x50/0xa0
[  219.816580]  [805e1a8a] __device_release_driver+0x7a/0xf0
[  219.816584]  [805e1b1e] device_release_driver+0x1e/0x30
[  219.816588]  [805e1440] bus_remove_device+0x100/0x180
[  219.816593]  [805ddef1] device_del+0x121/0x1b0
[  219.816596]  [805ddf99] device_unregister+0x19/0x60
[  219.816601]  [805a56be] xenbus_dev_changed+0x9e/0x1e0
[  219.816606]  [8079d695] ?
_raw_spin_unlock_irqrestore+0x25/0x50
[  219.816611]  [805a41d0] ? unregister_xenbus_watch+0x200/0x200
[  219.816615]  [805a7420] frontend_changed+0x20/0x50
[  219.816619]  [805a426f] xenwatch_thread+0x9f/0x160
[  219.816624]  [802a50f0] ? prepare_to_wait_event+0xf0/0xf0
[  219.816628]  [8028485d] kthread+0xcd/0xf0
[  219.816633]  [80284790] ? kthread_create_on_node+0x170/0x170
[  219.816638]  [8079dcbc] ret_from_fork+0x7c/0xb0
[  219.816642]  [80284790] ? kthread_create_on_node+0x170/0x170
[  219.816645] Code: 8b af c0 00 00 00 48 c7 c7 c0 08 c7 80 e8 d1 e0 19
00 49 8b 84 24 c0 00 00 00 8b 90 48 01 00 00 85 d2 74 2f 48 8b 98 50 01
00 00 8b 43 10 85 c0 74 68 83 e8 01 85 c0 89 43 10 74 37 48 c7 c7 c0
[  219.816732] RIP  [805fdcf8] scsi_put_host_cmd_pool+0x38/0xb0
[  219.816747]  RSP 88003da9fc20
[  219.816750] CR2: 0010
[  219.816753] ---[ end trace c6915ea21a3d05f7 ]---

I should mention I've specified .cmd_len in the scsi_host_template. The
only other driver doing this seems to be virtio_scsi.c, so I assume the
same problem could occur with passed-through SCSI devices under KVM...


After looking into the code the reason seems to be rather obvious:

scsi_find_host_cmd_pool() returns shost-hostt-cmd_pool if .cmd_size is
set. But shost-hostt-cmd_pool is never set

Re: Oops in scsi_put_host_cmd_pool

[Juergen Gross]

On 08/01/2014 09:05 AM, James Bottomley wrote:

On Fri, 2014-08-01 at 08:02 +0200, Juergen Gross wrote:

On 08/01/2014 07:41 AM, Juergen Gross wrote:

During test of Xen pvSCSI frontend module I found the following issue:

When unplugging a passed-through SCSI-device the SCSI Host is removed.
When calling the final scsi_host_put() from the driver an Oops is
happening:

[  219.816292] (file=drivers/scsi/xen-scsifront.c, line=808)
scsifront_remove: device/vscsi/1 removed
[  219.816371] BUG: unable to handle kernel NULL pointer dereference at
0010
[  219.816380] IP: [805fdcf8] scsi_put_host_cmd_pool+0x38/0xb0
[  219.816390] PGD 3bd60067 PUD 3d353067 PMD 0
[  219.816396] Oops:  [#1] SMP
[  219.816400] Modules linked in: nls_utf8 sr_mod cdrom xen_scsifront
xt_pkttype xt_LOG xt_limit af_packet ip6t_REJECT xt_tcpudp
nf_conntrack_ipv6 nf_defrag_ipv6 ip6table_raw ipt_REJECT iptable_raw
xt_CT iptable_filter ip6table_mangle nf_conntrack_netbios_ns
nf_conntrack_broadcast nf_conntrack_ipv4 nf_defrag_ipv4 ip_tables
xt_conntrack nf_conntrack ip6table_filter ip6_tables x_tables
x86_pkg_temp_thermal thermal_sys coretemp hwmon crc32_pclmul
crc32c_intel ghash_clmulni_intel aesni_intel ablk_helper cryptd lrw
gf128mul glue_helper aes_x86_64 microcode pcspkr sg dm_mod autofs4
scsi_dh_alua scsi_dh_emc scsi_dh_rdac scsi_dh_hp_sw scsi_dh xen_blkfront
xen_netfront
[  219.816458] CPU: 0 PID: 23 Comm: xenwatch Not tainted
3.16.0-rc6-11-xen+ #66
[  219.816463] task: 88003da985d0 ti: 88003da9c000 task.ti:
88003da9c000
[  219.816467] RIP: e030:[805fdcf8]  [805fdcf8]
scsi_put_host_cmd_pool+0x38/0xb0
[  219.816474] RSP: e02b:88003da9fc20  EFLAGS: 00010202
[  219.816477] RAX: a01a50c0 RBX:  RCX:
0003
[  219.816481] RDX: 0240 RSI: 88003d242b80 RDI:
80c708c0
[  219.816485] RBP: 88003da9fc38 R08: 4f7e974a31ed0290 R09:

[  219.816488] R10: 7ff0 R11: 0001 R12:
8800038f8000
[  219.816491] R13: a01a50c0 R14:  R15:

[  219.816498] FS:  7fe2e2eeb700() GS:88003f80()
knlGS:
[  219.816502] CS:  e033 DS:  ES:  CR0: 80050033
[  219.816505] CR2: 0010 CR3: 3d20c000 CR4:
00042660
[  219.816509] Stack:
[  219.816511]  8800038f8000 8800038f8030 880003ae3400
88003da9fc58
[  219.816516]  805fe78b 8800038f8000 88003bb82c40
88003da9fc80
[  219.816521]  805ff587 8800038f81a0 8800038f8190
880003ae3400
[  219.816527] Call Trace:
[  219.816533]  [805fe78b]
scsi_destroy_command_freelist+0x5b/0x60
[  219.816538]  [805ff587] scsi_host_dev_release+0x97/0xe0
[  219.816543]  [805dd71d] device_release+0x2d/0xa0
[  219.816548]  [804dbec7] kobject_cleanup+0x77/0x1b0
[  219.816553]  [804dbd70] kobject_put+0x30/0x60
[  219.816556]  [805dd9e2] put_device+0x12/0x20
[  219.816560]  [805ff490] scsi_host_put+0x10/0x20
[  219.816565]  [a01a2042] scsifront_free+0x42/0x90
[xen_scsifront]
[  219.816569]  [a01a20ad] scsifront_remove+0x1d/0x50
[xen_scsifront]
[  219.816576]  [805a4ee0] xenbus_dev_remove+0x50/0xa0
[  219.816580]  [805e1a8a] __device_release_driver+0x7a/0xf0
[  219.816584]  [805e1b1e] device_release_driver+0x1e/0x30
[  219.816588]  [805e1440] bus_remove_device+0x100/0x180
[  219.816593]  [805ddef1] device_del+0x121/0x1b0
[  219.816596]  [805ddf99] device_unregister+0x19/0x60
[  219.816601]  [805a56be] xenbus_dev_changed+0x9e/0x1e0
[  219.816606]  [8079d695] ?
_raw_spin_unlock_irqrestore+0x25/0x50
[  219.816611]  [805a41d0] ? unregister_xenbus_watch+0x200/0x200
[  219.816615]  [805a7420] frontend_changed+0x20/0x50
[  219.816619]  [805a426f] xenwatch_thread+0x9f/0x160
[  219.816624]  [802a50f0] ? prepare_to_wait_event+0xf0/0xf0
[  219.816628]  [8028485d] kthread+0xcd/0xf0
[  219.816633]  [80284790] ? kthread_create_on_node+0x170/0x170
[  219.816638]  [8079dcbc] ret_from_fork+0x7c/0xb0
[  219.816642]  [80284790] ? kthread_create_on_node+0x170/0x170
[  219.816645] Code: 8b af c0 00 00 00 48 c7 c7 c0 08 c7 80 e8 d1 e0 19
00 49 8b 84 24 c0 00 00 00 8b 90 48 01 00 00 85 d2 74 2f 48 8b 98 50 01
00 00 8b 43 10 85 c0 74 68 83 e8 01 85 c0 89 43 10 74 37 48 c7 c7 c0
[  219.816732] RIP  [805fdcf8] scsi_put_host_cmd_pool+0x38/0xb0
[  219.816747]  RSP 88003da9fc20
[  219.816750] CR2: 0010
[  219.816753] ---[ end trace c6915ea21a3d05f7 ]---

I should mention I've specified .cmd_len in the scsi_host_template. The
only other driver doing this seems to be virtio_scsi.c, so I assume the
same problem could occur with passed-through SCSI devices under KVM...


After looking into the code the reason seems to be rather obvious

Re: [Xen-devel] [PATCH V2 2/4] Introduce xen-scsifront module

[Juergen Gross]

On 08/01/2014 02:08 PM, Christoph Hellwig wrote:

On Wed, Jul 30, 2014 at 06:53:59AM +0200, Juergen Gross wrote:

Hmm, I looked into scsi_add_device(). It seems as if the caller can't
distinguish between a new created and an already existing device. Am I
missing something?


That's right.  If you need that I still think it's better to add a
variant of scsi_add_device helping you with that.


I'm open to that solution.

Do you have preferences how to do it (IOW: can you give me a hint)?


The race is not existing: scsi_add_device() (and scsi_remove_device()
as well) for this scsi_host is called in scsifront_do_lun_hotplug()
only, and this function is always called in the same thread (xenbus
watch). A comment seems to be a good idea.


Do you disable scanning through procfs and sysfs as well?


No, I don't.

OTOH I don't think I see the problem. What could go wrong? Either
scsi_device_lookup() does find an existing device, then I refuse to add
it again. If I don't find it, it will be added. I can't see how any harm
would be done in either case when the device is added/removed between
the check and the action. What am I missing?

Juergen
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Oops in scsi_put_host_cmd_pool

[Juergen Gross]

During test of Xen pvSCSI frontend module I found the following issue:

When unplugging a passed-through SCSI-device the SCSI Host is removed.
When calling the final scsi_host_put() from the driver an Oops is
happening:

[  219.816292] (file=drivers/scsi/xen-scsifront.c, line=808) 
scsifront_remove: device/vscsi/1 removed
[  219.816371] BUG: unable to handle kernel NULL pointer dereference at 
0010

[  219.816380] IP: [805fdcf8] scsi_put_host_cmd_pool+0x38/0xb0
[  219.816390] PGD 3bd60067 PUD 3d353067 PMD 0
[  219.816396] Oops:  [#1] SMP
[  219.816400] Modules linked in: nls_utf8 sr_mod cdrom xen_scsifront 
xt_pkttype xt_LOG xt_limit af_packet ip6t_REJECT xt_tcpudp 
nf_conntrack_ipv6 nf_defrag_ipv6 ip6table_raw ipt_REJECT iptable_raw 
xt_CT iptable_filter ip6table_mangle nf_conntrack_netbios_ns 
nf_conntrack_broadcast nf_conntrack_ipv4 nf_defrag_ipv4 ip_tables 
xt_conntrack nf_conntrack ip6table_filter ip6_tables x_tables 
x86_pkg_temp_thermal thermal_sys coretemp hwmon crc32_pclmul 
crc32c_intel ghash_clmulni_intel aesni_intel ablk_helper cryptd lrw 
gf128mul glue_helper aes_x86_64 microcode pcspkr sg dm_mod autofs4 
scsi_dh_alua scsi_dh_emc scsi_dh_rdac scsi_dh_hp_sw scsi_dh xen_blkfront 
xen_netfront
[  219.816458] CPU: 0 PID: 23 Comm: xenwatch Not tainted 
3.16.0-rc6-11-xen+ #66
[  219.816463] task: 88003da985d0 ti: 88003da9c000 task.ti: 
88003da9c000
[  219.816467] RIP: e030:[805fdcf8]  [805fdcf8] 
scsi_put_host_cmd_pool+0x38/0xb0

[  219.816474] RSP: e02b:88003da9fc20  EFLAGS: 00010202
[  219.816477] RAX: a01a50c0 RBX:  RCX: 
0003
[  219.816481] RDX: 0240 RSI: 88003d242b80 RDI: 
80c708c0
[  219.816485] RBP: 88003da9fc38 R08: 4f7e974a31ed0290 R09: 

[  219.816488] R10: 7ff0 R11: 0001 R12: 
8800038f8000
[  219.816491] R13: a01a50c0 R14:  R15: 

[  219.816498] FS:  7fe2e2eeb700() GS:88003f80() 
knlGS:

[  219.816502] CS:  e033 DS:  ES:  CR0: 80050033
[  219.816505] CR2: 0010 CR3: 3d20c000 CR4: 
00042660

[  219.816509] Stack:
[  219.816511]  8800038f8000 8800038f8030 880003ae3400 
88003da9fc58
[  219.816516]  805fe78b 8800038f8000 88003bb82c40 
88003da9fc80
[  219.816521]  805ff587 8800038f81a0 8800038f8190 
880003ae3400

[  219.816527] Call Trace:
[  219.816533]  [805fe78b] scsi_destroy_command_freelist+0x5b/0x60
[  219.816538]  [805ff587] scsi_host_dev_release+0x97/0xe0
[  219.816543]  [805dd71d] device_release+0x2d/0xa0
[  219.816548]  [804dbec7] kobject_cleanup+0x77/0x1b0
[  219.816553]  [804dbd70] kobject_put+0x30/0x60
[  219.816556]  [805dd9e2] put_device+0x12/0x20
[  219.816560]  [805ff490] scsi_host_put+0x10/0x20
[  219.816565]  [a01a2042] scsifront_free+0x42/0x90 
[xen_scsifront]
[  219.816569]  [a01a20ad] scsifront_remove+0x1d/0x50 
[xen_scsifront]

[  219.816576]  [805a4ee0] xenbus_dev_remove+0x50/0xa0
[  219.816580]  [805e1a8a] __device_release_driver+0x7a/0xf0
[  219.816584]  [805e1b1e] device_release_driver+0x1e/0x30
[  219.816588]  [805e1440] bus_remove_device+0x100/0x180
[  219.816593]  [805ddef1] device_del+0x121/0x1b0
[  219.816596]  [805ddf99] device_unregister+0x19/0x60
[  219.816601]  [805a56be] xenbus_dev_changed+0x9e/0x1e0
[  219.816606]  [8079d695] ? _raw_spin_unlock_irqrestore+0x25/0x50
[  219.816611]  [805a41d0] ? unregister_xenbus_watch+0x200/0x200
[  219.816615]  [805a7420] frontend_changed+0x20/0x50
[  219.816619]  [805a426f] xenwatch_thread+0x9f/0x160
[  219.816624]  [802a50f0] ? prepare_to_wait_event+0xf0/0xf0
[  219.816628]  [8028485d] kthread+0xcd/0xf0
[  219.816633]  [80284790] ? kthread_create_on_node+0x170/0x170
[  219.816638]  [8079dcbc] ret_from_fork+0x7c/0xb0
[  219.816642]  [80284790] ? kthread_create_on_node+0x170/0x170
[  219.816645] Code: 8b af c0 00 00 00 48 c7 c7 c0 08 c7 80 e8 d1 e0 19 
00 49 8b 84 24 c0 00 00 00 8b 90 48 01 00 00 85 d2 74 2f 48 8b 98 50 01 
00 00 8b 43 10 85 c0 74 68 83 e8 01 85 c0 89 43 10 74 37 48 c7 c7 c0

[  219.816732] RIP  [805fdcf8] scsi_put_host_cmd_pool+0x38/0xb0
[  219.816747]  RSP 88003da9fc20
[  219.816750] CR2: 0010
[  219.816753] ---[ end trace c6915ea21a3d05f7 ]---

I should mention I've specified .cmd_len in the scsi_host_template. The
only other driver doing this seems to be virtio_scsi.c, so I assume the
same problem could occur with passed-through SCSI devices under KVM...


Juergen
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  

Re: [Xen-devel] [PATCH V2 1/4] Add XEN pvSCSI protocol description

[Juergen Gross]

On 07/28/2014 04:28 PM, David Vrabel wrote:

On 25/07/14 12:37, jgr...@suse.com wrote:

From: Juergen Gross jgr...@suse.com

Add the definition of pvSCSI protocol used between the pvSCSI frontend in a
XEN domU and the pvSCSI backend in a XEN driver domain (usually Dom0).

This header was originally provided by Fujitsu for XEN based on Linux 2.6.18.
Changes are:
- added comment
- adapt to Linux style guide
- add support for larger SG-lists by putting them in an own granted page
- remove stale definitions


This is a protocol extension and needs to be reviewed separately, and
the Xen header needs updating as well.


Sure. The review can start right now. Updating the Xen header will be
done as soon as the drivers are accepted in Linux.


The documentation for this protocol is rather lacking.  In particular it
is missing the connection states.


Regarding documentation: I'll add some.

Regarding the connection states: I'd rather move the description in
blkif.h to xenbus.h than duplicating it in vscsiif.h. What do you think?


Juergen
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Xen-devel] [PATCH V2 2/4] Introduce xen-scsifront module

[Juergen Gross]

On 07/29/2014 03:53 PM, Christoph Hellwig wrote:

+ * Patched to support 2TB drives
+ * 2010, Samuel Kvasnica, IMS Nanofabrication AG
+ */


This doesn't really belong into the top of the file comment and should
be moved to the patch description.


Okay.


+
+#include linux/version.h


not needed.


Okay.


+static int get_id_from_freelist(struct vscsifrnt_info *info)
+{
+   unsigned long flags;
+   uint32_t free;
+
+   spin_lock_irqsave(info-shadow_lock, flags);
+
+   free = info-shadow_free;
+   BUG_ON(free = VSCSIIF_MAX_REQS);
+   info-shadow_free = info-shadow[free].next_free;
+   info-shadow[free].next_free = VSCSIIF_MAX_REQS;
+   info-shadow[free].wait_reset = 0;
+
+   spin_unlock_irqrestore(info-shadow_lock, flags);
+
+   return free;


Is the shadow array exposed to the hypervisor?  If not it might be a
better idea to just allocate the driver private data with the command
by setting the cmd_size field in the host template.  Take a look
at the virtio_scsi driver in recent kernels for an example.


Ah, okay. I'll still need an array for managing the request ids, but
this will be much smaller...


+static irqreturn_t scsifront_intr(int irq, void *dev_id)
+{
+   scsifront_notify_work((struct vscsifrnt_info *)dev_id);
+   return IRQ_HANDLED;


Seems like you should simply use threaded interrupt handlers.  If that
doesn't work for you at least this should use a workqueue instead of
manually reimplementing it using a kthread.


A threaded interrupt handler seems to be a good idea. I'll check if it
is working.


+/* debug printk to identify more missing scsi commands
+   shost_printk(KERN_INFO scsicmd: , sc-device-host,
+len=%u %#x,%#x,%#x,%#x,%#x,%#x,%#x,%#x,%#x,%#x\n,
+sc-cmd_len, sc-cmnd[0], sc-cmnd[1],
+sc-cmnd[2], sc-cmnd[3], sc-cmnd[4], sc-cmnd[5],
+sc-cmnd[6], sc-cmnd[7], sc-cmnd[8], sc-cmnd[9]);
+*/


We already have pretty good ways to debug this in the midlayer, I don't
think this is needed.


Okay, I'll remove it.


+   spin_lock_irqsave(shost-host_lock, flags);


What's actually protected by the host lock in this driver?  It has
ample of driver-local locking, so avoiding to touch the host lock
would be a clear win.


The host_lock protects the ring buffers used to communicate with Dom0.
I think this is the correct lock for this operation, as there is exactly
one such ring buffer pair for each host structure allocated in
scsifront_probe().


+   scsi_cmd_get_serial(shost, sc);


What do you need the serial number for?  Generally only legacy drivers
use it.


Historical reasons :-)
I'll remove it.


+static int scsifront_eh_abort_handler(struct scsi_cmnd *sc)
+{
+   return scsifront_action_handler(sc, VSCSIIF_ACT_SCSI_ABORT);
+}


The amount of waiting that your command abort handler does inside
scsifront_action_handler looks wrong to me.  Can you explain the
theory of operation for error handling in this driver?  How much
of error recovery is supposed to be handled by the hypervisor,
and much is supposed to be handle in the intiator side driver?


The initiator side just forwards the abort/reset request to Dom0
and waits for an answer (okay or error).


+static void scsifront_free(struct vscsifrnt_info *info)
+{
+   struct Scsi_Host *host = info-host;
+
+   if (host-shost_state != SHOST_DEL)
+   scsi_remove_host(info-host);


A driver really shouldn't look at shost_state like this.  Is there some
SCM history explaining where this comes from?


I'll investigate it. It was added soon after the first version was
available, but I haven't found a reason yet.


+   switch (op) {
+   case VSCSIFRONT_OP_ADD_LUN:
+   if (device_state == XenbusStateInitialised) {
+   sdev = scsi_device_lookup(info-host, chn, tgt,
+ lun);
+   if (sdev) {
+   dev_err(dev-dev,
+   Device already in use.\n);
+   scsi_device_put(sdev);
+   xenbus_printf(XBT_NIL, dev-nodename,
+ state_str, %d,
+ XenbusStateClosed);
+   } else {
+   scsi_add_device(info-host, chn, tgt,
+   lun);
+   xenbus_printf(XBT_NIL, dev-nodename,
+ state_str, %d,
+ XenbusStateConnected);
+   }
+   }
+   break;


scsi_add_device handles an already existing device just 

Re: [Xen-devel] [PATCH V2 2/4] Introduce xen-scsifront module

[Juergen Gross]

On 07/29/2014 04:57 PM, Juergen Gross wrote:

On 07/29/2014 03:53 PM, Christoph Hellwig wrote:



+switch (op) {
+case VSCSIFRONT_OP_ADD_LUN:
+if (device_state == XenbusStateInitialised) {
+sdev = scsi_device_lookup(info-host, chn, tgt,
+  lun);
+if (sdev) {
+dev_err(dev-dev,
+Device already in use.\n);
+scsi_device_put(sdev);
+xenbus_printf(XBT_NIL, dev-nodename,
+  state_str, %d,
+  XenbusStateClosed);
+} else {
+scsi_add_device(info-host, chn, tgt,
+lun);
+xenbus_printf(XBT_NIL, dev-nodename,
+  state_str, %d,
+  XenbusStateConnected);
+}
+}
+break;


scsi_add_device handles an already existing device just fine, and unlike
this construct isn't racy.


Okay. I'll change it.


Hmm, I looked into scsi_add_device(). It seems as if the caller can't
distinguish between a new created and an already existing device. Am I
missing something?

The race is not existing: scsi_add_device() (and scsi_remove_device()
as well) for this scsi_host is called in scsifront_do_lun_hotplug()
only, and this function is always called in the same thread (xenbus
watch). A comment seems to be a good idea.


Juergen
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V2 0/4] Add XEN pvSCSI support

[Juergen Gross]

On 07/25/2014 07:38 PM, Konrad Rzeszutek Wilk wrote:

On Fri, Jul 25, 2014 at 01:37:29PM +0200, jgr...@suse.com wrote:

This series adds XEN pvSCSI support. With pvSCSI it is possible to use physical
SCSI devices from a XEN domain.

The support consists of a backend in the privileged Domain-0 doing the real
I/O and a frontend in the unprivileged domU passing I/O-requests to the backend.



About the question that Christopher Hellwig asked - was that ever answered?


Sure. That's the first item under Changes in V2.




The code is taken (and adapted) from the original pvSCSI implementation done
for Linux 2.6 in 2008 by Fujitsu.

[PATCH V2 1/4] Add XEN pvSCSI protocol description
[PATCH V2 2/4] Introduce xen-scsifront module
[PATCH V2 3/4] Introduce XEN scsiback module
[PATCH V2 4/4] add xen pvscsi maintainer

Changes in V2:
- use core target infrastructure by backend instead of pure SCSI passthrough
- add support for larger SG lists by putting them in grant page(s)
- add command abort capability




--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Re: [PATCH 3/4] Introduce XEN scsiback module

[Juergen Gross]

On 06/28/2014 08:09 PM, Christoph Hellwig wrote:

On Fri, Jun 27, 2014 at 04:34:35PM +0200, jgr...@suse.com wrote:

From: Juergen Gross jgr...@suse.com

Introduces the XEN pvSCSI backend. With pvSCSI it is possible for a XEN domU
to issue SCSI commands to a SCSI LUN assigned to that domU. The SCSI commands
are passed to the pvSCSI backend in a driver domain (usually Dom0) which is
owner of the physical device. This allows e.g. to use SCSI tape drives in a
XEN domU.


This should be written against the generic target core infrastructure
in drivers/target, instead of introducing another simplistic pass
through only SCSI target.
.



Just to be sure: you mean something like a combined version of
tcm_vhost and virtio_scsi?


Juergen
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Xen-devel] [PATCH 1/4] Add XEN pvSCSI protocol description

[Juergen Gross]

On 06/27/2014 07:11 PM, Konrad Rzeszutek Wilk wrote:

On Fri, Jun 27, 2014 at 04:34:33PM +0200, jgr...@suse.com wrote:

From: Juergen Gross jgr...@suse.com

Add the definition of pvSCSI protocol used between the pvSCSI frontend in a
XEN domU and the pvSCSI backend in a XEN driver domain (usually Dom0).

This header was originally provided by Fujitsu for XEN based on Linux 2.6.18.
Changes are:
- added comment
- adapt to Linux style guide

Signed-off-by: Juergen Gross jgr...@suse.com
---
  include/xen/interface/io/vscsiif.h | 110 +
  1 file changed, 110 insertions(+)
  create mode 100644 include/xen/interface/io/vscsiif.h

diff --git a/include/xen/interface/io/vscsiif.h 
b/include/xen/interface/io/vscsiif.h
new file mode 100644
index 000..f8420f3
--- /dev/null
+++ b/include/xen/interface/io/vscsiif.h
@@ -0,0 +1,110 @@
+/**
+ * vscsiif.h
+ *
+ * Based on the blkif.h code.
+ *
+ * This interface is to be regarded as a stable API between XEN domains
+ * running potentially different Linux kernel versions.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the Software), to
+ * deal in the Software without restriction, including without limitation the
+ * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
+ * sell copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED AS IS, WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+ * DEALINGS IN THE SOFTWARE.
+ *
+ * Copyright(c) FUJITSU Limited 2008.
+ */
+
+#ifndef __XEN__PUBLIC_IO_SCSI_H__
+#define __XEN__PUBLIC_IO_SCSI_H__
+
+#include ring.h
+#include ../grant_table.h
+
+/* commands between backend and frontend */
+#define VSCSIIF_ACT_SCSI_CDB   1   /* SCSI CDB command */
+#define VSCSIIF_ACT_SCSI_ABORT 2   /* SCSI Device(Lun) Abort*/
+#define VSCSIIF_ACT_SCSI_RESET 3   /* SCSI Device(Lun) Reset*/
+#define VSCSIIF_ACT_SCSI_SG_PRESET 4   /* Preset SG elements */
+
+/*
+ * Maximum scatter/gather segments per request.
+ *
+ * Considering balance between allocating at least 16 vscsiif_request
+ * structures on one page (4096 bytes) and the number of scatter/gather
+ * elements needed, we decided to use 26 as a magic number.
+ */
+#define VSCSIIF_SG_TABLESIZE   26
+
+/*
+ * based on Linux kernel 2.6.18


This being a bit more .. new, - do these sizes make sense anymore?
Should they be extended a bit? Or have support for using the
old ones (as default) and then negotiate new sizes with the kernel?
(If of course there is a difference?)


The sizes are still the same. Additionally the sizes can't be changed
without breaking the interface (the request structure must not change
size). Added a comment.


+ */
+#define VSCSIIF_MAX_COMMAND_SIZE   16
+#define VSCSIIF_SENSE_BUFFERSIZE   96
+
+struct scsiif_request_segment {
+   grant_ref_t gref;
+   uint16_t offset;
+   uint16_t length;
+};
+typedef struct scsiif_request_segment vscsiif_segment_t;


No typedefs please.


Okay.


+
+struct vscsiif_request {
+   uint16_t rqid;  /* private guest value, echoed in resp  */
+   uint8_t act;/* command between backend and frontend */
+   uint8_t cmd_len;
+
+   uint8_t cmnd[VSCSIIF_MAX_COMMAND_SIZE];
+   uint16_t timeout_per_command;   /* The command is issued by twice
+  the value in Backend. */
+   uint16_t channel, id, lun;
+   uint16_t padding;
+   uint8_t sc_data_direction;  /* for DMA_TO_DEVICE(1)
+  DMA_FROM_DEVICE(2)
+  DMA_NONE(3) requests */
+   uint8_t nr_segments;/* Number of pieces of scatter-gather */
+
+   vscsiif_segment_t seg[VSCSIIF_SG_TABLESIZE];
+   uint32_t reserved[3];
+};


Could you add a comment saying what the size should be. Just in case
somebody extends this in the future - they will know the limits.


Okay.




+typedef struct vscsiif_request vscsiif_request_t;


Ditto.

+
+#define VSCSIIF_SG_LIST_SIZE ((sizeof(vscsiif_request_t) - 4) /
\
+   sizeof(vscsiif_segment_t))


That -4 ought to have a comment. In case

Re: [Xen-devel] [PATCH 1/4] Add XEN pvSCSI protocol description

[Juergen Gross]

On 06/30/2014 01:02 PM, Jan Beulich wrote:

On 27.06.14 at 19:11, konrad.w...@oracle.com wrote:

On Fri, Jun 27, 2014 at 04:34:33PM +0200, jgr...@suse.com wrote:

+/*
+ * Maximum scatter/gather segments per request.
+ *
+ * Considering balance between allocating at least 16 vscsiif_request
+ * structures on one page (4096 bytes) and the number of scatter/gather
+ * elements needed, we decided to use 26 as a magic number.
+ */
+#define VSCSIIF_SG_TABLESIZE   26
+
+/*
+ * based on Linux kernel 2.6.18


This being a bit more .. new, - do these sizes make sense anymore?
Should they be extended a bit? Or have support for using the
old ones (as default) and then negotiate new sizes with the kernel?
(If of course there is a difference?)


As Jürgen already said (and as you should have noticed yourself) -
this is an interface definition that we can't just change. Negotiation
of larger counts is an option, which is what VSCSIIF_ACT_SCSI_SG_PRESET
is intended for (the implementation of which isn't part of this patchset
afaics, but could be made available on top of it).


I don't think this is the proper way to handle larger SG lists. The
VSCSIIF_ACT_SCSI_SG_PRESET option would just bump the maximum length
up to 31 (currently 26). Or was it thought to be incremental (multiple
presets for one request)? I'd rather add a way to specify SG lists
residing in an own (granted) page (or even multiple pages). This would
allow 512*26 SG entries without having to change the request structure.
The capability to handle this feature could be indicated via xenstore.


Most if not all of your other comments are a little questionable too
in this context - any parts you aren't happy about would really
better be addressed towards the canonical header in the Xen tree.
(I know you had other interface headers diverge too in their Linux
incarnation, but personally I don't think this is an appropriate thing
to do, perhaps apart for pure coding style adjustments.)



Juergen

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Xen-devel] [PATCH 2/4] Introduce xen-scsifront module

[Juergen Gross]

On 06/30/2014 03:35 PM, David Vrabel wrote:

On 27/06/14 15:34, jgr...@suse.com wrote:


+/* .resume = scsifront_resume, */


Following on from the recent discussion on migration with front/backends
I thought I'd review how this new driver does it.


New is an attribute I wouldn't use in this case. The driver was
introduced in 2008, this is just a rework to be able to put it in
the upstream kernel.


Is there an expectation that a VM with a PV scsi device cannot be
restored or migrated?



It has been so in the past.

To change this is on my todo list, but that's not top priority.


Juergen
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html