Re: [PATCH 05/26] dpkg: Fix shared-locking of build task

[Claudius Heine <claudius.heine.ext@siemens.com>]

Hi Jan,

On 26/08/2019 08.26, [ext] Jan Kiszka wrote:
> From: Jan Kiszka <jan.kiszka@siemens.com>
> 
> Bitbake file locks are not just exclusive flock calls. For whatever
> reason, bitbake prefers to delete the lock file before unlocking them.
> That obviously creates a nasty race with other locker requests: Those
> already waiting will get access to the removed lock, those coming later
> will create a new one. To resolve that race, bitbakes lockfile() tests
> if the inode of the acquired lock file is identical to the one that is
> currently visible via the lock name. If not, it retries the lock. What a
> pointless mess...
> 
> Our open-coded "flock -s" does not do that, thus will often acquire only
> a stale lock. Fix that by using bitbake's services for locking. That
> requires to move the lock acquisition at the top level of the task, ie.
> do_build. Thus, also dpkg-raw and kernel builds are now under the shared
> (read) lock. The better fix for this issue would actually be private
> buildchroots for each build, but that will come later.
> 
> CC: Cedric Hombourger <Cedric_Hombourger@mentor.com>
> Fixes: 568072765b08 ("dpkg: acquire a read (shared) locks while building packages")
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>   meta/classes/dpkg-base.bbclass | 11 +++++++----
>   meta/classes/dpkg.bbclass      |  5 ++---
>   2 files changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/meta/classes/dpkg-base.bbclass b/meta/classes/dpkg-base.bbclass
> index 2d26418..8146328 100644
> --- a/meta/classes/dpkg-base.bbclass
> +++ b/meta/classes/dpkg-base.bbclass
> @@ -87,10 +87,13 @@ dpkg_runbuild() {
>       die "This should never be called, overwrite it in your derived class"
>   }
>   
> -do_build() {
> -    dpkg_do_mounts
> -    dpkg_runbuild
> -    dpkg_undo_mounts
> +python do_build() {
> +    lock = bb.utils.lockfile(d.getVar("REPO_ISAR_DIR") + "/isar.lock",
> +                             shared=True)
> +    bb.build.exec_func("dpkg_do_mounts", d)
> +    bb.build.exec_func("dpkg_runbuild", d)
> +    bb.build.exec_func("dpkg_undo_mounts", d)
> +    bb.utils.unlockfile(lock)

What is the difference between this and

do_build[lockfiles] += "${REPO_ISAR_DIR}/isar.lock"

?

regards,
Claudius

>   }
>   
>   CLEANFUNCS += "repo_clean"
> diff --git a/meta/classes/dpkg.bbclass b/meta/classes/dpkg.bbclass
> index dd123c3..df47ced 100644
> --- a/meta/classes/dpkg.bbclass
> +++ b/meta/classes/dpkg.bbclass
> @@ -18,7 +18,6 @@ do_install_builddeps[lockfiles] += "${REPO_ISAR_DIR}/isar.lock"
>   # Build package from sources using build script
>   dpkg_runbuild() {
>       E="${@ bb.utils.export_proxies(d)}"
> -    flock -s "${REPO_ISAR_DIR}/isar.lock" \
> -        sudo -E chroot --userspec=$( id -u ):$( id -g ) ${BUILDCHROOT_DIR} \
> -             /isar/build.sh ${PP}/${PPS} ${DISTRO_ARCH}
> +    sudo -E chroot --userspec=$( id -u ):$( id -g ) ${BUILDCHROOT_DIR} \
> +         /isar/build.sh ${PP}/${PPS} ${DISTRO_ARCH}
>   }
> 

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-54 Fax: (+49)-8142-66989-80 Email: ch@denx.de