Re: [PATCH 1/2] file-posix: Correctly read max_segments of SG nodes

[Maxim Levitsky <mlevitsk@redhat.com>]

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