Re: [PATCH] 9pfs: Convert reclaim list to QSLIST

[Christian Schoenebeck <qemu_oss@crudebyte.com>]

On Freitag, 22. Januar 2021 15:35:14 CET Greg Kurz wrote:
> Use QSLIST instead of open-coding for a slightly improved readability.
> 
> No behavioral change.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

In general LGTM, so:

Reviewed-by: Christian Schoenebeck <qemu_oss@crudebyte.com>

Some comments below.

> ---
>  hw/9pfs/9p.c | 17 ++++++++---------
>  hw/9pfs/9p.h |  2 +-
>  2 files changed, 9 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index 3864d014b43c..5a6e2c9d3d7f 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -416,7 +416,9 @@ void coroutine_fn v9fs_reclaim_fd(V9fsPDU *pdu)
>  {
>      int reclaim_count = 0;
>      V9fsState *s = pdu->s;
> -    V9fsFidState *f, *reclaim_list = NULL;
> +    V9fsFidState *f;
> +    QSLIST_HEAD(, V9fsFidState) reclaim_list =
> +        QSLIST_HEAD_INITIALIZER(reclaim_list);
> 
>      QSIMPLEQ_FOREACH(f, &s->fid_list, next) {
>          /*
> @@ -448,8 +450,7 @@ void coroutine_fn v9fs_reclaim_fd(V9fsPDU *pdu)
>                   * a clunk request won't free this fid
>                   */
>                  f->ref++;
> -                f->rclm_lst = reclaim_list;
> -                reclaim_list = f;
> +                QSLIST_INSERT_HEAD(&reclaim_list, f, reclaim_next);

Yeah, that's actually much more readable this way.

>                  f->fs_reclaim.fd = f->fs.fd;
>                  f->fs.fd = -1;
>                  reclaim_count++;
> @@ -461,8 +462,7 @@ void coroutine_fn v9fs_reclaim_fd(V9fsPDU *pdu)
>                   * a clunk request won't free this fid
>                   */
>                  f->ref++;
> -                f->rclm_lst = reclaim_list;
> -                reclaim_list = f;
> +                QSLIST_INSERT_HEAD(&reclaim_list, f, reclaim_next);
>                  f->fs_reclaim.dir.stream = f->fs.dir.stream;
>                  f->fs.dir.stream = NULL;
>                  reclaim_count++;
> @@ -476,15 +476,14 @@ void coroutine_fn v9fs_reclaim_fd(V9fsPDU *pdu)
>       * Now close the fid in reclaim list. Free them if they
>       * are already clunked.
>       */
> -    while (reclaim_list) {
> -        f = reclaim_list;
> -        reclaim_list = f->rclm_lst;
> +    while (!QSLIST_EMPTY(&reclaim_list)) {
> +        f = QSLIST_FIRST(&reclaim_list);
> +        QSLIST_REMOVE(&reclaim_list, f, V9fsFidState, reclaim_next);
>          if (f->fid_type == P9_FID_FILE) {
>              v9fs_co_close(pdu, &f->fs_reclaim);
>          } else if (f->fid_type == P9_FID_DIR) {
>              v9fs_co_closedir(pdu, &f->fs_reclaim);
>          }
> -        f->rclm_lst = NULL;
>          /*
>           * Now drop the fid reference, free it
>           * if clunked.
> diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
> index 85fb6930b0ca..00381591ffa2 100644
> --- a/hw/9pfs/9p.h
> +++ b/hw/9pfs/9p.h
> @@ -281,7 +281,7 @@ struct V9fsFidState {
>      int ref;
>      bool clunked;
>      QSIMPLEQ_ENTRY(V9fsFidState) next;
> -    V9fsFidState *rclm_lst;
> +    QSLIST_ENTRY(V9fsFidState) reclaim_next;
>  };
> 
>  typedef enum AffixType_t {

The following is actually independent of this patch here, but related. I don't 
know about you, but this looks weird to me:

static void __attribute__((__constructor__)) v9fs_set_fd_limit(void)
{
    struct rlimit rlim;
    if (getrlimit(RLIMIT_NOFILE, &rlim) < 0) {
        error_report("Failed to get the resource limit");
        exit(1);
    }
    open_fd_hw = rlim.rlim_cur - MIN(400, rlim.rlim_cur / 3);
    open_fd_rc = rlim.rlim_cur / 2;
}

'open_fd_rc' is supposed to be the soft limit I guess and 'open_fd_hw' the 
hard limit. One thing is this combination of arbitrary divisions + MIN() + 
arbitrary constant value of 400, but okay, the calculation does not appear to 
be wrong at least.

The other thing is how reliable is it to assume this resource limit to be 
constant for the entire process life time? I know on Linux this is usually not 
an issue as you can have plenty FDs, but macOS for instance (on my TODO list) 
is rather stingy when it comes to the soft limit of a process' FDs. Might 
account to BSD as well.

I also have some doubts about how reliable the overall reclaim algorithm 
actually is. It takes a while to actually hit this algorithm in practice. 
Probably worth adding a test case 'one day'^TM.

Best regards,
Christian Schoenebeck