Re: [PATCH 3/3] 9pfs: reduce latency of Twalk

[Christian Schoenebeck]

On Donnerstag, 27. Mai 2021 20:24:23 CEST Christian Schoenebeck wrote:
> On Donnerstag, 27. Mai 2021 19:05:43 CEST Christian Schoenebeck wrote:
> > As on the previous performance optimization on Treaddir handling;
> > reduce the overall latency, i.e. overall time spent on processing
> > a Twalk request by reducing the amount of thread hops between the
> > 9p server's main thread and fs worker thread(s).
> > 
> > In fact this patch even reduces the thread hops for Twalk handling
> > to its theoritical minimum of exactly 2 thread hops:
> > 
> > main thread -> fs worker thread -> main thread
> > 
> > This is achieved by doing all the required fs driver tasks altogether
> > in a single v9fs_co_run_in_worker({ ... }); code block.
> > 
> > This patches also changes the way how an arbitrary path is
> > identified to whether it equals the 9p export root. Previously
> > QIDs were compared for this, which forces to be done on main thread
> > for resolving individual path element QIDs. For that reason POSIX
> > stat device number and inode number pairs are compared instead now.
> > Accordingly, as 9p server's root_qid member variable is no longer
> > used, nor are functions fid_to_qid() and not_same_qid(), hence drop
> > them.
> > 
> > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> > ---
> > 
> >  hw/9pfs/9p.c | 118 +++++++++++++++++++++++++++++++++------------------
> >  hw/9pfs/9p.h |   1 -
> >  2 files changed, 76 insertions(+), 43 deletions(-)
> > 
> > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> > index 825de1561d..cc1b176eb5 100644
> > --- a/hw/9pfs/9p.c
> > +++ b/hw/9pfs/9p.c
> > @@ -971,23 +971,6 @@ static int stat_to_qid(V9fsPDU *pdu, const struct
> > stat
> > *stbuf, V9fsQID *qidp) return 0;
> > 
> >  }
> > 
> > -static int coroutine_fn fid_to_qid(V9fsPDU *pdu, V9fsFidState *fidp,
> > -                                   V9fsQID *qidp)
> > -{
> > -    struct stat stbuf;
> > -    int err;
> > -
> > -    err = v9fs_co_lstat(pdu, &fidp->path, &stbuf);
> > -    if (err < 0) {
> > -        return err;
> > -    }
> > -    err = stat_to_qid(pdu, &stbuf, qidp);
> > -    if (err < 0) {
> > -        return err;
> > -    }
> > -    return 0;
> > -}
> > -
> > 
> >  V9fsPDU *pdu_alloc(V9fsState *s)
> >  {
> >  
> >      V9fsPDU *pdu = NULL;
> > 
> > @@ -1461,7 +1444,6 @@ static void coroutine_fn v9fs_attach(void *opaque)
> > 
> >      }
> >      err += offset;
> > 
> > -    memcpy(&s->root_qid, &qid, sizeof(qid));
> > 
> >      memcpy(&s->root_st, &stbuf, sizeof(struct stat));
> >      trace_v9fs_attach_return(pdu->tag, pdu->id,
> >      
> >                               qid.type, qid.version, qid.path);
> > 
> > @@ -1713,12 +1695,9 @@ static bool name_is_illegal(const char *name)
> > 
> >      return !*name || strchr(name, '/') != NULL;
> >  
> >  }
> > 
> > -static bool not_same_qid(const V9fsQID *qid1, const V9fsQID *qid2)
> > +static bool same_stat_id(const struct stat *a, const struct stat *b)
> > 
> >  {
> > 
> > -    return
> > -        qid1->type != qid2->type ||
> > -        qid1->version != qid2->version ||
> > -        qid1->path != qid2->path;
> > +    return a->st_dev == b->st_dev && a->st_ino == b->st_ino;
> > 
> >  }

I forgot: this ^ can be split out to a separate, preparatory patch.

> >  
> >  static void coroutine_fn v9fs_walk(void *opaque)
> > 
> > @@ -1726,9 +1705,9 @@ static void coroutine_fn v9fs_walk(void *opaque)
> > 
> >      int name_idx;
> >      V9fsQID *qids = NULL;
> >      int i, err = 0;
> > 
> > -    V9fsPath dpath, path;
> > +    V9fsPath dpath, path, *pathes = NULL;
> > 
> >      uint16_t nwnames;
> > 
> > -    struct stat stbuf;
> > +    struct stat stbuf, fidst, *stbufs = NULL;
> > 
> >      size_t offset = 7;
> >      int32_t fid, newfid;
> >      V9fsString *wnames = NULL;
> > 
> > @@ -1754,6 +1733,8 @@ static void coroutine_fn v9fs_walk(void *opaque)
> > 
> >      if (nwnames) {
> >      
> >          wnames = g_new0(V9fsString, nwnames);
> >          qids   = g_new0(V9fsQID, nwnames);
> > 
> > +        stbufs = g_new0(struct stat, nwnames);
> > +        pathes = g_new0(V9fsPath, nwnames);
> > 
> >          for (i = 0; i < nwnames; i++) {
> >          
> >              err = pdu_unmarshal(pdu, offset, "s", &wnames[i]);
> >              if (err < 0) {
> > 
> > @@ -1774,35 +1755,85 @@ static void coroutine_fn v9fs_walk(void *opaque)
> > 
> >      v9fs_path_init(&dpath);
> >      v9fs_path_init(&path);
> > 
> > +    /*
> > +     * Both dpath and path initially point to fidp.
> > +     * Needed to handle request with nwnames == 0
> > +     */
> > +    v9fs_path_copy(&dpath, &fidp->path);
> > +    v9fs_path_copy(&path, &fidp->path);
> > 
> > -    err = fid_to_qid(pdu, fidp, &qid);
> > +    /*
> > +     * To keep latency (i.e. overall execution time for processing this
> > +     * Twalk client request) as small as possible, run all the required
> > fs
> > +     * driver code altogether inside the following block.
> > +     */
> > +    v9fs_co_run_in_worker({
> > +        if (v9fs_request_cancelled(pdu)) {
> > +            err = -EINTR;
> > +            break;
> > +        }
> > +        err = s->ops->lstat(&s->ctx, &dpath, &fidst);
> > +        if (err < 0) {
> > +            err = -errno;
> > +            break;
> > +        }
> > +        stbuf = fidst;
> > +        for (name_idx = 0; name_idx < nwnames; name_idx++) {
> > +            if (v9fs_request_cancelled(pdu)) {
> > +                err = -EINTR;
> > +                break;
> > +            }
> > +            if (!same_stat_id(&pdu->s->root_st, &stbuf) ||
> > +                strcmp("..", wnames[name_idx].data))
> > +            {
> > +                err = s->ops->name_to_path(&s->ctx, &dpath,
> > +                                        wnames[name_idx].data, &path);
> > +                if (err < 0) {
> > +                    err = -errno;
> > +                    break;
> > +                }
> > +                if (v9fs_request_cancelled(pdu)) {
> > +                    err = -EINTR;
> > +                    break;
> > +                }
> > +                err = s->ops->lstat(&s->ctx, &path, &stbuf);
> > +                if (err < 0) {
> > +                    err = -errno;
> > +                    break;
> > +                }
> > +                stbufs[name_idx] = stbuf;
> > +                v9fs_path_copy(&dpath, &path);
> > +                v9fs_path_copy(&pathes[name_idx], &path);
> > +            }
> > +        }
> > +    });
> > +    /*
> > +     * Handle all the rest of this Twalk request on main thread ...
> > +     */
> > 
> >      if (err < 0) {
> >      
> >          goto out;
> >      
> >      }
> 
> Depending on my last question below, this check might be wrong, i.e.
> according to the specs this should only error out if the first element
> failed.
> > -    /*
> > -     * Both dpath and path initially poin to fidp.
> > -     * Needed to handle request with nwnames == 0
> > -     */
> > +    err = stat_to_qid(pdu, &fidst, &qid);
> > +    if (err < 0) {
> > +        goto out;
> > +    }
> > +    stbuf = fidst;
> > +
> > +    /* reset dpath and path */
> > 
> >      v9fs_path_copy(&dpath, &fidp->path);
> >      v9fs_path_copy(&path, &fidp->path);
> 
> I am not sure what the expectation of a potential mutation of fid is here.
> Right now this code (as the previous one) assumes that fid does not mutate
> during entire Twalk request handling. But the same issue actually applies to
> any 9p request handling, not just Twalk.
> 
> > -    for (name_idx = 0; name_idx < nwnames; name_idx++) {
> > -        if (not_same_qid(&pdu->s->root_qid, &qid) ||
> > -            strcmp("..", wnames[name_idx].data)) {
> > -            err = v9fs_co_name_to_path(pdu, &dpath,
> > wnames[name_idx].data,
> > -                                       &path);
> > -            if (err < 0) {
> > -                goto out;
> > -            }
> > 
> > -            err = v9fs_co_lstat(pdu, &path, &stbuf);
> > -            if (err < 0) {
> > -                goto out;
> > -            }
> > +    for (name_idx = 0; name_idx < nwnames; name_idx++) {
> > +        if (!same_stat_id(&pdu->s->root_st, &stbuf) ||
> > +            strcmp("..", wnames[name_idx].data))
> > +        {
> > +            stbuf = stbufs[name_idx];
> > 
> >              err = stat_to_qid(pdu, &stbuf, &qid);
> >              if (err < 0) {
> >              
> >                  goto out;
> >              
> >              }
> > 
> > +            v9fs_path_copy(&path, &pathes[name_idx]);
> > 
> >              v9fs_path_copy(&dpath, &path);
> >          
> >          }
> >          memcpy(&qids[name_idx], &qid, sizeof(qid));
> 
> So far, replicating just the previous Twalk behaviour here (yet). Right now
> the request returns Rerror if any of the transmitted path elements could not
> be walked. Looking at the specs though, this seems to be wrong:
> 
> "If the first element cannot be walked for any reason, Rerror is returned.
> Otherwise, the walk will return an Rwalk mes- sage containing nwqid qids
> corresponding, in order, to the files that are visited by the nwqid
> successful elementwise walks; nwqid is therefore either nwname or the index
> of the first elementwise walk that failed. The value of nwqid can- not be
> zero unless nwname is zero. Also, nwqid will always be less than or equal
> to nwname. Only if it is equal, how- ever, will newfid be affected, in
> which case newfid will represent the file reached by the final elementwise
> walk requested in the message."
> 
> http://ericvh.github.io/9p-rfc/rfc9p2000.html#anchor33

I think it makes sense not mixing this purely internal performance 
optimization here with publicly visible protocol behaviour changes within the 
same patch. So I would retain the protocol behaviour of this patch for now and 
postpone the protocol fix to its own patch series a bit later, especially as 
the latter would also want that protocol behaviour change to be covered by a 
synth test case appropriately.

> 
> > @@ -1838,9 +1869,12 @@ out_nofid:
> >      if (nwnames && nwnames <= P9_MAXWELEM) {
> >      
> >          for (name_idx = 0; name_idx < nwnames; name_idx++) {
> >          
> >              v9fs_string_free(&wnames[name_idx]);
> > 
> > +            v9fs_path_free(&pathes[name_idx]);
> > 
> >          }
> >          g_free(wnames);
> >          g_free(qids);
> > 
> > +        g_free(stbufs);
> > +        g_free(pathes);
> > 
> >      }
> >  
> >  }
> > 
> > diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
> > index 6f0b4c78c0..1567b67841 100644
> > --- a/hw/9pfs/9p.h
> > +++ b/hw/9pfs/9p.h
> > @@ -355,7 +355,6 @@ struct V9fsState {
> > 
> >      int32_t root_fid;
> >      Error *migration_blocker;
> >      V9fsConf fsconf;
> > 
> > -    V9fsQID root_qid;
> > 
> >      struct stat root_st;
> >      dev_t dev_id;
> >      struct qht qpd_table;
> 
> Best regards,
> Christian Schoenebeck