hw/9pfs/9p.c | 10 +++++++++- hw/9pfs/codir.c | 7 ++++++- hw/9pfs/cofile.c | 7 ++++++- 3 files changed, 21 insertions(+), 3 deletions(-)
According to 'man 2 close' errors returned by close() should only be used
for either diagnostic purposes or for catching data loss due to a previous
write error, as an error result of close() usually indicates a deferred
error of a previous write operation.
Therefore not decrementing 'total_open_fd' on a close() error is wrong
and would yield in a higher open file descriptor count than actually the
case, leading to 9p server reclaiming open file descriptors too soon.
Based-on: <20250312152933.383967-7-groug@kaod.org>
Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
---
V2: log a warning message on unexpected close() -> EBADF case
hw/9pfs/9p.c | 10 +++++++++-
hw/9pfs/codir.c | 7 ++++++-
hw/9pfs/cofile.c | 7 ++++++-
3 files changed, 21 insertions(+), 3 deletions(-)
diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index b22df3aa2b..8b001b9112 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -510,7 +510,15 @@ void coroutine_fn v9fs_reclaim_fd(V9fsPDU *pdu)
err = (f->fid_type == P9_FID_DIR) ?
s->ops->closedir(&s->ctx, &f->fs_reclaim) :
s->ops->close(&s->ctx, &f->fs_reclaim);
- if (!err) {
+
+ /* 'man 2 close' suggests to ignore close() errors except of EBADF */
+ if (unlikely(err && errno == EBADF)) {
+ /*
+ * unexpected case as FIDs were picked above by having a valid
+ * file descriptor
+ */
+ error_report("9pfs: v9fs_reclaim_fd() WARNING: close() failed with EBADF");
+ } else {
/* total_open_fd must only be mutated on main thread */
nclosed++;
}
diff --git a/hw/9pfs/codir.c b/hw/9pfs/codir.c
index 2068a4779d..bce7dd96e9 100644
--- a/hw/9pfs/codir.c
+++ b/hw/9pfs/codir.c
@@ -20,6 +20,7 @@
#include "fsdev/qemu-fsdev.h"
#include "qemu/thread.h"
#include "qemu/main-loop.h"
+#include "qemu/error-report.h"
#include "coth.h"
#include "9p-xattr.h"
#include "9p-util.h"
@@ -353,7 +354,11 @@ int coroutine_fn v9fs_co_closedir(V9fsPDU *pdu, V9fsFidOpenState *fs)
err = -errno;
}
});
- if (!err) {
+ /* 'man 2 close' suggests to ignore close() errors except of EBADF */
+ if (unlikely(err && errno == EBADF)) {
+ /* unexpected case as we should have checked for a valid file handle */
+ error_report("9pfs: WARNING: v9fs_co_closedir() failed with EBADF");
+ } else {
total_open_fd--;
}
return err;
diff --git a/hw/9pfs/cofile.c b/hw/9pfs/cofile.c
index 71174c3e4a..6e775c8e41 100644
--- a/hw/9pfs/cofile.c
+++ b/hw/9pfs/cofile.c
@@ -20,6 +20,7 @@
#include "fsdev/qemu-fsdev.h"
#include "qemu/thread.h"
#include "qemu/main-loop.h"
+#include "qemu/error-report.h"
#include "coth.h"
int coroutine_fn v9fs_co_st_gen(V9fsPDU *pdu, V9fsPath *path, mode_t st_mode,
@@ -197,7 +198,11 @@ int coroutine_fn v9fs_co_close(V9fsPDU *pdu, V9fsFidOpenState *fs)
err = -errno;
}
});
- if (!err) {
+ /* 'man 2 close' suggests to ignore close() errors except of EBADF */
+ if (unlikely(err && errno == EBADF)) {
+ /* unexpected case as we should have checked for a valid file handle */
+ error_report("9pfs: WARNING: v9fs_co_close() failed with EBADF");
+ } else {
total_open_fd--;
}
return err;
--
2.39.5On Thursday, March 20, 2025 1:16:20 PM CET Christian Schoenebeck wrote: > According to 'man 2 close' errors returned by close() should only be used > for either diagnostic purposes or for catching data loss due to a previous > write error, as an error result of close() usually indicates a deferred > error of a previous write operation. > > Therefore not decrementing 'total_open_fd' on a close() error is wrong > and would yield in a higher open file descriptor count than actually the > case, leading to 9p server reclaiming open file descriptors too soon. > > Based-on: <20250312152933.383967-7-groug@kaod.org> > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com> > --- > V2: log a warning message on unexpected close() -> EBADF case > > hw/9pfs/9p.c | 10 +++++++++- > hw/9pfs/codir.c | 7 ++++++- > hw/9pfs/cofile.c | 7 ++++++- > 3 files changed, 21 insertions(+), 3 deletions(-) Queued on 9p.next: https://github.com/cschoenebeck/qemu/commits/9p.next Thanks! /Christian
On Thu, 20 Mar 2025 13:16:20 +0100
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> According to 'man 2 close' errors returned by close() should only be used
> for either diagnostic purposes or for catching data loss due to a previous
> write error, as an error result of close() usually indicates a deferred
> error of a previous write operation.
>
> Therefore not decrementing 'total_open_fd' on a close() error is wrong
> and would yield in a higher open file descriptor count than actually the
> case, leading to 9p server reclaiming open file descriptors too soon.
>
> Based-on: <20250312152933.383967-7-groug@kaod.org>
> Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> ---
> V2: log a warning message on unexpected close() -> EBADF case
>
> hw/9pfs/9p.c | 10 +++++++++-
> hw/9pfs/codir.c | 7 ++++++-
> hw/9pfs/cofile.c | 7 ++++++-
> 3 files changed, 21 insertions(+), 3 deletions(-)
>
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index b22df3aa2b..8b001b9112 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -510,7 +510,15 @@ void coroutine_fn v9fs_reclaim_fd(V9fsPDU *pdu)
> err = (f->fid_type == P9_FID_DIR) ?
> s->ops->closedir(&s->ctx, &f->fs_reclaim) :
> s->ops->close(&s->ctx, &f->fs_reclaim);
> - if (!err) {
> +
> + /* 'man 2 close' suggests to ignore close() errors except of EBADF */
> + if (unlikely(err && errno == EBADF)) {
> + /*
> + * unexpected case as FIDs were picked above by having a valid
> + * file descriptor
> + */
> + error_report("9pfs: v9fs_reclaim_fd() WARNING: close() failed with EBADF");
Reviewed-by: Greg Kurz <groug@kaod.org>
I just hope there isn't a way to reach this 100% or we'll end up
saturating the logs. ;-)
> + } else {
> /* total_open_fd must only be mutated on main thread */
> nclosed++;
> }
> diff --git a/hw/9pfs/codir.c b/hw/9pfs/codir.c
> index 2068a4779d..bce7dd96e9 100644
> --- a/hw/9pfs/codir.c
> +++ b/hw/9pfs/codir.c
> @@ -20,6 +20,7 @@
> #include "fsdev/qemu-fsdev.h"
> #include "qemu/thread.h"
> #include "qemu/main-loop.h"
> +#include "qemu/error-report.h"
> #include "coth.h"
> #include "9p-xattr.h"
> #include "9p-util.h"
> @@ -353,7 +354,11 @@ int coroutine_fn v9fs_co_closedir(V9fsPDU *pdu, V9fsFidOpenState *fs)
> err = -errno;
> }
> });
> - if (!err) {
> + /* 'man 2 close' suggests to ignore close() errors except of EBADF */
> + if (unlikely(err && errno == EBADF)) {
> + /* unexpected case as we should have checked for a valid file handle */
> + error_report("9pfs: WARNING: v9fs_co_closedir() failed with EBADF");
> + } else {
> total_open_fd--;
> }
> return err;
> diff --git a/hw/9pfs/cofile.c b/hw/9pfs/cofile.c
> index 71174c3e4a..6e775c8e41 100644
> --- a/hw/9pfs/cofile.c
> +++ b/hw/9pfs/cofile.c
> @@ -20,6 +20,7 @@
> #include "fsdev/qemu-fsdev.h"
> #include "qemu/thread.h"
> #include "qemu/main-loop.h"
> +#include "qemu/error-report.h"
> #include "coth.h"
>
> int coroutine_fn v9fs_co_st_gen(V9fsPDU *pdu, V9fsPath *path, mode_t st_mode,
> @@ -197,7 +198,11 @@ int coroutine_fn v9fs_co_close(V9fsPDU *pdu, V9fsFidOpenState *fs)
> err = -errno;
> }
> });
> - if (!err) {
> + /* 'man 2 close' suggests to ignore close() errors except of EBADF */
> + if (unlikely(err && errno == EBADF)) {
> + /* unexpected case as we should have checked for a valid file handle */
> + error_report("9pfs: WARNING: v9fs_co_close() failed with EBADF");
> + } else {
> total_open_fd--;
> }
> return err;
--
Greg
© 2016 - 2025 Red Hat, Inc.