WelcomeWelcome | FAQFAQ | DownloadsDownloads | WikiWiki

Author Topic: Very well, closing this. your patch is wrong (on /usr/sbin/rebuildfstab)  (Read 2727 times)

Offline alphons

  • Newbie
  • *
  • Posts: 42
    • building vmtux.net as of feb 14, 2023
Hello All,

I submitted a patch for /usr/sbin/rebuildfstab at github. And then it started to get weird.
--
The current rebuildfstab script is flawed having a big penalty in user, but more over system time.
The script is run every time a block device is added or removed (udev rules)
I made some changes to get it right, removed the bad loops in the script.
Ok, so far so good. These are some results on a system having some disks and more than 200 loop devices.
----
Old rebuildfstab
real    0m 0.47s
user    0m 0.08s
sys     0m 0.38s

New rebuildfstab
real    0m 0.14s
user    0m 0.03s
sys     0m 0.08s
----
These are execellent results.
When i made a pull request things got out of hand, to my opinion.

--clbr--
Thank you for the patch. While your numbers look good, it would change the behavior, and possibly have speed regressions in some envs. The current code supports manual fstab entries, while this patch would add those a second time. Running blkid without any options does a slow scan, which with optical drives and floppies can be slow (I'm not sure if the -s option also triggers this - it's possible that also does a scan).

Perhaps a large part of the time taken is by the "find" command, where adding "-maxdepth 2" may give some speedup.

If you'd like to submit the ext4 change separately, please do.

--clbr--
Oh, about the header edits: removing the includes also may change behavior, and the header comment seems improper when no other contributors have been added therein.
---------

So this is a big NO-GO to opinion o *clbr*

These are my remarks on the so called 'code review'

  • "it would change the behavior" NO
  • "The current code supports manual fstab entries, while this patch would add those a second time" Also NO, this update does exactly what the other code does, only better and faster
  • "blkid without any options does a slow scan", NO, have a look at the source code.
  • "with optical drives and floppies can be slow", sure also the NFS mounts can be slow, this has nothing to do with 99% of us
  • "The current code supports manual fstab entries, while this patch would add those a second time" NO. The behaviour of this code is exactly the same as the original code, have a look at that source, and come back with better comments
  • "If you'd like to submit the ext4 change separately, please do." NO, it is all or Nothing, but I get the picture already.

To bad, this code review does not have the high quality i want.

  • "removing the includes also may change behavior" NO, no functions are used from the header includes.
  • "header comment seems improper when no other contributors have been added therein" NO, I want some more comment, what the script does exactly and so on, this is a start

--clbr--
Very well, closing this. "come back with better comments" when your patch is wrong is simply rude.
--------

Is this collaboration??? I am not on your payrol mister.
Prove us where the script is wrong and can be made better and faster.

Cheers.

ps, Attached my dmesg of a test system having the patch, running /init after booting on 0.6 sec (!!!) mounting swap and data disk and nic up. All under a total of 1.7 seconds (!!!)

« Last Edit: February 14, 2023, 09:45:49 AM by alphons »
VMTux 1.0 - kernel 6.2.11 - glib 37 - alpha release March 2023 - https://vmtux.net/

Offline alphons

  • Newbie
  • *
  • Posts: 42
    • building vmtux.net as of feb 14, 2023
Re: Very well, closing this. your patch is wrong (on /usr/sbin/rebuildfstab)
« Reply #1 on: February 15, 2023, 12:16:06 AM »
Without installed any extensions (no loop devices), the timing of my new rebuildfstab goes down to 40mS.....

Screenshot attached, including free, df and /etc/fstab

But, nobody cares.
VMTux 1.0 - kernel 6.2.11 - glib 37 - alpha release March 2023 - https://vmtux.net/

Offline hiro

  • Hero Member
  • *****
  • Posts: 1228
Re: Very well, closing this. your patch is wrong (on /usr/sbin/rebuildfstab)
« Reply #2 on: February 15, 2023, 12:39:13 AM »
u havent even linked the patch here, how am i supposed to judge now

Offline GNUser

  • Hero Member
  • *****
  • Posts: 1378
Re: Very well, closing this. your patch is wrong (on /usr/sbin/rebuildfstab)
« Reply #3 on: February 15, 2023, 08:22:28 AM »
What hiro said.

P.S. TCL is overseen by a small number of developers. They are extremely conservative. In an age where software bloat and complexity is spreading like cancer, I think the temperament of the caretakers is an asset. I've been using TCL for several years and have noticed that the developers prioritize simplicity and compatibility with old hardware (e.g., optical drives) over performance. I haven't seen your patch but if it adds complexity or the potential to break compatibility with old hardware, I'm not surprised that you encountered some resistance.
« Last Edit: February 15, 2023, 08:24:01 AM by GNUser »

Offline nick65go

  • Hero Member
  • *****
  • Posts: 805
Re: Very well, closing this. your patch is wrong (on /usr/sbin/rebuildfstab)
« Reply #4 on: February 15, 2023, 11:02:03 AM »
I do not do programming, but for who ever is interested in the good/bad patch discussion/improvement and justify it why, please find the original link here:
https://github.com/tinycorelinux/Core-scripts/pull/39/commits/abc8b009b7ac25810aae49d7b3ec4652fbcb0d6b
From my lay-man point of view:1. in usr/sbin/rebuildfstab
- wrong eliminated busybox compatibility sintax (useBusybox ) ?
- no need to add copyright for every pi-nuts
- eliminated CDROM cases:  case "$CDROMS" in *"$DEVROOT/$DEVNAME"*) FSTYPE="auto" ;; esac

but some good tthings like:
- DEVMAJOR="$(cat $BLOCKDEV|cut -f1 -d:)"
+ DEVMAJOR="$(cat /sys/class/block/$DEVNAME/dev |cut -f1 -d:)"

-    ext2|ext3) OPTIONS="${OPTIONS},relatime" ;;
+    ext2|ext3|ext4) OPTIONS="${OPTIONS},relatime" ;;

-  mkdir -p "/mnt/$DEVNAME" 2>/dev/null >/dev/null
+ mkdir -p "/mnt/$DEVNAME" >/dev/null 2>&1

so the main thing time-consuming remains awk loop.
« Last Edit: February 15, 2023, 11:30:52 AM by nick65go »

Offline GNUser

  • Hero Member
  • *****
  • Posts: 1378
Re: Very well, closing this. your patch is wrong (on /usr/sbin/rebuildfstab)
« Reply #5 on: February 15, 2023, 11:44:53 AM »
It seems the issue is only partly technical. Main issue seems to be social/interpersonal. I'm sorry to have gotten involved.

Offline hiro

  • Hero Member
  • *****
  • Posts: 1228
Re: Very well, closing this. your patch is wrong (on /usr/sbin/rebuildfstab)
« Reply #6 on: February 15, 2023, 02:49:57 PM »
both sides are rude here. but there seems to be a language barrier, too.

i'm sure we can make something out of your idea, whether with this patch or another one later.

without testing anything, just from looking at your patch, i guess the main performance bottleneck you found is that blkid is run multiple times?

Offline hiro

  • Hero Member
  • *****
  • Posts: 1228
Re: Very well, closing this. your patch is wrong (on /usr/sbin/rebuildfstab)
« Reply #7 on: February 15, 2023, 03:10:43 PM »
for reference, this is what blkid's -s tag does (from the man page):

For each (specified) device, show only the tags that match tag. It is possible to specify multiple -s options. If no tag is specified, then all tokens are shown for all (specified) devices. In order to just refresh the cache without showing any tokens, use -s none with no other options.

Offline hiro

  • Hero Member
  • *****
  • Posts: 1228
Re: Very well, closing this. your patch is wrong (on /usr/sbin/rebuildfstab)
« Reply #8 on: February 15, 2023, 03:13:17 PM »
  • "blkid without any options does a slow scan", NO, have a look at the source code.

sure you aren't being actively misleading here?
as said in the last message, i can't imagine any other reason apart from blkid's slowness in the first place.

Offline curaga

  • Administrator
  • Hero Member
  • *****
  • Posts: 10974
Re: Very well, closing this. your patch is wrong (on /usr/sbin/rebuildfstab)
« Reply #9 on: February 15, 2023, 11:15:30 PM »
Alphons does not understand what the original code did, mindlessly removes it, then insists he is correct without understanding the issue, and insists on visible credit when his contribution is much smaller than previous contributions by aswhj etc, and when in the git era the credit info is kept in git commits.

"Also NO, this update does exactly what the other code does, only better and faster"
Let me spell out how it changes behavior and breaks the custom mounts. Say you have a custom fstab line mounting /dev/sda1 to /files. After the patch, it would add /dev/sda1 a second time, breaking the custom mountpoint.

" NO, no functions are used from the header includes."
Wrong. He didn't even read the code he changed, and then insists he is right. useBusybox is the function called, which affects among others cat, grep, sort, sync, etc. called in the script.
The only barriers that can stop you are the ones you create yourself.

Offline hiro

  • Hero Member
  • *****
  • Posts: 1228
Re: Very well, closing this. your patch is wrong (on /usr/sbin/rebuildfstab)
« Reply #10 on: February 16, 2023, 03:19:52 AM »
he seemed to try to fish for credit, but i gave it a pass bec. he might have seen the pattern of adding the authors in another file that he also had sent a patch for.

but i also found an older patch of them about colors which had to be reverted bec. it also unintendedly broke other things.
a lot of momentum, but sadly it's a wasted effort here...

you'll have to move fast and break things on another project.

Offline FlyingDutchman

  • Newbie
  • *
  • Posts: 36
Re: Very well, closing this. your patch is wrong (on /usr/sbin/rebuildfstab)
« Reply #11 on: February 19, 2023, 02:17:20 AM »
I have followed this conversation with interest. I'm not going to judge who is wrong or right here (that is not my place), but as a TCL enthusiast, who wants to learn how TCL exactly works, I can share that it is hard to understand what the standard scripts do that ship with the release.

There are almost no comments in the code and there is no header section or alike, with an overview of what the intent of the script is. So you always have to read the code and reverse engineer. Then it is easy to overlook use cases or nuances if you change the code.

Offline hiro

  • Hero Member
  • *****
  • Posts: 1228
Re: Very well, closing this. your patch is wrong (on /usr/sbin/rebuildfstab)
« Reply #12 on: February 19, 2023, 02:25:05 AM »
So you always have to read the code.

welcome to the real world

Offline FlyingDutchman

  • Newbie
  • *
  • Posts: 36
Re: Very well, closing this. your patch is wrong (on /usr/sbin/rebuildfstab)
« Reply #13 on: February 19, 2023, 03:07:23 AM »
Quote
welcome to the real world

In the real world, writing and maintaining comments is considered a good thing. If you mean that despite this, in the real world useful comments are often missing so you have to reverse engineer the code anyway, then I agree.

Offline Rich

  • Administrator
  • Hero Member
  • *****
  • Posts: 11283
Re: Very well, closing this. your patch is wrong (on /usr/sbin/rebuildfstab)
« Reply #14 on: February 19, 2023, 06:23:33 AM »
Hi FlyingDutchman
... useful comments are often missing so you have to reverse engineer the code ...
I've run into that issue too:
Hi Juanito
I'd almost forgotten about that - thanks  :)
I rediscovered this thread about 6 weeks ago and left an open tab to it in my browser. That solved the forgetting part, but
not the procrastination because I found it difficult to follow and understand the code. You can't fix what you don't understand. ::)
Had I reformatted and commented the code from the get go, I would have been done a lot sooner. :-[ ...