[PATCH] rootfs: Use separate mounts lock file

[PATCH] rootfs: Use separate mounts lock file

[Anton Mikanovich]

MOUNTS_LOCKFILE value inside rootfs.bbclass is overwritten by later
inherited buildchroot.bbclass. We need to use other value name.

Signed-off-by: Anton Mikanovich <amikan@ilbers.de>
---
 meta/classes/rootfs.bbclass | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/meta/classes/rootfs.bbclass b/meta/classes/rootfs.bbclass
index 50d6408..6ce2cf9 100644
--- a/meta/classes/rootfs.bbclass
+++ b/meta/classes/rootfs.bbclass
@@ -26,7 +26,7 @@ export LANG = "C"
 export LANGUAGE = "C"
 export LC_ALL = "C"
 
-MOUNT_LOCKFILE = "${ROOTFSDIR}.lock"
+IMAGE_MOUNT_LOCKFILE = "${ROOTFSDIR}.lock"
 
 rootfs_do_mounts[weight] = "3"
 rootfs_do_mounts() {
@@ -70,7 +70,7 @@ rootfs_do_mounts() {
                 mount --bind '${REPO_BASE_DIR}' '${ROOTFSDIR}/base-apt'
         fi
 
-        ) 9>'${MOUNT_LOCKFILE}'
+        ) 9>'${IMAGE_MOUNT_LOCKFILE}'
 EOSUDO
 }
 
@@ -103,7 +103,7 @@ rootfs_undo_mounts() {
             umount -R ${ROOTFSDIR}/proc
         mountpoint -q '${ROOTFSDIR}/dev' && \
             umount -R ${ROOTFSDIR}/dev
-        ) 9>'${MOUNT_LOCKFILE}'
+        ) 9>'${IMAGE_MOUNT_LOCKFILE}'
 EOSUDO
 }
 
-- 
2.20.1

Re: [PATCH] rootfs: Use separate mounts lock file

[Jan Kiszka]

On 08.07.21 17:22, Anton Mikanovich wrote:
> MOUNTS_LOCKFILE value inside rootfs.bbclass is overwritten by later
> inherited buildchroot.bbclass. We need to use other value name.
> 
> Signed-off-by: Anton Mikanovich <amikan@ilbers.de>
> ---
>  meta/classes/rootfs.bbclass | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/meta/classes/rootfs.bbclass b/meta/classes/rootfs.bbclass
> index 50d6408..6ce2cf9 100644
> --- a/meta/classes/rootfs.bbclass
> +++ b/meta/classes/rootfs.bbclass
> @@ -26,7 +26,7 @@ export LANG = "C"
>  export LANGUAGE = "C"
>  export LC_ALL = "C"
>  
> -MOUNT_LOCKFILE = "${ROOTFSDIR}.lock"
> +IMAGE_MOUNT_LOCKFILE = "${ROOTFSDIR}.lock"
>  
>  rootfs_do_mounts[weight] = "3"
>  rootfs_do_mounts() {
> @@ -70,7 +70,7 @@ rootfs_do_mounts() {
>                  mount --bind '${REPO_BASE_DIR}' '${ROOTFSDIR}/base-apt'
>          fi
>  
> -        ) 9>'${MOUNT_LOCKFILE}'
> +        ) 9>'${IMAGE_MOUNT_LOCKFILE}'
>  EOSUDO
>  }
>  
> @@ -103,7 +103,7 @@ rootfs_undo_mounts() {
>              umount -R ${ROOTFSDIR}/proc
>          mountpoint -q '${ROOTFSDIR}/dev' && \
>              umount -R ${ROOTFSDIR}/dev
> -        ) 9>'${MOUNT_LOCKFILE}'
> +        ) 9>'${IMAGE_MOUNT_LOCKFILE}'
>  EOSUDO
>  }
>  
> 

Definitely a good catch, but I'm still wrapping my head around it. We
either have really separated rootfs'es here so that separated lock files
are fine, or we rather want rootfs_do_mounts to be locked against
buildchroot_do_mounts as well. At least conceptually, even if they well
never run in parallel in practice. In that case, MOUNT_LOCKFILE in
rootfs.bbclass should be made a weak assignment, to clarify that
buildchroot.bbclass will win, irrespective of any ordering.

Jan

-- 
Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux

Re: [PATCH] rootfs: Use separate mounts lock file

[Anton Mikanovich]

08.07.2021 20:37, Jan Kiszka wrote:
> Definitely a good catch, but I'm still wrapping my head around it. We
> either have really separated rootfs'es here so that separated lock files
> are fine, or we rather want rootfs_do_mounts to be locked against
> buildchroot_do_mounts as well. At least conceptually, even if they well
> never run in parallel in practice. In that case, MOUNT_LOCKFILE in
> rootfs.bbclass should be made a weak assignment, to clarify that
> buildchroot.bbclass will win, irrespective of any ordering.

It mostly depends on what MOUNT_LOCKFILE was originally introduced for.
Looks like it's all starts from 48ed716 Move buildchroot mounts into 
dpkg-base class.

-- 
Anton Mikanovich
Promwad Ltd.
External service provider of ilbers GmbH
Maria-Merian-Str. 8
85521 Ottobrunn, Germany
+49 (89) 122 67 24-0
Commercial register Munich, HRB 214197
General Manager: Baurzhan Ismagulov

Re: [PATCH] rootfs: Use separate mounts lock file

[Anton Mikanovich]

08.07.2021 20:37, Jan Kiszka wrote:
> Definitely a good catch, but I'm still wrapping my head around it. We
> either have really separated rootfs'es here so that separated lock files
> are fine, or we rather want rootfs_do_mounts to be locked against
> buildchroot_do_mounts as well. At least conceptually, even if they well
> never run in parallel in practice. In that case, MOUNT_LOCKFILE in
> rootfs.bbclass should be made a weak assignment, to clarify that
> buildchroot.bbclass will win, irrespective of any ordering.
>
> Jan
>
We've performed additional checking and testing of parallel 
mounting/unmounting and didn't find any issues there.
So I'm pretty sure this should be merged.

-- 
Anton Mikanovich
Promwad Ltd.
External service provider of ilbers GmbH
Maria-Merian-Str. 8
85521 Ottobrunn, Germany
+49 (89) 122 67 24-0
Commercial register Munich, HRB 214197
General Manager: Baurzhan Ismagulov

Re: [PATCH] rootfs: Use separate mounts lock file

[Baurzhan Ismagulov]

On Mon, Jul 26, 2021 at 05:44:26PM +0300, Anton Mikanovich wrote:
> > Definitely a good catch, but I'm still wrapping my head around it. We
> > either have really separated rootfs'es here so that separated lock files
> > are fine, or we rather want rootfs_do_mounts to be locked against
> > buildchroot_do_mounts as well. At least conceptually, even if they well
> > never run in parallel in practice. In that case, MOUNT_LOCKFILE in
> > rootfs.bbclass should be made a weak assignment, to clarify that
> > buildchroot.bbclass will win, irrespective of any ordering.
> > 
> We've performed additional checking and testing of parallel
> mounting/unmounting and didn't find any issues there.
> So I'm pretty sure this should be merged.

To explain the background, we couldn't identify any resource sharing in any
combination of buildchroot and rootfses, so a separate lock has been recently
introduced in e438c8f6 "rootfs: Unmount rootfs mounts if not needed".

This didn't work correctly, as described in the commit message.

Regarding *_do_mounts, they would need to be serialized if mount(8) would have
unlocked shared resources. At least a simple script running 100 mounts and
umounts in parallel didn't reveal any issues.

With kind regards,
Baurzhan.

Re: [PATCH] rootfs: Use separate mounts lock file

[Jan Kiszka]

On 26.07.21 17:46, Baurzhan Ismagulov wrote:
> On Mon, Jul 26, 2021 at 05:44:26PM +0300, Anton Mikanovich wrote:
>>> Definitely a good catch, but I'm still wrapping my head around it. We
>>> either have really separated rootfs'es here so that separated lock files
>>> are fine, or we rather want rootfs_do_mounts to be locked against
>>> buildchroot_do_mounts as well. At least conceptually, even if they well
>>> never run in parallel in practice. In that case, MOUNT_LOCKFILE in
>>> rootfs.bbclass should be made a weak assignment, to clarify that
>>> buildchroot.bbclass will win, irrespective of any ordering.
>>>
>> We've performed additional checking and testing of parallel
>> mounting/unmounting and didn't find any issues there.
>> So I'm pretty sure this should be merged.
> 
> To explain the background, we couldn't identify any resource sharing in any
> combination of buildchroot and rootfses, so a separate lock has been recently
> introduced in e438c8f6 "rootfs: Unmount rootfs mounts if not needed".
> 
> This didn't work correctly, as described in the commit message.
> 
> Regarding *_do_mounts, they would need to be serialized if mount(8) would have
> unlocked shared resources. At least a simple script running 100 mounts and
> umounts in parallel didn't reveal any issues.
> 

OK, then let's document that in the commit and merge it.

Jan

-- 
Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux

Re: [PATCH] rootfs: Use separate mounts lock file

[Anton Mikanovich]

08.07.2021 18:22, Anton Mikanovich wrote:
> MOUNTS_LOCKFILE value inside rootfs.bbclass is overwritten by later
> inherited buildchroot.bbclass. We need to use other value name.
>
> Signed-off-by: Anton Mikanovich <amikan@ilbers.de>
>   

Applied to next with the following commit message:

> We couldn't identify any resource sharing in any combination of
> buildchroot and rootfses, so a separate lock has been recently
> introduced in e438c8f6 "rootfs: Unmount rootfs mounts if not needed".
>
> This didn't work correctly: MOUNTS_LOCKFILE value inside rootfs.bbclass
> is overwritten by buildchroot.bbclass inherited later.
>
> rootfs_do_mounts and buildchroot_do_mounts would only need to be
> serialized if mount(8) would have unlocked shared resources. At least a
> simple script running 100 mounts and umounts in parallel didn't reveal
> any issues.
>
> Use a different lock file.
>
> Signed-off-by: Anton Mikanovich <amikan@ilbers.de>
>   

-- 
Anton Mikanovich
Promwad Ltd.
External service provider of ilbers GmbH
Maria-Merian-Str. 8
85521 Ottobrunn, Germany
+49 (89) 122 67 24-0
Commercial register Munich, HRB 214197
General Manager: Baurzhan Ismagulov