[LEDE-DEV] fstools: ext4 overlay support - rootfs mounted twice bug

[Josua Mayer]

Am 29.06.2016 um 12:21 schrieb John Crispin:
> 
> 
> On 29/06/2016 12:11, Josua Mayer wrote:
>> Hi John,
>>
>> thansk for taking a look. I actually sent it this way by intention,
>> looking to get in touch with someone who had the original bug.
>>
>> Am 29.06.2016 um 08:30 schrieb John Crispin:
>>> Hi,
>>>
>>> the patch is an attachement making inline commenting impossible. please
>>> send patches inline
>> From 59a8fa6d7490ba3da76aec710a1beb241ffaa089 Mon Sep 17 00:00:00 2001
>> From: Josua Mayer <josua.mayer97 at gmail.com>
>> Date: Thu, 16 Jun 2016 18:35:30 +0200
>> Subject: [PATCH 2/4] mount_root: Don't mount ext4 rootfs twice
>>
>> When there is a) no rootfs_data overlay partition,
>> and b) /dev/root points to an ext4 partition
>> the partition would be mounted twice, once as / and then as /overlay.
>> The essence of this change is to return before mounting /overlay,
>> if /dev/root has been mounted as /.
>>>
>> Signed-off-by: Josua Mayer <josua.mayer97 at gmail.com>
>> ---
>>  mount_root.c | 40 ++++++++++++++++++++++++++++++----------
>>  1 file changed, 30 insertions(+), 10 deletions(-)
>>
>> diff --git a/mount_root.c b/mount_root.c
>> index 608ce5d..13e5772 100644
>> --- a/mount_root.c
>> +++ b/mount_root.c
>> @@ -37,25 +37,45 @@ start(int argc, char *argv[1])
>>  	if (!getenv("PREINIT") && stat("/tmp/.preinit", &s))
>>  		return -1;
>>
>> +	/*
>> +	 * When the default overlay partition name rootfs_data can not be found,
>> +	 * fall back to the special /dev/root device.
>> +	 */
>>  	if (!data) {
>>  		root = volume_find("rootfs");
>>  		volume_init(root);
>> +
>> +		// mount /dev/root at /
>>  		ULOG_NOTE("mounting /dev/root\n");
>>  		mount("/dev/root", "/", NULL, MS_NOATIME | MS_REMOUNT, 0);
>> -	}
>>
>> -	/*
>> -	 * Before trying to mount and use "rootfs_data" let's check if there is
>> -	 * extroot configured. Following call will handle reading config from
>> -	 * the "rootfs_data" on its own.
>> -	 */
>> -	extroot_prefix = "";
>> -	if (!mount_extroot()) {
>> -		ULOG_NOTE("switched to extroot\n");
>> +		/*
>> +		 * Now that / has been mounted, and there is no overlay device,
>> +		 * see if extroot is configured.
>> +		 *
>> +		 * The following call will handle reading configuration from
>> +		 * rootfs on its own.
>> +		 */
>> +		extroot_prefix = "";
>> +		if (!mount_extroot()) {
>> +			ULOG_NOTE("switched to extroot\n");
>> +			/*
>> +			 * extroot succeeded mounting an overlay partition, return.
>> +			 */
>> +			return 0;
>> +		}
>> +
>> +		/*
>> +		 * Even if extroot was not configured, considering that no overlay
>> +		 * partition was found, and / was mounted, return now.
>> +		 */
>>  		return 0;
>>  	}
>>
>> -	/* There isn't extroot, so just try to mount "rootfs_data" */
>> +	/*
>> +	 * neither /dev/root nor extroot were used.
>> +	 * Attempt to mount the overlay partition.
>> +	 */
>>  	switch (volume_identify(data)) {
>>  	case FS_NONE:
>>  		ULOG_WARN("no usable overlay filesystem found, using tmpfs overlay\n");
>>
> 
> derived from looking at the patch. as the patch is not inline i am not
> able to actually import it locally so i can only judge it by looking at it.
> 
> after applying your patch, mount_extroot() would be hidden behind a
> if(!data) {} which means that it will only ever be called if there is no
> rootfs_data
That is only partially right. Like I pointed out, it is called inside
mount_overlay().
If there is a way to fix this, I need to understand what should happen:
code path before my patch:
data = volume_find("rootfs_data");
if (!data) {/* mount /dev/root as / */}
mount_extroot()
switch (volume_identify(data))

>From this artificial trace, I extracted these runtime conditions for
extroot:
1. rootfs_data exists but not mounted yet
2. rootfs_data does not exist and /dev/root mounted as /
I then guessed that mount_extroot only makes sense when there is an
overlay partition, or the read-write rootfs mounted in order to have its
configuration available.
So my guess was: if overlay isnt mounted yet but will be later, no point
in executing mount_extroot. Especially considering it will be inherently
called from the call to mount_overlay thats occuring later on.
> 
> 	John
>