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

[Pavel Emelyanov]

>>> +
>>> +       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?

Yes, some better name is OK. I assume that just 0 will work too.

>>> +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.

This doesn't matter. I can imagine quite a few real-life usecases
when we have a container with plenty of files pointing to the
same dev:ino pair. We can even throw the pos out and it will still
work OK (remember, the sys_kcmp returns actual COMPARISON, not just
equals/notequals).

> Pavel, while you at it -- the main problem is the fact itself
> that we have two fields in image?

Yes. I want it to be just u64 ID and that's it.

> 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
> .
>