[CRIU] [RFC PATCH 0/5] introduce --enable-fs cli option

[Oleg Nesterov]

On 04/10, Pavel Emelyanov wrote:
>
> On 04/10/2015 03:19 PM, Oleg Nesterov wrote:
> >>> 	   Why do we expose "enum fstype" to protobufs at all? Please
> >>> 	   note that typeof(mnt_entry->fstype) is uint32, not "fstype".
> >>
> >> Yes, we have this fact mentioned on the "what's bad with v1 images" page.
> >> We want it to be the "fstype".
> >
> > Yes, and thus it is not clear what could I do if I wanted to implement
> > "dump/restore auto fs names once".
> >
> > Because a single FSTYPE__AUTO code can't work in this case. This is trivial,
> > just we need
> >
> >
> > 	#define FSTYPE__AUTO		1024	// random big value
> > 	static int fstype_auto_cur = FSTYPE__AUTO;
> >
> > 	fscode_is_auto(code)
> > 	{
> > 		return code >= FSTYPE__AUTO;
> > 	}
>
> I don't get it -- why would we need more AUTO codes than just one? If you
> keep the fsname in the image anyway.

If we do not add ->fsname into mnt_entry, then we obviously can't use
a single FSTYPE__AUTO in mnt_entry->fstype.

What we need to do is:

	1. generate the new AUTO code (just fstype_auto_cur++) every
	   time we add the new AUTO entry.

	2. Move write_img_inventory() down in cr_dump_tasks() to make
	   it the last "dump".

	3. Save the (say) comma-separated list of the used auto fsnames
	   in write_img_inventory(). This needs the new member, say,
	   inventory_entry->auto_names.

Now "restore" should just restore the auto entries in check_img_inventory()
from he->auto_names, decode_fstype(me->fstype) will work just fine.

But again, this means that we need the "typeless" mnt_entry->fstype.

> > Everything should work just fine exactly because mnt_entry->fstype is not
> > "fstype". But since this typeof() looks like mistake, probably we should
> > not abuse mnt_entry->fstype this way.
>
> It's quite simple, you can introduce the optional bool auto in the entry and
> then check it. Would this work?

See above.

> > Not sure I understand... or perhaps you misunderstood me. I meant that
> > even if we turn fstypes[] into list_head, we still need fstypes_bultin[]
> > and constructor which walks this array and adds everything into the list.
>
> But you don't have to turn fstypes into list-head. The current implementation
> (seems to) works pretty well -- you just keep a string of "auto" fsnames and
> scan it without any need to add more than one entry in the fstypes[] array.

Ah! So I misunderstood you.

Yes, I thought about a single "auto" entry too. Firstly this doesn't look
clean to me because for example mounts_equal() checks c->fstype != mi->fstype.
Yes, yes, this doesn't really matter, it will work anyway. Still doesn't
look nice, I'd like to keep the "different fstypes should differ" property.

But this is minor. The main problem is: what should "dump" write into
->fsname? With this series dump_one_mountpoint() simply does

	if (me.fstype == FSTYPE__AUTO)
		me.fsname = pm->fstype->name;

If we make a single "auto" entry, we forget the actual fsname immediately
after find_fstype_by_name(). So we need the new mount_info->fsname member
initialized by parse_mountinfo_ent(). Doesn't look very nice.


> > So imho this change should start with "[PATCH 1/2] extract fstypes[0]
> > into fstype_unsupported".
>
> Frankly speaking I don't see much difference between
>
> struct fstype types[] = { { .type = UNSUPPORTED, }, <everyting else> };
>
> vs
>
> struct fstype unsupported = { .type = UNSUPPORTED, }, types[] = { <everything else> };
>
> ;)

Sure, I agree, this is subjective.

But lets suppose we turn it into list_head. Then

	find_fstype_by_name(fst)
	{
		list_for_each_entry(fstype, fstypes_list, list)
			if (!strcmp(fstype->name, fst))
				return fstype;

		return list_first_entry(fstypes_list, list);
	}

looks ugly imho. But again, again, this is really minor and subjective.
I agree in advance with everything ;)

> > Right now it is not clear to me how can I write this test-case. It needs to
> > mount the unsupported filesystem somehow, but how can it know that (say)
> > hugetlbfs a) is still not supported and b) CONFIG_HUGETLBFS is enabled?
> > I'll try to think.
>
> First thing that comes to my mind is that we have the "criu check --feature"
> that can report such stuff. We can easily add "check --feature fs_enabled"
> this will solve the "a". The "b" part is cat /proc/filesystems :)

Not sure I understand how "check" can help, but thanks. I'll try to think.

Oleg.