Re: [PATCH v4 1/7] file-posix: fix max_iov for /dev/sg devices
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
---
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, ) == 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
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
---
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, ) == 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
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
> > ---
> > 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, ) == 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
Re: [PATCH v4 1/7] file-posix: fix max_iov for /dev/sg devices
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
---
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, ) == 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
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
> ---
> 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, ) == 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
[PATCH v4 1/7] file-posix: fix max_iov for /dev/sg devices
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
---
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, ) == 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
