1. Patchew
  2. QEMU
  3. [PATCH v4 0/7] block: file-posix queue
  4. View patch

[PATCH v4 1/7] file-posix: fix max_iov for /dev/sg devices

Paolo Bonzini posted 7 patches 4 years, 8 months ago
Maintainers: Paolo Bonzini <pbonzini@redhat.com>, Max Reitz <mreitz@redhat.com>, Eric Blake <eblake@redhat.com>, Markus Armbruster <armbru@redhat.com>, Stefan Hajnoczi <stefanha@redhat.com>, Fam Zheng <fam@euphon.net>, Kevin Wolf <kwolf@redhat.com>
There is a newer version of this series
[PATCH v4 1/7] file-posix: fix max_iov for /dev/sg devices
Posted by Paolo Bonzini 4 years, 8 months ago 
Even though it was only called for devices that have bs->sg set (which
must be character devices), sg_get_max_segments looked at /sys/dev/block
which only works for block devices.

On Linux the sg driver has its own way to provide the maximum number of
iovecs in a scatter/gather list, so add support for it.  The block device
path is kept because it will be reinstated in the next patches.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/file-posix.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/block/file-posix.c b/block/file-posix.c
index f37dfc10b3..536998a1d6 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1180,6 +1180,17 @@ static int sg_get_max_segments(int fd)
         goto out;
     }
 
+    if (S_ISCHR(st.st_mode)) {
+        if (ioctl(fd, SG_GET_SG_TABLESIZE, &ret) == 0) {
+            return ret;
+        }
+        return -ENOTSUP;
+    }
+
+    if (!S_ISBLK(st.st_mode)) {
+        return -ENOTSUP;
+    }
+
     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);
-- 
2.31.1
Re: [PATCH v4 1/7] file-posix: fix max_iov for /dev/sg devices
Posted by Eric Blake 4 years, 8 months ago 
On Tue, Jun 08, 2021 at 03:16:28PM +0200, Paolo Bonzini wrote:
> Even though it was only called for devices that have bs->sg set (which
> must be character devices), sg_get_max_segments looked at /sys/dev/block
> which only works for block devices.
> 
> On Linux the sg driver has its own way to provide the maximum number of
> iovecs in a scatter/gather list, so add support for it.  The block device
> path is kept because it will be reinstated in the next patches.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  block/file-posix.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/block/file-posix.c b/block/file-posix.c
> index f37dfc10b3..536998a1d6 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -1180,6 +1180,17 @@ static int sg_get_max_segments(int fd)
>          goto out;
>      }
>  
> +    if (S_ISCHR(st.st_mode)) {
> +        if (ioctl(fd, SG_GET_SG_TABLESIZE, &ret) == 0) {

Do we need to do any conditional compilation based on whether
SG_GET_SG_TABLESIZE is a known ioctl, or is it old enough to be
assumed present on all platforms we care about?

> +            return ret;
> +        }
> +        return -ENOTSUP;
> +    }
> +
> +    if (!S_ISBLK(st.st_mode)) {
> +        return -ENOTSUP;
> +    }
> +
>      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);

Otherwise looks good to me.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org
Re: [PATCH v4 1/7] file-posix: fix max_iov for /dev/sg devices
Posted by Vladimir Sementsov-Ogievskiy 4 years, 8 months ago 
08.06.2021 16:16, Paolo Bonzini wrote:
> Even though it was only called for devices that have bs->sg set (which
> must be character devices), sg_get_max_segments looked at /sys/dev/block
> which only works for block devices.
> 
> On Linux the sg driver has its own way to provide the maximum number of
> iovecs in a scatter/gather list, so add support for it.  The block device
> path is kept because it will be reinstated in the next patches.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   block/file-posix.c | 11 +++++++++++
>   1 file changed, 11 insertions(+)
> 
> diff --git a/block/file-posix.c b/block/file-posix.c
> index f37dfc10b3..536998a1d6 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -1180,6 +1180,17 @@ static int sg_get_max_segments(int fd)
>           goto out;
>       }
>   
> +    if (S_ISCHR(st.st_mode)) {

Why not check "if (bs->sg) {" instead? It seems to be more consistent with issuing SG_ ioctl. Or what I miss?

> +        if (ioctl(fd, SG_GET_SG_TABLESIZE, &ret) == 0) {
> +            return ret;
> +        }
> +        return -ENOTSUP;
> +    }
> +
> +    if (!S_ISBLK(st.st_mode)) {
> +        return -ENOTSUP;
> +    }
> +
>       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);
> 


-- 
Best regards,
Vladimir
Re: [PATCH v4 1/7] file-posix: fix max_iov for /dev/sg devices
Posted by Max Reitz 4 years, 7 months ago 
On 08.06.21 21:14, Vladimir Sementsov-Ogievskiy wrote:
> 08.06.2021 16:16, Paolo Bonzini wrote:
>> Even though it was only called for devices that have bs->sg set (which
>> must be character devices), sg_get_max_segments looked at /sys/dev/block
>> which only works for block devices.
>>
>> On Linux the sg driver has its own way to provide the maximum number of
>> iovecs in a scatter/gather list, so add support for it.  The block 
>> device
>> path is kept because it will be reinstated in the next patches.
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>   block/file-posix.c | 11 +++++++++++
>>   1 file changed, 11 insertions(+)
>>
>> diff --git a/block/file-posix.c b/block/file-posix.c
>> index f37dfc10b3..536998a1d6 100644
>> --- a/block/file-posix.c
>> +++ b/block/file-posix.c
>> @@ -1180,6 +1180,17 @@ static int sg_get_max_segments(int fd)
>>           goto out;
>>       }
>>   +    if (S_ISCHR(st.st_mode)) {
>
> Why not check "if (bs->sg) {" instead? It seems to be more consistent 
> with issuing SG_ ioctl. Or what I miss?

I dismissed this in v3, because I didn’t understand why you’d raise this 
point.  The function is called sg_*(), and it’s only called if bs->sg is 
true anyway.  So clearly we can use SG_ ioctls, because the whole 
function is intended only for SG devices anyway.

This time, I looked forward, and perhaps starting at patch 4 I can 
understand where you’re coming from, because then the function is used 
for host devices in general.

So now I don’t particularly mind.  I think it’s still clear that if 
there’s a host device here that’s a character device, then that’s going 
to be an SG device, so I don’t really have a preference between 
S_ISCHR() and bs->sg.

Max

>> +        if (ioctl(fd, SG_GET_SG_TABLESIZE, &ret) == 0) {
>> +            return ret;
>> +        }
>> +        return -ENOTSUP;
>> +    }
>> +
>> +    if (!S_ISBLK(st.st_mode)) {
>> +        return -ENOTSUP;
>> +    }
>> +
>>       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);
>>
>
>
Re: [PATCH v4 1/7] file-posix: fix max_iov for /dev/sg devices
Posted by Vladimir Sementsov-Ogievskiy 4 years, 7 months ago 
23.06.2021 18:42, Max Reitz wrote:
> On 08.06.21 21:14, Vladimir Sementsov-Ogievskiy wrote:
>> 08.06.2021 16:16, Paolo Bonzini wrote:
>>> Even though it was only called for devices that have bs->sg set (which
>>> must be character devices), sg_get_max_segments looked at /sys/dev/block
>>> which only works for block devices.
>>>
>>> On Linux the sg driver has its own way to provide the maximum number of
>>> iovecs in a scatter/gather list, so add support for it.  The block device
>>> path is kept because it will be reinstated in the next patches.
>>>
>>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>>> ---
>>>   block/file-posix.c | 11 +++++++++++
>>>   1 file changed, 11 insertions(+)
>>>
>>> diff --git a/block/file-posix.c b/block/file-posix.c
>>> index f37dfc10b3..536998a1d6 100644
>>> --- a/block/file-posix.c
>>> +++ b/block/file-posix.c
>>> @@ -1180,6 +1180,17 @@ static int sg_get_max_segments(int fd)
>>>           goto out;
>>>       }
>>>   +    if (S_ISCHR(st.st_mode)) {
>>
>> Why not check "if (bs->sg) {" instead? It seems to be more consistent with issuing SG_ ioctl. Or what I miss?
> 
> I dismissed this in v3, because I didn’t understand why you’d raise this point.  The function is called sg_*(), and it’s only called if bs->sg is true anyway.  So clearly we can use SG_ ioctls, because the whole function is intended only for SG devices anyway.
> 
> This time, I looked forward, and perhaps starting at patch 4 I can understand where you’re coming from, because then the function is used for host devices in general.
> 
> So now I don’t particularly mind.  I think it’s still clear that if there’s a host device here that’s a character device, then that’s going to be an SG device, so I don’t really have a preference between S_ISCHR() and bs->sg.
> 

If I understand all correctly:

In this patch we don't need neither S_ISCHR nor bs->sg check: they both must pass for sg devices. Starting from patch 4 we'll need here if (bs->sg) check.

> 
>>> +        if (ioctl(fd, SG_GET_SG_TABLESIZE, &ret) == 0) {
>>> +            return ret;
>>> +        }
>>> +        return -ENOTSUP;
>>> +    }
>>> +
>>> +    if (!S_ISBLK(st.st_mode)) {
>>> +        return -ENOTSUP;
>>> +    }
>>> +
>>>       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);
>>>
>>
>>
> 


-- 
Best regards,
Vladimir
Re: [PATCH v4 1/7] file-posix: fix max_iov for /dev/sg devices
Posted by Maxim Levitsky 4 years, 8 months ago 
On Tue, 2021-06-08 at 22:14 +0300, Vladimir Sementsov-Ogievskiy wrote:
> 08.06.2021 16:16, Paolo Bonzini wrote:
> > Even though it was only called for devices that have bs->sg set (which
> > must be character devices), sg_get_max_segments looked at /sys/dev/block
> > which only works for block devices.
> > 
> > On Linux the sg driver has its own way to provide the maximum number of
> > iovecs in a scatter/gather list, so add support for it.  The block device
> > path is kept because it will be reinstated in the next patches.
> > 
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > ---
> >   block/file-posix.c | 11 +++++++++++
> >   1 file changed, 11 insertions(+)
> > 
> > diff --git a/block/file-posix.c b/block/file-posix.c
> > index f37dfc10b3..536998a1d6 100644
> > --- a/block/file-posix.c
> > +++ b/block/file-posix.c
> > @@ -1180,6 +1180,17 @@ static int sg_get_max_segments(int fd)
> >           goto out;
> >       }
> >   
> > +    if (S_ISCHR(st.st_mode)) {
> 
> Why not check "if (bs->sg) {" instead? It seems to be more consistent with issuing SG_ ioctl. Or what I miss?

I also think so. Actually the 'hdev_is_sg' has a check for character device as well, 
in addition to a few more checks that make sure that we are really 
dealing with the quirky /dev/sg character device.

> 
> > +        if (ioctl(fd, SG_GET_SG_TABLESIZE, &ret) == 0) {
> > +            return ret;
> > +        }
> > +        return -ENOTSUP;
> > +    }
> > +
> > +    if (!S_ISBLK(st.st_mode)) {
> > +        return -ENOTSUP;
> > +    }
> > +
> >       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);
> > 
> 
> 

Other than that, this is the same as the patch from Tom Yan:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg768262.html

In this version he does check if the SG_GET_SG_TABLESIZE is defined, so
you might want to do this as well.


Best regards,
	Maxim Levitsky

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.