[CRIU] Re: [PATCH 10/10] files: Use sys_kcmp to find file descriptor duplicates

[Cyrill Gorcunov]

On Mon, Feb 27, 2012 at 05:34:38PM +0400, Pavel Emelyanov wrote:
> > +static struct fd_id_entry *
> > +lookup_alloc_subtree(struct fd_id_entry *e, u64 genid, pid_t pid, int fd)
> 
> OK, I like the overall idea. It should be written on wiki as a separate page,
> and a summary comment should be put here.

Yeah, I'll wrte the big comment here and probably draw a tree example.

...
> > +
> > +       rb_link_node(&sub->subtree_node, parent, new);
> > +       rb_insert_color(&sub->subtree_node, &e->subtree_root);
> 
> Same code in the below function. Merge them.
> 

OK

> > +
> > +       e = alloc_fd_id_entry(genid, pid, fd);
> > +       if (!e)
> > +               goto err;
> 
> Here the subid will be INVALID. This is ... wrong.
> 
> > +       rb_link_node(&e->node, parent, new);
> > +       rb_insert_color(&e->node, &fd_id_root);
> > +err:
> > +       return e;
> > +}
> > +

No, it's OK, the only _subsequent_ subid become a numbers from global
counter. Maybe I should simply rename FD_SUBID_INVALID to say FD_SUBID_FIRST
to eliminate confusion?

> > +struct fd_id_entry *fd_id_entry_collect(u64 genid, pid_t pid, int fd)
> > +{
> > +       return lookup_alloc(genid, pid, fd);
> 
> :\

Heh ;) Sure I'll merge it.

> 
> > +struct fd_id_entry {
> > +       struct rb_node  node;
> > +
> > +       struct rb_root  subtree_root;
> > +       struct rb_node  subtree_node;
> > +
> > +       u64             genid;  /* generic id, may have duplicates */
> > +       u64             subid;  /* subid is always unique */
> > +
> > +       pid_t           pid;
> > +       int             fd;
> > +} __aligned(sizeof(long));
> > +
> > +/* Position in file is u64 so we can't shrink it */
> > +#define MAKE_FD_GENID(dev, ino, pos) \
> > +       ((u64)(dev) ^ (u64)(ino) ^ (u64)(pos))
> 
> Since genid is just a hint, we can make it 32bit and just trim the pos.
> So can we make subid be 32 bit and ...
> 
> >  struct fdinfo_entry {
> >         u8      type;
> >         u8      len;
> >         u16     flags;
> >         u32     pos;
> >         u64     addr;
> > -       u8      id[FD_ID_SIZE];
> > +       u64     genid;
> > +       u64     subid;
> 
> ... make this a single 64bit ID field.
> 
> >         u8      name[0];
> >  } __packed;
> 

Sure we can do so but it potentially may increase the number
of syscalls we need to yield if position is somewhere on top
32 bits.

Pavel, while you at it -- the main problem is the fact itself
that we have two fields in image? I could make it simply
u64 id[2] then (it's still far better than u8 id[50] we've
had before then (it's still far better than u8 id[50] we've
had before :).

	Cyrill