diff mbox series

util: use RETRY_ON_EINTR() on open() more consistently

Message ID 20240731151714.326879-1-philipp.reisner@linbit.com (mailing list archive)
State New, archived
Headers show
Series util: use RETRY_ON_EINTR() on open() more consistently | expand

Commit Message

Philipp Reisner July 31, 2024, 3:17 p.m. UTC
As with many syscalls, open() might be interrupted by a signal.

The call trace
img_open_file()
  blk_new_open()
    raw_open()
      raw_open_common()
        qemu_open()
          qemu_open_internal()
            qemu_open_cloexec()

Ended up in calling open() without a retry loop around it.

The experienced logfile entry is:
qemu-system-x86_64: -device virtio-blk-pci,bus=pci.0,addr=0x7,drive=libvirt-2-format,id=virtio-disk0,bootindex=2,write-cache=on,serial=1b990c4d13b74a4e90ea: Could not open '/dev/drbd1003': Interrupted system call

Add the RETRY_ON_EINTR() in qemu_open_cloexec() and remove it on
call-sites using qemu_open_old().

Signed-off-by: Philipp Reisner <philipp.reisner@linbit.com>
---
 chardev/char-fd.c   | 2 +-
 chardev/char-pipe.c | 4 ++--
 os-posix.c          | 2 +-
 util/osdep.c        | 4 ++--
 4 files changed, 6 insertions(+), 6 deletions(-)

Comments

Philippe Mathieu-Daudé Aug. 1, 2024, 12:48 p.m. UTC | #1
Hi Philipp,

On 31/7/24 17:17, Philipp Reisner wrote:
> As with many syscalls, open() might be interrupted by a signal.
> 
> The call trace
> img_open_file()
>    blk_new_open()
>      raw_open()
>        raw_open_common()
>          qemu_open()
>            qemu_open_internal()
>              qemu_open_cloexec()
> 
> Ended up in calling open() without a retry loop around it.
> 
> The experienced logfile entry is:
> qemu-system-x86_64: -device virtio-blk-pci,bus=pci.0,addr=0x7,drive=libvirt-2-format,id=virtio-disk0,bootindex=2,write-cache=on,serial=1b990c4d13b74a4e90ea: Could not open '/dev/drbd1003': Interrupted system call
> 
> Add the RETRY_ON_EINTR() in qemu_open_cloexec() and remove it on
> call-sites using qemu_open_old().
> 
> Signed-off-by: Philipp Reisner <philipp.reisner@linbit.com>
> ---
>   chardev/char-fd.c   | 2 +-
>   chardev/char-pipe.c | 4 ++--
>   os-posix.c          | 2 +-
>   util/osdep.c        | 4 ++--
>   4 files changed, 6 insertions(+), 6 deletions(-)

If you need to repost, please set the next version in your patch
(which will be v4). Also avoid replying to previous version /
discussions and your patch could be missed, see
https://www.qemu.org/docs/master/devel/submitting-a-patch.html#when-resending-patches-add-a-version-tag:

   Send each new revision as a new top-level thread, rather than
   burying it in-reply-to an earlier revision, as many reviewers
   are not looking inside deep threads for new patches.

Also please Cc qemu-block@nongnu.org since the block layer is
involved via the blk_new_open() call.

Regards,

Phil.
Philipp Reisner Aug. 2, 2024, 7:26 a.m. UTC | #2
Hi Philippe and everyone else on the thread,

In the meantime, I learned that we can cause the kernel to restart the
open() syscall by returning ERESTARTSYS from the kernel-level block
device driver side. It was our mistake from the DRBD side that we
returned EINTR instead.

The proposed patch to qemu is okay but not necessary. I will no
further pursuit the inclusion of this patch.

Sorry for the noise.

Best regards,
 Philipp



On Thu, Aug 1, 2024 at 2:48 PM Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Hi Philipp,
>
> On 31/7/24 17:17, Philipp Reisner wrote:
> > As with many syscalls, open() might be interrupted by a signal.
> >
> > The call trace
> > img_open_file()
> >    blk_new_open()
> >      raw_open()
> >        raw_open_common()
> >          qemu_open()
> >            qemu_open_internal()
> >              qemu_open_cloexec()
> >
> > Ended up in calling open() without a retry loop around it.
> >
> > The experienced logfile entry is:
> > qemu-system-x86_64: -device virtio-blk-pci,bus=pci.0,addr=0x7,drive=libvirt-2-format,id=virtio-disk0,bootindex=2,write-cache=on,serial=1b990c4d13b74a4e90ea: Could not open '/dev/drbd1003': Interrupted system call
> >
> > Add the RETRY_ON_EINTR() in qemu_open_cloexec() and remove it on
> > call-sites using qemu_open_old().
> >
> > Signed-off-by: Philipp Reisner <philipp.reisner@linbit.com>
> > ---
> >   chardev/char-fd.c   | 2 +-
> >   chardev/char-pipe.c | 4 ++--
> >   os-posix.c          | 2 +-
> >   util/osdep.c        | 4 ++--
> >   4 files changed, 6 insertions(+), 6 deletions(-)
>
> If you need to repost, please set the next version in your patch
> (which will be v4). Also avoid replying to previous version /
> discussions and your patch could be missed, see
> https://www.qemu.org/docs/master/devel/submitting-a-patch.html#when-resending-patches-add-a-version-tag:
>
>    Send each new revision as a new top-level thread, rather than
>    burying it in-reply-to an earlier revision, as many reviewers
>    are not looking inside deep threads for new patches.
>
> Also please Cc qemu-block@nongnu.org since the block layer is
> involved via the blk_new_open() call.
>
> Regards,
>
> Phil.
diff mbox series

Patch

diff --git a/chardev/char-fd.c b/chardev/char-fd.c
index d2c4923359..00a225bc48 100644
--- a/chardev/char-fd.c
+++ b/chardev/char-fd.c
@@ -198,7 +198,7 @@  int qmp_chardev_open_file_source(char *src, int flags, Error **errp)
 {
     int fd = -1;
 
-    fd = RETRY_ON_EINTR(qemu_open_old(src, flags, 0666));
+    fd = qemu_open_old(src, flags, 0666);
     if (fd == -1) {
         error_setg_file_open(errp, errno, src);
     }
diff --git a/chardev/char-pipe.c b/chardev/char-pipe.c
index 5ad30bcc59..c9dc793434 100644
--- a/chardev/char-pipe.c
+++ b/chardev/char-pipe.c
@@ -131,8 +131,8 @@  static void qemu_chr_open_pipe(Chardev *chr,
 
     filename_in = g_strdup_printf("%s.in", filename);
     filename_out = g_strdup_printf("%s.out", filename);
-    fd_in = RETRY_ON_EINTR(qemu_open_old(filename_in, O_RDWR | O_BINARY));
-    fd_out = RETRY_ON_EINTR(qemu_open_old(filename_out, O_RDWR | O_BINARY));
+    fd_in = qemu_open_old(filename_in, O_RDWR | O_BINARY);
+    fd_out = qemu_open_old(filename_out, O_RDWR | O_BINARY);
     g_free(filename_in);
     g_free(filename_out);
     if (fd_in < 0 || fd_out < 0) {
diff --git a/os-posix.c b/os-posix.c
index 43f9a43f3f..7401e7da6b 100644
--- a/os-posix.c
+++ b/os-posix.c
@@ -291,7 +291,7 @@  void os_setup_post(void)
             error_report("not able to chdir to /: %s", strerror(errno));
             exit(1);
         }
-        fd = RETRY_ON_EINTR(qemu_open_old("/dev/null", O_RDWR));
+        fd = qemu_open_old("/dev/null", O_RDWR);
         if (fd == -1) {
             exit(1);
         }
diff --git a/util/osdep.c b/util/osdep.c
index 770369831b..13a09d0dd5 100644
--- a/util/osdep.c
+++ b/util/osdep.c
@@ -295,9 +295,9 @@  static int qemu_open_cloexec(const char *name, int flags, mode_t mode)
 {
     int ret;
 #ifdef O_CLOEXEC
-    ret = open(name, flags | O_CLOEXEC, mode);
+    ret = RETRY_ON_EINTR(open(name, flags | O_CLOEXEC, mode));
 #else
-    ret = open(name, flags, mode);
+    ret = RETRY_ON_EINTR(open(name, flags, mode));
     if (ret >= 0) {
         qemu_set_cloexec(ret);
     }