* Re: [PATCH] Add an array that describes which opcodes are supported by the RDWR and SHEEPDOG backends. [not found] ` <1381588740-25968-2-git-send-email-ronniesahlberg@gmail.com> @ 2013-10-16 16:05 ` ronnie sahlberg 2013-10-17 13:47 ` Hitoshi Mitake ` (2 subsequent siblings) 3 siblings, 0 replies; 16+ messages in thread From: ronnie sahlberg @ 2013-10-16 16:05 UTC (permalink / raw) To: stgt; +Cc: Ronnie Sahlberg ping? On Sat, Oct 12, 2013 at 7:38 AM, Ronnie Sahlberg <ronniesahlberg@gmail.com> wrote: > While RDWR supports all SBC opcodes that TGTD implement SHEEPDOG > only supports a subset and lacks the following opcodes: > WRITE_VERIFY10/12/16 VERIFY10/12/16 PREFETCH10/16 > WRITE_SAME10/16 UNMAP and ORWRITE > > This allows backends to specify which opcodes it is prepared to process > and which commands should fail with invalid op code > and allows SHEEPDOG backed LUNs to respond with INVALID_OP_CODE > correctly. > > This is most useful for block devices where we have several different backens > and where some backends only support a subset of the commands > > Signed-off-by: Ronnie Sahlberg <ronniesahlberg@gmail.com> > --- > usr/bs.c | 8 ++++++++ > usr/bs_rdwr.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++ > usr/bs_sheepdog.c | 38 ++++++++++++++++++++++++++++++++++++++ > usr/scsi.c | 6 ++++++ > usr/tgtd.h | 1 + > 5 files changed, 104 insertions(+), 0 deletions(-) > > diff --git a/usr/bs.c b/usr/bs.c > index 636f53b..5299498 100644 > --- a/usr/bs.c > +++ b/usr/bs.c > @@ -463,3 +463,11 @@ int bs_thread_cmd_submit(struct scsi_cmd *cmd) > > return 0; > } > + > +void bs_create_opcode_map(unsigned char *map, unsigned char *opcodes, int num) > +{ > + int i; > + > + for (i = 0; i < num; i++) > + map[opcodes[i]] = 1; > +} > diff --git a/usr/bs_rdwr.c b/usr/bs_rdwr.c > index 47d2d99..6a88205 100644 > --- a/usr/bs_rdwr.c > +++ b/usr/bs_rdwr.c > @@ -412,6 +412,8 @@ static void bs_rdwr_exit(struct scsi_lu *lu) > bs_thread_close(info); > } > > +static char bs_ops_supported[256]; > + > static struct backingstore_template rdwr_bst = { > .bs_name = "rdwr", > .bs_datasize = sizeof(struct bs_thread_info), > @@ -421,9 +423,58 @@ static struct backingstore_template rdwr_bst = { > .bs_exit = bs_rdwr_exit, > .bs_cmd_submit = bs_thread_cmd_submit, > .bs_oflags_supported = O_SYNC | O_DIRECT, > + .bs_ops_supported = bs_ops_supported, > }; > > __attribute__((constructor)) static void bs_rdwr_constructor(void) > { > + static char opcodes[] = { > + ALLOW_MEDIUM_REMOVAL, > + COMPARE_AND_WRITE, > + FORMAT_UNIT, > + INQUIRY, > + MAINT_PROTOCOL_IN, > + MODE_SELECT, > + MODE_SELECT_10, > + MODE_SENSE, > + MODE_SENSE_10, > + ORWRITE_16, > + PERSISTENT_RESERVE_IN, > + PERSISTENT_RESERVE_OUT, > + PRE_FETCH_10, > + PRE_FETCH_16, > + READ_10, > + READ_12, > + READ_16, > + READ_6, > + READ_CAPACITY, > + RELEASE, > + REPORT_LUNS, > + REQUEST_SENSE, > + RESERVE, > + SEND_DIAGNOSTIC, > + SERVICE_ACTION_IN, > + START_STOP, > + SYNCHRONIZE_CACHE, > + SYNCHRONIZE_CACHE_16, > + TEST_UNIT_READY, > + UNMAP, > + VERIFY_10, > + VERIFY_12, > + VERIFY_16, > + WRITE_10, > + WRITE_12, > + WRITE_16, > + WRITE_6, > + WRITE_SAME, > + WRITE_SAME_16, > + WRITE_VERIFY, > + WRITE_VERIFY_12, > + WRITE_VERIFY_16 > + }; > + > + bs_create_opcode_map(bs_ops_supported, &opcodes[0], > + sizeof(opcodes) / sizeof(opcodes[0])); > + > register_backingstore_template(&rdwr_bst); > } > diff --git a/usr/bs_sheepdog.c b/usr/bs_sheepdog.c > index 4b5a951..3099b58 100644 > --- a/usr/bs_sheepdog.c > +++ b/usr/bs_sheepdog.c > @@ -1017,6 +1017,8 @@ static void bs_sheepdog_exit(struct scsi_lu *lu) > } > } > > +static char bs_ops_supported[256]; > + > static struct backingstore_template sheepdog_bst = { > .bs_name = "sheepdog", > .bs_datasize = > @@ -1026,9 +1028,45 @@ static struct backingstore_template sheepdog_bst = { > .bs_init = bs_sheepdog_init, > .bs_exit = bs_sheepdog_exit, > .bs_cmd_submit = bs_thread_cmd_submit, > + .bs_ops_supported = bs_ops_supported, > }; > > __attribute__((constructor)) static void __constructor(void) > { > + static char opcodes[] = { > + ALLOW_MEDIUM_REMOVAL, > + FORMAT_UNIT, > + INQUIRY, > + MAINT_PROTOCOL_IN, > + MODE_SELECT, > + MODE_SELECT_10, > + MODE_SENSE, > + MODE_SENSE_10, > + PERSISTENT_RESERVE_IN, > + PERSISTENT_RESERVE_OUT, > + READ_10, > + READ_12, > + READ_16, > + READ_6, > + READ_CAPACITY, > + RELEASE, > + REPORT_LUNS, > + REQUEST_SENSE, > + RESERVE, > + SEND_DIAGNOSTIC, > + SERVICE_ACTION_IN, > + START_STOP, > + SYNCHRONIZE_CACHE, > + SYNCHRONIZE_CACHE_16, > + TEST_UNIT_READY, > + WRITE_10, > + WRITE_12, > + WRITE_16, > + WRITE_6 > + }; > + > + bs_create_opcode_map(bs_ops_supported, &opcodes[0], > + sizeof(opcodes) / sizeof(opcodes[0])); > + > register_backingstore_template(&sheepdog_bst); > } > diff --git a/usr/scsi.c b/usr/scsi.c > index 2636a5c..bce5ae0 100644 > --- a/usr/scsi.c > +++ b/usr/scsi.c > @@ -492,6 +492,12 @@ int scsi_cmd_perform(int host_no, struct scsi_cmd *cmd) > if (spc_access_check(cmd)) > return SAM_STAT_RESERVATION_CONFLICT; > > + if (cmd->dev->bst->bs_ops_supported > + && !cmd->dev->bst->bs_ops_supported[op]) { > + sense_data_build(cmd, ILLEGAL_REQUEST, ASC_INVALID_OP_CODE); > + return SAM_STAT_CHECK_CONDITION; > + } > + > return cmd->dev->dev_type_template.ops[op].cmd_perform(host_no, cmd); > } > > diff --git a/usr/tgtd.h b/usr/tgtd.h > index 484e6e9..70f6306 100644 > --- a/usr/tgtd.h > +++ b/usr/tgtd.h > @@ -165,6 +165,7 @@ struct backingstore_template { > void (*bs_exit)(struct scsi_lu *dev); > int (*bs_cmd_submit)(struct scsi_cmd *cmd); > int bs_oflags_supported; > + const char *bs_ops_supported; > > struct list_head backingstore_siblings; > }; > -- > 1.7.3.1 > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Add an array that describes which opcodes are supported by the RDWR and SHEEPDOG backends. [not found] ` <1381588740-25968-2-git-send-email-ronniesahlberg@gmail.com> 2013-10-16 16:05 ` [PATCH] Add an array that describes which opcodes are supported by the RDWR and SHEEPDOG backends ronnie sahlberg @ 2013-10-17 13:47 ` Hitoshi Mitake 2013-10-20 13:38 ` ronnie sahlberg 2013-10-21 1:34 ` FUJITA Tomonori 3 siblings, 0 replies; 16+ messages in thread From: Hitoshi Mitake @ 2013-10-17 13:47 UTC (permalink / raw) To: Ronnie Sahlberg; +Cc: stgt Hi Ronnie, At Sat, 12 Oct 2013 07:38:59 -0700, Ronnie Sahlberg wrote: > > While RDWR supports all SBC opcodes that TGTD implement SHEEPDOG > only supports a subset and lacks the following opcodes: > WRITE_VERIFY10/12/16 VERIFY10/12/16 PREFETCH10/16 > WRITE_SAME10/16 UNMAP and ORWRITE > > This allows backends to specify which opcodes it is prepared to process > and which commands should fail with invalid op code > and allows SHEEPDOG backed LUNs to respond with INVALID_OP_CODE > correctly. > > This is most useful for block devices where we have several different backens > and where some backends only support a subset of the commands > > Signed-off-by: Ronnie Sahlberg <ronniesahlberg@gmail.com> > --- > usr/bs.c | 8 ++++++++ > usr/bs_rdwr.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++ > usr/bs_sheepdog.c | 38 ++++++++++++++++++++++++++++++++++++++ > usr/scsi.c | 6 ++++++ > usr/tgtd.h | 1 + > 5 files changed, 104 insertions(+), 0 deletions(-) > Thanks a lot for the change! This patch looks good to me. I'll try to implement these missing opcode in bs_sheepdog.c, but it will take time and your solution is a great help. Thanks, Hitoshi ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Add an array that describes which opcodes are supported by the RDWR and SHEEPDOG backends. [not found] ` <1381588740-25968-2-git-send-email-ronniesahlberg@gmail.com> 2013-10-16 16:05 ` [PATCH] Add an array that describes which opcodes are supported by the RDWR and SHEEPDOG backends ronnie sahlberg 2013-10-17 13:47 ` Hitoshi Mitake @ 2013-10-20 13:38 ` ronnie sahlberg 2013-10-21 1:34 ` FUJITA Tomonori 3 siblings, 0 replies; 16+ messages in thread From: ronnie sahlberg @ 2013-10-20 13:38 UTC (permalink / raw) To: stgt; +Cc: Ronnie Sahlberg ping? On Sat, Oct 12, 2013 at 7:38 AM, Ronnie Sahlberg <ronniesahlberg@gmail.com> wrote: > While RDWR supports all SBC opcodes that TGTD implement SHEEPDOG > only supports a subset and lacks the following opcodes: > WRITE_VERIFY10/12/16 VERIFY10/12/16 PREFETCH10/16 > WRITE_SAME10/16 UNMAP and ORWRITE > > This allows backends to specify which opcodes it is prepared to process > and which commands should fail with invalid op code > and allows SHEEPDOG backed LUNs to respond with INVALID_OP_CODE > correctly. > > This is most useful for block devices where we have several different backens > and where some backends only support a subset of the commands > > Signed-off-by: Ronnie Sahlberg <ronniesahlberg@gmail.com> > --- > usr/bs.c | 8 ++++++++ > usr/bs_rdwr.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++ > usr/bs_sheepdog.c | 38 ++++++++++++++++++++++++++++++++++++++ > usr/scsi.c | 6 ++++++ > usr/tgtd.h | 1 + > 5 files changed, 104 insertions(+), 0 deletions(-) > > diff --git a/usr/bs.c b/usr/bs.c > index 636f53b..5299498 100644 > --- a/usr/bs.c > +++ b/usr/bs.c > @@ -463,3 +463,11 @@ int bs_thread_cmd_submit(struct scsi_cmd *cmd) > > return 0; > } > + > +void bs_create_opcode_map(unsigned char *map, unsigned char *opcodes, int num) > +{ > + int i; > + > + for (i = 0; i < num; i++) > + map[opcodes[i]] = 1; > +} > diff --git a/usr/bs_rdwr.c b/usr/bs_rdwr.c > index 47d2d99..6a88205 100644 > --- a/usr/bs_rdwr.c > +++ b/usr/bs_rdwr.c > @@ -412,6 +412,8 @@ static void bs_rdwr_exit(struct scsi_lu *lu) > bs_thread_close(info); > } > > +static char bs_ops_supported[256]; > + > static struct backingstore_template rdwr_bst = { > .bs_name = "rdwr", > .bs_datasize = sizeof(struct bs_thread_info), > @@ -421,9 +423,58 @@ static struct backingstore_template rdwr_bst = { > .bs_exit = bs_rdwr_exit, > .bs_cmd_submit = bs_thread_cmd_submit, > .bs_oflags_supported = O_SYNC | O_DIRECT, > + .bs_ops_supported = bs_ops_supported, > }; > > __attribute__((constructor)) static void bs_rdwr_constructor(void) > { > + static char opcodes[] = { > + ALLOW_MEDIUM_REMOVAL, > + COMPARE_AND_WRITE, > + FORMAT_UNIT, > + INQUIRY, > + MAINT_PROTOCOL_IN, > + MODE_SELECT, > + MODE_SELECT_10, > + MODE_SENSE, > + MODE_SENSE_10, > + ORWRITE_16, > + PERSISTENT_RESERVE_IN, > + PERSISTENT_RESERVE_OUT, > + PRE_FETCH_10, > + PRE_FETCH_16, > + READ_10, > + READ_12, > + READ_16, > + READ_6, > + READ_CAPACITY, > + RELEASE, > + REPORT_LUNS, > + REQUEST_SENSE, > + RESERVE, > + SEND_DIAGNOSTIC, > + SERVICE_ACTION_IN, > + START_STOP, > + SYNCHRONIZE_CACHE, > + SYNCHRONIZE_CACHE_16, > + TEST_UNIT_READY, > + UNMAP, > + VERIFY_10, > + VERIFY_12, > + VERIFY_16, > + WRITE_10, > + WRITE_12, > + WRITE_16, > + WRITE_6, > + WRITE_SAME, > + WRITE_SAME_16, > + WRITE_VERIFY, > + WRITE_VERIFY_12, > + WRITE_VERIFY_16 > + }; > + > + bs_create_opcode_map(bs_ops_supported, &opcodes[0], > + sizeof(opcodes) / sizeof(opcodes[0])); > + > register_backingstore_template(&rdwr_bst); > } > diff --git a/usr/bs_sheepdog.c b/usr/bs_sheepdog.c > index 4b5a951..3099b58 100644 > --- a/usr/bs_sheepdog.c > +++ b/usr/bs_sheepdog.c > @@ -1017,6 +1017,8 @@ static void bs_sheepdog_exit(struct scsi_lu *lu) > } > } > > +static char bs_ops_supported[256]; > + > static struct backingstore_template sheepdog_bst = { > .bs_name = "sheepdog", > .bs_datasize = > @@ -1026,9 +1028,45 @@ static struct backingstore_template sheepdog_bst = { > .bs_init = bs_sheepdog_init, > .bs_exit = bs_sheepdog_exit, > .bs_cmd_submit = bs_thread_cmd_submit, > + .bs_ops_supported = bs_ops_supported, > }; > > __attribute__((constructor)) static void __constructor(void) > { > + static char opcodes[] = { > + ALLOW_MEDIUM_REMOVAL, > + FORMAT_UNIT, > + INQUIRY, > + MAINT_PROTOCOL_IN, > + MODE_SELECT, > + MODE_SELECT_10, > + MODE_SENSE, > + MODE_SENSE_10, > + PERSISTENT_RESERVE_IN, > + PERSISTENT_RESERVE_OUT, > + READ_10, > + READ_12, > + READ_16, > + READ_6, > + READ_CAPACITY, > + RELEASE, > + REPORT_LUNS, > + REQUEST_SENSE, > + RESERVE, > + SEND_DIAGNOSTIC, > + SERVICE_ACTION_IN, > + START_STOP, > + SYNCHRONIZE_CACHE, > + SYNCHRONIZE_CACHE_16, > + TEST_UNIT_READY, > + WRITE_10, > + WRITE_12, > + WRITE_16, > + WRITE_6 > + }; > + > + bs_create_opcode_map(bs_ops_supported, &opcodes[0], > + sizeof(opcodes) / sizeof(opcodes[0])); > + > register_backingstore_template(&sheepdog_bst); > } > diff --git a/usr/scsi.c b/usr/scsi.c > index 2636a5c..bce5ae0 100644 > --- a/usr/scsi.c > +++ b/usr/scsi.c > @@ -492,6 +492,12 @@ int scsi_cmd_perform(int host_no, struct scsi_cmd *cmd) > if (spc_access_check(cmd)) > return SAM_STAT_RESERVATION_CONFLICT; > > + if (cmd->dev->bst->bs_ops_supported > + && !cmd->dev->bst->bs_ops_supported[op]) { > + sense_data_build(cmd, ILLEGAL_REQUEST, ASC_INVALID_OP_CODE); > + return SAM_STAT_CHECK_CONDITION; > + } > + > return cmd->dev->dev_type_template.ops[op].cmd_perform(host_no, cmd); > } > > diff --git a/usr/tgtd.h b/usr/tgtd.h > index 484e6e9..70f6306 100644 > --- a/usr/tgtd.h > +++ b/usr/tgtd.h > @@ -165,6 +165,7 @@ struct backingstore_template { > void (*bs_exit)(struct scsi_lu *dev); > int (*bs_cmd_submit)(struct scsi_cmd *cmd); > int bs_oflags_supported; > + const char *bs_ops_supported; > > struct list_head backingstore_siblings; > }; > -- > 1.7.3.1 > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Add an array that describes which opcodes are supported by the RDWR and SHEEPDOG backends. [not found] ` <1381588740-25968-2-git-send-email-ronniesahlberg@gmail.com> ` (2 preceding siblings ...) 2013-10-20 13:38 ` ronnie sahlberg @ 2013-10-21 1:34 ` FUJITA Tomonori 2013-10-31 1:44 ` ronnie sahlberg 3 siblings, 1 reply; 16+ messages in thread From: FUJITA Tomonori @ 2013-10-21 1:34 UTC (permalink / raw) To: ronniesahlberg; +Cc: stgt On Sat, 12 Oct 2013 07:38:59 -0700 Ronnie Sahlberg <ronniesahlberg@gmail.com> wrote: > While RDWR supports all SBC opcodes that TGTD implement SHEEPDOG > only supports a subset and lacks the following opcodes: > WRITE_VERIFY10/12/16 VERIFY10/12/16 PREFETCH10/16 > WRITE_SAME10/16 UNMAP and ORWRITE > > This allows backends to specify which opcodes it is prepared to process > and which commands should fail with invalid op code > and allows SHEEPDOG backed LUNs to respond with INVALID_OP_CODE > correctly. > > This is most useful for block devices where we have several different backens > and where some backends only support a subset of the commands > > Signed-off-by: Ronnie Sahlberg <ronniesahlberg@gmail.com> > --- > usr/bs.c | 8 ++++++++ > usr/bs_rdwr.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++ > usr/bs_sheepdog.c | 38 ++++++++++++++++++++++++++++++++++++++ > usr/scsi.c | 6 ++++++ > usr/tgtd.h | 1 + > 5 files changed, 104 insertions(+), 0 deletions(-) Why backend store's bs_cmd_submit cannot return a proper error and sbr_rc returns that error to an initiator? ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Add an array that describes which opcodes are supported by the RDWR and SHEEPDOG backends. 2013-10-21 1:34 ` FUJITA Tomonori @ 2013-10-31 1:44 ` ronnie sahlberg 2013-11-05 14:39 ` ronnie sahlberg 2013-11-07 7:07 ` FUJITA Tomonori 0 siblings, 2 replies; 16+ messages in thread From: ronnie sahlberg @ 2013-10-31 1:44 UTC (permalink / raw) To: FUJITA Tomonori; +Cc: stgt On Sun, Oct 20, 2013 at 6:34 PM, FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote: > On Sat, 12 Oct 2013 07:38:59 -0700 > Ronnie Sahlberg <ronniesahlberg@gmail.com> wrote: > >> While RDWR supports all SBC opcodes that TGTD implement SHEEPDOG >> only supports a subset and lacks the following opcodes: >> WRITE_VERIFY10/12/16 VERIFY10/12/16 PREFETCH10/16 >> WRITE_SAME10/16 UNMAP and ORWRITE >> >> This allows backends to specify which opcodes it is prepared to process >> and which commands should fail with invalid op code >> and allows SHEEPDOG backed LUNs to respond with INVALID_OP_CODE >> correctly. >> >> This is most useful for block devices where we have several different backens >> and where some backends only support a subset of the commands >> >> Signed-off-by: Ronnie Sahlberg <ronniesahlberg@gmail.com> >> --- >> usr/bs.c | 8 ++++++++ >> usr/bs_rdwr.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++ >> usr/bs_sheepdog.c | 38 ++++++++++++++++++++++++++++++++++++++ >> usr/scsi.c | 6 ++++++ >> usr/tgtd.h | 1 + >> 5 files changed, 104 insertions(+), 0 deletions(-) > > Why backend store's bs_cmd_submit cannot return a proper error and > sbr_rc returns that error to an initiator? It could, but this way it is more explicit which opcodes are available, and a simple compare of the array between opcodes makes it easy for non-bs_rdwr backend developers to see the delta between their backend and rdwr. A second reason is for a followup patch. The followup patch will use this array so that it can prune which opcodes to report back for the REPORT_SUPPORTED_OPCODE opcode. So that when you issue that command to a sheepdog LUN you get a pruned list back that only lists the opcodes that sheepdog supports. regards ronnie sahlberg ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Add an array that describes which opcodes are supported by the RDWR and SHEEPDOG backends. 2013-10-31 1:44 ` ronnie sahlberg @ 2013-11-05 14:39 ` ronnie sahlberg 2013-11-07 7:07 ` FUJITA Tomonori 1 sibling, 0 replies; 16+ messages in thread From: ronnie sahlberg @ 2013-11-05 14:39 UTC (permalink / raw) To: FUJITA Tomonori; +Cc: stgt Ping? We need something like an array like this so that we can also update REPORT_SUPPORTED_OPCODES correctly and have this opcode only return the opcodes that the particular backend supports. On Wed, Oct 30, 2013 at 6:44 PM, ronnie sahlberg <ronniesahlberg@gmail.com> wrote: > On Sun, Oct 20, 2013 at 6:34 PM, FUJITA Tomonori > <fujita.tomonori@lab.ntt.co.jp> wrote: >> On Sat, 12 Oct 2013 07:38:59 -0700 >> Ronnie Sahlberg <ronniesahlberg@gmail.com> wrote: >> >>> While RDWR supports all SBC opcodes that TGTD implement SHEEPDOG >>> only supports a subset and lacks the following opcodes: >>> WRITE_VERIFY10/12/16 VERIFY10/12/16 PREFETCH10/16 >>> WRITE_SAME10/16 UNMAP and ORWRITE >>> >>> This allows backends to specify which opcodes it is prepared to process >>> and which commands should fail with invalid op code >>> and allows SHEEPDOG backed LUNs to respond with INVALID_OP_CODE >>> correctly. >>> >>> This is most useful for block devices where we have several different backens >>> and where some backends only support a subset of the commands >>> >>> Signed-off-by: Ronnie Sahlberg <ronniesahlberg@gmail.com> >>> --- >>> usr/bs.c | 8 ++++++++ >>> usr/bs_rdwr.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++ >>> usr/bs_sheepdog.c | 38 ++++++++++++++++++++++++++++++++++++++ >>> usr/scsi.c | 6 ++++++ >>> usr/tgtd.h | 1 + >>> 5 files changed, 104 insertions(+), 0 deletions(-) >> >> Why backend store's bs_cmd_submit cannot return a proper error and >> sbr_rc returns that error to an initiator? > > > It could, but this way it is more explicit which opcodes are > available, and a simple compare of the array between opcodes > makes it easy for non-bs_rdwr backend developers to see the delta > between their backend and rdwr. > > A second reason is for a followup patch. > The followup patch will use this array so that it can prune which > opcodes to report back for the REPORT_SUPPORTED_OPCODE opcode. > So that when you issue that command to a sheepdog LUN you get a > pruned list back that only lists the opcodes that sheepdog supports. > > > regards > ronnie sahlberg ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Add an array that describes which opcodes are supported by the RDWR and SHEEPDOG backends. 2013-10-31 1:44 ` ronnie sahlberg 2013-11-05 14:39 ` ronnie sahlberg @ 2013-11-07 7:07 ` FUJITA Tomonori 2013-11-07 16:42 ` ronnie sahlberg 1 sibling, 1 reply; 16+ messages in thread From: FUJITA Tomonori @ 2013-11-07 7:07 UTC (permalink / raw) To: ronniesahlberg; +Cc: fujita.tomonori, stgt On Wed, 30 Oct 2013 18:44:19 -0700 ronnie sahlberg <ronniesahlberg@gmail.com> wrote: > On Sun, Oct 20, 2013 at 6:34 PM, FUJITA Tomonori > <fujita.tomonori@lab.ntt.co.jp> wrote: > > On Sat, 12 Oct 2013 07:38:59 -0700 > > Ronnie Sahlberg <ronniesahlberg@gmail.com> wrote: > > > >> While RDWR supports all SBC opcodes that TGTD implement SHEEPDOG > >> only supports a subset and lacks the following opcodes: > >> WRITE_VERIFY10/12/16 VERIFY10/12/16 PREFETCH10/16 > >> WRITE_SAME10/16 UNMAP and ORWRITE > >> > >> This allows backends to specify which opcodes it is prepared to process > >> and which commands should fail with invalid op code > >> and allows SHEEPDOG backed LUNs to respond with INVALID_OP_CODE > >> correctly. > >> > >> This is most useful for block devices where we have several different backens > >> and where some backends only support a subset of the commands > >> > >> Signed-off-by: Ronnie Sahlberg <ronniesahlberg@gmail.com> > >> --- > >> usr/bs.c | 8 ++++++++ > >> usr/bs_rdwr.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++ > >> usr/bs_sheepdog.c | 38 ++++++++++++++++++++++++++++++++++++++ > >> usr/scsi.c | 6 ++++++ > >> usr/tgtd.h | 1 + > >> 5 files changed, 104 insertions(+), 0 deletions(-) > > > > Why backend store's bs_cmd_submit cannot return a proper error and > > sbr_rc returns that error to an initiator? > > > It could, but this way it is more explicit which opcodes are > available, and a simple compare of the array between opcodes > makes it easy for non-bs_rdwr backend developers to see the delta > between their backend and rdwr. > > A second reason is for a followup patch. > The followup patch will use this array so that it can prune which > opcodes to report back for the REPORT_SUPPORTED_OPCODE opcode. > So that when you issue that command to a sheepdog LUN you get a > pruned list back that only lists the opcodes that sheepdog supports. I see, thanks. Using the bitmaps is simpler than the array of char if you calculate delta and such? ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Add an array that describes which opcodes are supported by the RDWR and SHEEPDOG backends. 2013-11-07 7:07 ` FUJITA Tomonori @ 2013-11-07 16:42 ` ronnie sahlberg 2013-11-07 21:39 ` FUJITA Tomonori 0 siblings, 1 reply; 16+ messages in thread From: ronnie sahlberg @ 2013-11-07 16:42 UTC (permalink / raw) To: FUJITA Tomonori; +Cc: stgt On Wed, Nov 6, 2013 at 11:07 PM, FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote: > On Wed, 30 Oct 2013 18:44:19 -0700 > ronnie sahlberg <ronniesahlberg@gmail.com> wrote: > >> On Sun, Oct 20, 2013 at 6:34 PM, FUJITA Tomonori >> <fujita.tomonori@lab.ntt.co.jp> wrote: >> > On Sat, 12 Oct 2013 07:38:59 -0700 >> > Ronnie Sahlberg <ronniesahlberg@gmail.com> wrote: >> > >> >> While RDWR supports all SBC opcodes that TGTD implement SHEEPDOG >> >> only supports a subset and lacks the following opcodes: >> >> WRITE_VERIFY10/12/16 VERIFY10/12/16 PREFETCH10/16 >> >> WRITE_SAME10/16 UNMAP and ORWRITE >> >> >> >> This allows backends to specify which opcodes it is prepared to process >> >> and which commands should fail with invalid op code >> >> and allows SHEEPDOG backed LUNs to respond with INVALID_OP_CODE >> >> correctly. >> >> >> >> This is most useful for block devices where we have several different backens >> >> and where some backends only support a subset of the commands >> >> >> >> Signed-off-by: Ronnie Sahlberg <ronniesahlberg@gmail.com> >> >> --- >> >> usr/bs.c | 8 ++++++++ >> >> usr/bs_rdwr.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++ >> >> usr/bs_sheepdog.c | 38 ++++++++++++++++++++++++++++++++++++++ >> >> usr/scsi.c | 6 ++++++ >> >> usr/tgtd.h | 1 + >> >> 5 files changed, 104 insertions(+), 0 deletions(-) >> > >> > Why backend store's bs_cmd_submit cannot return a proper error and >> > sbr_rc returns that error to an initiator? >> >> >> It could, but this way it is more explicit which opcodes are >> available, and a simple compare of the array between opcodes >> makes it easy for non-bs_rdwr backend developers to see the delta >> between their backend and rdwr. >> >> A second reason is for a followup patch. >> The followup patch will use this array so that it can prune which >> opcodes to report back for the REPORT_SUPPORTED_OPCODE opcode. >> So that when you issue that command to a sheepdog LUN you get a >> pruned list back that only lists the opcodes that sheepdog supports. > > I see, thanks. Using the bitmaps is simpler than the array of char if > you calculate delta and such? I think using the array with opcode names is simpler for a human to comparing when reading the sourcecode. I had a bitmap of 32 bytes, one bit for each opcode, and I also tried using an array of 256 bytes, one byte 0/1 for each opcode but it was horrible to read from a human standpoint. When reading the code and the bitmap/array it was very difficult to see which opcodes were supported and which were not by just looking at the bits. It was also errorprone and I did several mistakes when building the bitmap manually. This approach uses an array of opcodes, making it really easy for a human to see which opcodes are supported and which are not. Then I call bs_create_opcode_map() to convert this array into a byte-array to make it efficient for tgtd to check if the opcodes are supported or not. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Add an array that describes which opcodes are supported by the RDWR and SHEEPDOG backends. 2013-11-07 16:42 ` ronnie sahlberg @ 2013-11-07 21:39 ` FUJITA Tomonori 2013-11-07 21:46 ` Dan Mick 0 siblings, 1 reply; 16+ messages in thread From: FUJITA Tomonori @ 2013-11-07 21:39 UTC (permalink / raw) To: ronniesahlberg; +Cc: fujita.tomonori, stgt On Thu, 7 Nov 2013 08:42:43 -0800 ronnie sahlberg <ronniesahlberg@gmail.com> wrote: > On Wed, Nov 6, 2013 at 11:07 PM, FUJITA Tomonori > <fujita.tomonori@lab.ntt.co.jp> wrote: > > On Wed, 30 Oct 2013 18:44:19 -0700 > > ronnie sahlberg <ronniesahlberg@gmail.com> wrote: > > > >> On Sun, Oct 20, 2013 at 6:34 PM, FUJITA Tomonori > >> <fujita.tomonori@lab.ntt.co.jp> wrote: > >> > On Sat, 12 Oct 2013 07:38:59 -0700 > >> > Ronnie Sahlberg <ronniesahlberg@gmail.com> wrote: > >> > > >> >> While RDWR supports all SBC opcodes that TGTD implement SHEEPDOG > >> >> only supports a subset and lacks the following opcodes: > >> >> WRITE_VERIFY10/12/16 VERIFY10/12/16 PREFETCH10/16 > >> >> WRITE_SAME10/16 UNMAP and ORWRITE > >> >> > >> >> This allows backends to specify which opcodes it is prepared to process > >> >> and which commands should fail with invalid op code > >> >> and allows SHEEPDOG backed LUNs to respond with INVALID_OP_CODE > >> >> correctly. > >> >> > >> >> This is most useful for block devices where we have several different backens > >> >> and where some backends only support a subset of the commands > >> >> > >> >> Signed-off-by: Ronnie Sahlberg <ronniesahlberg@gmail.com> > >> >> --- > >> >> usr/bs.c | 8 ++++++++ > >> >> usr/bs_rdwr.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++ > >> >> usr/bs_sheepdog.c | 38 ++++++++++++++++++++++++++++++++++++++ > >> >> usr/scsi.c | 6 ++++++ > >> >> usr/tgtd.h | 1 + > >> >> 5 files changed, 104 insertions(+), 0 deletions(-) > >> > > >> > Why backend store's bs_cmd_submit cannot return a proper error and > >> > sbr_rc returns that error to an initiator? > >> > >> > >> It could, but this way it is more explicit which opcodes are > >> available, and a simple compare of the array between opcodes > >> makes it easy for non-bs_rdwr backend developers to see the delta > >> between their backend and rdwr. > >> > >> A second reason is for a followup patch. > >> The followup patch will use this array so that it can prune which > >> opcodes to report back for the REPORT_SUPPORTED_OPCODE opcode. > >> So that when you issue that command to a sheepdog LUN you get a > >> pruned list back that only lists the opcodes that sheepdog supports. > > > > I see, thanks. Using the bitmaps is simpler than the array of char if > > you calculate delta and such? > > I think using the array with opcode names is simpler for a human to > comparing when reading the sourcecode. > > > I had a bitmap of 32 bytes, one bit for each opcode, and I also > tried using an array of 256 bytes, one byte 0/1 for each opcode > but it was horrible to read from a human standpoint. > > When reading the code and the bitmap/array it was very difficult to > see which opcodes were supported and which were not > by just looking at the bits. > It was also errorprone and I did several mistakes when building the > bitmap manually. Why you built it manually? You can do something like set_bit(WRITE_6, bitmap_addr); ? As usual, you can steal bitmap functions from Linux kernel. > > This approach uses an array of opcodes, making it really easy for a > human to see which opcodes are supported and which are not. > Then I call bs_create_opcode_map() to convert this array into a > byte-array to make it efficient for tgtd to check if the opcodes are > supported or not. > -- > To unsubscribe from this list: send the line "unsubscribe stgt" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Add an array that describes which opcodes are supported by the RDWR and SHEEPDOG backends. 2013-11-07 21:39 ` FUJITA Tomonori @ 2013-11-07 21:46 ` Dan Mick 2013-11-07 21:51 ` FUJITA Tomonori 0 siblings, 1 reply; 16+ messages in thread From: Dan Mick @ 2013-11-07 21:46 UTC (permalink / raw) To: FUJITA Tomonori, ronniesahlberg; +Cc: stgt >>> I see, thanks. Using the bitmaps is simpler than the array of char if >>> you calculate delta and such? >> >> I think using the array with opcode names is simpler for a human to >> comparing when reading the sourcecode. >> >> >> I had a bitmap of 32 bytes, one bit for each opcode, and I also >> tried using an array of 256 bytes, one byte 0/1 for each opcode >> but it was horrible to read from a human standpoint. >> >> When reading the code and the bitmap/array it was very difficult to >> see which opcodes were supported and which were not >> by just looking at the bits. >> It was also errorprone and I did several mistakes when building the >> bitmap manually. > > Why you built it manually? You can do something like > > set_bit(WRITE_6, bitmap_addr); > > ? > > As usual, you can steal bitmap functions from Linux kernel. Is it worth it to mess around with bits when chars are easily manipulated? Sure, it's 8x the data, but it's much easier to dump, examine in debuggers, etc., and it's 256 bytes; hardly worthy of notice. Bitmaps are a pain in general. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Add an array that describes which opcodes are supported by the RDWR and SHEEPDOG backends. 2013-11-07 21:46 ` Dan Mick @ 2013-11-07 21:51 ` FUJITA Tomonori 2013-11-07 22:15 ` Dan Mick 0 siblings, 1 reply; 16+ messages in thread From: FUJITA Tomonori @ 2013-11-07 21:51 UTC (permalink / raw) To: dan.mick; +Cc: ronniesahlberg, stgt On Thu, 07 Nov 2013 13:46:36 -0800 Dan Mick <dan.mick@inktank.com> wrote: > >>>> I see, thanks. Using the bitmaps is simpler than the array of char if >>>> you calculate delta and such? >>> >>> I think using the array with opcode names is simpler for a human to >>> comparing when reading the sourcecode. >>> >>> >>> I had a bitmap of 32 bytes, one bit for each opcode, and I also >>> tried using an array of 256 bytes, one byte 0/1 for each opcode >>> but it was horrible to read from a human standpoint. >>> >>> When reading the code and the bitmap/array it was very difficult to >>> see which opcodes were supported and which were not >>> by just looking at the bits. >>> It was also errorprone and I did several mistakes when building the >>> bitmap manually. >> >> Why you built it manually? You can do something like >> >> set_bit(WRITE_6, bitmap_addr); >> >> ? >> >> As usual, you can steal bitmap functions from Linux kernel. > > Is it worth it to mess around with bits when chars are easily > manipulated? Sure, it's 8x the data, but it's much easier to dump, > examine in debuggers, etc., and it's 256 bytes; hardly worthy of > notice. > Bitmaps are a pain in general. Most of us (kernel people) are familar with bitmap ops rather than functions that we would invent. That's my point. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Add an array that describes which opcodes are supported by the RDWR and SHEEPDOG backends. 2013-11-07 21:51 ` FUJITA Tomonori @ 2013-11-07 22:15 ` Dan Mick 0 siblings, 0 replies; 16+ messages in thread From: Dan Mick @ 2013-11-07 22:15 UTC (permalink / raw) To: FUJITA Tomonori; +Cc: ronniesahlberg, stgt On 11/07/2013 01:51 PM, FUJITA Tomonori wrote: > On Thu, 07 Nov 2013 13:46:36 -0800 > Dan Mick <dan.mick@inktank.com> wrote: > >> >>>>> I see, thanks. Using the bitmaps is simpler than the array of char if >>>>> you calculate delta and such? >>>> >>>> I think using the array with opcode names is simpler for a human to >>>> comparing when reading the sourcecode. >>>> >>>> >>>> I had a bitmap of 32 bytes, one bit for each opcode, and I also >>>> tried using an array of 256 bytes, one byte 0/1 for each opcode >>>> but it was horrible to read from a human standpoint. >>>> >>>> When reading the code and the bitmap/array it was very difficult to >>>> see which opcodes were supported and which were not >>>> by just looking at the bits. >>>> It was also errorprone and I did several mistakes when building the >>>> bitmap manually. >>> >>> Why you built it manually? You can do something like >>> >>> set_bit(WRITE_6, bitmap_addr); >>> >>> ? >>> >>> As usual, you can steal bitmap functions from Linux kernel. >> >> Is it worth it to mess around with bits when chars are easily >> manipulated? Sure, it's 8x the data, but it's much easier to dump, >> examine in debuggers, etc., and it's 256 bytes; hardly worthy of >> notice. >> Bitmaps are a pain in general. > > Most of us (kernel people) are familar with bitmap ops rather than > functions that we would invent. That's my point. > Sure, I get that; I just mean it's not worth it for the space savings when the usability hit is there, IMO. But it's a matter of opinion. ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH] Add an array that describes which opcodes are supported by backends @ 2013-11-17 1:18 Ronnie Sahlberg 2013-11-17 1:18 ` [PATCH] Add an array that describes which opcodes are supported by the RDWR and SHEEPDOG backends Ronnie Sahlberg 0 siblings, 1 reply; 16+ messages in thread From: Ronnie Sahlberg @ 2013-11-17 1:18 UTC (permalink / raw) To: stgt; +Cc: fujita.tomonori Tomo, List Please find a new version of the patch to add per-backend bitmap to control which opcodes are supported and which are not. This version is changed to keep a 32 byte bitmap for the 256 opcodes instead of using a 256 byte array. Please review, apply. regards ronnie sahlberg ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH] Add an array that describes which opcodes are supported by the RDWR and SHEEPDOG backends. 2013-11-17 1:18 [PATCH] Add an array that describes which opcodes are supported by backends Ronnie Sahlberg @ 2013-11-17 1:18 ` Ronnie Sahlberg 2013-11-17 13:23 ` FUJITA Tomonori 0 siblings, 1 reply; 16+ messages in thread From: Ronnie Sahlberg @ 2013-11-17 1:18 UTC (permalink / raw) To: stgt; +Cc: fujita.tomonori, Ronnie Sahlberg While RDWR supports all SBC opcodes that TGTD implement SHEEPDOG only supports a subset and lacks the following opcodes: WRITE_VERIFY10/12/16 VERIFY10/12/16 PREFETCH10/16 WRITE_SAME10/16 UNMAP and ORWRITE This allows backends to specify which opcodes it is prepared to process and which commands should fail with invalid op code and allows SHEEPDOG backed LUNs to respond with INVALID_OP_CODE correctly. This is most useful for block devices where we have several different backends and where some backends only support a subset of the commands Signed-off-by: Ronnie Sahlberg <ronniesahlberg@gmail.com> --- usr/bs.c | 8 ++++++++ usr/bs_rdwr.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++ usr/bs_sheepdog.c | 38 ++++++++++++++++++++++++++++++++++++++ usr/scsi.c | 6 ++++++ usr/tgtd.h | 1 + 5 files changed, 104 insertions(+), 0 deletions(-) diff --git a/usr/bs.c b/usr/bs.c index 636f53b..a6a5852 100644 --- a/usr/bs.c +++ b/usr/bs.c @@ -463,3 +463,11 @@ int bs_thread_cmd_submit(struct scsi_cmd *cmd) return 0; } + +void bs_create_opcode_map(unsigned char *map, unsigned char *opcodes, int num) +{ + int i; + + for (i = 0; i < num; i++) + map[opcodes[i] >> 3] |= 1 << (opcodes[i] % 8); +} diff --git a/usr/bs_rdwr.c b/usr/bs_rdwr.c index ff009eb..01a710e 100644 --- a/usr/bs_rdwr.c +++ b/usr/bs_rdwr.c @@ -412,6 +412,8 @@ static void bs_rdwr_exit(struct scsi_lu *lu) bs_thread_close(info); } +static char bs_ops_supported[32]; + static struct backingstore_template rdwr_bst = { .bs_name = "rdwr", .bs_datasize = sizeof(struct bs_thread_info), @@ -421,9 +423,58 @@ static struct backingstore_template rdwr_bst = { .bs_exit = bs_rdwr_exit, .bs_cmd_submit = bs_thread_cmd_submit, .bs_oflags_supported = O_SYNC | O_DIRECT, + .bs_ops_supported = bs_ops_supported, }; __attribute__((constructor)) static void bs_rdwr_constructor(void) { + static char opcodes[] = { + ALLOW_MEDIUM_REMOVAL, + COMPARE_AND_WRITE, + FORMAT_UNIT, + INQUIRY, + MAINT_PROTOCOL_IN, + MODE_SELECT, + MODE_SELECT_10, + MODE_SENSE, + MODE_SENSE_10, + ORWRITE_16, + PERSISTENT_RESERVE_IN, + PERSISTENT_RESERVE_OUT, + PRE_FETCH_10, + PRE_FETCH_16, + READ_10, + READ_12, + READ_16, + READ_6, + READ_CAPACITY, + RELEASE, + REPORT_LUNS, + REQUEST_SENSE, + RESERVE, + SEND_DIAGNOSTIC, + SERVICE_ACTION_IN, + START_STOP, + SYNCHRONIZE_CACHE, + SYNCHRONIZE_CACHE_16, + TEST_UNIT_READY, + UNMAP, + VERIFY_10, + VERIFY_12, + VERIFY_16, + WRITE_10, + WRITE_12, + WRITE_16, + WRITE_6, + WRITE_SAME, + WRITE_SAME_16, + WRITE_VERIFY, + WRITE_VERIFY_12, + WRITE_VERIFY_16 + }; + + bs_create_opcode_map(bs_ops_supported, &opcodes[0], + sizeof(opcodes) / sizeof(opcodes[0])); + register_backingstore_template(&rdwr_bst); } diff --git a/usr/bs_sheepdog.c b/usr/bs_sheepdog.c index 1dda915..2d2b8a1 100644 --- a/usr/bs_sheepdog.c +++ b/usr/bs_sheepdog.c @@ -1283,6 +1283,8 @@ static void bs_sheepdog_exit(struct scsi_lu *lu) dprintf("cleaned logical unit %p safely\n", lu); } +static char bs_ops_supported[32]; + static struct backingstore_template sheepdog_bst = { .bs_name = "sheepdog", .bs_datasize = @@ -1292,9 +1294,45 @@ static struct backingstore_template sheepdog_bst = { .bs_init = bs_sheepdog_init, .bs_exit = bs_sheepdog_exit, .bs_cmd_submit = bs_thread_cmd_submit, + .bs_ops_supported = bs_ops_supported, }; __attribute__((constructor)) static void __constructor(void) { + static char opcodes[] = { + ALLOW_MEDIUM_REMOVAL, + FORMAT_UNIT, + INQUIRY, + MAINT_PROTOCOL_IN, + MODE_SELECT, + MODE_SELECT_10, + MODE_SENSE, + MODE_SENSE_10, + PERSISTENT_RESERVE_IN, + PERSISTENT_RESERVE_OUT, + READ_10, + READ_12, + READ_16, + READ_6, + READ_CAPACITY, + RELEASE, + REPORT_LUNS, + REQUEST_SENSE, + RESERVE, + SEND_DIAGNOSTIC, + SERVICE_ACTION_IN, + START_STOP, + SYNCHRONIZE_CACHE, + SYNCHRONIZE_CACHE_16, + TEST_UNIT_READY, + WRITE_10, + WRITE_12, + WRITE_16, + WRITE_6 + }; + + bs_create_opcode_map(bs_ops_supported, &opcodes[0], + sizeof(opcodes) / sizeof(opcodes[0])); + register_backingstore_template(&sheepdog_bst); } diff --git a/usr/scsi.c b/usr/scsi.c index 2636a5c..14c7c33 100644 --- a/usr/scsi.c +++ b/usr/scsi.c @@ -492,6 +492,12 @@ int scsi_cmd_perform(int host_no, struct scsi_cmd *cmd) if (spc_access_check(cmd)) return SAM_STAT_RESERVATION_CONFLICT; + if (cmd->dev->bst->bs_ops_supported + && !(cmd->dev->bst->bs_ops_supported[op >> 3] & (1 << (op % 8)))) { + sense_data_build(cmd, ILLEGAL_REQUEST, ASC_INVALID_OP_CODE); + return SAM_STAT_CHECK_CONDITION; + } + return cmd->dev->dev_type_template.ops[op].cmd_perform(host_no, cmd); } diff --git a/usr/tgtd.h b/usr/tgtd.h index b0528b4..568da01 100644 --- a/usr/tgtd.h +++ b/usr/tgtd.h @@ -165,6 +165,7 @@ struct backingstore_template { void (*bs_exit)(struct scsi_lu *dev); int (*bs_cmd_submit)(struct scsi_cmd *cmd); int bs_oflags_supported; + const char *bs_ops_supported; struct list_head backingstore_siblings; }; -- 1.7.3.1 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH] Add an array that describes which opcodes are supported by the RDWR and SHEEPDOG backends. 2013-11-17 1:18 ` [PATCH] Add an array that describes which opcodes are supported by the RDWR and SHEEPDOG backends Ronnie Sahlberg @ 2013-11-17 13:23 ` FUJITA Tomonori 2013-11-24 16:39 ` ronnie sahlberg 0 siblings, 1 reply; 16+ messages in thread From: FUJITA Tomonori @ 2013-11-17 13:23 UTC (permalink / raw) To: ronniesahlberg; +Cc: stgt, fujita.tomonori On Sat, 16 Nov 2013 17:18:26 -0800 Ronnie Sahlberg <ronniesahlberg@gmail.com> wrote: > While RDWR supports all SBC opcodes that TGTD implement SHEEPDOG > only supports a subset and lacks the following opcodes: > WRITE_VERIFY10/12/16 VERIFY10/12/16 PREFETCH10/16 > WRITE_SAME10/16 UNMAP and ORWRITE > > This allows backends to specify which opcodes it is prepared to process > and which commands should fail with invalid op code > and allows SHEEPDOG backed LUNs to respond with INVALID_OP_CODE > correctly. > > This is most useful for block devices where we have several different backends > and where some backends only support a subset of the commands > > Signed-off-by: Ronnie Sahlberg <ronniesahlberg@gmail.com> > --- > usr/bs.c | 8 ++++++++ > usr/bs_rdwr.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++ > usr/bs_sheepdog.c | 38 ++++++++++++++++++++++++++++++++++++++ > usr/scsi.c | 6 ++++++ > usr/tgtd.h | 1 + > 5 files changed, 104 insertions(+), 0 deletions(-) > > diff --git a/usr/bs.c b/usr/bs.c > index 636f53b..a6a5852 100644 > --- a/usr/bs.c > +++ b/usr/bs.c > @@ -463,3 +463,11 @@ int bs_thread_cmd_submit(struct scsi_cmd *cmd) > > return 0; > } > + > +void bs_create_opcode_map(unsigned char *map, unsigned char *opcodes, int num) > +{ > + int i; > + > + for (i = 0; i < num; i++) > + map[opcodes[i] >> 3] |= 1 << (opcodes[i] % 8); > +} > diff --git a/usr/bs_rdwr.c b/usr/bs_rdwr.c > index ff009eb..01a710e 100644 > --- a/usr/bs_rdwr.c > +++ b/usr/bs_rdwr.c > @@ -412,6 +412,8 @@ static void bs_rdwr_exit(struct scsi_lu *lu) > bs_thread_close(info); > } > > +static char bs_ops_supported[32]; I prefer to put this array to backingstore_template rather than let each backing store because all backing stores should to have this array. > static struct backingstore_template rdwr_bst = { > .bs_name = "rdwr", > .bs_datasize = sizeof(struct bs_thread_info), > @@ -421,9 +423,58 @@ static struct backingstore_template rdwr_bst = { > .bs_exit = bs_rdwr_exit, > .bs_cmd_submit = bs_thread_cmd_submit, > .bs_oflags_supported = O_SYNC | O_DIRECT, > + .bs_ops_supported = bs_ops_supported, > }; > > __attribute__((constructor)) static void bs_rdwr_constructor(void) > { > + static char opcodes[] = { > + ALLOW_MEDIUM_REMOVAL, > + COMPARE_AND_WRITE, > + FORMAT_UNIT, > + INQUIRY, > + MAINT_PROTOCOL_IN, > + MODE_SELECT, > + MODE_SELECT_10, > + MODE_SENSE, > + MODE_SENSE_10, > + ORWRITE_16, > + PERSISTENT_RESERVE_IN, > + PERSISTENT_RESERVE_OUT, > + PRE_FETCH_10, > + PRE_FETCH_16, > + READ_10, > + READ_12, > + READ_16, > + READ_6, > + READ_CAPACITY, > + RELEASE, > + REPORT_LUNS, > + REQUEST_SENSE, > + RESERVE, > + SEND_DIAGNOSTIC, > + SERVICE_ACTION_IN, > + START_STOP, > + SYNCHRONIZE_CACHE, > + SYNCHRONIZE_CACHE_16, > + TEST_UNIT_READY, > + UNMAP, > + VERIFY_10, > + VERIFY_12, > + VERIFY_16, > + WRITE_10, > + WRITE_12, > + WRITE_16, > + WRITE_6, > + WRITE_SAME, > + WRITE_SAME_16, > + WRITE_VERIFY, > + WRITE_VERIFY_12, > + WRITE_VERIFY_16 > + }; > + > + bs_create_opcode_map(bs_ops_supported, &opcodes[0], > + sizeof(opcodes) / sizeof(opcodes[0])); ARRAY_SIZE macro? > register_backingstore_template(&rdwr_bst); > } > diff --git a/usr/bs_sheepdog.c b/usr/bs_sheepdog.c > index 1dda915..2d2b8a1 100644 > --- a/usr/bs_sheepdog.c > +++ b/usr/bs_sheepdog.c > @@ -1283,6 +1283,8 @@ static void bs_sheepdog_exit(struct scsi_lu *lu) > dprintf("cleaned logical unit %p safely\n", lu); > } > > +static char bs_ops_supported[32]; > + > static struct backingstore_template sheepdog_bst = { > .bs_name = "sheepdog", > .bs_datasize = > @@ -1292,9 +1294,45 @@ static struct backingstore_template sheepdog_bst = { > .bs_init = bs_sheepdog_init, > .bs_exit = bs_sheepdog_exit, > .bs_cmd_submit = bs_thread_cmd_submit, > + .bs_ops_supported = bs_ops_supported, > }; > > __attribute__((constructor)) static void __constructor(void) > { > + static char opcodes[] = { > + ALLOW_MEDIUM_REMOVAL, > + FORMAT_UNIT, > + INQUIRY, > + MAINT_PROTOCOL_IN, > + MODE_SELECT, > + MODE_SELECT_10, > + MODE_SENSE, > + MODE_SENSE_10, > + PERSISTENT_RESERVE_IN, > + PERSISTENT_RESERVE_OUT, > + READ_10, > + READ_12, > + READ_16, > + READ_6, > + READ_CAPACITY, > + RELEASE, > + REPORT_LUNS, > + REQUEST_SENSE, > + RESERVE, > + SEND_DIAGNOSTIC, > + SERVICE_ACTION_IN, > + START_STOP, > + SYNCHRONIZE_CACHE, > + SYNCHRONIZE_CACHE_16, > + TEST_UNIT_READY, > + WRITE_10, > + WRITE_12, > + WRITE_16, > + WRITE_6 > + }; > + > + bs_create_opcode_map(bs_ops_supported, &opcodes[0], > + sizeof(opcodes) / sizeof(opcodes[0])); > + > register_backingstore_template(&sheepdog_bst); > } > diff --git a/usr/scsi.c b/usr/scsi.c > index 2636a5c..14c7c33 100644 > --- a/usr/scsi.c > +++ b/usr/scsi.c > @@ -492,6 +492,12 @@ int scsi_cmd_perform(int host_no, struct scsi_cmd *cmd) > if (spc_access_check(cmd)) > return SAM_STAT_RESERVATION_CONFLICT; > > + if (cmd->dev->bst->bs_ops_supported > + && !(cmd->dev->bst->bs_ops_supported[op >> 3] & (1 << (op % 8)))) { > + sense_data_build(cmd, ILLEGAL_REQUEST, ASC_INVALID_OP_CODE); > + return SAM_STAT_CHECK_CONDITION; > + } I think that the details of the array had better to exist in the same file including bs_create_opcode_map(). Here's the modified version. Not tested. diff --git a/usr/bs.c b/usr/bs.c index 636f53b..2c353af 100644 --- a/usr/bs.c +++ b/usr/bs.c @@ -45,6 +45,7 @@ #include "tgtadm_error.h" #include "util.h" #include "bs_thread.h" +#include "scsi.h" LIST_HEAD(bst_list); @@ -63,6 +64,27 @@ static pthread_t ack_thread; static LIST_HEAD(ack_list); static pthread_cond_t finished_cond; + +void bs_create_opcode_map(struct backingstore_template *bst, unsigned char *opcodes, int num) +{ + int i; + + for (i = 0; i < num; i++) + set_bit(opcodes[i], bst->bs_supported_ops); +} + +int is_bs_support_opcode(struct backingstore_template *bst, int op) +{ + /* + * assumes that this bs doesn't support supported_ops yet so + * returns success for the compatibility. + */ + if (!test_bit(TEST_UNIT_READY, bst->bs_supported_ops)) + return 1; + + return test_bit(op, bst->bs_supported_ops); +} + int register_backingstore_template(struct backingstore_template *bst) { list_add(&bst->backingstore_siblings, &bst_list); @@ -463,3 +485,4 @@ int bs_thread_cmd_submit(struct scsi_cmd *cmd) return 0; } + diff --git a/usr/bs_rdwr.c b/usr/bs_rdwr.c index ff009eb..a6301a7 100644 --- a/usr/bs_rdwr.c +++ b/usr/bs_rdwr.c @@ -425,5 +425,52 @@ static struct backingstore_template rdwr_bst = { __attribute__((constructor)) static void bs_rdwr_constructor(void) { + unsigned char opcodes[] = { + ALLOW_MEDIUM_REMOVAL, + COMPARE_AND_WRITE, + FORMAT_UNIT, + INQUIRY, + MAINT_PROTOCOL_IN, + MODE_SELECT, + MODE_SELECT_10, + MODE_SENSE, + MODE_SENSE_10, + ORWRITE_16, + PERSISTENT_RESERVE_IN, + PERSISTENT_RESERVE_OUT, + PRE_FETCH_10, + PRE_FETCH_16, + READ_10, + READ_12, + READ_16, + READ_6, + READ_CAPACITY, + RELEASE, + REPORT_LUNS, + REQUEST_SENSE, + RESERVE, + SEND_DIAGNOSTIC, + SERVICE_ACTION_IN, + START_STOP, + SYNCHRONIZE_CACHE, + SYNCHRONIZE_CACHE_16, + TEST_UNIT_READY, + UNMAP, + VERIFY_10, + VERIFY_12, + VERIFY_16, + WRITE_10, + WRITE_12, + WRITE_16, + WRITE_6, + WRITE_SAME, + WRITE_SAME_16, + WRITE_VERIFY, + WRITE_VERIFY_12, + WRITE_VERIFY_16 + }; + + bs_create_opcode_map(&rdwr_bst, opcodes, ARRAY_SIZE(opcodes)); + register_backingstore_template(&rdwr_bst); } diff --git a/usr/bs_sheepdog.c b/usr/bs_sheepdog.c index 1dda915..30af8ed 100644 --- a/usr/bs_sheepdog.c +++ b/usr/bs_sheepdog.c @@ -1296,5 +1296,39 @@ static struct backingstore_template sheepdog_bst = { __attribute__((constructor)) static void __constructor(void) { + unsigned char opcodes[] = { + ALLOW_MEDIUM_REMOVAL, + FORMAT_UNIT, + INQUIRY, + MAINT_PROTOCOL_IN, + MODE_SELECT, + MODE_SELECT_10, + MODE_SENSE, + MODE_SENSE_10, + PERSISTENT_RESERVE_IN, + PERSISTENT_RESERVE_OUT, + READ_10, + READ_12, + READ_16, + READ_6, + READ_CAPACITY, + RELEASE, + REPORT_LUNS, + REQUEST_SENSE, + RESERVE, + SEND_DIAGNOSTIC, + SERVICE_ACTION_IN, + START_STOP, + SYNCHRONIZE_CACHE, + SYNCHRONIZE_CACHE_16, + TEST_UNIT_READY, + WRITE_10, + WRITE_12, + WRITE_16, + WRITE_6 + }; + + bs_create_opcode_map(&sheepdog_bst, opcodes, ARRAY_SIZE(opcodes)); + register_backingstore_template(&sheepdog_bst); } diff --git a/usr/scsi.c b/usr/scsi.c index 2636a5c..d7c0095 100644 --- a/usr/scsi.c +++ b/usr/scsi.c @@ -492,6 +492,11 @@ int scsi_cmd_perform(int host_no, struct scsi_cmd *cmd) if (spc_access_check(cmd)) return SAM_STAT_RESERVATION_CONFLICT; + if (!is_bs_support_opcode(cmd->dev->bst, op)) { + sense_data_build(cmd, ILLEGAL_REQUEST, ASC_INVALID_OP_CODE); + return SAM_STAT_CHECK_CONDITION; + } + return cmd->dev->dev_type_template.ops[op].cmd_perform(host_no, cmd); } diff --git a/usr/tgtd.h b/usr/tgtd.h index b0528b4..510b025 100644 --- a/usr/tgtd.h +++ b/usr/tgtd.h @@ -7,6 +7,8 @@ struct concat_buf; +#define NR_SCSI_OPCODES 256 + #define SCSI_ID_LEN 36 #define SCSI_SN_LEN 36 @@ -165,6 +167,7 @@ struct backingstore_template { void (*bs_exit)(struct scsi_lu *dev); int (*bs_cmd_submit)(struct scsi_cmd *cmd); int bs_oflags_supported; + unsigned long bs_supported_ops[NR_SCSI_OPCODES / __WORDSIZE]; struct list_head backingstore_siblings; }; @@ -369,6 +372,8 @@ extern tgtadm_err dtd_check_removable(int tid, uint64_t lun); extern int register_backingstore_template(struct backingstore_template *bst); extern struct backingstore_template *get_backingstore_template(const char *name); +extern void bs_create_opcode_map(struct backingstore_template *bst, unsigned char *opcodes, int num); +extern int is_bs_support_opcode(struct backingstore_template *bst, int op); extern int lld_init_one(int lld_index); diff --git a/usr/util.h b/usr/util.h index d01d58b..08a6dd3 100644 --- a/usr/util.h +++ b/usr/util.h @@ -19,6 +19,7 @@ #include "be_byteshift.h" #define roundup(x, y) ((((x) + ((y) - 1)) / (y)) * (y)) +#define DIV_ROUND_UP(x, y) (((x) + (y) - 1) / (y)) #define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0])) #define ALIGN(x,a) (((x)+(a)-1)&~((a)-1)) @@ -214,10 +215,29 @@ static inline int unmap_file_region(int fd, off_t offset, off_t length) #ifdef FALLOC_FL_PUNCH_HOLE if (fallocate(fd, FALLOC_FL_KEEP_SIZE | FALLOC_FL_PUNCH_HOLE, offset, length) == 0) - return 0; + return 0; #endif return -1; } +#define BITS_PER_LONG __WORDSIZE +#define BITS_PER_BYTE 8 +#define BITS_TO_LONGS(nr) DIV_ROUND_UP(nr, BITS_PER_BYTE * sizeof(long)) + +static inline void set_bit(int nr, unsigned long *addr) +{ + addr[nr / BITS_PER_LONG] |= 1UL << (nr % BITS_PER_LONG); +} + +static inline void clear_bit(int nr, unsigned long *addr) +{ + addr[nr / BITS_PER_LONG] &= ~(1UL << (nr % BITS_PER_LONG)); +} + +static __always_inline int test_bit(unsigned int nr, const unsigned long *addr) +{ + return ((1UL << (nr % BITS_PER_LONG)) & + (((unsigned long *)addr)[nr / BITS_PER_LONG])) != 0; +} #endif ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH] Add an array that describes which opcodes are supported by the RDWR and SHEEPDOG backends. 2013-11-17 13:23 ` FUJITA Tomonori @ 2013-11-24 16:39 ` ronnie sahlberg 2013-11-24 21:06 ` FUJITA Tomonori 0 siblings, 1 reply; 16+ messages in thread From: ronnie sahlberg @ 2013-11-24 16:39 UTC (permalink / raw) To: FUJITA Tomonori; +Cc: stgt Thanks! This looks like it is working in my tests. Will you commit it or do you want me to send an updated patch with your version? On Sun, Nov 17, 2013 at 5:23 AM, FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote: > On Sat, 16 Nov 2013 17:18:26 -0800 > Ronnie Sahlberg <ronniesahlberg@gmail.com> wrote: > >> While RDWR supports all SBC opcodes that TGTD implement SHEEPDOG >> only supports a subset and lacks the following opcodes: >> WRITE_VERIFY10/12/16 VERIFY10/12/16 PREFETCH10/16 >> WRITE_SAME10/16 UNMAP and ORWRITE >> >> This allows backends to specify which opcodes it is prepared to process >> and which commands should fail with invalid op code >> and allows SHEEPDOG backed LUNs to respond with INVALID_OP_CODE >> correctly. >> >> This is most useful for block devices where we have several different backends >> and where some backends only support a subset of the commands >> >> Signed-off-by: Ronnie Sahlberg <ronniesahlberg@gmail.com> >> --- >> usr/bs.c | 8 ++++++++ >> usr/bs_rdwr.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++ >> usr/bs_sheepdog.c | 38 ++++++++++++++++++++++++++++++++++++++ >> usr/scsi.c | 6 ++++++ >> usr/tgtd.h | 1 + >> 5 files changed, 104 insertions(+), 0 deletions(-) >> >> diff --git a/usr/bs.c b/usr/bs.c >> index 636f53b..a6a5852 100644 >> --- a/usr/bs.c >> +++ b/usr/bs.c >> @@ -463,3 +463,11 @@ int bs_thread_cmd_submit(struct scsi_cmd *cmd) >> >> return 0; >> } >> + >> +void bs_create_opcode_map(unsigned char *map, unsigned char *opcodes, int num) >> +{ >> + int i; >> + >> + for (i = 0; i < num; i++) >> + map[opcodes[i] >> 3] |= 1 << (opcodes[i] % 8); >> +} >> diff --git a/usr/bs_rdwr.c b/usr/bs_rdwr.c >> index ff009eb..01a710e 100644 >> --- a/usr/bs_rdwr.c >> +++ b/usr/bs_rdwr.c >> @@ -412,6 +412,8 @@ static void bs_rdwr_exit(struct scsi_lu *lu) >> bs_thread_close(info); >> } >> >> +static char bs_ops_supported[32]; > > I prefer to put this array to backingstore_template rather than let > each backing store because all backing stores should to have this > array. > > >> static struct backingstore_template rdwr_bst = { >> .bs_name = "rdwr", >> .bs_datasize = sizeof(struct bs_thread_info), >> @@ -421,9 +423,58 @@ static struct backingstore_template rdwr_bst = { >> .bs_exit = bs_rdwr_exit, >> .bs_cmd_submit = bs_thread_cmd_submit, >> .bs_oflags_supported = O_SYNC | O_DIRECT, >> + .bs_ops_supported = bs_ops_supported, >> }; >> >> __attribute__((constructor)) static void bs_rdwr_constructor(void) >> { >> + static char opcodes[] = { >> + ALLOW_MEDIUM_REMOVAL, >> + COMPARE_AND_WRITE, >> + FORMAT_UNIT, >> + INQUIRY, >> + MAINT_PROTOCOL_IN, >> + MODE_SELECT, >> + MODE_SELECT_10, >> + MODE_SENSE, >> + MODE_SENSE_10, >> + ORWRITE_16, >> + PERSISTENT_RESERVE_IN, >> + PERSISTENT_RESERVE_OUT, >> + PRE_FETCH_10, >> + PRE_FETCH_16, >> + READ_10, >> + READ_12, >> + READ_16, >> + READ_6, >> + READ_CAPACITY, >> + RELEASE, >> + REPORT_LUNS, >> + REQUEST_SENSE, >> + RESERVE, >> + SEND_DIAGNOSTIC, >> + SERVICE_ACTION_IN, >> + START_STOP, >> + SYNCHRONIZE_CACHE, >> + SYNCHRONIZE_CACHE_16, >> + TEST_UNIT_READY, >> + UNMAP, >> + VERIFY_10, >> + VERIFY_12, >> + VERIFY_16, >> + WRITE_10, >> + WRITE_12, >> + WRITE_16, >> + WRITE_6, >> + WRITE_SAME, >> + WRITE_SAME_16, >> + WRITE_VERIFY, >> + WRITE_VERIFY_12, >> + WRITE_VERIFY_16 >> + }; >> + >> + bs_create_opcode_map(bs_ops_supported, &opcodes[0], >> + sizeof(opcodes) / sizeof(opcodes[0])); > > ARRAY_SIZE macro? > >> register_backingstore_template(&rdwr_bst); >> } >> diff --git a/usr/bs_sheepdog.c b/usr/bs_sheepdog.c >> index 1dda915..2d2b8a1 100644 >> --- a/usr/bs_sheepdog.c >> +++ b/usr/bs_sheepdog.c >> @@ -1283,6 +1283,8 @@ static void bs_sheepdog_exit(struct scsi_lu *lu) >> dprintf("cleaned logical unit %p safely\n", lu); >> } >> >> +static char bs_ops_supported[32]; >> + >> static struct backingstore_template sheepdog_bst = { >> .bs_name = "sheepdog", >> .bs_datasize = >> @@ -1292,9 +1294,45 @@ static struct backingstore_template sheepdog_bst = { >> .bs_init = bs_sheepdog_init, >> .bs_exit = bs_sheepdog_exit, >> .bs_cmd_submit = bs_thread_cmd_submit, >> + .bs_ops_supported = bs_ops_supported, >> }; >> >> __attribute__((constructor)) static void __constructor(void) >> { >> + static char opcodes[] = { >> + ALLOW_MEDIUM_REMOVAL, >> + FORMAT_UNIT, >> + INQUIRY, >> + MAINT_PROTOCOL_IN, >> + MODE_SELECT, >> + MODE_SELECT_10, >> + MODE_SENSE, >> + MODE_SENSE_10, >> + PERSISTENT_RESERVE_IN, >> + PERSISTENT_RESERVE_OUT, >> + READ_10, >> + READ_12, >> + READ_16, >> + READ_6, >> + READ_CAPACITY, >> + RELEASE, >> + REPORT_LUNS, >> + REQUEST_SENSE, >> + RESERVE, >> + SEND_DIAGNOSTIC, >> + SERVICE_ACTION_IN, >> + START_STOP, >> + SYNCHRONIZE_CACHE, >> + SYNCHRONIZE_CACHE_16, >> + TEST_UNIT_READY, >> + WRITE_10, >> + WRITE_12, >> + WRITE_16, >> + WRITE_6 >> + }; >> + >> + bs_create_opcode_map(bs_ops_supported, &opcodes[0], >> + sizeof(opcodes) / sizeof(opcodes[0])); >> + >> register_backingstore_template(&sheepdog_bst); >> } >> diff --git a/usr/scsi.c b/usr/scsi.c >> index 2636a5c..14c7c33 100644 >> --- a/usr/scsi.c >> +++ b/usr/scsi.c >> @@ -492,6 +492,12 @@ int scsi_cmd_perform(int host_no, struct scsi_cmd *cmd) >> if (spc_access_check(cmd)) >> return SAM_STAT_RESERVATION_CONFLICT; >> >> + if (cmd->dev->bst->bs_ops_supported >> + && !(cmd->dev->bst->bs_ops_supported[op >> 3] & (1 << (op % 8)))) { >> + sense_data_build(cmd, ILLEGAL_REQUEST, ASC_INVALID_OP_CODE); >> + return SAM_STAT_CHECK_CONDITION; >> + } > > I think that the details of the array had better to exist in the same > file including bs_create_opcode_map(). > > Here's the modified version. Not tested. > > diff --git a/usr/bs.c b/usr/bs.c > index 636f53b..2c353af 100644 > --- a/usr/bs.c > +++ b/usr/bs.c > @@ -45,6 +45,7 @@ > #include "tgtadm_error.h" > #include "util.h" > #include "bs_thread.h" > +#include "scsi.h" > > LIST_HEAD(bst_list); > > @@ -63,6 +64,27 @@ static pthread_t ack_thread; > static LIST_HEAD(ack_list); > static pthread_cond_t finished_cond; > > + > +void bs_create_opcode_map(struct backingstore_template *bst, unsigned char *opcodes, int num) > +{ > + int i; > + > + for (i = 0; i < num; i++) > + set_bit(opcodes[i], bst->bs_supported_ops); > +} > + > +int is_bs_support_opcode(struct backingstore_template *bst, int op) > +{ > + /* > + * assumes that this bs doesn't support supported_ops yet so > + * returns success for the compatibility. > + */ > + if (!test_bit(TEST_UNIT_READY, bst->bs_supported_ops)) > + return 1; > + > + return test_bit(op, bst->bs_supported_ops); > +} > + > int register_backingstore_template(struct backingstore_template *bst) > { > list_add(&bst->backingstore_siblings, &bst_list); > @@ -463,3 +485,4 @@ int bs_thread_cmd_submit(struct scsi_cmd *cmd) > > return 0; > } > + > diff --git a/usr/bs_rdwr.c b/usr/bs_rdwr.c > index ff009eb..a6301a7 100644 > --- a/usr/bs_rdwr.c > +++ b/usr/bs_rdwr.c > @@ -425,5 +425,52 @@ static struct backingstore_template rdwr_bst = { > > __attribute__((constructor)) static void bs_rdwr_constructor(void) > { > + unsigned char opcodes[] = { > + ALLOW_MEDIUM_REMOVAL, > + COMPARE_AND_WRITE, > + FORMAT_UNIT, > + INQUIRY, > + MAINT_PROTOCOL_IN, > + MODE_SELECT, > + MODE_SELECT_10, > + MODE_SENSE, > + MODE_SENSE_10, > + ORWRITE_16, > + PERSISTENT_RESERVE_IN, > + PERSISTENT_RESERVE_OUT, > + PRE_FETCH_10, > + PRE_FETCH_16, > + READ_10, > + READ_12, > + READ_16, > + READ_6, > + READ_CAPACITY, > + RELEASE, > + REPORT_LUNS, > + REQUEST_SENSE, > + RESERVE, > + SEND_DIAGNOSTIC, > + SERVICE_ACTION_IN, > + START_STOP, > + SYNCHRONIZE_CACHE, > + SYNCHRONIZE_CACHE_16, > + TEST_UNIT_READY, > + UNMAP, > + VERIFY_10, > + VERIFY_12, > + VERIFY_16, > + WRITE_10, > + WRITE_12, > + WRITE_16, > + WRITE_6, > + WRITE_SAME, > + WRITE_SAME_16, > + WRITE_VERIFY, > + WRITE_VERIFY_12, > + WRITE_VERIFY_16 > + }; > + > + bs_create_opcode_map(&rdwr_bst, opcodes, ARRAY_SIZE(opcodes)); > + > register_backingstore_template(&rdwr_bst); > } > diff --git a/usr/bs_sheepdog.c b/usr/bs_sheepdog.c > index 1dda915..30af8ed 100644 > --- a/usr/bs_sheepdog.c > +++ b/usr/bs_sheepdog.c > @@ -1296,5 +1296,39 @@ static struct backingstore_template sheepdog_bst = { > > __attribute__((constructor)) static void __constructor(void) > { > + unsigned char opcodes[] = { > + ALLOW_MEDIUM_REMOVAL, > + FORMAT_UNIT, > + INQUIRY, > + MAINT_PROTOCOL_IN, > + MODE_SELECT, > + MODE_SELECT_10, > + MODE_SENSE, > + MODE_SENSE_10, > + PERSISTENT_RESERVE_IN, > + PERSISTENT_RESERVE_OUT, > + READ_10, > + READ_12, > + READ_16, > + READ_6, > + READ_CAPACITY, > + RELEASE, > + REPORT_LUNS, > + REQUEST_SENSE, > + RESERVE, > + SEND_DIAGNOSTIC, > + SERVICE_ACTION_IN, > + START_STOP, > + SYNCHRONIZE_CACHE, > + SYNCHRONIZE_CACHE_16, > + TEST_UNIT_READY, > + WRITE_10, > + WRITE_12, > + WRITE_16, > + WRITE_6 > + }; > + > + bs_create_opcode_map(&sheepdog_bst, opcodes, ARRAY_SIZE(opcodes)); > + > register_backingstore_template(&sheepdog_bst); > } > diff --git a/usr/scsi.c b/usr/scsi.c > index 2636a5c..d7c0095 100644 > --- a/usr/scsi.c > +++ b/usr/scsi.c > @@ -492,6 +492,11 @@ int scsi_cmd_perform(int host_no, struct scsi_cmd *cmd) > if (spc_access_check(cmd)) > return SAM_STAT_RESERVATION_CONFLICT; > > + if (!is_bs_support_opcode(cmd->dev->bst, op)) { > + sense_data_build(cmd, ILLEGAL_REQUEST, ASC_INVALID_OP_CODE); > + return SAM_STAT_CHECK_CONDITION; > + } > + > return cmd->dev->dev_type_template.ops[op].cmd_perform(host_no, cmd); > } > > diff --git a/usr/tgtd.h b/usr/tgtd.h > index b0528b4..510b025 100644 > --- a/usr/tgtd.h > +++ b/usr/tgtd.h > @@ -7,6 +7,8 @@ > > struct concat_buf; > > +#define NR_SCSI_OPCODES 256 > + > #define SCSI_ID_LEN 36 > #define SCSI_SN_LEN 36 > > @@ -165,6 +167,7 @@ struct backingstore_template { > void (*bs_exit)(struct scsi_lu *dev); > int (*bs_cmd_submit)(struct scsi_cmd *cmd); > int bs_oflags_supported; > + unsigned long bs_supported_ops[NR_SCSI_OPCODES / __WORDSIZE]; > > struct list_head backingstore_siblings; > }; > @@ -369,6 +372,8 @@ extern tgtadm_err dtd_check_removable(int tid, uint64_t lun); > > extern int register_backingstore_template(struct backingstore_template *bst); > extern struct backingstore_template *get_backingstore_template(const char *name); > +extern void bs_create_opcode_map(struct backingstore_template *bst, unsigned char *opcodes, int num); > +extern int is_bs_support_opcode(struct backingstore_template *bst, int op); > > extern int lld_init_one(int lld_index); > > diff --git a/usr/util.h b/usr/util.h > index d01d58b..08a6dd3 100644 > --- a/usr/util.h > +++ b/usr/util.h > @@ -19,6 +19,7 @@ > #include "be_byteshift.h" > > #define roundup(x, y) ((((x) + ((y) - 1)) / (y)) * (y)) > +#define DIV_ROUND_UP(x, y) (((x) + (y) - 1) / (y)) > #define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0])) > #define ALIGN(x,a) (((x)+(a)-1)&~((a)-1)) > > @@ -214,10 +215,29 @@ static inline int unmap_file_region(int fd, off_t offset, off_t length) > #ifdef FALLOC_FL_PUNCH_HOLE > if (fallocate(fd, FALLOC_FL_KEEP_SIZE | FALLOC_FL_PUNCH_HOLE, > offset, length) == 0) > - return 0; > + return 0; > #endif > return -1; > } > > +#define BITS_PER_LONG __WORDSIZE > +#define BITS_PER_BYTE 8 > +#define BITS_TO_LONGS(nr) DIV_ROUND_UP(nr, BITS_PER_BYTE * sizeof(long)) > + > +static inline void set_bit(int nr, unsigned long *addr) > +{ > + addr[nr / BITS_PER_LONG] |= 1UL << (nr % BITS_PER_LONG); > +} > + > +static inline void clear_bit(int nr, unsigned long *addr) > +{ > + addr[nr / BITS_PER_LONG] &= ~(1UL << (nr % BITS_PER_LONG)); > +} > + > +static __always_inline int test_bit(unsigned int nr, const unsigned long *addr) > +{ > + return ((1UL << (nr % BITS_PER_LONG)) & > + (((unsigned long *)addr)[nr / BITS_PER_LONG])) != 0; > +} > > #endif ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Add an array that describes which opcodes are supported by the RDWR and SHEEPDOG backends. 2013-11-24 16:39 ` ronnie sahlberg @ 2013-11-24 21:06 ` FUJITA Tomonori 0 siblings, 0 replies; 16+ messages in thread From: FUJITA Tomonori @ 2013-11-24 21:06 UTC (permalink / raw) To: ronniesahlberg; +Cc: fujita.tomonori, stgt On Sun, 24 Nov 2013 08:39:05 -0800 ronnie sahlberg <ronniesahlberg@gmail.com> wrote: > This looks like it is working in my tests. Applied the following, thanks! = From b747c18406b1adb688cac6e92611b82ca9baa980 Mon Sep 17 00:00:00 2001 From: Ronnie Sahlberg <ronniesahlberg@gmail.com> Date: Mon, 25 Nov 2013 05:36:52 +0900 Subject: [PATCH] Add an array that describes which opcodes are supported While RDWR supports all SBC opcodes that TGTD implement SHEEPDOG only supports a subset and lacks the following opcodes: WRITE_VERIFY10/12/16 VERIFY10/12/16 PREFETCH10/16 WRITE_SAME10/16 UNMAP and ORWRITE This allows backends to specify which opcodes it is prepared to process and which commands should fail with invalid op code and allows SHEEPDOG backed LUNs to respond with INVALID_OP_CODE correctly. This is most useful for block devices where we have several different backends and where some backends only support a subset of the commands Changed by FUJITA Tomonori: - use ARRAY_SIZE() - use Linux bitmap functions - put the supported ops array into struct backingstore_template Signed-off-by: Ronnie Sahlberg <ronniesahlberg@gmail.com> Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> --- usr/bs.c | 24 ++++++++++++++++++++++++ usr/bs_rdwr.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++ usr/bs_sheepdog.c | 34 ++++++++++++++++++++++++++++++++++ usr/scsi.c | 5 +++++ usr/tgtd.h | 6 ++++++ usr/util.h | 22 +++++++++++++++++++++- 6 files changed, 137 insertions(+), 1 deletions(-) diff --git a/usr/bs.c b/usr/bs.c index 636f53b..b0ee66f 100644 --- a/usr/bs.c +++ b/usr/bs.c @@ -45,6 +45,7 @@ #include "tgtadm_error.h" #include "util.h" #include "bs_thread.h" +#include "scsi.h" LIST_HEAD(bst_list); @@ -63,6 +64,28 @@ static pthread_t ack_thread; static LIST_HEAD(ack_list); static pthread_cond_t finished_cond; + +void bs_create_opcode_map(struct backingstore_template *bst, + unsigned char *opcodes, int num) +{ + int i; + + for (i = 0; i < num; i++) + set_bit(opcodes[i], bst->bs_supported_ops); +} + +int is_bs_support_opcode(struct backingstore_template *bst, int op) +{ + /* + * assumes that this bs doesn't support supported_ops yet so + * returns success for the compatibility. + */ + if (!test_bit(TEST_UNIT_READY, bst->bs_supported_ops)) + return 1; + + return test_bit(op, bst->bs_supported_ops); +} + int register_backingstore_template(struct backingstore_template *bst) { list_add(&bst->backingstore_siblings, &bst_list); @@ -463,3 +486,4 @@ int bs_thread_cmd_submit(struct scsi_cmd *cmd) return 0; } + diff --git a/usr/bs_rdwr.c b/usr/bs_rdwr.c index ff009eb..a6301a7 100644 --- a/usr/bs_rdwr.c +++ b/usr/bs_rdwr.c @@ -425,5 +425,52 @@ static struct backingstore_template rdwr_bst = { __attribute__((constructor)) static void bs_rdwr_constructor(void) { + unsigned char opcodes[] = { + ALLOW_MEDIUM_REMOVAL, + COMPARE_AND_WRITE, + FORMAT_UNIT, + INQUIRY, + MAINT_PROTOCOL_IN, + MODE_SELECT, + MODE_SELECT_10, + MODE_SENSE, + MODE_SENSE_10, + ORWRITE_16, + PERSISTENT_RESERVE_IN, + PERSISTENT_RESERVE_OUT, + PRE_FETCH_10, + PRE_FETCH_16, + READ_10, + READ_12, + READ_16, + READ_6, + READ_CAPACITY, + RELEASE, + REPORT_LUNS, + REQUEST_SENSE, + RESERVE, + SEND_DIAGNOSTIC, + SERVICE_ACTION_IN, + START_STOP, + SYNCHRONIZE_CACHE, + SYNCHRONIZE_CACHE_16, + TEST_UNIT_READY, + UNMAP, + VERIFY_10, + VERIFY_12, + VERIFY_16, + WRITE_10, + WRITE_12, + WRITE_16, + WRITE_6, + WRITE_SAME, + WRITE_SAME_16, + WRITE_VERIFY, + WRITE_VERIFY_12, + WRITE_VERIFY_16 + }; + + bs_create_opcode_map(&rdwr_bst, opcodes, ARRAY_SIZE(opcodes)); + register_backingstore_template(&rdwr_bst); } diff --git a/usr/bs_sheepdog.c b/usr/bs_sheepdog.c index 16547d0..d3572db 100644 --- a/usr/bs_sheepdog.c +++ b/usr/bs_sheepdog.c @@ -1296,5 +1296,39 @@ static struct backingstore_template sheepdog_bst = { __attribute__((constructor)) static void __constructor(void) { + unsigned char opcodes[] = { + ALLOW_MEDIUM_REMOVAL, + FORMAT_UNIT, + INQUIRY, + MAINT_PROTOCOL_IN, + MODE_SELECT, + MODE_SELECT_10, + MODE_SENSE, + MODE_SENSE_10, + PERSISTENT_RESERVE_IN, + PERSISTENT_RESERVE_OUT, + READ_10, + READ_12, + READ_16, + READ_6, + READ_CAPACITY, + RELEASE, + REPORT_LUNS, + REQUEST_SENSE, + RESERVE, + SEND_DIAGNOSTIC, + SERVICE_ACTION_IN, + START_STOP, + SYNCHRONIZE_CACHE, + SYNCHRONIZE_CACHE_16, + TEST_UNIT_READY, + WRITE_10, + WRITE_12, + WRITE_16, + WRITE_6 + }; + + bs_create_opcode_map(&sheepdog_bst, opcodes, ARRAY_SIZE(opcodes)); + register_backingstore_template(&sheepdog_bst); } diff --git a/usr/scsi.c b/usr/scsi.c index 2636a5c..d7c0095 100644 --- a/usr/scsi.c +++ b/usr/scsi.c @@ -492,6 +492,11 @@ int scsi_cmd_perform(int host_no, struct scsi_cmd *cmd) if (spc_access_check(cmd)) return SAM_STAT_RESERVATION_CONFLICT; + if (!is_bs_support_opcode(cmd->dev->bst, op)) { + sense_data_build(cmd, ILLEGAL_REQUEST, ASC_INVALID_OP_CODE); + return SAM_STAT_CHECK_CONDITION; + } + return cmd->dev->dev_type_template.ops[op].cmd_perform(host_no, cmd); } diff --git a/usr/tgtd.h b/usr/tgtd.h index b0528b4..850a110 100644 --- a/usr/tgtd.h +++ b/usr/tgtd.h @@ -7,6 +7,8 @@ struct concat_buf; +#define NR_SCSI_OPCODES 256 + #define SCSI_ID_LEN 36 #define SCSI_SN_LEN 36 @@ -165,6 +167,7 @@ struct backingstore_template { void (*bs_exit)(struct scsi_lu *dev); int (*bs_cmd_submit)(struct scsi_cmd *cmd); int bs_oflags_supported; + unsigned long bs_supported_ops[NR_SCSI_OPCODES / __WORDSIZE]; struct list_head backingstore_siblings; }; @@ -369,6 +372,9 @@ extern tgtadm_err dtd_check_removable(int tid, uint64_t lun); extern int register_backingstore_template(struct backingstore_template *bst); extern struct backingstore_template *get_backingstore_template(const char *name); +extern void bs_create_opcode_map(struct backingstore_template *bst, + unsigned char *opcodes, int num); +extern int is_bs_support_opcode(struct backingstore_template *bst, int op); extern int lld_init_one(int lld_index); diff --git a/usr/util.h b/usr/util.h index d01d58b..08a6dd3 100644 --- a/usr/util.h +++ b/usr/util.h @@ -19,6 +19,7 @@ #include "be_byteshift.h" #define roundup(x, y) ((((x) + ((y) - 1)) / (y)) * (y)) +#define DIV_ROUND_UP(x, y) (((x) + (y) - 1) / (y)) #define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0])) #define ALIGN(x,a) (((x)+(a)-1)&~((a)-1)) @@ -214,10 +215,29 @@ static inline int unmap_file_region(int fd, off_t offset, off_t length) #ifdef FALLOC_FL_PUNCH_HOLE if (fallocate(fd, FALLOC_FL_KEEP_SIZE | FALLOC_FL_PUNCH_HOLE, offset, length) == 0) - return 0; + return 0; #endif return -1; } +#define BITS_PER_LONG __WORDSIZE +#define BITS_PER_BYTE 8 +#define BITS_TO_LONGS(nr) DIV_ROUND_UP(nr, BITS_PER_BYTE * sizeof(long)) + +static inline void set_bit(int nr, unsigned long *addr) +{ + addr[nr / BITS_PER_LONG] |= 1UL << (nr % BITS_PER_LONG); +} + +static inline void clear_bit(int nr, unsigned long *addr) +{ + addr[nr / BITS_PER_LONG] &= ~(1UL << (nr % BITS_PER_LONG)); +} + +static __always_inline int test_bit(unsigned int nr, const unsigned long *addr) +{ + return ((1UL << (nr % BITS_PER_LONG)) & + (((unsigned long *)addr)[nr / BITS_PER_LONG])) != 0; +} #endif -- 1.7.2.5 ^ permalink raw reply related [flat|nested] 16+ messages in thread
end of thread, other threads:[~2013-11-24 21:06 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <1381588740-25968-1-git-send-email-ronniesahlberg@gmail.com>
[not found] ` <1381588740-25968-2-git-send-email-ronniesahlberg@gmail.com>
2013-10-16 16:05 ` [PATCH] Add an array that describes which opcodes are supported by the RDWR and SHEEPDOG backends ronnie sahlberg
2013-10-17 13:47 ` Hitoshi Mitake
2013-10-20 13:38 ` ronnie sahlberg
2013-10-21 1:34 ` FUJITA Tomonori
2013-10-31 1:44 ` ronnie sahlberg
2013-11-05 14:39 ` ronnie sahlberg
2013-11-07 7:07 ` FUJITA Tomonori
2013-11-07 16:42 ` ronnie sahlberg
2013-11-07 21:39 ` FUJITA Tomonori
2013-11-07 21:46 ` Dan Mick
2013-11-07 21:51 ` FUJITA Tomonori
2013-11-07 22:15 ` Dan Mick
2013-11-17 1:18 [PATCH] Add an array that describes which opcodes are supported by backends Ronnie Sahlberg
2013-11-17 1:18 ` [PATCH] Add an array that describes which opcodes are supported by the RDWR and SHEEPDOG backends Ronnie Sahlberg
2013-11-17 13:23 ` FUJITA Tomonori
2013-11-24 16:39 ` ronnie sahlberg
2013-11-24 21:06 ` FUJITA Tomonori
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.