| Message ID | 20221026231945.6609-5-michael.christie@oracle.com |
|---|---|
| State | New |
| Headers | show |
| Series | Use block pr_ops in LIO | expand |
Hi Mike,
I love your patch! Perhaps something to improve:
[auto build test WARNING on mkp-scsi/for-next]
[also build test WARNING on jejb-scsi/for-next device-mapper-dm/for-next linus/master v6.1-rc2 next-20221026]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Mike-Christie/Use-block-pr_ops-in-LIO/20221027-072653
base: https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git for-next
patch link: https://lore.kernel.org/r/20221026231945.6609-5-michael.christie%40oracle.com
patch subject: [PATCH v3 04/19] scsi: Add support for block PR read keys/reservation
config: x86_64-allyesconfig
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
reproduce (this is a W=1 build):
# https://github.com/intel-lab-lkp/linux/commit/f8aa9652526890d87007c8a643bd69ac723c053b
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Mike-Christie/Use-block-pr_ops-in-LIO/20221027-072653
git checkout f8aa9652526890d87007c8a643bd69ac723c053b
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/scsi/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
drivers/scsi/sd.c: In function 'sd_pr_in_command':
drivers/scsi/sd.c:1692:29: error: array type has incomplete element type 'struct scsi_failure'
1692 | struct scsi_failure failures[] = {
| ^~~~~~~~
drivers/scsi/sd.c:1695:32: error: 'SCMD_FAILURE_ASC_ANY' undeclared (first use in this function)
1695 | .asc = SCMD_FAILURE_ASC_ANY,
| ^~~~~~~~~~~~~~~~~~~~
drivers/scsi/sd.c:1695:32: note: each undeclared identifier is reported only once for each function it appears in
drivers/scsi/sd.c:1696:33: error: 'SCMD_FAILURE_ASCQ_ANY' undeclared (first use in this function)
1696 | .ascq = SCMD_FAILURE_ASCQ_ANY,
| ^~~~~~~~~~~~~~~~~~~~~
drivers/scsi/sd.c:1706:18: error: implicit declaration of function 'scsi_exec_req'; did you mean 'scsi_execute_req'? [-Werror=implicit-function-declaration]
1706 | result = scsi_exec_req(((struct scsi_exec_args) {
| ^~~~~~~~~~~~~
| scsi_execute_req
drivers/scsi/sd.c:1707:42: error: 'struct scsi_exec_args' has no member named 'sdev'
1707 | .sdev = sdev,
| ^~~~
drivers/scsi/sd.c:1707:49: warning: excess elements in struct initializer
1707 | .sdev = sdev,
| ^~~~
drivers/scsi/sd.c:1707:49: note: (near initialization for '(anonymous)')
drivers/scsi/sd.c:1708:42: error: 'struct scsi_exec_args' has no member named 'cmd'
1708 | .cmd = cmd,
| ^~~
drivers/scsi/sd.c:1708:48: warning: excess elements in struct initializer
1708 | .cmd = cmd,
| ^~~
drivers/scsi/sd.c:1708:48: note: (near initialization for '(anonymous)')
drivers/scsi/sd.c:1709:42: error: 'struct scsi_exec_args' has no member named 'data_dir'
1709 | .data_dir = DMA_FROM_DEVICE,
| ^~~~~~~~
drivers/scsi/sd.c:1709:53: warning: excess elements in struct initializer
1709 | .data_dir = DMA_FROM_DEVICE,
| ^~~~~~~~~~~~~~~
drivers/scsi/sd.c:1709:53: note: (near initialization for '(anonymous)')
drivers/scsi/sd.c:1710:42: error: 'struct scsi_exec_args' has no member named 'buf'
1710 | .buf = data,
| ^~~
drivers/scsi/sd.c:1710:48: warning: excess elements in struct initializer
1710 | .buf = data,
| ^~~~
drivers/scsi/sd.c:1710:48: note: (near initialization for '(anonymous)')
drivers/scsi/sd.c:1711:42: error: 'struct scsi_exec_args' has no member named 'buf_len'
1711 | .buf_len = data_len,
| ^~~~~~~
drivers/scsi/sd.c:1711:52: warning: excess elements in struct initializer
1711 | .buf_len = data_len,
| ^~~~~~~~
drivers/scsi/sd.c:1711:52: note: (near initialization for '(anonymous)')
drivers/scsi/sd.c:1712:42: error: 'struct scsi_exec_args' has no member named 'sshdr'
1712 | .sshdr = &sshdr,
| ^~~~~
drivers/scsi/sd.c:1712:50: warning: excess elements in struct initializer
1712 | .sshdr = &sshdr,
| ^
drivers/scsi/sd.c:1712:50: note: (near initialization for '(anonymous)')
drivers/scsi/sd.c:1713:42: error: 'struct scsi_exec_args' has no member named 'timeout'
1713 | .timeout = SD_TIMEOUT,
| ^~~~~~~
In file included from drivers/scsi/sd.c:72:
drivers/scsi/sd.h:15:33: warning: excess elements in struct initializer
15 | #define SD_TIMEOUT (30 * HZ)
| ^
drivers/scsi/sd.c:1713:52: note: in expansion of macro 'SD_TIMEOUT'
1713 | .timeout = SD_TIMEOUT,
| ^~~~~~~~~~
drivers/scsi/sd.h:15:33: note: (near initialization for '(anonymous)')
15 | #define SD_TIMEOUT (30 * HZ)
| ^
drivers/scsi/sd.c:1713:52: note: in expansion of macro 'SD_TIMEOUT'
1713 | .timeout = SD_TIMEOUT,
| ^~~~~~~~~~
drivers/scsi/sd.c:1714:42: error: 'struct scsi_exec_args' has no member named 'retries'
1714 | .retries = sdkp->max_retries,
| ^~~~~~~
drivers/scsi/sd.c:1714:52: warning: excess elements in struct initializer
1714 | .retries = sdkp->max_retries,
| ^~~~
drivers/scsi/sd.c:1714:52: note: (near initialization for '(anonymous)')
drivers/scsi/sd.c:1715:42: error: 'struct scsi_exec_args' has no member named 'failures'
1715 | .failures = failures }));
| ^~~~~~~~
drivers/scsi/sd.c:1715:53: warning: excess elements in struct initializer
1715 | .failures = failures }));
| ^~~~~~~~
drivers/scsi/sd.c:1715:53: note: (near initialization for '(anonymous)')
drivers/scsi/sd.c:1706:57: error: invalid use of undefined type 'struct scsi_exec_args'
1706 | result = scsi_exec_req(((struct scsi_exec_args) {
| ^
>> drivers/scsi/sd.c:1692:29: warning: unused variable 'failures' [-Wunused-variable]
1692 | struct scsi_failure failures[] = {
| ^~~~~~~~
cc1: some warnings being treated as errors
vim +/failures +1692 drivers/scsi/sd.c
1684
1685 static int sd_pr_in_command(struct block_device *bdev, u8 sa,
1686 unsigned char *data, int data_len)
1687 {
1688 struct scsi_disk *sdkp = scsi_disk(bdev->bd_disk);
1689 struct scsi_device *sdev = sdkp->device;
1690 struct scsi_sense_hdr sshdr;
1691 u8 cmd[10] = { PERSISTENT_RESERVE_IN, sa };
> 1692 struct scsi_failure failures[] = {
1693 {
1694 .sense = UNIT_ATTENTION,
1695 .asc = SCMD_FAILURE_ASC_ANY,
1696 .ascq = SCMD_FAILURE_ASCQ_ANY,
1697 .allowed = 5,
1698 .result = SAM_STAT_CHECK_CONDITION,
1699 },
1700 {},
1701 };
1702 int result;
1703
1704 put_unaligned_be16(data_len, &cmd[7]);
1705
1706 result = scsi_exec_req(((struct scsi_exec_args) {
1707 .sdev = sdev,
1708 .cmd = cmd,
1709 .data_dir = DMA_FROM_DEVICE,
1710 .buf = data,
1711 .buf_len = data_len,
1712 .sshdr = &sshdr,
1713 .timeout = SD_TIMEOUT,
1714 .retries = sdkp->max_retries,
1715 .failures = failures }));
1716 if (scsi_status_is_check_condition(result) &&
1717 scsi_sense_valid(&sshdr)) {
1718 sdev_printk(KERN_INFO, sdev, "PR command failed: %d\n", result);
1719 scsi_print_sense_hdr(sdev, NULL, &sshdr);
1720 }
1721
1722 return result;
1723 }
1724
Hi Mike,
I love your patch! Yet something to improve:
[auto build test ERROR on mkp-scsi/for-next]
[also build test ERROR on jejb-scsi/for-next device-mapper-dm/for-next linus/master v6.1-rc2 next-20221027]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Mike-Christie/Use-block-pr_ops-in-LIO/20221027-072653
base: https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git for-next
patch link: https://lore.kernel.org/r/20221026231945.6609-5-michael.christie%40oracle.com
patch subject: [PATCH v3 04/19] scsi: Add support for block PR read keys/reservation
config: powerpc-randconfig-r006-20221026
compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project 791a7ae1ba3efd6bca96338e10ffde557ba83920)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install powerpc cross compiling tool for clang build
# apt-get install binutils-powerpc-linux-gnu
# https://github.com/intel-lab-lkp/linux/commit/f8aa9652526890d87007c8a643bd69ac723c053b
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Mike-Christie/Use-block-pr_ops-in-LIO/20221027-072653
git checkout f8aa9652526890d87007c8a643bd69ac723c053b
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=powerpc SHELL=/bin/bash drivers/scsi/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
>> drivers/scsi/sd.c:1692:30: error: array has incomplete element type 'struct scsi_failure'
struct scsi_failure failures[] = {
^
drivers/scsi/sd.c:1692:9: note: forward declaration of 'struct scsi_failure'
struct scsi_failure failures[] = {
^
>> drivers/scsi/sd.c:1695:11: error: use of undeclared identifier 'SCMD_FAILURE_ASC_ANY'
.asc = SCMD_FAILURE_ASC_ANY,
^
>> drivers/scsi/sd.c:1696:12: error: use of undeclared identifier 'SCMD_FAILURE_ASCQ_ANY'
.ascq = SCMD_FAILURE_ASCQ_ANY,
^
>> drivers/scsi/sd.c:1706:11: error: call to undeclared function 'scsi_exec_req'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
result = scsi_exec_req(((struct scsi_exec_args) {
^
drivers/scsi/sd.c:1706:11: note: did you mean 'scsi_execute_req'?
include/scsi/scsi_device.h:472:19: note: 'scsi_execute_req' declared here
static inline int scsi_execute_req(struct scsi_device *sdev,
^
>> drivers/scsi/sd.c:1706:26: error: variable has incomplete type 'struct scsi_exec_args'
result = scsi_exec_req(((struct scsi_exec_args) {
^~~~~~~~~~~~~~~~~~~~~~~~~
drivers/scsi/sd.c:1706:34: note: forward declaration of 'struct scsi_exec_args'
result = scsi_exec_req(((struct scsi_exec_args) {
^
5 errors generated.
vim +1692 drivers/scsi/sd.c
1684
1685 static int sd_pr_in_command(struct block_device *bdev, u8 sa,
1686 unsigned char *data, int data_len)
1687 {
1688 struct scsi_disk *sdkp = scsi_disk(bdev->bd_disk);
1689 struct scsi_device *sdev = sdkp->device;
1690 struct scsi_sense_hdr sshdr;
1691 u8 cmd[10] = { PERSISTENT_RESERVE_IN, sa };
> 1692 struct scsi_failure failures[] = {
1693 {
1694 .sense = UNIT_ATTENTION,
> 1695 .asc = SCMD_FAILURE_ASC_ANY,
> 1696 .ascq = SCMD_FAILURE_ASCQ_ANY,
1697 .allowed = 5,
1698 .result = SAM_STAT_CHECK_CONDITION,
1699 },
1700 {},
1701 };
1702 int result;
1703
1704 put_unaligned_be16(data_len, &cmd[7]);
1705
> 1706 result = scsi_exec_req(((struct scsi_exec_args) {
1707 .sdev = sdev,
1708 .cmd = cmd,
1709 .data_dir = DMA_FROM_DEVICE,
1710 .buf = data,
1711 .buf_len = data_len,
1712 .sshdr = &sshdr,
1713 .timeout = SD_TIMEOUT,
1714 .retries = sdkp->max_retries,
1715 .failures = failures }));
1716 if (scsi_status_is_check_condition(result) &&
1717 scsi_sense_valid(&sshdr)) {
1718 sdev_printk(KERN_INFO, sdev, "PR command failed: %d\n", result);
1719 scsi_print_sense_hdr(sdev, NULL, &sshdr);
1720 }
1721
1722 return result;
1723 }
1724
> +static inline enum pr_type scsi_pr_type_to_block(enum scsi_pr_type type) > +{ > + switch (type) { > + case SCSI_PR_WRITE_EXCLUSIVE: > + return PR_WRITE_EXCLUSIVE; > + case SCSI_PR_EXCLUSIVE_ACCESS: > + return PR_EXCLUSIVE_ACCESS; > + case SCSI_PR_WRITE_EXCLUSIVE_REG_ONLY: > + return PR_WRITE_EXCLUSIVE_REG_ONLY; > + case SCSI_PR_EXCLUSIVE_ACCESS_REG_ONLY: > + return PR_EXCLUSIVE_ACCESS_REG_ONLY; > + case SCSI_PR_WRITE_EXCLUSIVE_ALL_REGS: > + return PR_WRITE_EXCLUSIVE_ALL_REGS; > + case SCSI_PR_EXCLUSIVE_ACCESS_ALL_REGS: > + return PR_EXCLUSIVE_ACCESS_ALL_REGS; > + default: > + return 0; > + } > +} > + same here as previous comment, unless there is strong reason not to do that ... -ck
On 10/26/22 16:19, Mike Christie wrote: > +static inline enum pr_type scsi_pr_type_to_block(enum scsi_pr_type type) > +{ > + switch (type) { > + case SCSI_PR_WRITE_EXCLUSIVE: > + return PR_WRITE_EXCLUSIVE; > + case SCSI_PR_EXCLUSIVE_ACCESS: > + return PR_EXCLUSIVE_ACCESS; > + case SCSI_PR_WRITE_EXCLUSIVE_REG_ONLY: > + return PR_WRITE_EXCLUSIVE_REG_ONLY; > + case SCSI_PR_EXCLUSIVE_ACCESS_REG_ONLY: > + return PR_EXCLUSIVE_ACCESS_REG_ONLY; > + case SCSI_PR_WRITE_EXCLUSIVE_ALL_REGS: > + return PR_WRITE_EXCLUSIVE_ALL_REGS; > + case SCSI_PR_EXCLUSIVE_ACCESS_ALL_REGS: > + return PR_EXCLUSIVE_ACCESS_ALL_REGS; > + default: > + return 0; > + } > +} Also for this function, how about moving the "return 0" statement out of the switch statement? Thanks, Bart.
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c index ad9374b07585..86b602399000 100644 --- a/drivers/scsi/sd.c +++ b/drivers/scsi/sd.c @@ -1695,6 +1695,108 @@ static int sd_get_unique_id(struct gendisk *disk, u8 id[16], return ret; } +static int sd_pr_in_command(struct block_device *bdev, u8 sa, + unsigned char *data, int data_len) +{ + struct scsi_disk *sdkp = scsi_disk(bdev->bd_disk); + struct scsi_device *sdev = sdkp->device; + struct scsi_sense_hdr sshdr; + u8 cmd[10] = { PERSISTENT_RESERVE_IN, sa }; + struct scsi_failure failures[] = { + { + .sense = UNIT_ATTENTION, + .asc = SCMD_FAILURE_ASC_ANY, + .ascq = SCMD_FAILURE_ASCQ_ANY, + .allowed = 5, + .result = SAM_STAT_CHECK_CONDITION, + }, + {}, + }; + int result; + + put_unaligned_be16(data_len, &cmd[7]); + + result = scsi_exec_req(((struct scsi_exec_args) { + .sdev = sdev, + .cmd = cmd, + .data_dir = DMA_FROM_DEVICE, + .buf = data, + .buf_len = data_len, + .sshdr = &sshdr, + .timeout = SD_TIMEOUT, + .retries = sdkp->max_retries, + .failures = failures })); + if (scsi_status_is_check_condition(result) && + scsi_sense_valid(&sshdr)) { + sdev_printk(KERN_INFO, sdev, "PR command failed: %d\n", result); + scsi_print_sense_hdr(sdev, NULL, &sshdr); + } + + return result; +} + +static int sd_pr_read_keys(struct block_device *bdev, struct pr_keys *keys_info, + u32 keys_len) +{ + int result, i, data_offset, num_copy_keys; + int data_len = keys_len + 8; + u8 *data; + + data = kzalloc(data_len, GFP_KERNEL); + if (!data) + return -ENOMEM; + + result = sd_pr_in_command(bdev, READ_KEYS, data, data_len); + if (result) + goto free_data; + + keys_info->generation = get_unaligned_be32(&data[0]); + keys_info->num_keys = get_unaligned_be32(&data[4]) / 8; + + data_offset = 8; + num_copy_keys = min(keys_len / 8, keys_info->num_keys); + + for (i = 0; i < num_copy_keys; i++) { + keys_info->keys[i] = get_unaligned_be64(&data[data_offset]); + data_offset += 8; + } + +free_data: + kfree(data); + return result; +} + +static int sd_pr_read_reservation(struct block_device *bdev, + struct pr_held_reservation *rsv) +{ + struct scsi_disk *sdkp = scsi_disk(bdev->bd_disk); + struct scsi_device *sdev = sdkp->device; + u8 data[24] = { 0, }; + int result, len; + + result = sd_pr_in_command(bdev, READ_RESERVATION, data, sizeof(data)); + if (result) + return result; + + memset(rsv, 0, sizeof(*rsv)); + len = get_unaligned_be32(&data[4]); + if (!len) + return result; + + /* Make sure we have at least the key and type */ + if (len < 14) { + sdev_printk(KERN_INFO, sdev, + "READ RESERVATION failed due to short return buffer of %d bytes\n", + len); + return -EINVAL; + } + + rsv->generation = get_unaligned_be32(&data[0]); + rsv->key = get_unaligned_be64(&data[8]); + rsv->type = scsi_pr_type_to_block(data[21] & 0x0f); + return 0; +} + static int sd_pr_out_command(struct block_device *bdev, u8 sa, u64 key, u64 sa_key, enum scsi_pr_type type, u8 flags) { @@ -1787,6 +1889,8 @@ static const struct pr_ops sd_pr_ops = { .pr_release = sd_pr_release, .pr_preempt = sd_pr_preempt, .pr_clear = sd_pr_clear, + .pr_read_keys = sd_pr_read_keys, + .pr_read_reservation = sd_pr_read_reservation, }; static void scsi_disk_free_disk(struct gendisk *disk) diff --git a/include/scsi/scsi_block_pr.h b/include/scsi/scsi_block_pr.h index 6e99f844330d..137bf2247273 100644 --- a/include/scsi/scsi_block_pr.h +++ b/include/scsi/scsi_block_pr.h @@ -33,4 +33,24 @@ static inline enum scsi_pr_type block_pr_type_to_scsi(enum pr_type type) } }; +static inline enum pr_type scsi_pr_type_to_block(enum scsi_pr_type type) +{ + switch (type) { + case SCSI_PR_WRITE_EXCLUSIVE: + return PR_WRITE_EXCLUSIVE; + case SCSI_PR_EXCLUSIVE_ACCESS: + return PR_EXCLUSIVE_ACCESS; + case SCSI_PR_WRITE_EXCLUSIVE_REG_ONLY: + return PR_WRITE_EXCLUSIVE_REG_ONLY; + case SCSI_PR_EXCLUSIVE_ACCESS_REG_ONLY: + return PR_EXCLUSIVE_ACCESS_REG_ONLY; + case SCSI_PR_WRITE_EXCLUSIVE_ALL_REGS: + return PR_WRITE_EXCLUSIVE_ALL_REGS; + case SCSI_PR_EXCLUSIVE_ACCESS_ALL_REGS: + return PR_EXCLUSIVE_ACCESS_ALL_REGS; + default: + return 0; + } +} + #endif diff --git a/include/scsi/scsi_proto.h b/include/scsi/scsi_proto.h index c03e35fc382c..0fd6e295375a 100644 --- a/include/scsi/scsi_proto.h +++ b/include/scsi/scsi_proto.h @@ -151,6 +151,11 @@ #define ZO_FINISH_ZONE 0x02 #define ZO_OPEN_ZONE 0x03 #define ZO_RESET_WRITE_POINTER 0x04 +/* values for PR in service action */ +#define READ_KEYS 0x00 +#define READ_RESERVATION 0x01 +#define REPORT_CAPABILITES 0x02 +#define READ_FULL_STATUS 0x03 /* values for variable length command */ #define XDREAD_32 0x03 #define XDWRITE_32 0x04
This adds support in sd.c for the block PR read keys and read reservation callouts. Signed-off-by: Mike Christie <michael.christie@oracle.com> --- drivers/scsi/sd.c | 104 +++++++++++++++++++++++++++++++++++ include/scsi/scsi_block_pr.h | 20 +++++++ include/scsi/scsi_proto.h | 5 ++ 3 files changed, 129 insertions(+)