Skip to contentSkip to conversation

In sg_io_v4 what to use as flag for "queue at_head"

Q Boaz Harrosh 3 participants 8 messages Jan 19, 2009 – Jan 21, 2009 SCSI

lore
Share & tools
lore

Boaz Harrosh

author
#0 lore raw

Douglas Gilbert <[email protected]> wrote ...

Quoted 21 lines
struct sg_io_v4 {
	__s32 guard;		/* [i] 'Q' to differentiate from v3 */
	__u32 protocol;		/* [i] 0 -> SCSI , .... */
	__u32 subprotocol;	/* [i] 0 -> SCSI command, 1 -> SCSI task
				   management function, .... */

	__u32 request_len;	/* [i] in bytes */
	__u64 request;		/* [i], [*i] {SCSI: cdb} */
	__u64 request_tag;	/* [i] {SCSI: task tag (only if flagged)} */
	__u32 request_attr;	/* [i] {SCSI: task attribute} */
	__u32 request_priority;	/* [i] {SCSI: task priority} */
	__u32 request_extra;	/* [i] {spare, for padding} */
	__u32 max_response_len;	/* [i] in bytes */
	__u64 response;		/* [i], [*o] {SCSI: (auto)sense data} */

        /* "dout_": data out (to device); "din_": data in (from device) */
	__u32 dout_iovec_count;	/* [i] 0 -> "flat" dout transfer else
				   dout_xfer points to array of iovec */
	__u32 dout_xfer_len;	/* [i] bytes to be transferred to device */
	__u32 din_iovec_count;	/* [i] 0 -> "flat" din transfer */
	__u32 din_xfer_len;	/* [i] bytes to be transferred from device */
Quoted 8 lines
	__u64 dout_xferp;	/* [i], [*i] */
	__u64 din_xferp;	/* [i], [*o] */

	__u32 timeout;		/* [i] units: millisecond */
	__u32 flags;		/* [i] bit mask */
	__u64 usr_ptr;		/* [i->o] unused internally */
	__u32 spare_in;		/* [i] */
      ...

Currently because of a twisted accident sg and bsg queue their async request with the "at_head" flag set when calling blk_execute_rq_nowait.

Since sg_io_v4 is new and as plenty of possible choices, what would be the best place to put the "at_head" flag?

Douglas ?
What was your intention with the above:
__u32 request_priority; /* [i] {SCSI: task priority} */

[And while at it, what was:
__u32 request_attr; /* [i] {SCSI: task attribute} */
]

Should I define (bsg.h):
enum {
BSG_AT_HEAD = 0, /* compatible with old code */ BSG_AT_TAIL,
}
And put it in request_priority

or define:
#define SG_FLAG_AT_TAIL 0x10 /* See sg.h for more/other flags (sg.h flags are ignored by bsg) */

And OR that into the old flags member that is otherwise unused by current bsg code

Thanks
Boaz

#1 lore raw
Quoted 37 lines · Boaz Harrosh

Douglas Gilbert <[email protected]> wrote ...

Quoted 21 lines
struct sg_io_v4 {
	__s32 guard;		/* [i] 'Q' to differentiate from v3 */
	__u32 protocol;		/* [i] 0 -> SCSI , .... */
	__u32 subprotocol;	/* [i] 0 -> SCSI command, 1 -> SCSI task
				   management function, .... */

	__u32 request_len;	/* [i] in bytes */
	__u64 request;		/* [i], [*i] {SCSI: cdb} */
	__u64 request_tag;	/* [i] {SCSI: task tag (only if flagged)} */
	__u32 request_attr;	/* [i] {SCSI: task attribute} */
	__u32 request_priority;	/* [i] {SCSI: task priority} */
	__u32 request_extra;	/* [i] {spare, for padding} */
	__u32 max_response_len;	/* [i] in bytes */
	__u64 response;		/* [i], [*o] {SCSI: (auto)sense data} */

        /* "dout_": data out (to device); "din_": data in (from device) */
	__u32 dout_iovec_count;	/* [i] 0 -> "flat" dout transfer else
				   dout_xfer points to array of iovec */
	__u32 dout_xfer_len;	/* [i] bytes to be transferred to device */
	__u32 din_iovec_count;	/* [i] 0 -> "flat" din transfer */
	__u32 din_xfer_len;	/* [i] bytes to be transferred from device */
Quoted 8 lines
	__u64 dout_xferp;	/* [i], [*i] */
	__u64 din_xferp;	/* [i], [*o] */

	__u32 timeout;		/* [i] units: millisecond */
	__u32 flags;		/* [i] bit mask */
	__u64 usr_ptr;		/* [i->o] unused internally */
	__u32 spare_in;		/* [i] */
      ...

Currently because of a twisted accident sg and bsg queue their async request with the "at_head" flag set when calling blk_execute_rq_nowait.

Since sg_io_v4 is new and as plenty of possible choices, what would be the best place to put the "at_head" flag?

Quoted 6 lines

Douglas ?
What was your intention with the above:
__u32 request_priority; /* [i] {SCSI: task priority} */

[And while at it, what was:
__u32 request_attr; /* [i] {SCSI: task attribute} */

Yes, task attribute. That is the term used by SAM-4 (and earlier). Unfortunately SAM-4 defines the names but not values; those values are left up to the transport. That presents a problem for a transport agnostic SCSI pass-through interface like sg_io_v4.

For backward compatibility we probably need to make the value 0 mean "head of queue" (or whatever the mid-level default task attribute is for pass-through SCSI commands).

Also we probably don't have a clean pass-through of task attributes in the SCSI mid-level currently. I mean "clean" in the sense of "leave them alone" and do not act on the command completion status (e.g. TASK SET FULL).

Quoted 6 lines

Should I define (bsg.h):
enum {
BSG_AT_HEAD = 0, /* compatible with old code */ BSG_AT_TAIL,
}
And put it in request_priority

or (adding "_TA_" for task attribute):

  enum {
        BSG_TA_DEFAULT = 0,  // lk 2.4, 2.6 series: head of queue
        BSG_TA_HEAD_OF_Q = 0x1000,
        BSG_TA_SIMPLE,
        BSG_TA_ORDERED,
        BSG_TA_ACA,
        ....
Quoted 6 lines

or define:
#define SG_FLAG_AT_TAIL 0x10 /* See sg.h for more/other flags (sg.h flags are ignored by bsg) */

And OR that into the old flags member that is otherwise unused by current bsg code

I would prefer using the request_attr field.

Tomo, what do you think?

Doug Gilbert

#2 lore raw
Quoted 77 lines · Douglas Gilbert
Quoted 38 lines · Boaz Harrosh

Douglas Gilbert <[email protected]> wrote ...

Quoted 22 lines
struct sg_io_v4 {
    __s32 guard;        /* [i] 'Q' to differentiate from v3 */
    __u32 protocol;        /* [i] 0 -> SCSI , .... */
    __u32 subprotocol;    /* [i] 0 -> SCSI command, 1 -> SCSI task
                   management function, .... */

    __u32 request_len;    /* [i] in bytes */
    __u64 request;        /* [i], [*i] {SCSI: cdb} */
    __u64 request_tag;    /* [i] {SCSI: task tag (only if flagged)} */
    __u32 request_attr;    /* [i] {SCSI: task attribute} */
    __u32 request_priority;    /* [i] {SCSI: task priority} */
    __u32 request_extra;    /* [i] {spare, for padding} */
    __u32 max_response_len;    /* [i] in bytes */
    __u64 response;        /* [i], [*o] {SCSI: (auto)sense data} */

        /* "dout_": data out (to device); "din_": data in (from 
device) */
    __u32 dout_iovec_count;    /* [i] 0 -> "flat" dout transfer else
                   dout_xfer points to array of iovec */
    __u32 dout_xfer_len;    /* [i] bytes to be transferred to device */
    __u32 din_iovec_count;    /* [i] 0 -> "flat" din transfer */
    __u32 din_xfer_len;    /* [i] bytes to be transferred from device */
Quoted 8 lines
    __u64 dout_xferp;    /* [i], [*i] */
    __u64 din_xferp;    /* [i], [*o] */

    __u32 timeout;        /* [i] units: millisecond */
    __u32 flags;        /* [i] bit mask */
    __u64 usr_ptr;        /* [i->o] unused internally */
    __u32 spare_in;        /* [i] */
      ...

Currently because of a twisted accident sg and bsg queue their async request with the "at_head" flag set when calling blk_execute_rq_nowait.

Since sg_io_v4 is new and as plenty of possible choices, what would be the best place to put the "at_head" flag?

Quoted 6 lines

Douglas ?
What was your intention with the above:

     __u32 request_priority;    /* [i] {SCSI: task priority} */

[And while at it, what was:

     __u32 request_attr;    /* [i] {SCSI: task attribute} */

Yes, task attribute. That is the term used by SAM-4 (and earlier). Unfortunately SAM-4 defines the names but not values; those values are left up to the transport. That presents a problem for a transport agnostic SCSI pass-through interface like sg_io_v4.

For backward compatibility we probably need to make the value 0 mean "head of queue" (or whatever the mid-level default task attribute is for pass-through SCSI commands).

Also we probably don't have a clean pass-through of task attributes in the SCSI mid-level currently. I mean "clean" in the sense of "leave them alone" and do not act on the command completion status (e.g. TASK SET FULL).

Quoted 6 lines

Should I define (bsg.h):
enum {
BSG_AT_HEAD = 0, /* compatible with old code */ BSG_AT_TAIL,
}
And put it in request_priority

or (adding "_TA_" for task attribute):

  enum {
        BSG_TA_DEFAULT = 0,  // lk 2.4, 2.6 series: head of queue
        BSG_TA_HEAD_OF_Q = 0x1000,
        BSG_TA_SIMPLE,
        BSG_TA_ORDERED,
        BSG_TA_ACA,
        ....
Quoted 1 line
Quoted 7 lines
Quoted 5 lines

or define:
#define SG_FLAG_AT_TAIL 0x10 /* See sg.h for more/other flags (sg.h flags are ignored by bsg) */

And OR that into the old flags member that is otherwise unused by current bsg code

I would prefer using the request_attr field.

Ah, another idea: use a SG_FLAG_REQUEST_ATTR_ACTIVE flag or-ed into the flag mix. Then the transport native task attributes could be sent through the request_attr field (and request_priority field).

Doug Gilbert

author
#3 lore raw
In reply to Douglas Gilbert
Quoted 9 lines · Douglas Gilbert

or (adding "_TA_" for task attribute):

   enum {
         BSG_TA_DEFAULT = 0,  // lk 2.4, 2.6 series: head of queue
         BSG_TA_HEAD_OF_Q = 0x1000,
         BSG_TA_SIMPLE,
         BSG_TA_ORDERED,
         BSG_TA_ACA,
         ....

BSG_TA_HEAD_OF_Q I understand that's like today. What are the meaning of the other values? Anyway I only have 2 values to implement I don't want to add more values then I use, and have to comment NOT SUPPORTED next to them. When used I can add them later.

Please Note that I was asking about the at_head=0/1 of the blk_execute_rq_xxx calls. This means that it is not transport specific at all, it is a Boolean behavior common to all transports, governed by the request submission layer.
If task_attribute is something related to SAM-4 then surly that is not it, because I'm looking for a flag that is independent of scsi. If later I will need that SAM-4 thing it will be taken.

Quoted 7 lines

I would prefer using the request_attr field.

Tomo, what do you think?

Doug Gilbert

And please, you never explained, what is "request_priority" for? What was your original intention?

Thanks
Boaz

#4 lore raw
Quoted 24 lines · Boaz Harrosh
Quoted 8 lines · Douglas Gilbert

or (adding "_TA_" for task attribute):

   enum {
         BSG_TA_DEFAULT = 0,  // lk 2.4, 2.6 series: head of queue
         BSG_TA_HEAD_OF_Q = 0x1000,
         BSG_TA_SIMPLE,
         BSG_TA_ORDERED,
         BSG_TA_ACA,
         ....

BSG_TA_HEAD_OF_Q I understand that's like today. What are the meaning of the other values? Anyway I only have 2 values to implement I don't want to add more values then I use, and have to comment NOT SUPPORTED next to them. When used I can add them later.

Please Note that I was asking about the at_head=0/1 of the blk_execute_rq_xxx calls. This means that it is not transport specific at all, it is a Boolean behavior common to all transports, governed by the request submission layer.
If task_attribute is something related to SAM-4 then surly that is not it, because I'm looking for a flag that is independent of scsi. If later I will need that SAM-4 thing it will be taken.

Quoted 2 lines

I would prefer using the request_attr field.

Quoted 7 lines
Quoted 4 lines

Tomo, what do you think?

Doug Gilbert

And please, you never explained, what is "request_priority" for? What was your original intention?

Boaz,
As my comment indicates "request_priority" is "task priority" in SCSI. I should have added "... in SAM-3" because the good folks at t10 changed that to "command priority" in SAM-4 (sam4r14.pdf section 8.7). [I didn't like the term "task ..." either which is one reason why I used "request ...".]

So the "queue" that I'm talking about is the one on the device (target or LU, take your pick). A SCSI pass-through wants the thinnest possible layer between itself and the SCSI initiator, so in that context I view a queue in the scsi mid-level or a LLD as evil.

Note also that several SCSI commands have an implicit "head of queue" attribute (e.g. INQUIRY and REPORT LUNS) within the device.

Doug Gilbert

#5 lore raw
In reply to Boaz Harrosh
Quoted 11 lines · On Tue, 20 Jan 2009 09:05:41 +0200 Boaz Harrosh <[email protected]> wrote:
Quoted 9 lines · Douglas Gilbert

or (adding "_TA_" for task attribute):

   enum {
         BSG_TA_DEFAULT = 0,  // lk 2.4, 2.6 series: head of queue
         BSG_TA_HEAD_OF_Q = 0x1000,
         BSG_TA_SIMPLE,
         BSG_TA_ORDERED,
         BSG_TA_ACA,
         ....
Quoted 4 lines

BSG_TA_HEAD_OF_Q I understand that's like today. What are the meaning of the other values? Anyway I only have 2 values to implement I don't want to add more values then I use, and have to comment NOT SUPPORTED next to them. When used I can add them later.

No, as Doug said, the request_attr is for SCSI task attribute. Other protocols could use the protocol specific purpose. It's not for Linux's request queue.

Quoted 4 lines

Please Note that I was asking about the at_head=0/1 of the blk_execute_rq_xxx calls. This means that it is not transport specific at all, it is a Boolean behavior common to all transports, governed by the request submission layer.

Does 'at_head' matter? I think that blk_execute_rq_nowait inserts a request and plugs a queue either way with queue_lock held.

Quoted 7 lines

If task_attribute is something related to SAM-4 then surly that is not it, because I'm looking for a flag that is independent of scsi. If later I will need that SAM-4 thing it will be taken.

Quoted 3 lines

I would prefer using the request_attr field.

Quoted 7 lines
Quoted 4 lines

Tomo, what do you think?

Doug Gilbert

And please, you never explained, what is "request_priority" for? What was your original intention?

Wasn't it obvious? :)

#6 lore raw
In reply to Douglas Gilbert
Quoted 79 lines · On Mon, 19 Jan 2009 12:43:33 -0500 Douglas Gilbert <[email protected]>…
Quoted 37 lines · Boaz Harrosh

Douglas Gilbert <[email protected]> wrote ...

Quoted 21 lines
struct sg_io_v4 {
	__s32 guard;		/* [i] 'Q' to differentiate from v3 */
	__u32 protocol;		/* [i] 0 -> SCSI , .... */
	__u32 subprotocol;	/* [i] 0 -> SCSI command, 1 -> SCSI task
				   management function, .... */

	__u32 request_len;	/* [i] in bytes */
	__u64 request;		/* [i], [*i] {SCSI: cdb} */
	__u64 request_tag;	/* [i] {SCSI: task tag (only if flagged)} */
	__u32 request_attr;	/* [i] {SCSI: task attribute} */
	__u32 request_priority;	/* [i] {SCSI: task priority} */
	__u32 request_extra;	/* [i] {spare, for padding} */
	__u32 max_response_len;	/* [i] in bytes */
	__u64 response;		/* [i], [*o] {SCSI: (auto)sense data} */

        /* "dout_": data out (to device); "din_": data in (from device) */
	__u32 dout_iovec_count;	/* [i] 0 -> "flat" dout transfer else
				   dout_xfer points to array of iovec */
	__u32 dout_xfer_len;	/* [i] bytes to be transferred to device */
	__u32 din_iovec_count;	/* [i] 0 -> "flat" din transfer */
	__u32 din_xfer_len;	/* [i] bytes to be transferred from device */
Quoted 8 lines
	__u64 dout_xferp;	/* [i], [*i] */
	__u64 din_xferp;	/* [i], [*o] */

	__u32 timeout;		/* [i] units: millisecond */
	__u32 flags;		/* [i] bit mask */
	__u64 usr_ptr;		/* [i->o] unused internally */
	__u32 spare_in;		/* [i] */
      ...

Currently because of a twisted accident sg and bsg queue their async request with the "at_head" flag set when calling blk_execute_rq_nowait.

Since sg_io_v4 is new and as plenty of possible choices, what would be the best place to put the "at_head" flag?

Quoted 6 lines

Douglas ?
What was your intention with the above:
__u32 request_priority; /* [i] {SCSI: task priority} */

[And while at it, what was:
__u32 request_attr; /* [i] {SCSI: task attribute} */

Yes, task attribute. That is the term used by SAM-4 (and earlier). Unfortunately SAM-4 defines the names but not values; those values are left up to the transport. That presents a problem for a transport agnostic SCSI pass-through interface like sg_io_v4.

For backward compatibility we probably need to make the value 0 mean "head of queue" (or whatever the mid-level default task attribute is for pass-through SCSI commands).

Also we probably don't have a clean pass-through of task attributes in the SCSI mid-level currently. I mean "clean" in the sense of "leave them alone" and do not act on the command completion status (e.g. TASK SET FULL).

Quoted 6 lines

Should I define (bsg.h):
enum {
BSG_AT_HEAD = 0, /* compatible with old code */ BSG_AT_TAIL,
}
And put it in request_priority

or (adding "_TA_" for task attribute):

   enum {
         BSG_TA_DEFAULT = 0,  // lk 2.4, 2.6 series: head of queue
         BSG_TA_HEAD_OF_Q = 0x1000,
         BSG_TA_SIMPLE,
         BSG_TA_ORDERED,
         BSG_TA_ACA,
         ....
Quoted 4 lines

or define:
#define SG_FLAG_AT_TAIL 0x10 /* See sg.h for more/other flags (sg.h flags are ignored by bsg) */

Quoted 6 lines
Quoted 2 lines

And OR that into the old flags member that is otherwise unused by current bsg code

I would prefer using the request_attr field.

Tomo, what do you think?

It should be request_attr, as you planed. request_priority is for the task (command) priority.

I'm not sure about inventing BSG_TA_* thing. If we do that, the transport classes need to have the own function to convert BSG_TA_* to their own attribute values, iscsi_bsg_to_attr(), fc_bsg_to_attr... If there are tons of users who want task attribute support in bsg, then it's worth doing that but...

How about letting users to do the proper thing? That's is, if he wants to send a command to iSCSI devices, he needs to set iSCSI task attribute value.

author
#7 lore raw
Quoted 93 lines · FUJITA Tomonori
Quoted 72 lines · On Mon, 19 Jan 2009 12:43:33 -0500 Douglas Gilbert <[email protected]>…
Quoted 36 lines · Boaz Harrosh

Douglas Gilbert <[email protected]> wrote ...

Quoted 21 lines
struct sg_io_v4 {
	__s32 guard;		/* [i] 'Q' to differentiate from v3 */
	__u32 protocol;		/* [i] 0 -> SCSI , .... */
	__u32 subprotocol;	/* [i] 0 -> SCSI command, 1 -> SCSI task
				   management function, .... */

	__u32 request_len;	/* [i] in bytes */
	__u64 request;		/* [i], [*i] {SCSI: cdb} */
	__u64 request_tag;	/* [i] {SCSI: task tag (only if flagged)} */
	__u32 request_attr;	/* [i] {SCSI: task attribute} */
	__u32 request_priority;	/* [i] {SCSI: task priority} */
	__u32 request_extra;	/* [i] {spare, for padding} */
	__u32 max_response_len;	/* [i] in bytes */
	__u64 response;		/* [i], [*o] {SCSI: (auto)sense data} */

        /* "dout_": data out (to device); "din_": data in (from device) */
	__u32 dout_iovec_count;	/* [i] 0 -> "flat" dout transfer else
				   dout_xfer points to array of iovec */
	__u32 dout_xfer_len;	/* [i] bytes to be transferred to device */
	__u32 din_iovec_count;	/* [i] 0 -> "flat" din transfer */
	__u32 din_xfer_len;	/* [i] bytes to be transferred from device */
Quoted 8 lines
	__u64 dout_xferp;	/* [i], [*i] */
	__u64 din_xferp;	/* [i], [*o] */

	__u32 timeout;		/* [i] units: millisecond */
	__u32 flags;		/* [i] bit mask */
	__u64 usr_ptr;		/* [i->o] unused internally */
	__u32 spare_in;		/* [i] */
      ...

Currently because of a twisted accident sg and bsg queue their async request with the "at_head" flag set when calling blk_execute_rq_nowait.

Since sg_io_v4 is new and as plenty of possible choices, what would be the best place to put the "at_head" flag?

Quoted 6 lines

Douglas ?
What was your intention with the above:
__u32 request_priority; /* [i] {SCSI: task priority} */

[And while at it, what was:
__u32 request_attr; /* [i] {SCSI: task attribute} */

Yes, task attribute. That is the term used by SAM-4 (and earlier). Unfortunately SAM-4 defines the names but not values; those values are left up to the transport. That presents a problem for a transport agnostic SCSI pass-through interface like sg_io_v4.

For backward compatibility we probably need to make the value 0 mean "head of queue" (or whatever the mid-level default task attribute is for pass-through SCSI commands).

Also we probably don't have a clean pass-through of task attributes in the SCSI mid-level currently. I mean "clean" in the sense of "leave them alone" and do not act on the command completion status (e.g. TASK SET FULL).

Quoted 6 lines

Should I define (bsg.h):
enum {
BSG_AT_HEAD = 0, /* compatible with old code */ BSG_AT_TAIL,
}
And put it in request_priority

or (adding "_TA_" for task attribute):

   enum {
         BSG_TA_DEFAULT = 0,  // lk 2.4, 2.6 series: head of queue
         BSG_TA_HEAD_OF_Q = 0x1000,
         BSG_TA_SIMPLE,
         BSG_TA_ORDERED,
         BSG_TA_ACA,
         ....
Quoted 8 lines
Quoted 5 lines

or define:
#define SG_FLAG_AT_TAIL 0x10 /* See sg.h for more/other flags (sg.h flags are ignored by bsg) */

And OR that into the old flags member that is otherwise unused by current bsg code

I would prefer using the request_attr field.

Tomo, what do you think?

It should be request_attr, as you planed. request_priority is for the task (command) priority.

I'm not sure about inventing BSG_TA_* thing. If we do that, the transport classes need to have the own function to convert BSG_TA_* to their own attribute values, iscsi_bsg_to_attr(), fc_bsg_to_attr... If there are tons of users who want task attribute support in bsg, then it's worth doing that but...

Quoted 3 lines

How about letting users to do the proper thing? That's is, if he wants to send a command to iSCSI devices, he needs to set iSCSI task attribute value.

TOMO, Doug.

You have both not addressed my little need. I need a boolean flag to indicate "at_head". Clearly request_attr and request_priority is not it since they have a SCSI meaning, however unused. I will go head with my bit in flags.

And yes at_head matters a lot. Because the order of commands matter. For one bsg's write() supports multiple requests so I will need to reverse order them, and even so It is playing Russian roulette since the commands can start executing and you can never know when. It is even worse if I don't use multiple commands. I start queuing commands some are executed, new one comes in I can never know if they execute in order. Come on clearly an "at_head" is a mess, you must realize that. It is exactly for when things get messy, then it's needed.

I will go with a new bit at the second nibble of flags.

Thanks
Boaz