| 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 |
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.
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 --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); }
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(-)