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.thumsh...@wdc.com>
> > > Fixes: 9103f1ceb46614b150bcbc3c9a4fbc72b47fedcc
> > > Signed-off-by: Dmitry Fomichev <dmitry.fomic...@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
>
>
