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

[Maxim Levitsky]

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
> 
>