Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tune + fix /usr/sbin/mount-sd.sh #19

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

V10lator
Copy link

See the commits for details.

V10lator added 5 commits June 23, 2015 12:20
We trigger indexing anyway on line 63, so no need to delay the mounting and annoy users.
This tells the tracker to re-index, so it knows about the removed files. Also it removes the temporary mount directory as it's no longer needed (in fact line 63 checks that the directory hasn't been deleted, so this is just right).
This is needed for the next commit to be golden.
No need to trigger indexing on line 36: At that time there won't be any new files to track.
@foolab
Copy link
Contributor

foolab commented Jun 24, 2015

Please submit one PR per topic.
With all the bundled commits it's hard to accept some and discuss/reject some.

@iamer
Copy link
Contributor

iamer commented Jun 24, 2015

The reason it is done like this is not annoy users. Tracker has removable volume support watching based on Glib2 and /proc/mounts and /dev/mmcblk1 is added to the watched volumes (https://github.com/mer-packages/glib2/blob/master/rpm/0001-Add-dev-mmcblk-to-the-list-of-devices-to-be-detected.patch).

When a removable volume is removed (but the mount point has to stay around) the filesystem tree is marked as not available and when it is reinserted the tree is marked as available and quickly scanned for changes. This is much faster than a full reindex.

As for the TRIM option, it would be nice to handle it in a separate pull request with links to documentation on how it helps (benchmarks etc.. if possible)

@V10lator
Copy link
Author

@foolab The reason I did it bundled was that for example V10lator@e361899 depends on V10lator@2811de0 - so individual requests wouldn't work here.

@iamer I know it's not done to annoy users, that was a joke. :) I see the theory behind fast reindexing but with the current implementation it's broken none the less: https://github.com/nemomobile/sd-utils/blob/master/scripts/mount-sd.sh#L49 triggers a re-index on the empty directory no matter if it was mounted before or not, so https://github.com/nemomobile/sd-utils/blob/master/scripts/mount-sd.sh#L63 will do a full reindex every time. Please correct me if my understanding is wrong.

If needed I'll split that. Can't promise that the pull requests will stay auto-mergable that way through. As for benchmarks and other documentation: I don't have any. The reason behind enabling it is that it's already enabled for vfat and exfat. So I thought enabling it for more FSes is the next thing and benchmarks etc. have already been done on your end.

@iamer
Copy link
Contributor

iamer commented Jun 26, 2015

Lines https://github.com/nemomobile/sd-utils/blob/master/scripts/mount-sd.sh#L28 to 32 will bail out if the device is already mounted. So the rest of the add block will only be reached if the volume was actually really just mounted :)

On my Fedora man mount says for ext4:
discard/nodiscard Controls whether ext4 should issue discard/TRIM commands to the underlying block device when blocks are freed. This is useful for SSD devices and sparse/thinly-provisioned LUNs, but it is off by default until sufficient testing has been done.

for btrfs:
discard Disable/enable the discard mount option. The discard function issues frequent commands to let the block device reclaim space freed by the filesystem. This is useful for SSD devices, thinly provisioned LUNs and virtual machine images, but may have a significant performance impact. (The fstrim command is also available to initiate batch trims from userspace.)

I am not so sure about turning it on right now.

@V10lator
Copy link
Author

We talked about re-mounting (pulling card out, then back in) so it must be unmounted at line 28? Or is there some magic I don't understand that doesn't unmount a card when you pull it out of the device (but when does it get unmounted then?) ?

discard works the same on all filesystems: When a file gets deleted (or any other action frees space) TRIM commands are send for the freed blocks, so the underlying FTL (flash translation layer) also knows these blocks are free and is able to optimize wear-leveling. Now again: It's already enabled for vfat and exfat. If the used FTL has performance issues with TRIM this should never have happened in the first place, so why exactly is vftat/exfat allowed to TRIM but other FSes are not?

//EDIT: But I agree with the mount documentation: A COW FS like btrfs will trigger TRIM issues at the FTL way more often than "traditional" FSes like vfat/ext4.

@M4rtinK
Copy link

M4rtinK commented Jun 28, 2015

As far as I know mounting a filesystem with the trim mount option (so that it issues trim commands in real time) is usually discouraged as it can incur significant performance penalties. It is usually advised to rather add a cron job that periodically runs fstrim on the ssd & thin-LV mount points. This results in the trims being batched, which should improve performance (the trim operation should be shorter). For more information, see:
http://blog.neutrino.es/2013/howto-properly-activate-trim-for-your-ssd-on-linux-fstrim-lvm-and-dmcrypt/

Also - do SD cards actually support trim ? Results from a quick Google search seem to indicate that they don't.

While officially not supported there's community support: https://openrepos.net/content/v10lator/f2fs
Also exfat and ntfs aren't officially supported but handled here, so I can't see anything wrong with adding f2fs.
@V10lator
Copy link
Author

V10lator commented Jul 4, 2015

@M4rtinK

# cat /sys/block/mmcblk1/queue/discard_max_bytes
2199023255040
# cat /sys/block/mmcblk1/queue/discard_zeroes_data
1

suggests that TRIM is supported. I wanted to confirm (see http://lightrush.ndoytchev.com/random-1/checkiftrimonext4isenabledandworking ) but hdparm --read-sector fails:

# /home/nemo/horst-NFS/hdparm-9.48/hdparm --read-sector 32768 /dev/mmcblk1

/dev/mmcblk1:
reading sector 32768: The running kernel lacks CONFIG_IDE_TASK_IOCTL support for this device.
FAILED: Invalid argument

So the only one who's able to confirm is Jolla Oy I guess?

For performance: Again: This hardly depends on the FTL. On my SSDs I'm using queued TRIM without any performance impacts, for example. On the Jolla Smartphone trimming 36.8 GB (done with "fstrim -v /media/sdcard/d742de61-e9be-4618-ad04-40e7e66d7b40/") took around a second, so I don't think you'll notice any impact.

//EDIT: I googled and readed docs and... It seems you're right: SD cards do not support TRIM. None the less you should use it on them cause the Linux kernel is smart enough to translate the TRIM (SCSI command) commands to ERASE (SDHCI command) commands for SD cards. ;)

//EDIT²: See also https://en.wikipedia.org/wiki/Trim_%28computing%29#SD.2FMMC

@M4rtinK
Copy link

M4rtinK commented Jul 4, 2015

@V10lator Just an (off-topic to this pull request) note about queued trim - some drives claim to support it but will actually trash your data if you use it, for example:
http://forums.crucial.com/t5/Crucial-SSDs/M500-M5x0-QUEUED-TRIM-data-corruption-alert-mostly-for-Linux/td-p/151028

@V10lator
Copy link
Author

V10lator commented Jul 5, 2015

@M4rtinK Thanks for the heads up but I know about that. My drive is safe (thanks to a firmware update, that's me: https://bugzilla.kernel.org/show_bug.cgi?id=71371#c34 ). :)
Also the drives you linked have queued TRIM blacklisted, so as long as your kernel isn't years old all should be fine.

To detect and fix corruption.
@V10lator
Copy link
Author

Almost 100 downloads at https://openrepos.net/content/v10lator/bleeding-edge-sd-utils with zero bug reports so far.

# ext and btrfs are both able to handly TRIM. Add more to the list if needed.
ext4|btrfs|f2fs)
MOUNT_OPTS+=",discard"
;;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Identation is a bit off here.

@saukko
Copy link

saukko commented Aug 28, 2015

Other than the indentation to me this looks ok.

@saukko
Copy link

saukko commented Aug 28, 2015

And personally I would leave the trim still out as there is change that it can cause issues eventhough it shouldn't but there is change and dataloss is always bad even if the likelyhood is very low.

@V10lator
Copy link
Author

@saukko I think then it should be disabled for fat, too, as it makes no sense to enable discard only for a single FS. Also I'm using this with f2fs and it works like a charm even with heavy sd usage (I have a lot of symlinks / bind mounts to reduce internal flash usage as much as possible).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants