util/osdep.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)
As with many syscalls, open() might be interrupted by a signal.
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
Retry it until it is not interrupted by a signal.
FYI, dd has the same kind of loop aroud open().
https://github.com/coreutils/coreutils/blob/1ae98dbda7322427e8226356fd110d2553f5fac9/src/dd.c#L1294-L1299
Signed-off-by: Philipp Reisner <philipp.reisner@linbit.com>
---
util/osdep.c | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/util/osdep.c b/util/osdep.c
index 770369831b..a1269d9345 100644
--- a/util/osdep.c
+++ b/util/osdep.c
@@ -294,14 +294,17 @@ bool qemu_has_direct_io(void)
static int qemu_open_cloexec(const char *name, int flags, mode_t mode)
{
int ret;
+ do {
#ifdef O_CLOEXEC
- ret = open(name, flags | O_CLOEXEC, mode);
+ ret = open(name, flags | O_CLOEXEC, mode);
#else
- ret = open(name, flags, mode);
- if (ret >= 0) {
- qemu_set_cloexec(ret);
- }
+ ret = open(name, flags, mode);
+ if (ret >= 0) {
+ qemu_set_cloexec(ret);
+ }
#endif
+ } while (ret == -1 && errno == EINTR);
+
return ret;
}
--
2.45.2On Wed, Jul 31, 2024 at 03:25:24PM +0200, Philipp Reisner wrote:
> As with many syscalls, open() might be interrupted by a signal.
>
> 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
>
> Retry it until it is not interrupted by a signal.
As you say, many syscalls can be interruptted by signals, so
special casing open() isn't really a solution - its just
addressing one specific instance you happened to see.
If there are certain signals that we don't want to have a
fatal interruption for, it'd be better to set SA_RESTART
with sigaction, which will auto-restart a large set of
syscalls, while allowing other signals to be fatal.
> FYI, dd has the same kind of loop aroud open().
> https://github.com/coreutils/coreutils/blob/1ae98dbda7322427e8226356fd110d2553f5fac9/src/dd.c#L1294-L1299
>
> Signed-off-by: Philipp Reisner <philipp.reisner@linbit.com>
> ---
> util/osdep.c | 13 ++++++++-----
> 1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/util/osdep.c b/util/osdep.c
> index 770369831b..a1269d9345 100644
> --- a/util/osdep.c
> +++ b/util/osdep.c
> @@ -294,14 +294,17 @@ bool qemu_has_direct_io(void)
> static int qemu_open_cloexec(const char *name, int flags, mode_t mode)
> {
> int ret;
> + do {
> #ifdef O_CLOEXEC
> - ret = open(name, flags | O_CLOEXEC, mode);
> + ret = open(name, flags | O_CLOEXEC, mode);
> #else
> - ret = open(name, flags, mode);
> - if (ret >= 0) {
> - qemu_set_cloexec(ret);
> - }
> + ret = open(name, flags, mode);
> + if (ret >= 0) {
> + qemu_set_cloexec(ret);
> + }
> #endif
> + } while (ret == -1 && errno == EINTR);
> +
> return ret;
> }
>
> --
> 2.45.2
>
>
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Wed, 31 Jul 2024 at 15:11, Daniel P. Berrangé <berrange@redhat.com> wrote: > > On Wed, Jul 31, 2024 at 03:25:24PM +0200, Philipp Reisner wrote: > > As with many syscalls, open() might be interrupted by a signal. > > > > 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 > > > > Retry it until it is not interrupted by a signal. > > As you say, many syscalls can be interruptted by signals, so > special casing open() isn't really a solution - its just > addressing one specific instance you happened to see. > > If there are certain signals that we don't want to have a > fatal interruption for, it'd be better to set SA_RESTART > with sigaction, which will auto-restart a large set of > syscalls, while allowing other signals to be fatal. This is why we have the RETRY_ON_EINTR() macro, right? Currently we have some places that call qemu_open_old() inside RETRY_ON_EINTR -- we should decide whether we want to handle EINTR inside the qemu_open family of functions, or make the caller deal with it, and put the macro uses in the right place consistently. I agree that it would be nicer if we could use SA_RESTART, but presumably there's a reason why we don't. (At any rate code that's shared with the user-mode emulation has to be EINTR-resistant, because we can't force the user-mode guest code to avoid registering signal handlers that aren't SA_RESTART.) thanks -- PMM
On Wed, Jul 31, 2024 at 03:32:52PM +0100, Peter Maydell wrote: > On Wed, 31 Jul 2024 at 15:11, Daniel P. Berrangé <berrange@redhat.com> wrote: > > > > On Wed, Jul 31, 2024 at 03:25:24PM +0200, Philipp Reisner wrote: > > > As with many syscalls, open() might be interrupted by a signal. > > > > > > 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 What is the actual signal you are seeing that impacts QEMU in this way ? > > > Retry it until it is not interrupted by a signal. > > > > As you say, many syscalls can be interruptted by signals, so > > special casing open() isn't really a solution - its just > > addressing one specific instance you happened to see. > > > > If there are certain signals that we don't want to have a > > fatal interruption for, it'd be better to set SA_RESTART > > with sigaction, which will auto-restart a large set of > > syscalls, while allowing other signals to be fatal. > > This is why we have the RETRY_ON_EINTR() macro, right? > > Currently we have some places that call qemu_open_old() inside > RETRY_ON_EINTR -- we should decide whether we want to > handle EINTR inside the qemu_open family of functions, > or make the caller deal with it, and put the macro uses > in the right place consistently. It is incredibly arbitrary where we use RETRY_ON_EINTR, which I think points towards it being a sub-optimal solution to the general problem. > > I agree that it would be nicer if we could use SA_RESTART, > but presumably there's a reason why we don't. (At any > rate code that's shared with the user-mode emulation > has to be EINTR-resistant, because we can't force the > user-mode guest code to avoid registering signal handlers > that aren't SA_RESTART.) For user mode emulation isn't it valid to just propagage the EINTR back up to the application, since EINTR is a valid errno they have to be willing to handle unless the app has itself use SA_RESTART. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Hi Daniel, > > > > 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 > > What is the actual signal you are seeing that impacts QEMU > in this way ? > I do not know at this point. This only reproduces on a customer's system we do not have access to. We do not see it in our in-house lab. And qemu is called through libvirt through ApacheCloudStack. And it affects only about 10%-20% of the VM start operations. I will wrap my head around bpftrace and see if I can instruct the customer to run that on his systems. So, maybe I can answer the question regarding the signal in a few days. Maybe next week. The backing device we use (drbd) does an "auto promote" action in its open implementation. That involves exchanging some packets with some peers on the local network. I guess that takes between 1ms to 10ms. So, it exposes a larger time window than other backing block devices, which probably have a shorter running open implementation. So this is why we see it sometimes. with regards, Philipp
On Wed, 31 Jul 2024 at 16:21, Daniel P. Berrangé <berrange@redhat.com> wrote: > > On Wed, Jul 31, 2024 at 03:32:52PM +0100, Peter Maydell wrote: > > This is why we have the RETRY_ON_EINTR() macro, right? > > > > Currently we have some places that call qemu_open_old() inside > > RETRY_ON_EINTR -- we should decide whether we want to > > handle EINTR inside the qemu_open family of functions, > > or make the caller deal with it, and put the macro uses > > in the right place consistently. > > It is incredibly arbitrary where we use RETRY_ON_EINTR, which I think > points towards it being a sub-optimal solution to the general problem. Agreed (and agreed that SA_RESTART is the usual approach to avoid this mess). Partly I just vaguely recall discussions about this back when we added/improved the RETRY_ON_EINTR macro in the first place: maybe there's a reason we have it still... > > I agree that it would be nicer if we could use SA_RESTART, > > but presumably there's a reason why we don't. (At any > > rate code that's shared with the user-mode emulation > > has to be EINTR-resistant, because we can't force the > > user-mode guest code to avoid registering signal handlers > > that aren't SA_RESTART.) > > For user mode emulation isn't it valid to just propagage the > EINTR back up to the application, since EINTR is a valid errno > they have to be willing to handle unless the app has itself > use SA_RESTART. Yes, that's what we must do for cases where we are doing some syscall on behalf of the guest. But for cases where we're doing a syscall because of something QEMU itself needs to do, we may need to retry, because we might not be in a position to be able to back out of what we're doing (or we might not even be inside the "handle a guest syscall" codepath at all). -- PMM
On Wed, Jul 31, 2024 at 04:24:45PM +0100, Peter Maydell wrote: > On Wed, 31 Jul 2024 at 16:21, Daniel P. Berrangé <berrange@redhat.com> wrote: > > > > On Wed, Jul 31, 2024 at 03:32:52PM +0100, Peter Maydell wrote: > > > This is why we have the RETRY_ON_EINTR() macro, right? > > > > > > Currently we have some places that call qemu_open_old() inside > > > RETRY_ON_EINTR -- we should decide whether we want to > > > handle EINTR inside the qemu_open family of functions, > > > or make the caller deal with it, and put the macro uses > > > in the right place consistently. > > > > It is incredibly arbitrary where we use RETRY_ON_EINTR, which I think > > points towards it being a sub-optimal solution to the general problem. > > Agreed (and agreed that SA_RESTART is the usual approach to > avoid this mess). Partly I just vaguely recall discussions > about this back when we added/improved the RETRY_ON_EINTR > macro in the first place: maybe there's a reason we have it > still... > > > > I agree that it would be nicer if we could use SA_RESTART, > > > but presumably there's a reason why we don't. (At any > > > rate code that's shared with the user-mode emulation > > > has to be EINTR-resistant, because we can't force the > > > user-mode guest code to avoid registering signal handlers > > > that aren't SA_RESTART.) > > > > For user mode emulation isn't it valid to just propagage the > > EINTR back up to the application, since EINTR is a valid errno > > they have to be willing to handle unless the app has itself > > use SA_RESTART. > > Yes, that's what we must do for cases where we are doing some > syscall on behalf of the guest. But for cases where we're > doing a syscall because of something QEMU itself needs to do, > we may need to retry, because we might not be in a position > to be able to back out of what we're doing (or we might not > even be inside the "handle a guest syscall" codepath at all). Ah ok, so RETRY_ON_EINTR conceivably makes sense in the linux-user / bsd-user code in certain scenarios......but it seems almost every single use today is in system emulator code ! With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
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(-)
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);
}
--
2.45.2Hi 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.
As with many syscalls, open() might be interrupted by a signal.
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
Retry it until it is not interrupted by a signal.
FYI, dd has the same kind of loop aroud open().
https://github.com/coreutils/coreutils/blob/1ae98dbda7322427e8226356fd110d2553f5fac9/src/dd.c#L1294-L1299
Signed-off-by: Philipp Reisner <philipp.reisner@linbit.com>
---
util/osdep.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
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);
}
--
2.45.2On 31.07.24 15:25, Philipp Reisner wrote:
> As with many syscalls, open() might be interrupted by a signal.
>
> 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
>
> Retry it until it is not interrupted by a signal.
> FYI, dd has the same kind of loop aroud open().
> https://github.com/coreutils/coreutils/blob/1ae98dbda7322427e8226356fd110d2553f5fac9/src/dd.c#L1294-L1299
>
> Signed-off-by: Philipp Reisner <philipp.reisner@linbit.com>
> ---
> util/osdep.c | 13 ++++++++-----
> 1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/util/osdep.c b/util/osdep.c
> index 770369831b..a1269d9345 100644
> --- a/util/osdep.c
> +++ b/util/osdep.c
> @@ -294,14 +294,17 @@ bool qemu_has_direct_io(void)
> static int qemu_open_cloexec(const char *name, int flags, mode_t mode)
> {
> int ret;
> + do {
> #ifdef O_CLOEXEC
> - ret = open(name, flags | O_CLOEXEC, mode);
> + ret = open(name, flags | O_CLOEXEC, mode);
> #else
> - ret = open(name, flags, mode);
> - if (ret >= 0) {
> - qemu_set_cloexec(ret);
> - }
> + ret = open(name, flags, mode);
> + if (ret >= 0) {
> + qemu_set_cloexec(ret);
> + }
> #endif
> + } while (ret == -1 && errno == EINTR);
> +
> return ret;
> }
>
Reviewed-by: David Hildenbrand <david@redhat.com>
--
Cheers,
David / dhildenb
© 2016 - 2024 Red Hat, Inc.