| Message ID | CAENjB=+u_QMpxPPNjC3bzLM9MxxQTAf+jPufbUi66LcRqwf0kA@mail.gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | John Crispin |
| Headers | show |
Hi, few comments 1) the patch needs to be split into 2, ext2 change/lzo change 2) send them inline please and not as attachement 3) the V3 needs to be in the subject prefix [PATCH V3] procd: ... 4) the Signed-off-by: line is missing John On 03/07/2016 08:14, Luke McKee wrote: > To quote Arjen: > If the only reason to switch to ext2 is to remove the journal, why not > just add > > -O ^has_journal > > to the mount options? > > ---- > > That's not a mount option. That's a tune2fs option. > > Journaling isn't the the only problem. The biggest problem is BLOAT. > You need libext2+e2fsprogs in a 4MB flash root image with the current > zram setup. > Would someone want /tmp on zram if they had to add all this to the 4MB > squashfs image. > > -rw-r--r-- 1 lmc users 199739 Jul 3 06:15 e2fsprogs_1.43.1-1_mips_24kc.ipk > -rw-r--r-- 1 lmc users 151873 Jul 3 06:15 libext2fs_1.43.1-1_mips_24kc.ipk > > The simple answer is most openwrt users would say no to this feature > do to how it's implemented. > > I actually was going for ext2 like the original patch, but I was sold > that ext4 can mount ext2 formatted filesystems (without a journal) > (see man ext4). The ext4 filesystem driver vs ext2 does bloat a bit, > but I don't mind it as I can mount ext4 over nbd for my extroot. > > Busybox is only 40k of source including the headers. mkfs.ext4 is 360k > compressed! > https://git.busybox.net/busybox/tree/util-linux/mkfs_ext2.c > > Also -O ^has_journal is an tune2fs option not a mount option. And that > tune2fs feature isn't in busybox, hence the bloat issue. > > "BusyBox v1.24.2 () multi-call binary. > > Usage: tune2fs [-c MAX_MOUNT_COUNT] [-i DAYS] [-C MOUNT_COUNT] [-L > LABEL] BLOCKDEV > > Adjust filesystem options on ext[23] filesystems" > Busybox tune2fs is useless. > > So 90% of the userbase with size constrained flash file-systems will > want to use busybox mkfs.ext2 instead. busybox mkfs.ext2 and mkfs.ext2 > don't conflict because the package installs in /usr/sbin and busybox > installs in /sbin > > I move that the feature be dependent ONLY on busybox mkfs.ext2. > Busybox mount isn't required because procd includes sys/mount.h > In order words set the path to /sbin/mkfs.ext2 (busybox) > > Attached is the new patch. Allow users in the Makefile configuration > to choose to use LZ4 (more speed ~15% less compression) if they so > desire. > John can do the makefile if and when he merges the patch. > > I'm out of the debate now, I got it in my > lede/packgaes/system/procd/patches/ directory :) I'm happy >
--- a/initd/zram.c 2016-07-03 06:39:51.011999930 +0700 +++ b/initd/zram.c 2016-07-03 07:00:34.143492847 +0700 @@ -82,7 +82,7 @@ int mount_zram_on_tmp(void) { - char *mkfs[] = { "/usr/sbin/mkfs.ext4", "-b", "4096", "-F", "-L", "TEMP", "-m", "0", "/dev/zram0", NULL }; + char *mkfs[] = { "/sbin/mkfs.ext2", "-b", "4096", "-F", "-L", "TEMP", "-m", "0", "/dev/zram0", NULL }; FILE *fp; long zramsize; pid_t pid; @@ -94,6 +95,15 @@ } mkdev("*", 0600); +#ifdef ZRAM_TMPFS_LZ4 + fp = fopen("/sys/block/zram0/comp_algorithm", "r+"); + if (fp == NULL) { + ERROR("Can't open /sys/block/zram0/comp_algorithm: %s\n", strerror(errno)); + return errno; + } + fprintf(fp, "%s", "lz4" ); + fclose(fp); +#endif zramsize = proc_meminfo() / 2; fp = fopen("/sys/block/zram0/disksize", "r+"); @@ -107,10 +116,10 @@ pid = fork(); if (!pid) { execvp(mkfs[0], mkfs); - ERROR("Can't exec /sbin/mkfs.ext4\n"); + ERROR("Can't exec /sbin/mkfs.ext2\n"); exit(-1); } else if (pid <= 0) { - ERROR("Can't exec /sbin/mkfs.ext4\n"); + ERROR("Can't exec /sbin/mkfs.ext2\n"); return -1; } else { waitpid(pid, NULL, 0);
To quote Arjen: If the only reason to switch to ext2 is to remove the journal, why not just add -O ^has_journal to the mount options? ---- That's not a mount option. That's a tune2fs option. Journaling isn't the the only problem. The biggest problem is BLOAT. You need libext2+e2fsprogs in a 4MB flash root image with the current zram setup. Would someone want /tmp on zram if they had to add all this to the 4MB squashfs image. -rw-r--r-- 1 lmc users 199739 Jul 3 06:15 e2fsprogs_1.43.1-1_mips_24kc.ipk -rw-r--r-- 1 lmc users 151873 Jul 3 06:15 libext2fs_1.43.1-1_mips_24kc.ipk The simple answer is most openwrt users would say no to this feature do to how it's implemented. I actually was going for ext2 like the original patch, but I was sold that ext4 can mount ext2 formatted filesystems (without a journal) (see man ext4). The ext4 filesystem driver vs ext2 does bloat a bit, but I don't mind it as I can mount ext4 over nbd for my extroot. Busybox is only 40k of source including the headers. mkfs.ext4 is 360k compressed! https://git.busybox.net/busybox/tree/util-linux/mkfs_ext2.c Also -O ^has_journal is an tune2fs option not a mount option. And that tune2fs feature isn't in busybox, hence the bloat issue. "BusyBox v1.24.2 () multi-call binary. Usage: tune2fs [-c MAX_MOUNT_COUNT] [-i DAYS] [-C MOUNT_COUNT] [-L LABEL] BLOCKDEV Adjust filesystem options on ext[23] filesystems" Busybox tune2fs is useless. So 90% of the userbase with size constrained flash file-systems will want to use busybox mkfs.ext2 instead. busybox mkfs.ext2 and mkfs.ext2 don't conflict because the package installs in /usr/sbin and busybox installs in /sbin I move that the feature be dependent ONLY on busybox mkfs.ext2. Busybox mount isn't required because procd includes sys/mount.h In order words set the path to /sbin/mkfs.ext2 (busybox) Attached is the new patch. Allow users in the Makefile configuration to choose to use LZ4 (more speed ~15% less compression) if they so desire. John can do the makefile if and when he merges the patch. I'm out of the debate now, I got it in my lede/packgaes/system/procd/patches/ directory :) I'm happy