Re: [PATCH] 9pfs: fix 'total_open_fd' decrementation

[Christian Schoenebeck <qemu_oss@crudebyte.com>]

On Wednesday, March 19, 2025 11:08:58 AM 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>
> ---
>  hw/9pfs/9p.c     | 14 ++++++++------
>  hw/9pfs/codir.c  |  3 ++-
>  hw/9pfs/cofile.c |  3 ++-
>  3 files changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index b22df3aa2b..f4ca8e4db5 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -434,7 +434,6 @@ void coroutine_fn v9fs_reclaim_fd(V9fsPDU *pdu)
>      V9fsFidState *f;
>      GHashTableIter iter;
>      gpointer fid;
> -    int err;
>      int nclosed = 0;
>  
>      /* prevent multiple coroutines running this function simultaniously */
> @@ -507,13 +506,16 @@ void coroutine_fn v9fs_reclaim_fd(V9fsPDU *pdu)
>       */
>      v9fs_co_run_in_worker({
>          QSLIST_FOREACH(f, &reclaim_list, reclaim_next) {
> -            err = (f->fid_type == P9_FID_DIR) ?
> +            /*
> +             * 'man 2 close' suggests to ignore close() errors except of EBADF,
> +             * not checking for EBADF here either as FIDs were picked above by
> +             * having a valid file descriptor
> +             */
> +            (f->fid_type == P9_FID_DIR) ?
>                  s->ops->closedir(&s->ctx, &f->fs_reclaim) :
>                  s->ops->close(&s->ctx, &f->fs_reclaim);
> -            if (!err) {
> -                /* total_open_fd must only be mutated on main thread */
> -                nclosed++;
> -            }
> +            /* total_open_fd must only be mutated on main thread */
> +            nclosed++;
>          }
>      });
>      total_open_fd -= nclosed;
> diff --git a/hw/9pfs/codir.c b/hw/9pfs/codir.c
> index 2068a4779d..f1fd97c8a7 100644
> --- a/hw/9pfs/codir.c
> +++ b/hw/9pfs/codir.c
> @@ -353,7 +353,8 @@ 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 (!err || errno != EBADF) {
>          total_open_fd--;
>      }
>      return err;

Or, as EBADF is somewhat unexpected here (assuming v9fs_co_closedir() was
called by checking for a valid file handle), maybe it would make sense to log
this?

    if (unlikely(err && errno == EBADF)) {
        error_report("v9fs_co_closedir() failed with EBADF");  
    } else {
        total_open_fd--;
    }

In the sense, if EBADF happens here, it's an indication for a bug in 9p
server.

> diff --git a/hw/9pfs/cofile.c b/hw/9pfs/cofile.c
> index 71174c3e4a..1e9f6da42a 100644
> --- a/hw/9pfs/cofile.c
> +++ b/hw/9pfs/cofile.c
> @@ -197,7 +197,8 @@ 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 (!err || errno != EBADF) {
>          total_open_fd--;
>      }
>      return err;

Same here then.

/Christian