| Message ID | 20210603133722.218465-2-pbonzini@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block: file-posix queue | expand |
03.06.2021 16:37, 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. > > 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..58db526cc2 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; So, this is a new most-probable path for char-dev, yes? Is it better do it at start of the function to avoid extra fstat() ? > + } > + return -EIO; > + } > + > + 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); >
07.06.2021 08:39, Vladimir Sementsov-Ogievskiy wrote: > 03.06.2021 16:37, 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. >> >> 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..58db526cc2 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; > > So, this is a new most-probable path for char-dev, yes? > > Is it better do it at start of the function to avoid extra fstat() ? Haha, stupid question, sorry my morning sleepy eyes. fstat is used for outer if condition. > >> + } >> + return -EIO; >> + } >> + >> + 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); >> > >
03.06.2021 16:37, 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. I assume, you keep /sys/dev/block code branch here only for following converting of the function to be hdev_get_max_hw_transfer()? Worth mentioning here? > > On Linux the sg driver has its own way to provide the maximum number of > iovecs in a scatter/gather list. > > 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..58db526cc2 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)) { Should it be instead "if (bs->sg) {" ? As SG_GET_SG_TABLESIZE looks like sg-specific, not for any chardev. Also, st is not a pointer, so here and in the next if condition it should be s/st->/st./ > + if (ioctl(fd, SG_GET_SG_TABLESIZE, &ret) == 0) { > + return ret; > + } > + return -EIO; > + } > + > + 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); >
On 03.06.21 15:37, 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. > > 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..58db526cc2 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 -EIO; > + } > + > + if (!S_ISBLK(st->st_mode)) { > + return -ENOTSUP; > + } > + With %s/->/./: Reviewed-by: Max Reitz <mreitz@redhat.com> (To answer Vladimir’s question, I don’t believe the condition should be bs->sg, because bs->sg == true is a given for this function anyway. Therefore, there’s no need to check whether the char device is an SG device.)
diff --git a/block/file-posix.c b/block/file-posix.c index f37dfc10b3..58db526cc2 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 -EIO; + } + + 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);
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. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- block/file-posix.c | 11 +++++++++++ 1 file changed, 11 insertions(+)