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

[Max Reitz]

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