kiwix-android icon indicating copy to clipboard operation
kiwix-android copied to clipboard

This APP Performs 4GB Large File Write Test which Seriously Harms the Phone/External SDCard's Flash Memory Lifespan

Open keepChatAlive opened this issue 2 years ago • 8 comments

Describe the bug Flash Memory's lifespan is limited, APP should not try to write a large amount of data at random interval just to test the ability to write large files.

Expected behavior Large file writing test should not be performed, or only to be performed manually when there is a problem

Steps to reproduce the behavior: 1.I saw that big file checker file in my SDCard 2.See the following code https://github.com/kiwix/kiwix-android/blob/2dccc865d835ac76a642cc870ce8ecf9af7ea4b4/app/src/main/java/org/kiwix/kiwixmobile/zim_manager/FileWritingFileSystemChecker.kt

Environment

  • Version of Kiwix Android : 3.5.0
  • Device : any
  • OS version : any

Logs none

keepChatAlive avatar Jul 03 '22 02:07 keepChatAlive

@keepChatAlive I completely agree. A couple of years ago, I suggested a simple solution to this problem: write a small text file in the Kiwix directory on the microSD card to indicate that the test had been done, so that it does not need to be repeated each time (https://github.com/kiwix/kiwix-android/issues/1664#issuecomment-573420698). It would seem like the obvious solution to me, and is really simple. Much better than potentially damaging the card by attempting to write a huge file to it every time the app is opened, which is really a kludge.

Jaifroid avatar Jul 03 '22 05:07 Jaifroid

As far as I know there is simply no known better method to know:

  • If the fs supports big files
  • If the fs has changed recently (new SD card loaded) @MohitMaliFtechiz You confirm?

To me the only improvement possible is implementing https://github.com/kiwix/libkiwix/issues/267 which would save time and SD card.

kelson42 avatar Jul 03 '22 08:07 kelson42

As far as I know there is simply no known better method to know:

  • If the fs supports big files
  • If the fs has changed recently (new SD card loaded) @MohitMaliFtechiz You confirm?

To me the only improvement possible is implementing kiwix/libkiwix#267 which would save time and SD card.

You can read SDCard ID (in the format of AAAA-AAAA, A stands for alphanumeric characters) from its mounting path /storage/AAAA-AAAA to know if SDCard is changed. Most modern SDCards use exfat by default and it supports large files. So maybe you don't need to detect them right away. You can add an option to let the user manually perform the check if he or she encountered problems.

keepChatAlive avatar Jul 03 '22 09:07 keepChatAlive

As far as I know there is simply no known better method to know:

  • If the fs supports big files
  • If the fs has changed recently (new SD card loaded) @MohitMaliFtechiz You confirm?

To me the only improvement possible is implementing kiwix/libkiwix#267 which would save time and SD card.

Also the risk far outweigh the benefit to do such detections. Better let error happen than let SDCard dies after a few month. Or you can make a banner to let flash-lifespan-aware users to use the .crx zim reader. Please give back the right of informed consent to users.

keepChatAlive avatar Jul 03 '22 09:07 keepChatAlive

Why not simply inform the user if they are downloading an archive larger than ~4GB that they need to check that their storage is able to accept files larger than 4GB?

In any case, personally I think one 4GB test is acceptable for the given card, so long as it is not repeated again for that card. Ideally, user could allow or disallow the test as @keepChatAlive suggests - simply ask before performing the test.

Jaifroid avatar Jul 04 '22 11:07 Jaifroid

hi @kelson42 , @keepChatAlive and @Jaifroid ,

I have checked the condition for this , we are saving information on CAN_WRITE_4GB and CANNOT_WRITE_4GB on .file_writing_result file currently. So basically after first run it will check whether we can write or not and save this information on .file_writing_result file. on Second run they pull information from this file and ignore the case in which we are saving large_file_test.txt of 4 GB.

please check the following code its doing its purpose to ignore writing 4 gb file again.

 override fun checkFilesystemSupports4GbFiles(path: String): FileSystemCapability {
    val resultFile = File("$path/.file_writing_result")
    if (resultFile.exists()) {
      when (val capability = readCapability(resultFile)) {
        CAN_WRITE_4GB,
        CANNOT_WRITE_4GB -> return capability
      }
    }

Let me know if you have any other thought on this ?

MohitMaliFtechiz avatar Jul 05 '22 11:07 MohitMaliFtechiz

I think @keepChatAlive would be better placed to verify that this is working as intended.

Jaifroid avatar Jul 05 '22 11:07 Jaifroid

hi @Jaifroid ,

any update on this ?

MohitMaliFtechiz avatar Aug 25 '22 13:08 MohitMaliFtechiz

@MohitMaliFtechiz Thank you for your feedback. We have been lazy on this but I would like to move on this. What about core/src/main/java/org/kiwix/kiwixmobile/core/zim_manager/MountPointProducer.kt which just read the fstab, its not used anymore?

kelson42 avatar Aug 18 '23 10:08 kelson42

@kelson42 We are using MountPointProducer.kt for checking whether the filesystem is support files over 4GB or not in MountFileSystemChecker.kt as well as we are using this in ErrorActivity to attach extra logs for system information. https://github.com/kiwix/kiwix-android/blob/3a788b939e616f31f486a100f33082f4b4da1dd6/core/src/main/java/org/kiwix/kiwixmobile/core/error/ErrorActivity.kt#L176

MohitMaliFtechiz avatar Aug 21 '23 08:08 MohitMaliFtechiz

@MohitMaliFtechiz So why do we write a 4gb file and do 't trust the mount point cobfiguration? please point to the piece of code with the logic.

Please make a patch anyway to rename the file ".kiwix_4gb_writing_test_result"

kelson42 avatar Aug 27 '23 05:08 kelson42

@MohitMaliFtechiz So why do we write a 4gb file and do 't trust the mount point cobfiguration? please point to the piece of code with the logic.

@kelson42 This logic was implemented in https://github.com/kiwix/kiwix-android/issues/1663 to efficiently check the filesystem, We are using both classes to check the filesystem.

https://github.com/kiwix/kiwix-android/blob/a8fdbc81147a041af5601db4dabf9299b5ca69c7/app/src/main/java/org/kiwix/kiwixmobile/di/modules/KiwixModule.kt#L46-L55

Here is the main logic where we are writing the 4Gb file in an SD card, If MountFileSystemChecker returns INCONCLUSIVE then we are writing the file in the file system otherwise according to this logic it returns the filesystem state whether it can write 4Gb file or not.

https://github.com/kiwix/kiwix-android/blob/a8fdbc81147a041af5601db4dabf9299b5ca69c7/app/src/main/java/org/kiwix/kiwixmobile/zimManager/Fat32Checker.kt#L91-L102

However, I have tested this method and it has some problems meaning it does not have all the possible supported filesystem lists, e.g. I have an SD card that has tmpfs filesystem and it can write files over 4GB. So, at that time all the possible filesystems not identified that can not write files over 4Gb, so if the filesystem is not recognized by the MountFileSystemChecker then it tries to write a file over 4GB to test if the filesystem can write files over 4GB or not.

Apart from this, we know only fat32 and vfat filesystems can not write files over 4GB but other filesystems can write files over 4GB so now we can improve this logic and avoid writing a 4GB file which will speed up to recognizing the filesystem limitation.

https://github.com/kiwix/kiwix-android/blob/a8fdbc81147a041af5601db4dabf9299b5ca69c7/core/src/main/java/org/kiwix/kiwixmobile/core/zim_manager/MountPointProducer.kt#L46-L50

MohitMaliFtechiz avatar Aug 28 '23 11:08 MohitMaliFtechiz

@MohitMaliFtechiz OK, so there is no obvious bug to me. Please implement my comment and anything else you think is important to improve the detection, so we can finally close this ticket?

kelson42 avatar Aug 28 '23 11:08 kelson42