On Wed, 2 Feb 2011 09:40:36 -0600
Shirish Pargaonkar <[email protected]> wrote:
> +
> > +static void
> > +id_rb_insert(struct rb_root *root, struct cifs_sid_id *new_sididptr,
> > + unsigned long id)
> > +{
> > + int rc;
> > + struct rb_node **new = &(root->rb_node), *parent = NULL;
> > + struct cifs_sid_id *sididptr;
> > +
> > + while (*new) {
> > + sididptr = rb_entry(*new, struct cifs_sid_id, rbnode);
> > + parent = *new;
> > +
> > + rc = compare_sids(&sididptr->sid, &new_sididptr->sid);
> > + if (rc < 0)
> > + new = &((*new)->rb_left);
> > + if (rc > 0)
> > + new = &((*new)->rb_right);
> > + else {
> > + kfree(new_sididptr);
> > + return;
> > + }
>
> Jeff, this change is not sufficient to avoid the race in id_rb_insert()?
> If compare_sids returns 0, that means the node/mapping already exists
> i.e. the first racing thread grabbed the lock and inserted the mapping,
> so the second racing thread discovers that and returns without inserting
> that node? All of this happening within spin lock?
>
Ahh ok, I missed that -- perhaps some comments in the id_rb_insert
function would be helpful? It seems a little odd to just make it a
no-op in that case, but if you're certain that the idmapping will never
change then I suppose it will work.
That said, I'm still not fond of this scheme. Why do you feel it's
better to make both threads do an upcall in this situation instead of
sticking an entry in the tree and having the second thread block until
construction is complete?
Bear in mind that keys upcalls are _expensive_. It's basically a
fork-and-exec every time. Minimizing the number of upcalls seems
like it ought to be a design goal here particularly since this is
likely to happen quite a lot...
--
Jeff Layton <[email protected]>
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
