[PATCHv2 1/2] iohelper: skip lseek() and ftruncate() on block devices

[Simon Rowe]

That's fine with me, thanks

Simon
________________________________
From: Michal Prívozník <mprivozn at redhat.com>
Sent: 24 August 2021 14:37
To: Simon Rowe <simon.rowe at nutanix.com>; libvir-list at redhat.com <libvir-list at redhat.com>
Subject: Re: [PATCHv2 1/2] iohelper: skip lseek() and ftruncate() on block devices

On 8/23/21 5:40 PM, Simon Rowe wrote:
> Signed-off-by: Simon Rowe <simon.rowe at nutanix.com>
> ---
>  src/util/iohelper.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/src/util/iohelper.c b/src/util/iohelper.c
> index b8810d16d3..e6eb178fde 100644
> --- a/src/util/iohelper.c
> +++ b/src/util/iohelper.c
> @@ -28,6 +28,8 @@
>  #include <unistd.h>
>  #include <fcntl.h>
>  #include <stdlib.h>
> +#include <sys/types.h>
> +#include <sys/stat.h>
>
>  #include "virthread.h"
>  #include "virfile.h"
> @@ -56,6 +58,8 @@ runIO(const char *path, int fd, int oflags)
>      unsigned long long total = 0;
>      bool direct = O_DIRECT && ((oflags & O_DIRECT) != 0);
>      off_t end = 0;
> +    struct stat sb;
> +    bool isBlockDev = false;
>
>  #if WITH_POSIX_MEMALIGN
>      if (posix_memalign(&base, alignMask + 1, buflen))
> @@ -86,9 +90,11 @@ runIO(const char *path, int fd, int oflags)
>          fdinname = "stdin";
>          fdout = fd;
>          fdoutname = path;
> +        if (fstat(fd, &sb) == 0)
> +            isBlockDev = S_ISBLK(sb.st_mode);

I think we can check for this before this switch() and report error if
fstat() fails. That way we can ..

>          /* To make the implementation simpler, we give up on any
>           * attempt to use O_DIRECT in a non-trivial manner.  */
> -        if (direct && (end = lseek(fd, 0, SEEK_END)) != 0) {
> +        if (!isBlockDev && direct && (end = lseek(fd, 0, SEEK_END)) != 0) {

.. do this change for O_RDONLY case too. IOW, I suggest squashing this
in:


diff --git i/src/util/iohelper.c w/src/util/iohelper.c
index e6eb178fde..2c91bf4f93 100644
--- i/src/util/iohelper.c
+++ w/src/util/iohelper.c
@@ -71,6 +71,14 @@ runIO(const char *path, int fd, int oflags)
     buf = (char *) (((intptr_t) base + alignMask) & ~alignMask);
 #endif

+    if (fstat(fd, &sb) < 0) {
+        virReportSystemError(errno,
+                             _("Unable to access file descriptor %d path %s"),
+                             fd, path);
+        goto cleanup;
+    }
+    isBlockDev = S_ISBLK(sb.st_mode);
+
     switch (oflags & O_ACCMODE) {
     case O_RDONLY:
         fdin = fd;
@@ -79,7 +87,7 @@ runIO(const char *path, int fd, int oflags)
         fdoutname = "stdout";
         /* To make the implementation simpler, we give up on any
          * attempt to use O_DIRECT in a non-trivial manner.  */
-        if (direct && ((end = lseek(fd, 0, SEEK_CUR)) != 0)) {
+        if (!isBlockDev && direct && ((end = lseek(fd, 0, SEEK_CUR)) != 0)) {
             virReportSystemError(end < 0 ? errno : EINVAL, "%s",
                                  _("O_DIRECT read needs entire seekable file"));
             goto cleanup;
@@ -90,8 +98,6 @@ runIO(const char *path, int fd, int oflags)
         fdinname = "stdin";
         fdout = fd;
         fdoutname = path;
-        if (fstat(fd, &sb) == 0)
-            isBlockDev = S_ISBLK(sb.st_mode);
         /* To make the implementation simpler, we give up on any
          * attempt to use O_DIRECT in a non-trivial manner.  */
         if (!isBlockDev && direct && (end = lseek(fd, 0, SEEK_END)) != 0) {


No need to resend, just let me know if you're okay with the suggested
change and I'll squash it in before pushing.

Michal

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20210824/fa18fbd6/attachment-0001.htm>