#3408 PR merged: New umount_mountpoint_retry_lazy function

Labels: enhancement, fixed / solved / done

jsmeix opened issue at 2025-02-26 15:33:

In lib/global-functions.sh added a new
umount_mountpoint_retry_lazy function
which is basically a copy of the code in
output/ISO/Linux-i386/700_create_efibootimg.sh
which had been added there
via https://github.com/rear/rear/pull/2909

Use the umount_mountpoint_retry_lazy function
in output/ISO/Linux-i386/700_create_efibootimg.sh
and also
in output/USB/Linux-i386/100_create_efiboot.sh
see https://github.com/rear/rear/issues/3397

jsmeix commented at 2025-02-27 11:03:

Tested "rear mkrescue" with OUTPUT=ISO
on a SLES15-SP6 KVM/QEMU VM with UEFI.

It works well for me because
for me 'umount' "just works"
(as it did for me all the time before).

Then I tested how it behaves when umount fails
by artificially changing in the function
umount_mountpoint_retry_lazy()
in lib/global-functions.sh

     # First attempt to umount:
-    umount $v "$mountpoint" && return 0
+    umount $v "$mountpoint-qq" && return 0
...
     # Retry the same umount as in the first attempt:
-    umount $v "$mountpoint" && return 0
+    umount $v "$mountpoint-qq" && return 0

and then I got

# usr/sbin/rear -D mkrescue
...
Found EFI system partition /dev/vda1 on /boot/efi type vfat
Using UEFI Boot Loader for Linux (USING_UEFI_BOOTLOADER=1)
Using '/usr/bin/xorrisofs' to create ISO filesystem images
...
Trying 'umount --lazy /var/tmp/rear.jz7oSXFsmXXvo1O/tmp/efi_virt' (normal umount failed)
Making ISO image
Wrote ISO image: /root/rear.jsmeix-umount_mountpoint_retry_lazy/var/lib/rear/output/rear-localhost.iso (199M)
...
Running exit tasks
To remove the build area you may use (with caution): rm -Rf --one-file-system /var/tmp/rear.jz7oSXFsmXXvo1O

Then I tested how it behaves when also 'umount lazy' fails
by additionally changing in the function
umount_mountpoint_retry_lazy()

-    umount $v --lazy "$mountpoint" && return 0
+    umount $v --lazy "$mountpoint-qq" && return 0

and then I got (as expected)

# usr/sbin/rear -D mkrescue
...
Found EFI system partition /dev/vda1 on /boot/efi type vfat
Using UEFI Boot Loader for Linux (USING_UEFI_BOOTLOADER=1)
Using '/usr/bin/xorrisofs' to create ISO filesystem images
...
Trying 'umount --lazy /var/tmp/rear.uXfdoRQPBm6KVhJ/tmp/efi_virt' (normal umount failed)
Could not umount EFI virtual image /var/tmp/rear.uXfdoRQPBm6KVhJ/tmp/efiboot.img at /var/tmp/rear.uXfdoRQPBm6KVhJ/tmp/efi_virt
Making ISO image
Wrote ISO image: /root/rear.jsmeix-umount_mountpoint_retry_lazy/var/lib/rear/output/rear-localhost.iso (199M)
...
Running exit tasks
Caution - there is something mounted within the build area
  /var/tmp/rear.uXfdoRQPBm6KVhJ/tmp/isofs/boot/efiboot.img on /var/tmp/rear.uXfdoRQPBm6KVhJ/tmp/efi_virt type vfat (rw,relatime,fmask=0077,dmask=0077,codepage=437,iocharset=iso8859-1,shortname=mixed,errors=remount-ro)
You must manually umount that before you may remove the build area
To remove the build area you may use (with caution): rm -Rf --one-file-system /var/tmp/rear.uXfdoRQPBm6KVhJ

# rm -Rf --one-file-system /var/tmp/rear.uXfdoRQPBm6KVhJ
rm: skipping '/var/tmp/rear.uXfdoRQPBm6KVhJ/tmp/efi_virt', since it's on a different device

# mount | grep efi
efivarfs on /sys/firmware/efi/efivars type efivarfs (rw,nosuid,nodev,noexec,relatime)
/dev/vda2 on /boot/grub2/x86_64-efi type btrfs (rw,relatime,discard=async,space_cache=v2,subvolid=264,subvol=/@/boot/grub2/x86_64-efi)
/dev/vda1 on /boot/efi type vfat (rw,relatime,fmask=0022,dmask=0022,codepage=437,iocharset=iso8859-1,shortname=mixed,utf8,errors=remount-ro)
/var/tmp/rear.uXfdoRQPBm6KVhJ/tmp/isofs/boot/efiboot.img (deleted) on /var/tmp/rear.uXfdoRQPBm6KVhJ/tmp/efi_virt type vfat (rw,relatime,fmask=0077,dmask=0077,codepage=437,iocharset=iso8859-1,shortname=mixed,errors=remount-ro)

# umount -v /var/tmp/rear.uXfdoRQPBm6KVhJ/tmp/efi_virt
umount: /var/tmp/rear.uXfdoRQPBm6KVhJ/tmp/efi_virt unmounted

# find /var/tmp/rear.uXfdoRQPBm6KVhJ
/var/tmp/rear.uXfdoRQPBm6KVhJ
/var/tmp/rear.uXfdoRQPBm6KVhJ/tmp
/var/tmp/rear.uXfdoRQPBm6KVhJ/tmp/efi_virt

# rm -Rf --one-file-system /var/tmp/rear.uXfdoRQPBm6KVhJ

jsmeix commented at 2025-02-28 14:26:

@rear/contributors
unless there are objections I would like to merge it
next week on Tuesday (March 04) afternoon.

jsmeix commented at 2025-03-03 14:31:

In general:
Almost always in practice I never find the time
to do general code cleanup work.
So yes, unfortunately in practice we have all the time
this mess of various code places here and there
that do almost same things.

The basic problem here is that I alone cannot do
general code cleanup - more precisely I alone could do it
but then only im my own way (so as if I was some kind of
absolute dictator - which I totally refuse to be).
I.e. the real problem with general code cleanup is
the very time consuming work to get general consensus
what the "one right way" is how to do something properly.

In this particular case:
@pcahyna implemented his umount_mountpoint_lazy()
as he specifically needs it for his specific use case
and I implemented my umount_mountpoint_retry_lazy()
as I specifically need it for my specific use case.

So for a general code cleanup of both some superior
"master developer" would be needed who fully understands
both specific use cases to create the generalized
"one right way" function that fulfills both needs.

I think each of us could do it in theory
but I think none of use finds the time
to actually get it done in practice.

Cf.
https://github.com/rear/rear/issues/799#issuecomment-531247109

"time permits" never happens in practice because of various
kind of annoying issues elsewhere ...
SO
in practice never time for long term improvements in ReaR
only time for some quick fix here some quick fix there
ad nauseam...

(dated Sep. 13 2019 and nothing really changed since then)

jsmeix commented at 2025-03-04 12:06:

Consolidating our several unmount functions
is not what this pull request is meant to do.
What this pull request is meant to do
is what is described in its initial description.
In particular this pull request is not meant
to change existing code.
This pull request is only meant to have existing code
(that was used at a single place where it was tested
so it has proven to work in practice as intended)
now as a function because this code is used a two places.
Advanced further things should not be done "by the way"
intermixed with what this pull request is meant to do.
Advanced further things should be done via separated
dedicated pull requests as needed, as time permits,
and by those who actually contribute further things.

schlomo commented at 2025-03-04 16:29:

I understand this. I'm worried that with this approach we just keep adding more as every ReaR developer essentially builds their "personal" ReaR within our shared codebase and nobody takes the time to clean up...

jsmeix commented at 2025-03-05 08:47:

@schlomo
I understand your concern and I have the same concern
and I know this concern is true and valid
from many times I looked at ReaR code.

But it is not true in general that
"nobody takes the time to clean up".
I spent a lot of time to clean up many things in ReaR
(I feel that cleanup work was most of what I contributed)
and also others spent a lot of time to clean up things in ReaR.

See the list of closed pull requests with "cleanup" label:
https://github.com/rear/rear/pulls?q=is%3Apr+is%3Aclosed+label%3Acleanup
This is only what is explicitly labeled as "cleanup".
Actually there was much more cleanup work done.
On the other hand only a small part of this cleanup work
was bigger generic cleanup, in particular code consolidating.

I told about my personal reasoning why there is
not much generic cleanup and code consolidating in
https://github.com/rear/rear/pull/3408#issuecomment-2694591158

I think generic cleanup and code consolidating
needs a lot of time - but not actual working time,
instead it needs time with the meaning of patience.

I think it is a precondition before someone could do
generic cleanup and code consolidating work properly
that a broad and sound understanding exists
so that the one who does the actual work
can implement it in an insight-driven way.

In the past I did a few generic cleanups and
for each of them I had known since a long time
that "this stuff needs some generic cleanup".
But it needed even longer time until I gained at least
somewhat sufficient experience by several real-world cases
to get at least some broad and sound understanding
of the issue as a whole until I felt sufficiently confident
that my generic cleanup work will (hopefully)
not cause unacceptable regressions for our users.

In this particular case here:
I had zero experience with umounting issues before
https://github.com/rear/rear/issues/2908
so I implemented
https://github.com/rear/rear/pull/2909
only at the one code place where this specific issue happened
to avoid possible regressions for our users at other code places.
See also
https://github.com/rear/rear/pull/2909#issuecomment-1429530985
where I had already told that

It is my goal behind to get in the end a
sufficiently simple and reliably working
umounting function in ReaR

But I am not yet there to implement such a function.
The recent
https://github.com/rear/rear/issues/3397
does not actually provide any new experience
because it is a duplicate of
https://github.com/rear/rear/issues/2908
at another code place so this pull request here
implements what matches for this two specific cases, cf.
https://github.com/rear/rear/pull/3408#issuecomment-2697323887

So currently I am not at all in a position to implement a
"sufficiently simple and reliably working umounting function".
To do that I need much more understanding and experience with
umounting in general via various different real-world cases.
This will likely take rather long time and patience.

This is only my personal point of view.
When someone else has sufficient knowledge to implement a
"sufficiently simple and reliably working umounting function"
I would appreciate a contribution which improves ReaR
for our users.

By the way:
I think we are rather good with generic cleanups in ReaR
compared to many other software projects.
In particular as long as the C standary library did not
clean up and consolidate its various similar functions
(in particular string related functions) into a few
that actually are sufficiently simple and reliable,
we do not need to worry about our cleanup speed in ReaR ;-)

schlomo commented at 2025-03-05 08:50:

Nice one, thank you. I can be happy with "ReaR is better than libc as a mindset" 🤣

Thanks a lot for what you are doing, you are indeed the biggest contributor as of late @jsmeix and I'm very much aware of it and I'm very thankful to you for doing that!

jsmeix commented at 2025-03-05 08:52:

You are welcome!

[Export of Github issue for rear/rear.]