* [PATCH 0/2] block;scsi-generic: Fix max transfer size calculation @ 2020-08-11 22:51 Dmitry Fomichev 2020-08-11 22:51 ` [PATCH 1/2] file-posix: Correctly read max_segments of SG nodes Dmitry Fomichev ` (2 more replies) 0 siblings, 3 replies; 11+ messages in thread From: Dmitry Fomichev @ 2020-08-11 22:51 UTC (permalink / raw) To: Kevin Wolf, Max Reitz, Paolo Bonzini, Fam Zheng, Maxim Levitsky, Philippe Mathieu-Daudé Cc: Dmitry Fomichev, Damien Le Moal, Alistair Francis, qemu-devel, qemu-block When a host-managed zoned device is passed through to the guest system using scsi-generic driver, the maximum i/o size for the drive at the guest may end up being larger than at the host, causing i/o errors while accessing the backing zoned drive at the host system. Two problems prevent correct setting of the maximum i/o transfer size at the guest in this configuration. One issue is specific to host-managed zone devices - scsi-generic driver doesn't recognize the SCSI type of HM-zoned devices. The other problem is that file-posix code for finding max_segments system value doesn't correctly handle SG nodes. The following two patches fix these problems. Based-on: <20200424084338.26803-16-armbru@redhat.com> Dmitry Fomichev (2): file-posix: Correctly read max_segments of SG nodes scsi-generic: Fix HM-zoned device scan block/file-posix.c | 55 +++++++++++++++++++++++----------------- hw/scsi/scsi-generic.c | 10 +++++--- include/scsi/constants.h | 1 + 3 files changed, 39 insertions(+), 27 deletions(-) -- 2.21.0 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/2] file-posix: Correctly read max_segments of SG nodes 2020-08-11 22:51 [PATCH 0/2] block;scsi-generic: Fix max transfer size calculation Dmitry Fomichev @ 2020-08-11 22:51 ` Dmitry Fomichev 2020-09-17 13:16 ` Max Reitz 2020-08-11 22:51 ` [PATCH 2/2] scsi-generic: Fix HM-zoned device scan Dmitry Fomichev 2020-08-17 16:38 ` [PATCH 0/2] block; scsi-generic: Fix max transfer size calculation Paolo Bonzini 2 siblings, 1 reply; 11+ messages in thread From: Dmitry Fomichev @ 2020-08-11 22:51 UTC (permalink / raw) To: Kevin Wolf, Max Reitz, Paolo Bonzini, Fam Zheng, Maxim Levitsky, Philippe Mathieu-Daudé Cc: Dmitry Fomichev, Damien Le Moal, Alistair Francis, qemu-devel, qemu-block If scsi-generic driver is in use, an SG node can be specified in the command line in place of a regular SCSI device. In this case, sg_get_max_segments() fails to open max_segments entry in sysfs because /dev/sgX is a character device. As the result, the maximum transfer size for the device may be calculated incorrectly, causing I/O errors if the maximum transfer size at the guest ends up to be larger compared to the host. Check system device type in sg_get_max_segments() and read the max_segments value differently if it is a character device. Reported-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> Fixes: 9103f1ceb46614b150bcbc3c9a4fbc72b47fedcc Signed-off-by: Dmitry Fomichev <dmitry.fomichev@wdc.com> --- block/file-posix.c | 55 +++++++++++++++++++++++++++------------------- 1 file changed, 32 insertions(+), 23 deletions(-) diff --git a/block/file-posix.c b/block/file-posix.c index 094e3b0212..f9e2424e8f 100644 --- a/block/file-posix.c +++ b/block/file-posix.c @@ -1108,6 +1108,7 @@ static int sg_get_max_segments(int fd) int ret; int sysfd = -1; long max_segments; + unsigned int max_segs; struct stat st; if (fstat(fd, &st)) { @@ -1115,30 +1116,38 @@ static int sg_get_max_segments(int fd) goto out; } - sysfspath = g_strdup_printf("/sys/dev/block/%u:%u/queue/max_segments", - major(st.st_rdev), minor(st.st_rdev)); - sysfd = open(sysfspath, O_RDONLY); - if (sysfd == -1) { - ret = -errno; - goto out; + if (S_ISBLK(st.st_mode)) { + sysfspath = g_strdup_printf("/sys/dev/block/%u:%u/queue/max_segments", + major(st.st_rdev), minor(st.st_rdev)); + sysfd = open(sysfspath, O_RDONLY); + if (sysfd == -1) { + ret = -errno; + goto out; + } + do { + ret = read(sysfd, buf, sizeof(buf) - 1); + } while (ret == -1 && errno == EINTR); + if (ret < 0) { + ret = -errno; + goto out; + } else if (ret == 0) { + ret = -EIO; + goto out; + } + buf[ret] = 0; + /* The file is ended with '\n', pass 'end' to accept that. */ + ret = qemu_strtol(buf, &end, 10, &max_segments); + if (ret == 0 && end && *end == '\n') { + ret = max_segments; + } + } else { + ret = ioctl(fd, SG_GET_SG_TABLESIZE, &max_segs); + if (ret != 0) { + ret = -errno; + goto out; + } + ret = max_segs; } - do { - ret = read(sysfd, buf, sizeof(buf) - 1); - } while (ret == -1 && errno == EINTR); - if (ret < 0) { - ret = -errno; - goto out; - } else if (ret == 0) { - ret = -EIO; - goto out; - } - buf[ret] = 0; - /* The file is ended with '\n', pass 'end' to accept that. */ - ret = qemu_strtol(buf, &end, 10, &max_segments); - if (ret == 0 && end && *end == '\n') { - ret = max_segments; - } - out: if (sysfd != -1) { close(sysfd); -- 2.21.0 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] file-posix: Correctly read max_segments of SG nodes 2020-08-11 22:51 ` [PATCH 1/2] file-posix: Correctly read max_segments of SG nodes Dmitry Fomichev @ 2020-09-17 13:16 ` Max Reitz 2020-09-17 13:22 ` Maxim Levitsky 2020-09-19 15:15 ` Paolo Bonzini 0 siblings, 2 replies; 11+ messages in thread From: Max Reitz @ 2020-09-17 13:16 UTC (permalink / raw) To: Dmitry Fomichev, Kevin Wolf, Paolo Bonzini, Fam Zheng, Maxim Levitsky, Philippe Mathieu-Daudé Cc: Damien Le Moal, Alistair Francis, qemu-devel, qemu-block On 12.08.20 00:51, Dmitry Fomichev wrote: > If scsi-generic driver is in use, an SG node can be specified in > the command line in place of a regular SCSI device. In this case, > sg_get_max_segments() fails to open max_segments entry in sysfs > because /dev/sgX is a character device. As the result, the maximum > transfer size for the device may be calculated incorrectly, causing > I/O errors if the maximum transfer size at the guest ends up to be > larger compared to the host. > > Check system device type in sg_get_max_segments() and read the > max_segments value differently if it is a character device. > > Reported-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> > Fixes: 9103f1ceb46614b150bcbc3c9a4fbc72b47fedcc > Signed-off-by: Dmitry Fomichev <dmitry.fomichev@wdc.com> > --- > block/file-posix.c | 55 +++++++++++++++++++++++++++------------------- > 1 file changed, 32 insertions(+), 23 deletions(-) > > diff --git a/block/file-posix.c b/block/file-posix.c > index 094e3b0212..f9e2424e8f 100644 > --- a/block/file-posix.c > +++ b/block/file-posix.c > @@ -1108,6 +1108,7 @@ static int sg_get_max_segments(int fd) > int ret; > int sysfd = -1; > long max_segments; > + unsigned int max_segs; > struct stat st; > > if (fstat(fd, &st)) { > @@ -1115,30 +1116,38 @@ static int sg_get_max_segments(int fd) > goto out; > } > > - sysfspath = g_strdup_printf("/sys/dev/block/%u:%u/queue/max_segments", > - major(st.st_rdev), minor(st.st_rdev)); > - sysfd = open(sysfspath, O_RDONLY); > - if (sysfd == -1) { > - ret = -errno; > - goto out; > + if (S_ISBLK(st.st_mode)) { > + sysfspath = g_strdup_printf("/sys/dev/block/%u:%u/queue/max_segments", > + major(st.st_rdev), minor(st.st_rdev)); Sounds reasonable, but this function is (naturally) only called if bs->sg is true, which is set by hdev_is_sg(), which returns true only if the device file is a character device. So is this path ever taken, or can we just replace it all with the ioctl? (Before 867eccfed84, this function was used for all host devices, which might explain why the code even exists.) Max ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] file-posix: Correctly read max_segments of SG nodes 2020-09-17 13:16 ` Max Reitz @ 2020-09-17 13:22 ` Maxim Levitsky 2020-09-17 13:24 ` Maxim Levitsky 2020-09-19 15:15 ` Paolo Bonzini 1 sibling, 1 reply; 11+ messages in thread From: Maxim Levitsky @ 2020-09-17 13:22 UTC (permalink / raw) To: Max Reitz, Dmitry Fomichev, Kevin Wolf, Paolo Bonzini, Fam Zheng, Philippe Mathieu-Daudé Cc: Damien Le Moal, Alistair Francis, qemu-devel, qemu-block On Thu, 2020-09-17 at 15:16 +0200, Max Reitz wrote: > On 12.08.20 00:51, Dmitry Fomichev wrote: > > If scsi-generic driver is in use, an SG node can be specified in > > the command line in place of a regular SCSI device. In this case, > > sg_get_max_segments() fails to open max_segments entry in sysfs > > because /dev/sgX is a character device. As the result, the maximum > > transfer size for the device may be calculated incorrectly, causing > > I/O errors if the maximum transfer size at the guest ends up to be > > larger compared to the host. > > > > Check system device type in sg_get_max_segments() and read the > > max_segments value differently if it is a character device. > > > > Reported-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> > > Fixes: 9103f1ceb46614b150bcbc3c9a4fbc72b47fedcc > > Signed-off-by: Dmitry Fomichev <dmitry.fomichev@wdc.com> > > --- > > block/file-posix.c | 55 +++++++++++++++++++++++++++------------------- > > 1 file changed, 32 insertions(+), 23 deletions(-) > > > > diff --git a/block/file-posix.c b/block/file-posix.c > > index 094e3b0212..f9e2424e8f 100644 > > --- a/block/file-posix.c > > +++ b/block/file-posix.c > > @@ -1108,6 +1108,7 @@ static int sg_get_max_segments(int fd) > > int ret; > > int sysfd = -1; > > long max_segments; > > + unsigned int max_segs; > > struct stat st; > > > > if (fstat(fd, &st)) { > > @@ -1115,30 +1116,38 @@ static int sg_get_max_segments(int fd) > > goto out; > > } > > > > - sysfspath = g_strdup_printf("/sys/dev/block/%u:%u/queue/max_segments", > > - major(st.st_rdev), minor(st.st_rdev)); > > - sysfd = open(sysfspath, O_RDONLY); > > - if (sysfd == -1) { > > - ret = -errno; > > - goto out; > > + if (S_ISBLK(st.st_mode)) { > > + sysfspath = g_strdup_printf("/sys/dev/block/%u:%u/queue/max_segments", > > + major(st.st_rdev), minor(st.st_rdev)); > > Sounds reasonable, but this function is (naturally) only called if > bs->sg is true, which is set by hdev_is_sg(), which returns true only if > the device file is a character device. > > So is this path ever taken, or can we just replace it all with the ioctl? > > (Before 867eccfed84, this function was used for all host devices, which > might explain why the code even exists.) > > Max I have another proposal which I am currently evaluating. How about we drop all the SG_IO limits code alltogher from the raw driver, and instead just let the scsi drivers (scsi-block and scsi-generic) query the device directly, since I don't think that the kernel (I will double check this)? Best regards, Maxim Levitsky ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] file-posix: Correctly read max_segments of SG nodes 2020-09-17 13:22 ` Maxim Levitsky @ 2020-09-17 13:24 ` Maxim Levitsky 2020-09-17 16:44 ` Dmitry Fomichev 0 siblings, 1 reply; 11+ messages in thread From: Maxim Levitsky @ 2020-09-17 13:24 UTC (permalink / raw) To: Max Reitz, Dmitry Fomichev, Kevin Wolf, Paolo Bonzini, Fam Zheng, Philippe Mathieu-Daudé Cc: Damien Le Moal, Alistair Francis, qemu-devel, qemu-block On Thu, 2020-09-17 at 16:22 +0300, Maxim Levitsky wrote: > On Thu, 2020-09-17 at 15:16 +0200, Max Reitz wrote: > > On 12.08.20 00:51, Dmitry Fomichev wrote: > > > If scsi-generic driver is in use, an SG node can be specified in > > > the command line in place of a regular SCSI device. In this case, > > > sg_get_max_segments() fails to open max_segments entry in sysfs > > > because /dev/sgX is a character device. As the result, the maximum > > > transfer size for the device may be calculated incorrectly, causing > > > I/O errors if the maximum transfer size at the guest ends up to be > > > larger compared to the host. > > > > > > Check system device type in sg_get_max_segments() and read the > > > max_segments value differently if it is a character device. > > > > > > Reported-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> > > > Fixes: 9103f1ceb46614b150bcbc3c9a4fbc72b47fedcc > > > Signed-off-by: Dmitry Fomichev <dmitry.fomichev@wdc.com> > > > --- > > > block/file-posix.c | 55 +++++++++++++++++++++++++++------------------- > > > 1 file changed, 32 insertions(+), 23 deletions(-) > > > > > > diff --git a/block/file-posix.c b/block/file-posix.c > > > index 094e3b0212..f9e2424e8f 100644 > > > --- a/block/file-posix.c > > > +++ b/block/file-posix.c > > > @@ -1108,6 +1108,7 @@ static int sg_get_max_segments(int fd) > > > int ret; > > > int sysfd = -1; > > > long max_segments; > > > + unsigned int max_segs; > > > struct stat st; > > > > > > if (fstat(fd, &st)) { > > > @@ -1115,30 +1116,38 @@ static int sg_get_max_segments(int fd) > > > goto out; > > > } > > > > > > - sysfspath = g_strdup_printf("/sys/dev/block/%u:%u/queue/max_segments", > > > - major(st.st_rdev), minor(st.st_rdev)); > > > - sysfd = open(sysfspath, O_RDONLY); > > > - if (sysfd == -1) { > > > - ret = -errno; > > > - goto out; > > > + if (S_ISBLK(st.st_mode)) { > > > + sysfspath = g_strdup_printf("/sys/dev/block/%u:%u/queue/max_segments", > > > + major(st.st_rdev), minor(st.st_rdev)); > > > > Sounds reasonable, but this function is (naturally) only called if > > bs->sg is true, which is set by hdev_is_sg(), which returns true only if > > the device file is a character device. > > > > So is this path ever taken, or can we just replace it all with the ioctl? > > > > (Before 867eccfed84, this function was used for all host devices, which > > might explain why the code even exists.) > > > > Max > > I have another proposal which I am currently evaluating. > > How about we drop all the SG_IO limits code alltogher from the raw driver, and > instead just let the scsi drivers (scsi-block and scsi-generic) query > the device directly, since I don't think that the kernel (I will double check this)? I hit send too soon. I mean I don't think that the kernel imposes its own limits on SG_IO. Best regards, Maxim Levitsky > > > Best regards, > Maxim Levitsky > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH 1/2] file-posix: Correctly read max_segments of SG nodes 2020-09-17 13:24 ` Maxim Levitsky @ 2020-09-17 16:44 ` Dmitry Fomichev 2020-09-19 15:18 ` Paolo Bonzini 0 siblings, 1 reply; 11+ messages in thread From: Dmitry Fomichev @ 2020-09-17 16:44 UTC (permalink / raw) To: Maxim Levitsky, Max Reitz, Kevin Wolf, Paolo Bonzini, Fam Zheng, Philippe Mathieu-Daudé Cc: Damien Le Moal, Alistair Francis, qemu-devel, qemu-block > -----Original Message----- > From: Maxim Levitsky <mlevitsk@redhat.com> > Sent: Thursday, September 17, 2020 9:24 AM > To: Max Reitz <mreitz@redhat.com>; Dmitry Fomichev > <Dmitry.Fomichev@wdc.com>; Kevin Wolf <kwolf@redhat.com>; Paolo > Bonzini <pbonzini@redhat.com>; Fam Zheng <fam@euphon.net>; Philippe > Mathieu-Daudé <philmd@redhat.com> > Cc: Alistair Francis <Alistair.Francis@wdc.com>; Damien Le Moal > <Damien.LeMoal@wdc.com>; qemu-block@nongnu.org; qemu- > devel@nongnu.org > Subject: Re: [PATCH 1/2] file-posix: Correctly read max_segments of SG > nodes > > On Thu, 2020-09-17 at 16:22 +0300, Maxim Levitsky wrote: > > On Thu, 2020-09-17 at 15:16 +0200, Max Reitz wrote: > > > On 12.08.20 00:51, Dmitry Fomichev wrote: > > > > If scsi-generic driver is in use, an SG node can be specified in > > > > the command line in place of a regular SCSI device. In this case, > > > > sg_get_max_segments() fails to open max_segments entry in sysfs > > > > because /dev/sgX is a character device. As the result, the maximum > > > > transfer size for the device may be calculated incorrectly, causing > > > > I/O errors if the maximum transfer size at the guest ends up to be > > > > larger compared to the host. > > > > > > > > Check system device type in sg_get_max_segments() and read the > > > > max_segments value differently if it is a character device. > > > > > > > > Reported-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> > > > > Fixes: 9103f1ceb46614b150bcbc3c9a4fbc72b47fedcc > > > > Signed-off-by: Dmitry Fomichev <dmitry.fomichev@wdc.com> > > > > --- > > > > block/file-posix.c | 55 +++++++++++++++++++++++++++---------------- > --- > > > > 1 file changed, 32 insertions(+), 23 deletions(-) > > > > > > > > diff --git a/block/file-posix.c b/block/file-posix.c > > > > index 094e3b0212..f9e2424e8f 100644 > > > > --- a/block/file-posix.c > > > > +++ b/block/file-posix.c > > > > @@ -1108,6 +1108,7 @@ static int sg_get_max_segments(int fd) > > > > int ret; > > > > int sysfd = -1; > > > > long max_segments; > > > > + unsigned int max_segs; > > > > struct stat st; > > > > > > > > if (fstat(fd, &st)) { > > > > @@ -1115,30 +1116,38 @@ static int sg_get_max_segments(int fd) > > > > goto out; > > > > } > > > > > > > > - sysfspath = > g_strdup_printf("/sys/dev/block/%u:%u/queue/max_segments", > > > > - major(st.st_rdev), minor(st.st_rdev)); > > > > - sysfd = open(sysfspath, O_RDONLY); > > > > - if (sysfd == -1) { > > > > - ret = -errno; > > > > - goto out; > > > > + if (S_ISBLK(st.st_mode)) { > > > > + sysfspath = > g_strdup_printf("/sys/dev/block/%u:%u/queue/max_segments", > > > > + major(st.st_rdev), minor(st.st_rdev)); > > > > > > Sounds reasonable, but this function is (naturally) only called if > > > bs->sg is true, which is set by hdev_is_sg(), which returns true only if > > > the device file is a character device. > > > > > > So is this path ever taken, or can we just replace it all with the ioctl? > > > > > > (Before 867eccfed84, this function was used for all host devices, which > > > might explain why the code even exists.) > > > > > > Max > > > > I have another proposal which I am currently evaluating. > > > > How about we drop all the SG_IO limits code alltogher from the raw driver, > and > > instead just let the scsi drivers (scsi-block and scsi-generic) query > > the device directly, since I don't think that the kernel (I will double check > this)? > > I hit send too soon. I mean I don't think that the kernel imposes its own limits > on SG_IO. > When I was making the fix, I had a feeling that this code should be moved someplace else where it could be called both for block devices and sg nodes, but that would make the patch more intrusive. Maxim, looks like you are on top of this problem and your approach sounds sensible too me. Just FYI, it is also possible to avoid using SG_GET_SG_TABLESIZE ioctl and rely entirely on sysfs, but the code gets a bit more complicated (see below) Cheers, Dmitry --- a/block/file-posix.c +++ b/block/file-posix.c @@ -1110,6 +1110,8 @@ static int sg_get_max_segments(int fd) char buf[32]; const char *end; char *sysfspath = NULL; + struct dirent *blkdev = NULL; + DIR *dir = NULL; int ret; int sysfd = -1; long max_segments; @@ -1120,8 +1122,30 @@ static int sg_get_max_segments(int fd) goto out; } - sysfspath = g_strdup_printf("/sys/dev/block/%u:%u/queue/max_segments", - major(st.st_rdev), minor(st.st_rdev)); + if (S_ISBLK(st.st_mode)) { + sysfspath = g_strdup_printf("/sys/dev/block/%u:%u/queue/max_segments", + major(st.st_rdev), minor(st.st_rdev)); + } else { + sysfspath = g_strdup_printf("/sys/dev/char/%u:%u/device/block", + major(st.st_rdev), minor(st.st_rdev)); + dir = opendir(sysfspath); + if (!dir) { + ret = -errno; + goto out; + } + do { + blkdev = readdir(dir); + if (!blkdev) { + ret = -errno; + closedir(dir); + goto out; + } + } while (*blkdev->d_name == '.'); + g_free(sysfspath); + sysfspath = g_strdup_printf("/sys/block/%s/queue/max_segments", + blkdev->d_name); + closedir(dir); + } sysfd = open(sysfspath, O_RDONLY); if (sysfd == -1) { ret = -errno; > Best regards, > Maxim Levitsky > > > > > > Best regards, > > Maxim Levitsky > > > > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] file-posix: Correctly read max_segments of SG nodes 2020-09-17 16:44 ` Dmitry Fomichev @ 2020-09-19 15:18 ` Paolo Bonzini 0 siblings, 0 replies; 11+ messages in thread From: Paolo Bonzini @ 2020-09-19 15:18 UTC (permalink / raw) To: Dmitry Fomichev, Maxim Levitsky, Max Reitz, Kevin Wolf, Fam Zheng, Philippe Mathieu-Daudé Cc: Damien Le Moal, Alistair Francis, qemu-devel, qemu-block On 17/09/20 18:44, Dmitry Fomichev wrote: > > Maxim, looks like you are on top of this problem and your approach sounds > sensible too me. Just FYI, it is also possible to avoid using SG_GET_SG_TABLESIZE > ioctl and rely entirely on sysfs, but the code gets a bit more complicated (see below) I would prefer to have the code in block/ but I have no problem if the hardware limits are placed in a new field of bs->bl. Then scsi-block and scsi-generic can consult it instead of bs->bl.max_transfer. Paolo ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] file-posix: Correctly read max_segments of SG nodes 2020-09-17 13:16 ` Max Reitz 2020-09-17 13:22 ` Maxim Levitsky @ 2020-09-19 15:15 ` Paolo Bonzini 1 sibling, 0 replies; 11+ messages in thread From: Paolo Bonzini @ 2020-09-19 15:15 UTC (permalink / raw) To: Max Reitz, Dmitry Fomichev, Kevin Wolf, Fam Zheng, Maxim Levitsky, Philippe Mathieu-Daudé Cc: Damien Le Moal, Alistair Francis, qemu-devel, qemu-block On 17/09/20 15:16, Max Reitz wrote: > So is this path ever taken, or can we just replace it all with the ioctl? > > (Before 867eccfed84, this function was used for all host devices, which > might explain why the code even exists.) Because 867eccfed84 is wrong. If you use /dev/sda* with SG_IO you do need to take into account the hardware max segment size/max segment count. Probably ->sg needs to be set by the front-end, not by the back-end. An even better way (but for which I'd leave the task to you and Kevin) could be to have a new permission BLK_PERM_WRITE_BYPASS and to reduce the limits to the hardware limits if anybody has requested that permission. I tried to implement that a couple years ago but I just couldn't wrap my mind around the permission code. Paolo ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/2] scsi-generic: Fix HM-zoned device scan 2020-08-11 22:51 [PATCH 0/2] block;scsi-generic: Fix max transfer size calculation Dmitry Fomichev 2020-08-11 22:51 ` [PATCH 1/2] file-posix: Correctly read max_segments of SG nodes Dmitry Fomichev @ 2020-08-11 22:51 ` Dmitry Fomichev 2020-08-17 15:58 ` Alistair Francis 2020-08-17 16:38 ` [PATCH 0/2] block; scsi-generic: Fix max transfer size calculation Paolo Bonzini 2 siblings, 1 reply; 11+ messages in thread From: Dmitry Fomichev @ 2020-08-11 22:51 UTC (permalink / raw) To: Kevin Wolf, Max Reitz, Paolo Bonzini, Fam Zheng, Maxim Levitsky, Philippe Mathieu-Daudé Cc: Dmitry Fomichev, Damien Le Moal, Alistair Francis, qemu-devel, qemu-block Several important steps during device scan depend on SCSI type of the device. For example, max_transfer property is only determined and assigned if the device has the type of TYPE_DISK. Host-managed ZBC disks retain most of the properties of regular SCSI drives, but they have their own SCSI device type, 0x14. This prevents the proper assignment of max_transfer property for HM-zoned devices in scsi-generic driver leading to I/O errors if the maximum i/o size calculated at the guest exceeds the host value. To fix this, define TYPE_ZBC to have the standard value from SCSI ZBC standard spec. Several scan steps that were previously done only for TYPE_DISK devices, are now performed for the SCSI devices having TYPE_ZBC too. Reported-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> Signed-off-by: Dmitry Fomichev <dmitry.fomichev@wdc.com> --- hw/scsi/scsi-generic.c | 10 ++++++---- include/scsi/constants.h | 1 + 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c index 86ed0a3822..2cb23ca891 100644 --- a/hw/scsi/scsi-generic.c +++ b/hw/scsi/scsi-generic.c @@ -162,7 +162,8 @@ static void scsi_handle_inquiry_reply(SCSIGenericReq *r, SCSIDevice *s) } } - if (s->type == TYPE_DISK && (r->req.cmd.buf[1] & 0x01)) { + if ((s->type == TYPE_DISK || s->type == TYPE_ZBC) && + (r->req.cmd.buf[1] & 0x01)) { page = r->req.cmd.buf[2]; if (page == 0xb0) { uint32_t max_transfer = @@ -299,10 +300,11 @@ static void scsi_read_complete(void * opaque, int ret) } blk_set_guest_block_size(s->conf.blk, s->blocksize); - /* Patch MODE SENSE device specific parameters if the BDS is opened + /* + * Patch MODE SENSE device specific parameters if the BDS is opened * readonly. */ - if ((s->type == TYPE_DISK || s->type == TYPE_TAPE) && + if ((s->type == TYPE_DISK || s->type == TYPE_TAPE || s->type == TYPE_ZBC) && blk_is_read_only(s->conf.blk) && (r->req.cmd.buf[0] == MODE_SENSE || r->req.cmd.buf[0] == MODE_SENSE_10) && @@ -617,7 +619,7 @@ static void scsi_generic_read_device_identification(SCSIDevice *s) void scsi_generic_read_device_inquiry(SCSIDevice *s) { scsi_generic_read_device_identification(s); - if (s->type == TYPE_DISK) { + if (s->type == TYPE_DISK || s->type == TYPE_ZBC) { scsi_generic_set_vpd_bl_emulation(s); } else { s->needs_vpd_bl_emulation = false; diff --git a/include/scsi/constants.h b/include/scsi/constants.h index 874176019e..2a32c08b5e 100644 --- a/include/scsi/constants.h +++ b/include/scsi/constants.h @@ -218,6 +218,7 @@ #define TYPE_ENCLOSURE 0x0d /* Enclosure Services Device */ #define TYPE_RBC 0x0e /* Simplified Direct-Access Device */ #define TYPE_OSD 0x11 /* Object-storage Device */ +#define TYPE_ZBC 0x14 /* Host-managed Zoned SCSI Device */ #define TYPE_WLUN 0x1e /* Well known LUN */ #define TYPE_NOT_PRESENT 0x1f #define TYPE_INACTIVE 0x20 -- 2.21.0 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] scsi-generic: Fix HM-zoned device scan 2020-08-11 22:51 ` [PATCH 2/2] scsi-generic: Fix HM-zoned device scan Dmitry Fomichev @ 2020-08-17 15:58 ` Alistair Francis 0 siblings, 0 replies; 11+ messages in thread From: Alistair Francis @ 2020-08-17 15:58 UTC (permalink / raw) To: Dmitry Fomichev Cc: Kevin Wolf, Damien Le Moal, Fam Zheng, Qemu-block, qemu-devel@nongnu.org Developers, Max Reitz, Alistair Francis, Paolo Bonzini, Philippe Mathieu-Daudé, Maxim Levitsky On Tue, Aug 11, 2020 at 3:52 PM Dmitry Fomichev <dmitry.fomichev@wdc.com> wrote: > > Several important steps during device scan depend on SCSI type of the > device. For example, max_transfer property is only determined and > assigned if the device has the type of TYPE_DISK. > > Host-managed ZBC disks retain most of the properties of regular SCSI > drives, but they have their own SCSI device type, 0x14. This prevents > the proper assignment of max_transfer property for HM-zoned devices in > scsi-generic driver leading to I/O errors if the maximum i/o size > calculated at the guest exceeds the host value. > > To fix this, define TYPE_ZBC to have the standard value from SCSI ZBC > standard spec. Several scan steps that were previously done only for > TYPE_DISK devices, are now performed for the SCSI devices having > TYPE_ZBC too. > > Reported-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> > Signed-off-by: Dmitry Fomichev <dmitry.fomichev@wdc.com> Reviewed-by: Alistair Francis <alistair.francis@wdc.com> Alistair > --- > hw/scsi/scsi-generic.c | 10 ++++++---- > include/scsi/constants.h | 1 + > 2 files changed, 7 insertions(+), 4 deletions(-) > > diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c > index 86ed0a3822..2cb23ca891 100644 > --- a/hw/scsi/scsi-generic.c > +++ b/hw/scsi/scsi-generic.c > @@ -162,7 +162,8 @@ static void scsi_handle_inquiry_reply(SCSIGenericReq *r, SCSIDevice *s) > } > } > > - if (s->type == TYPE_DISK && (r->req.cmd.buf[1] & 0x01)) { > + if ((s->type == TYPE_DISK || s->type == TYPE_ZBC) && > + (r->req.cmd.buf[1] & 0x01)) { > page = r->req.cmd.buf[2]; > if (page == 0xb0) { > uint32_t max_transfer = > @@ -299,10 +300,11 @@ static void scsi_read_complete(void * opaque, int ret) > } > blk_set_guest_block_size(s->conf.blk, s->blocksize); > > - /* Patch MODE SENSE device specific parameters if the BDS is opened > + /* > + * Patch MODE SENSE device specific parameters if the BDS is opened > * readonly. > */ > - if ((s->type == TYPE_DISK || s->type == TYPE_TAPE) && > + if ((s->type == TYPE_DISK || s->type == TYPE_TAPE || s->type == TYPE_ZBC) && > blk_is_read_only(s->conf.blk) && > (r->req.cmd.buf[0] == MODE_SENSE || > r->req.cmd.buf[0] == MODE_SENSE_10) && > @@ -617,7 +619,7 @@ static void scsi_generic_read_device_identification(SCSIDevice *s) > void scsi_generic_read_device_inquiry(SCSIDevice *s) > { > scsi_generic_read_device_identification(s); > - if (s->type == TYPE_DISK) { > + if (s->type == TYPE_DISK || s->type == TYPE_ZBC) { > scsi_generic_set_vpd_bl_emulation(s); > } else { > s->needs_vpd_bl_emulation = false; > diff --git a/include/scsi/constants.h b/include/scsi/constants.h > index 874176019e..2a32c08b5e 100644 > --- a/include/scsi/constants.h > +++ b/include/scsi/constants.h > @@ -218,6 +218,7 @@ > #define TYPE_ENCLOSURE 0x0d /* Enclosure Services Device */ > #define TYPE_RBC 0x0e /* Simplified Direct-Access Device */ > #define TYPE_OSD 0x11 /* Object-storage Device */ > +#define TYPE_ZBC 0x14 /* Host-managed Zoned SCSI Device */ > #define TYPE_WLUN 0x1e /* Well known LUN */ > #define TYPE_NOT_PRESENT 0x1f > #define TYPE_INACTIVE 0x20 > -- > 2.21.0 > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 0/2] block; scsi-generic: Fix max transfer size calculation 2020-08-11 22:51 [PATCH 0/2] block;scsi-generic: Fix max transfer size calculation Dmitry Fomichev 2020-08-11 22:51 ` [PATCH 1/2] file-posix: Correctly read max_segments of SG nodes Dmitry Fomichev 2020-08-11 22:51 ` [PATCH 2/2] scsi-generic: Fix HM-zoned device scan Dmitry Fomichev @ 2020-08-17 16:38 ` Paolo Bonzini 2 siblings, 0 replies; 11+ messages in thread From: Paolo Bonzini @ 2020-08-17 16:38 UTC (permalink / raw) To: Dmitry Fomichev, Kevin Wolf, Max Reitz, Fam Zheng, Maxim Levitsky, Philippe Mathieu-Daudé Cc: Damien Le Moal, Alistair Francis, qemu-devel, qemu-block On 12/08/20 00:51, Dmitry Fomichev wrote: > When a host-managed zoned device is passed through to the > guest system using scsi-generic driver, the maximum i/o size for the > drive at the guest may end up being larger than at the host, causing > i/o errors while accessing the backing zoned drive at the host system. > > Two problems prevent correct setting of the maximum i/o transfer size > at the guest in this configuration. One issue is specific to > host-managed zone devices - scsi-generic driver doesn't recognize the > SCSI type of HM-zoned devices. The other problem is that file-posix > code for finding max_segments system value doesn't correctly handle > SG nodes. > > The following two patches fix these problems. > > Based-on: <20200424084338.26803-16-armbru@redhat.com> > > Dmitry Fomichev (2): > file-posix: Correctly read max_segments of SG nodes > scsi-generic: Fix HM-zoned device scan > > block/file-posix.c | 55 +++++++++++++++++++++++----------------- > hw/scsi/scsi-generic.c | 10 +++++--- > include/scsi/constants.h | 1 + > 3 files changed, 39 insertions(+), 27 deletions(-) > The patches are more or less unrelated; I have queued the second, while the first is outside my maintenance area. Paolo ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2020-09-19 15:19 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-08-11 22:51 [PATCH 0/2] block;scsi-generic: Fix max transfer size calculation Dmitry Fomichev 2020-08-11 22:51 ` [PATCH 1/2] file-posix: Correctly read max_segments of SG nodes Dmitry Fomichev 2020-09-17 13:16 ` Max Reitz 2020-09-17 13:22 ` Maxim Levitsky 2020-09-17 13:24 ` Maxim Levitsky 2020-09-17 16:44 ` Dmitry Fomichev 2020-09-19 15:18 ` Paolo Bonzini 2020-09-19 15:15 ` Paolo Bonzini 2020-08-11 22:51 ` [PATCH 2/2] scsi-generic: Fix HM-zoned device scan Dmitry Fomichev 2020-08-17 15:58 ` Alistair Francis 2020-08-17 16:38 ` [PATCH 0/2] block; scsi-generic: Fix max transfer size calculation Paolo Bonzini
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).