rolinh
commented
8 years ago Hi @kilobyte,
Thanks for the patches, I will take a look at them.
Cheers
Open kilobyte opened 8 years ago
rolinh
commented
8 years ago Hi @kilobyte,
Thanks for the patches, I will take a look at them.
Cheers
rolinh
commented
8 years ago I went through your changes. I'm generally very OK with your changes, thanks a lot they will go in! :smiley:
The only thing I am a little concerned about is that we are now looping 4 times over the file systems in the disp() function. There may be room for an improvement here although I think 3 loops are unavoidable (first loop to set ignorable fs, second loop to update the maxwidth structure and the third one to finally display the file systems). I think the two first for loops could be merged into one, right?
rolinh
commented
8 years ago Ok, I tested a bit further and I think it is filtering a bit too much now:
$ dfc -T
dfc -T
FILESYSTEM TYPE (=) USED FREE (-) %USED AVAILABLE TOTAL MOUNTED ON
dev devtmpfs [--------------------] 0.0% 3.8G 3.8G /dev
/dev/sda1 ext4 [==================--] 89.6% 23.5G 226.7G /
tmpfs tmpfs [=-------------------] 4.9% 3.6G 3.8G /dev/shm
tmpfs tmpfs [--------------------] 0.0% 3.8G 3.8G /sys/fs/cgroup
tmpfs tmpfs [=-------------------] 0.0% 6.0G 6.0G /tmp Without your changes, I get this:
$ dfc -T
FILESYSTEM TYPE (=) USED FREE (-) %USED AVAILABLE TOTAL MOUNTED ON
dev devtmpfs [--------------------] 0.0% 3.8G 3.8G /dev
run tmpfs [=-------------------] 0.0% 3.8G 3.8G /run
/dev/sda1 ext4 [==================--] 89.6% 23.5G 226.7G /
tmpfs tmpfs [=-------------------] 4.9% 3.6G 3.8G /dev/shm
tmpfs tmpfs [--------------------] 0.0% 3.8G 3.8G /sys/fs/cgroup
tmpfs tmpfs [=-------------------] 0.0% 6.0G 6.0G /tmp
tmpfs tmpfs [=-------------------] 0.0% 776.5M 776.5M /run/user/1000 And this is actually the same result as df by default:
$ df -hT
Filesystem Type Size Used Avail Use% Mounted on
dev devtmpfs 3.8G 0 3.8G 0% /dev
run tmpfs 3.8G 1016K 3.8G 1% /run
/dev/sda1 ext4 227G 193G 24G 90% /
tmpfs tmpfs 3.8G 190M 3.7G 5% /dev/shm
tmpfs tmpfs 3.8G 0 3.8G 0% /sys/fs/cgroup
tmpfs tmpfs 6.0G 364K 6.0G 1% /tmp
tmpfs tmpfs 777M 8.0K 777M 1% /run/user/1000I think we don't want to filter out more than df by default. I also need to check on DragonFly BSD to see how it behaves with the PFSs.
kilobyte
commented
8 years ago I don't understand the concern about multiple loops. Is there any reason to stuff more logic into a single iteration instead of multiple, more readable loops? If it's about efficiency, then the cost is a few clock cycles in a loop with no more than a hundred or so iterations (on a really busy server with that many mounts), and because of cache locality it might be even faster...
kilobyte
commented
8 years ago Elimination of /run mounts is intentional, as they're not supposed to have any non-trivial files in it, and thus are not interesting to the user. There's often many of them -- 2 on your system but at least 3 or 4 on Debian.
Only reason to show them, IMHO, is when because of some error they're getting full anyway, that's why I special-cased them to show up when more than 50% filled.
kilobyte
commented
8 years ago I think it'd be nice to apply that /run treatment to devtmpfs mounts as well, with the same logic -- they're both filesystems whose capability to store real files is only an implementation details.
Thus, dfc should show them only if there's something unusual, ie, they're getting dangerously full.
rolinh
commented
8 years ago I don't understand the concern about multiple loops. Is there any reason to stuff more logic into a single iteration instead of multiple, more readable loops? If it's about efficiency, then the cost is a few clock cycles in a loop with no more than a hundred or so iterations (on a really busy server with that many mounts), and because of cache locality it might be even faster...
It would be really silly if about efficiency. No really, that's absolutely not the point. It's about clarity. 1st loop: set ignored fs 2nd loop: more ignore checks? 3rd loop: update maxwidth structure 4th loop: display the results
It is more like the first two loops kinda do similar things, right?
Elimination of /run mounts is intentional, as they're not supposed to have any non-trivial files in it, and thus are not interesting to the user. There's often many of them -- 2 on your system but at least 3 or 4 on Debian.
Only reason to show them, IMHO, is when because of some error they're getting full anyway, that's why I special-cased them to show up when more than 50% filled.
On the other hand, this would mean that dfc filters out more fs than standard df. This might be at least surprising for the users. But some fs reappearing if more than 50% full is even weirder or at least unexpected. I get your point about not displaying /run mounts but I think that the implemented behavior is a bit confusing. I have to think about it and maybe ask a few users what they think about this.
In any case, if I am to merge this part, I think that this behavior should at least be explained in the manual.
kilobyte
commented
8 years ago Another case where dfc is more spammy than df. First, the base state on one machine with this pull request applied:
[~]$ df
Filesystem 1K-blocks Used Available Use% Mounted on
udev 4034360 0 4034360 0% /dev
tmpfs 815828 816 815012 1% /run
/dev/sda1 225605632 165793404 59319108 74% /
tmpfs 5120 4 5116 1% /run/lock
tmpfs 3396200 0 3396200 0% /run/shm
/dev/sda1 225605632 165793404 59319108 74% /var/cache
/dev/sda1 225605632 165793404 59319108 74% /home
/dev/sda1 225605632 165793404 59319108 74% /mnt/btr1
/dev/sdb1 866042880 509706872 355226888 59% /home/kilobyte/mp3
/dev/sda1 225605632 165793404 59319108 74% /home/kilobyte/.cache
/dev/sdb1 866042880 509706872 355226888 59% /home/kilobyte/x
/dev/sda1 225605632 165793404 59319108 74% /home/kilobyte/tmp
tmpfs 4194304 72 4194232 1% /tmp
/dev/sdb1 866042880 509706872 355226888 59% /mnt/btr2
[~]$ dfc
FILESYSTEM (=) USED FREE (-) %USED AVAILABLE TOTAL MOUNTED ON
udev [--------------------] 0.0% 3.8G 3.8G /dev
/dev/sda1 [===============-----] 73.7% 56.6G 215.2G /
tmpfs [=-------------------] 0.0% 4.0G 4.0G /tmp
/dev/sdb1 [============--------] 59.0% 338.8G 825.9G /mnt/btr2 udev shouldn't be shown, but so far, good. But let's run:
[/srv/chroots]# for y in *;do (cd $y;for x in home proc sys dev tmp;do mount --rbind /$x $x;done);done
[~]$ df ✔
Filesystem 1K-blocks Used Available Use% Mounted on
udev 4034360 0 4034360 0% /dev
tmpfs 815828 816 815012 1% /run
/dev/sda1 225605632 165793404 59319108 74% /
tmpfs 5120 4 5116 1% /run/lock
tmpfs 3396200 0 3396200 0% /run/shm
/dev/sda1 225605632 165793404 59319108 74% /var/cache
/dev/sda1 225605632 165793404 59319108 74% /home
/dev/sda1 225605632 165793404 59319108 74% /mnt/btr1
/dev/sdb1 866042880 509710760 355223032 59% /home/kilobyte/mp3
/dev/sda1 225605632 165793404 59319108 74% /home/kilobyte/.cache
/dev/sdb1 866042880 509710760 355223032 59% /home/kilobyte/x
/dev/sda1 225605632 165793404 59319108 74% /home/kilobyte/tmp
tmpfs 4194304 72 4194232 1% /tmp
/dev/sdb1 866042880 509710760 355223032 59% /mnt/btr2
[~]$ dfc ✔
FILESYSTEM (=) USED FREE (-) %USED AVAILABLE TOTAL MOUNTED ON
udev [--------------------] 0.0% 3.8G 3.8G /dev
/dev/sda1 [===============-----] 73.7% 56.6G 215.2G /
tmpfs [=-------------------] 0.0% 4.0G 4.0G /tmp
/dev/sdb1 [============--------] 59.0% 338.8G 825.9G /mnt/btr2
udev [--------------------] 0.0% 3.8G 3.8G /srv/chroots/arm64/dev
tmpfs [=-------------------] 0.0% 4.0G 4.0G /srv/chroots/arm64/tmp
udev [--------------------] 0.0% 3.8G 3.8G /srv/chroots/armhf/dev
tmpfs [=-------------------] 0.0% 4.0G 4.0G /srv/chroots/armhf/tmp
udev [--------------------] 0.0% 3.8G 3.8G /srv/chroots/i386/dev
tmpfs [=-------------------] 0.0% 4.0G 4.0G /srv/chroots/i386/tmp
udev [--------------------] 0.0% 3.8G 3.8G /srv/chroots/jessie/dev
tmpfs [=-------------------] 0.0% 4.0G 4.0G /srv/chroots/jessie/tmp
udev [--------------------] 0.0% 3.8G 3.8G +/chroots/jessie-x32/dev
tmpfs [=-------------------] 0.0% 4.0G 4.0G +/chroots/jessie-x32/tmp
udev [--------------------] 0.0% 3.8G 3.8G /srv/chroots/mips/dev
tmpfs [=-------------------] 0.0% 4.0G 4.0G /srv/chroots/mips/tmp
udev [--------------------] 0.0% 3.8G 3.8G +rv/chroots/mips64el/dev
tmpfs [=-------------------] 0.0% 4.0G 4.0G +rv/chroots/mips64el/tmp
udev [--------------------] 0.0% 3.8G 3.8G /srv/chroots/powerpc/dev
tmpfs [=-------------------] 0.0% 4.0G 4.0G /srv/chroots/powerpc/tmp
udev [--------------------] 0.0% 3.8G 3.8G +/chroots/powerpcspe/dev
tmpfs [=-------------------] 0.0% 4.0G 4.0G +/chroots/powerpcspe/tmp
udev [--------------------] 0.0% 3.8G 3.8G /srv/chroots/ppc64el/dev
tmpfs [=-------------------] 0.0% 4.0G 4.0G /srv/chroots/ppc64el/tmp
udev [--------------------] 0.0% 3.8G 3.8G /srv/chroots/sh4/dev
tmpfs [=-------------------] 0.0% 4.0G 4.0G /srv/chroots/sh4/tmp
udev [--------------------] 0.0% 3.8G 3.8G /srv/chroots/squeeze/dev
tmpfs [=-------------------] 0.0% 4.0G 4.0G /srv/chroots/squeeze/tmp
udev [--------------------] 0.0% 3.8G 3.8G +rv/chroots/unstable/dev
tmpfs [=-------------------] 0.0% 4.0G 4.0G +rv/chroots/unstable/tmp
udev [--------------------] 0.0% 3.8G 3.8G /srv/chroots/wheezy/dev
tmpfs [=-------------------] 0.0% 4.0G 4.0G /srv/chroots/wheezy/tmp
udev [--------------------] 0.0% 3.8G 3.8G /srv/chroots/x32/dev
tmpfs [=-------------------] 0.0% 4.0G 4.0G /srv/chroots/x32/tmp -- df doesn't show those, dfc does. Without my patches, it'd show copies of all the real filesystems, so at least that's gone -- but we still have all those duplicated udev and tmpfs bind-mounts that df filtered away.
rolinh
commented
8 years ago OK, I think it makes sense not to display certain file systems as you suggest. However, I feel like the behavior of automagically displaying a file system if its usage goes above 50% is counter intuitive. I would rather it being always or never displayed. Maybe an option could be added to hide file systems with usage below a certain threshold but I'm not even fond of this idea either. I'll cherry-pick some of your changes for now but I will not merge the "auto-hide" stuff.
Here's a grab-bag of misc fixes and additions. I placed them in a single pull request as I prefer to cherry-pick over using GitHub's GUI, but can separate them if you wish. Apologies if I didn't notice something being already available another way -- I've learned about dfc just a few hours ago...
The commits are mostly independent, although some depend on fcd192014c8bbc0466bcdd4195b3d14c0ce006f9.
The changes are: