Fixes #s
authorBarret Rhoden <brho@cs.berkeley.edu>
Tue, 28 Jan 2014 22:04:54 +0000 (14:04 -0800)
committerBarret Rhoden <brho@cs.berkeley.edu>
Tue, 28 Jan 2014 22:04:54 +0000 (14:04 -0800)
Looks like we won't be able to hang reference counted objects off of
c->aux, such that those objects change as we gen new entries.  The final
culprit was devclone, which copies aux, but doesn't incref.  And later,
it may or may not call the device's close.

We can still put reference counted things on c->aux, but they need to
not change during walks.  Check out devpipe and devip for examples.

kern/drivers/dev/alarm.c
kern/drivers/dev/srv.c

index d7c131a..274612d 100644 (file)
@@ -264,8 +264,7 @@ static struct chan *alarmopen(struct chan *c, int omode)
                         * could have been closed, and the alarm decref'd and freed.  the
                         * qid is essentially an uncounted reference, and we need to go to
                         * the source to attempt to get a real ref.  Unfortunately, this is
-                        * another scan of the list, same as devsrv.  We could speed it up
-                        * by storing an "on_list" bool in the a_is. */
+                        * another scan of the list, same as devsrv. */
                        spin_lock(&p->alarmset.lock);
                        TAILQ_FOREACH(a_i, &p->alarmset.list, link) {
                                if (a_i == a) {
index 1691636..db43e80 100644 (file)
@@ -6,21 +6,20 @@
  * #s, but it's been completely rewritten to act like what I remember the plan9
  * one to be like.
  *
- * I'm trying a style where we hang items off c->aux, specific to each chan.
- * Instead of looking up via qid.path, we just look at the c->aux for our
- * struct.  This has been a pain in the ass, and might have issues still.
  *
- * basically, whenever we gen a response, the chan will now have the aux for
- * that response.  though we don't change the chan's qid (devwalk will do that
- * on a successful run).
+ * I tried a style where we hang reference counted objects off c->aux, specific
+ * to each chan.  Instead of looking up via qid.path, we just look at the c->aux
+ * for our struct.  I originally tried having those be reference counted
+ * structs, but that fails for a bunch of reasons.  Without them being reference
+ * counted, we're really just using c->aux as if it was qid.path.
  *
- * one consequence of the refcnt style is that so long as an FD is open, it'll
- * hold the chan and the srvfile in memory.  even if the file is removed from
- * #s, it can still be read and written.  removal only prevents future walks.
- *
- * i've seen other devices do the chan->aux thing, but not like this.  those
- * other ones had aux be for a device instance (like pipe - every attach creates
- * its own pipe instance). */
+ * We can't hang an external reference to an item off c->aux, and have that item
+ * change as we gen (but we can use it as a weak ref, uncounted ref).  The main
+ * thing is that devclone makes a 'half-chan' with a copy of c->aux.  This chan
+ * may or may not be closed later.  If we transfer refs via a gen, we first
+ * assumed we had a ref in the first place (devclone doesn't incref our srv),
+ * and then we might not close.  This ends up decreffing top_dir too much, and
+ * giving it's refs to some other file in the walk. */
 
 #include <vfs.h>
 #include <kfs.h>
@@ -45,7 +44,6 @@ struct srvfile {
        char *name;
        struct chan *chan;
        struct kref ref;                /* +1 for existing on create, -1 on remove */
-       bool on_list;
        char *user;
        uint32_t perm;
        atomic_t opens;                 /* used for exclusive open checks */
@@ -54,12 +52,27 @@ struct srvfile {
 struct srvfile *top_dir;
 TAILQ_HEAD(srvfilelist, srvfile) srvfiles = TAILQ_HEAD_INITIALIZER(srvfiles);
 /* the lock protects the list and its members.  we don't incref from a list ref
- * without the lock. (if you're on the list, we can grab a ref).  Nor do we
- * remove or mess with 'on_list' for any member without the lock. */
+ * without the lock. (if you're on the list, we can grab a ref). */
 spinlock_t srvlock = SPINLOCK_INITIALIZER;
 
 atomic_t nr_srvs = 0; /* debugging - concerned about leaking mem */
 
+/* Given a pointer (internal ref), we attempt to get a kref */
+static bool grab_ref(struct srvfile *srv)
+{
+       bool ret = FALSE;
+       struct srvfile *srv_i;
+       spin_lock(&srvlock);
+       TAILQ_FOREACH(srv_i, &srvfiles, link) {
+               if (srv_i == srv) {
+                       ret = kref_get_not_zero(&srv_i->ref, 1);
+                       break;
+               }
+       }
+       spin_unlock(&srvlock);
+       return ret;
+}
+
 static void srv_release(struct kref *kref)
 {
        struct srvfile *srv = container_of(kref, struct srvfile, ref);
@@ -79,56 +92,28 @@ static int srvgen(struct chan *c, char *name, struct dirtab *tab,
 
        if (s == DEVDOTDOT) {
                /* changing whatever c->aux was to be topdir */
-               kref_get(&top_dir->ref, 1);
-               kref_put(&((struct srvfile*)(c->aux))->ref);
                mkqid(&q, Qtopdir, 0, QTDIR);
                devdir(c, q, "#s", 0, eve, 0555, dp);
                return 1;
        }
-       /* we're genning the contents of the #s/ directory.  first time through, we
-        * just give the first item.  we have a c->aux from a previous run, though
-        * for s == 0, it's not necessarily the top_dir.
-        *
-        * while we do come in with c->aux, and it is a srvfile, it might be one
-        * that is removed from the list.  since we always add new items to the
-        * tail, if prev is still on the list, we can just return the next one.  but
-        * if we aren't on the list, we'll need to gen from scratch.
-        *
-        * This might be fucked up when we're genning and not starting from s = 0,
-        * and when prev isn't the actual previous one.  devdirread will start from
-        * other 's'es.  If prev is fucked up, we'll have to take out the on_list
-        * optimization. */
-       prev = c->aux;
        spin_lock(&srvlock);
-       if (s == 0) {
-               next = TAILQ_FIRST(&srvfiles);
-       } else if (prev->on_list) {
-               assert(prev != top_dir);
-               next = TAILQ_NEXT(prev, link);
-       } else {
-               /* fall back to the usual scan, which is normally O(n) (O(n^2) overall).
-                * though this only happens when there was some race and prev was
-                * removed, and next gen it should be O(1). */
-               TAILQ_FOREACH(next, &srvfiles, link) {
-                       /* come in with s == 0 on the first run */
-                       if (s-- == 0)
-                               break;
-               }
+       TAILQ_FOREACH(next, &srvfiles, link) {
+               /* come in with s == 0 on the first run */
+               if (s-- == 0)
+                       break;
        }
        if (!next) {
                spin_unlock(&srvlock);
                return -1;
        }
-       /* the list lock allows us to grab the ref */
-       kref_get(&next->ref, 1);        /* passing out via c->aux */
-       spin_unlock(&srvlock);
        /* update c to point to our new srvfile.  this keeps the chan and its srv in
-        * sync with what we're genning.  this does suck a bit, since we'll do a
-        * lot of increfs and decrefs. */
-       kref_put(&prev->ref);
-       c->aux = next;          
+        * sync with what we're genning. */
+       c->aux = next;  /* uncounted ref */
        mkqid(&q, Qsrvfile, 0, QTFILE);
-       devdir(c, q, next->name, 1/* length */, next->user, next->perm, dp);
+       /* once we release the lock, next could disappear, including next->name */
+       strncpy(get_cur_genbuf(), next->name, GENBUF_SZ);
+       devdir(c, q, get_cur_genbuf(), 1/* length */, next->user, next->perm, dp);
+       spin_unlock(&srvlock);
        return 1;
 }
 
@@ -155,8 +140,7 @@ static struct chan *srvattach(char *spec)
         * we're not sure that complexity is needed. */
        struct chan *c = devattach('s', spec);
        mkqid(&c->qid, Qtopdir, 0, QTDIR);
-       /* c->aux is a counted ref */
-       kref_get(&top_dir->ref, 1);
+       /* c->aux is an uncounted ref */
        c->aux = top_dir;
        return c;
 }
@@ -164,39 +148,7 @@ static struct chan *srvattach(char *spec)
 static struct walkqid *srvwalk(struct chan *c, struct chan *nc, char **name,
                                                           int nname)
 {
-       struct srvfile *srv = c->aux;
-       assert(srv);    /* all srv chans should have an aux. */
-       struct walkqid *wq = devwalk(c, nc, name, nname, 0, 0, srvgen);
-       /* I don't fully understand this, but when wq and wq->clone return, we need
-        * to account for the aux (example, devip, devpipe, etc, though those differ
-        * in that they have one aux for the entire #device).
-        *
-        * i think this is because the callers of our walk track and cclose the
-        * initial chan , regardless of what we did internally.  walk() does this
-        * (via *cp).  though the main one i've run into is cclone().  callers of
-        * cclone() close both the source chan and the cloned chan.
-        *
-        * So even though our gen moved along the steps of the state machine,
-        * tracking each change of the working chan (via inc and decref), the caller
-        * has an extra copy of the original c that it'll close.  
-        *
-        * So i think we should be increffing the original c->aux, and that we
-        * already accounted for wq->clone (when we ran gen, and genned the final
-        * answer).
-        *
-        * Regarding the clone != c check: many other devs do this, and it's
-        * probably correct.  One thing: the c->aux is per chan, and the srv ref
-        * only gets put once per chan close.  If the two chans (clone and c) are
-        * the same, then there is one real chan with a chan refcnt of 2.  We don't
-        * want a refcnt of 2 on the srv, since there is really only one srv ref.
-        *
-        * Oh, and FYI, when we're called from clone, nnames == 0, and some devs
-        * (like the original inferno srv) treat that case differently. */
-       if (wq && wq->clone && (wq->clone != c)) {
-               assert(wq->clone->aux); /* make sure this ran through gen */
-               kref_get(&srv->ref, 1);
-       }
-       return wq;
+       return devwalk(c, nc, name, nname, 0, 0, srvgen);
 }
 
 static int srvstat(struct chan *c, uint8_t * db, int n)
@@ -206,6 +158,7 @@ static int srvstat(struct chan *c, uint8_t * db, int n)
 
 static struct chan *srvopen(struct chan *c, int omode)
 {
+       ERRSTACK(1);
        struct srvfile *srv;
        openmode(omode);        /* used as an error checker in plan9, does little now */
        if (c->qid.type & QTDIR) {
@@ -216,8 +169,13 @@ static struct chan *srvopen(struct chan *c, int omode)
                c->offset = 0;
                return c;
        }
-       /* c->aux is a counted ref.  srv could be removed from the list already. */
        srv = c->aux;
+       if (!grab_ref(srv))
+               error("Unable to open srv file, concurrent removal");
+       if (waserror()) {
+               kref_put(&srv->ref);
+               nexterror();
+       }
        devpermcheck(srv->user, srv->perm, omode);
        /* No remove on close support yet */
        #if 0
@@ -237,9 +195,11 @@ static struct chan *srvopen(struct chan *c, int omode)
         * as gens work their way through the list */
        atomic_inc(&srv->opens);
        /* the magic of srv: open c, get c->srv->chan back */
-       cclose(c);      /* also closes/decrefs c->aux */
+       cclose(c);
        c = srv->chan;
        kref_get(&c->ref, 1);
+       poperror();
+       kref_put(&srv->ref);
        return c;
 }
 
@@ -252,14 +212,10 @@ static void srvcreate(struct chan *c, char *name, int omode, uint32_t perm)
        srv->perm = 0770;       /* TODO need some security thoughts */
        atomic_set(&srv->opens, 1);                     /* we return it opened */
        mkqid(&c->qid, Qsrvfile, 0, QTFILE);
-       /* we're switching the chan that came in from the old one to the one we
-        * created.  we're caleled from namec, where our c == cnew. */
-       kref_put(&((struct srvfile*)c->aux)->ref);
        c->aux = srv;
-       /* one ref for being on the list, another for c->aux */
-       kref_init(&srv->ref, srv_release, 2);
+       /* one ref for being on the list */
+       kref_init(&srv->ref, srv_release, 1);
        spin_lock(&srvlock);
-       srv->on_list = TRUE;
        TAILQ_INSERT_TAIL(&srvfiles, srv, link);
        spin_unlock(&srvlock);
        atomic_inc(&nr_srvs);
@@ -274,25 +230,26 @@ static int srvwstat(struct chan *c, uint8_t * dp, int n)
 static void srvclose(struct chan *c)
 {
        struct srvfile *srv = c->aux;
+       if (!grab_ref(srv))
+               return;
        atomic_dec(&srv->opens);
        kref_put(&srv->ref);
 }
 
 static void srvremove(struct chan *c)
 {
-       struct srvfile *srv = c->aux;
+       struct srvfile *srv_i, *temp;
 
        spin_lock(&srvlock);
-       /* could have a concurrent removal */
-       if (srv->on_list) {
-               TAILQ_REMOVE(&srvfiles, srv, link);
-               srv->on_list = FALSE;
-               /* for my sanity.  when closing, we should have two refs, one for being
-                * on the list, and the other for c->aux, which gets closed later. */
-               assert(kref_refcnt(&srv->ref) > 1);
-               kref_put(&srv->ref);    /* dropping ref from the list */
+       TAILQ_FOREACH_SAFE(srv_i, &srvfiles, link, temp) {
+               if (srv_i == c->aux) {
+                       TAILQ_REMOVE(&srvfiles, srv_i, link);
+                       break;
+               }
        }
        spin_unlock(&srvlock);
+       if (srv_i)
+               kref_put(&srv_i->ref);  /* dropping ref from the list */
 }
 
 /* N.B. srvopen gives the chan back. The only 'reading' we do
@@ -305,7 +262,7 @@ static long srvread(struct chan *c, void *va, long count, int64_t offset)
 
 static long srvwrite(struct chan *c, void *va, long count, int64_t offset)
 {
-       ERRSTACK(1);
+       ERRSTACK(2);
        struct srvfile *srv;
        struct chan *new_chan;
        char *kbuf = 0;
@@ -313,8 +270,13 @@ static long srvwrite(struct chan *c, void *va, long count, int64_t offset)
 
        if (c->qid.type & QTDIR)
                error(Eperm);
-       /* srv is refcnt'd, no fear of it disappearing */
        srv = c->aux;
+       if (!grab_ref(srv))
+               error("Unable to write srv file, concurrent removal");
+       if (waserror()) {
+               kref_put(&srv->ref);
+               nexterror();
+       }
        if (srv->chan)
                error("srv file already has a stored chan!");
        if (waserror()) {
@@ -335,6 +297,8 @@ static long srvwrite(struct chan *c, void *va, long count, int64_t offset)
        }
        poperror();
        kfree(kbuf);
+       poperror();
+       kref_put(&srv->ref);
        return count;
 }