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

[Pavel Emelyanov]

On 04/10/2015 03:19 PM, Oleg Nesterov wrote:
> On 04/09, Pavel Emelyanov wrote:
>>
>> On 04/09/2015 08:46 PM, Oleg Nesterov wrote:
>>> Hello, and sorry for delay.
>>>
>>> Almost the same as V1, just some cosmetics changes. Seems to work.
>>>
>>> I decided to keep mnt_entry->fsname and avoid the "clever" games with
>>> dump/resore auto fsnames once. Because:
>>>
>>> 	1. I am lazy and you seem to agree with ->fsname ;)
>>>
>>> 	2. Your protobuf/mnt.proto is stupid (wrong afaics) and this
>>> 	   means that I need to write more changes...
>>
>> Can you elaborate on this? :)
> 
> Which part? lazy or stupid ? ;)

The latter one :)

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

> and find_fstype_by_name() should use fstype_auto_cur++ if fsname_is_auto().
> 
> But then it is not clear how we can dump/restore this dynamic ->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?

>>> Because personally I think this makes
>>> no sense and this (trivial) code will look worse, so we can discuss
>>> this simple change separately, including the cosmetic issues like the
>>> necessary ctor for non-auto entries.
>>
>> But current implementation doesn't require this array to be bigger than
>> it actually is -- you don't add custom fsnames in it.
> 
> 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.

> Plus other cosmetic issues. Just for example, the "special" fstypes[0]
> already looks bad too me. But a special list_first_entry(fstypes) looks
> even worse imo.
> 
> 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> };

;)

> But note that all these issues are absolutely trivial, cosmetic, and
> subjective. So I'd like to discuss this separately, and redo this change
> if you do not like (say) fstype_unsupported or something else.
> 
>>> TODO: resolve_source() && FSTYPE__AUTO. I'll resend my hack later, if/
>>> when you accept this series. This needs another (more) discussion too.
>>
>> TODO2: Add a test (even trivial) for this functionality. Please. All this
>> stuff is quite easy to break, so any help in catching simple mistakes are
>> really helpful.
> 
> OK... agreed. But probably I will need your help... 

Sure!

> 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 :)

-- Pavel