| Message ID | E1tvEyJ-004dMa-So@kylie.crudebyte.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] 9pfs: fix 'total_open_fd' decrementation | expand |
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;
On 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
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;
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(-)