GNOME Bugzilla – Bug 787204
Minimum and maximum size of the UDF partition/disk
Last modified: 2018-03-19 17:09:23 UTC
Currently minimum and maximum UDF filesystem size is specified as: fs.MIN = MIN_UDF_BLOCKS * MIN_UDF_BLOCKSIZE; fs.MAX = MAX_UDF_BLOCKS * MAX_UDF_BLOCKSIZE; Where MIN_UDF_BLOCKSIZE and MAX_UDF_BLOCKSIZE are defined as common sector sizes found on hard disks. const Byte_Value MIN_UDF_BLOCKSIZE = 512; const Byte_Value MAX_UDF_BLOCKSIZE = 4096; (512 on classic, and 4096 on native 4K format disks) But that is not fully correct as minimum/maximum UDF filesystem size depends on medium logical (sector) size. For UDF there is just a restriction that it cannot contains more then MAX_UDF_BLOCKS and less then MIN_UDF_BLOCKS. As GParted's constants fs.MIN and fs.MAX are in byte units, but UDF restrictions are in sector size unit, it is currently not possible to specify this restriction correctly. It would be nice if GParted supports also sector size restrictions, as those byte size restrictions are not fully suitable for UDF filesystem.
Created attachment 364862 [details] [review] Add support for specifying filesystem MIN/MAX limits in sector size
I attached patch which implements these min/max limits for UDF filesystems.
Hi Pali, From trying out the patch in comment #1, I can see no difference in behaviour to how GParted normally works with MiB alignment and partitions being multiples of MiB in size. Can you provide an example of situation where this patch makes a difference for UDF file sytems? Further because this patch alters common code used for the resizing dialogs, it is important that you test all other file systems to ensure that no regressions are introduced. As a side note the patch does not cleanly apply due to trailing whitespace. Curtis
Hi! The difference is that with this patch, filesystems can specify MIN/MAX limits in number of blocks and not in MiB units. UDF does not have limits in MiB or bytes, but in blocks. And size of block (=logical sector size) depends on block device and hdd. Some have 512 bytes, some 4096 (e.g. new Advanced 4K native). Therefore as wrote in first comment, current MIN/MAX limits for UDF are wrong. And Gparted does not have support for specifying limits in block unit, instead of bytes. Therefore this patch implements MIN_block/MAX_blocks restriction of filesystem class.
Thanks Pali for the prompt response. Because GParted only supports creating partitions that are a whole multiple of MiB in size, adding the ability to specify sizes smaller than this is non-functional and contributes to code complexity. Again, can you provide a real-life example of where using GParted with this patch would make a difference when working with UDF partitions?
> Again, can you provide a real-life example of where using GParted with this patch would make a difference when working with UDF partitions? Yes, maximal size of UDF partition. When you have disk bigger then maximal size of UDF partition, then GParted should not allow user to choose bigger size of partition as is UDF limit.
Is there a reason why the maximum size cannot be specified without altering the resizing dialogs? Again, can you provide a real-life example complete with steps?
> Is there a reason why the maximum size cannot be specified without altering the resizing dialogs? Because dialag already access MIN/MAX limits which are in bytes and I cannot specify that upper limit in blocks. Or do you see a better way how to do it? > Again, can you provide a real-life example complete with steps? Take 3TB disk which has 512 bytes logical sector size and try to create UDF partition (using GPT scheme) which would be larger then 2TB (e.g. whole 3TB). Currently GParted dialog for new partition would allow such thing, even it is not possible for UDF (512 bytes * (2^32-1) is less then 3TB).
> Or do you see a better way how to do it? Have you looked at the way the FAT16 file system limits works? https://git.gnome.org/browse/gparted/tree/src/fat16.cc?h=GPARTED_0_30_0#n114 Although it may be technically possible to create a file system slightly larger 4095 MiB, because GParted only supports creating partitions which are whole multiples of MiB we choose the largest size in MiB that GParted can successfully create. Using your 3TB disk example, if you try to create a FAT16 partition on the disk, then the FAT16 partition will be limited to 4095 MiB and not take up the whole 3TB disk.
Yes, I looked. But that (4096 - 1) solve totally different problem as described here. Or at least I do not see how such off by one can solve block size limits.
It seems I may be missing something that you are trying to describe because I don't see why we must change the resizing dialogs to implement a file system size limitation. Perhaps if Mike has time then he can take a look at this.
Look, maximal lenght of UDF parition is blocksize*(2^32-1). Take a first HDD which has size 32TB and its logical sector (block) size is 512 bytes. Then on such disk, UDF partition can have maximal size 512*(2^32-1) which is 2199023255040 bytes (~~2TB). Next take a second HDD which has again size 32TB, but is native 4K and its logical sector size is 4096 bytes. Then on such disk you can have UDF partition with maximal size 17592186040320 bytes (~~16TB). In both cases you have 32TB disk, but in first case maximal limit for UDF partition is about 2TB and in second case about 16TB. Do you see where is the problem? Currently GParted specify filesystem MAX limit in bytes (res. MB), not in number of (logical) sectors.
Thank you Pali for this explanation. If I understand correctly UDF does not have a single value for maximum length. Instead the maximum length depends on the logical sector size of the disk. The challenge is that the file system support section in GParted is independent of sector size, and currently only supports a single value for MIN/MAX. My concern with the patch in comment #1 is that it adds additional logic to the copying/resizing dialogs and likely breaks edge cases for a number of file systems. For example some file systems do not support shrinking. If user copies a file system from Cylinder alignment to MiB alignment, and the GUI rounds down the partition size, then that creates a problem because the GUI may attempt to set the partition size smaller than the file system size. The current logic in the resizing dialog is messy, and ideally this section of code should be rethought and likely refactored to remove the logic from the GUI. However, in the mean time this code has been proven over time to work in the real world. It would be best if you could come up with a different approach that does not alter the current copying/resizing logic. Perhaps adding a method to file system support that provides the correct MIN/MAX values for file systems when requested by other methods such as the copy/resize dialogs. Mike may have some better ideas on how to approach this challenge.
(In reply to Curtis Gedak from comment #13) > The current logic in the resizing dialog is messy, and ideally this section > of code should be rethought and likely refactored to remove the logic from > the GUI. However, in the mean time this code has been proven over time to > work in the real world. > > It would be best if you could come up with a different approach that does > not alter the current copying/resizing logic. Perhaps adding a method to > file system support that provides the correct MIN/MAX values for file > systems when requested by other methods such as the copy/resize dialogs. > > Mike may have some better ideas on how to approach this challenge. As a specific suggestion how about replacing direct access to struct FS MIN and MAX with get_min() and get_max() accessors. It goes like this: 1) Add get_min() and get_max() methods in struct FS in Utils.h. (In C++ struct is just a class where everything is public). 2) Add parameter to get_min/max() of type either const Device & or Sector to pass in the sector size. This will additionally involve passing const Device & into Dialog_Partition_Copy and Dialog_Partition_Resize_Move. (It is already passed into Dialog_Partition_New). 3) Add new block size limits into struct FS and adapt get_min() and get_max() accordingly. It adds the minimum amount of abstraction to avoid changing the logic in the dialogs which have to handle size limits.
Mike, would you prepare these changes? Per Curtis comments, I would rather do not touch that dialog code if it is really fragile. But look at "[PATCH 3/3] Fix rounding of floating-point numbers", I think this is a real floating-point bug -- at least I saw it together with UDF code.
Stupid questions; What happens if udf is created with a larger block size than that of the underlying disk or optical media? What happens if GParted automatically sets the block size to 4K? What do Windows and Macs do with regard to using a block size when they create UDF file systems? Perhaps all of these questions are wrong because the spec requires the UDF block size to match that of the media. But then what happens is a dd copy is made of a UDF file system from an optical drive with 2K sector size is put on a 512b sector hard drive?
UDF specification requires that UDF block size and disk logical sector size must match. And Windows and Mac OS X system enfore it. If it does not match then Windows and Mac OS X systems do not recognize UDF disk (they show that disk is unknown and want to re-format it).
Hi Pali, Yes I will code the changes for refactoring MIN and MAX to accommodate sector size based limits as outlined in comment 14 above. Thanks, Mike
Created attachment 367161 [details] [review] Separating FS limits and adding UDF block/sector size limits (v1) Here's the patchset to refactor GParted to allow it to dynamically determine file system size limits. My suggested way of doing it in comment 14 was a load of rubbish. The code still meets the stated goal of not changing any of the resizing dialogs. What the code does is to add FileSystem::get_filesystem_limits() which takes a Partition object and returns the file system limits. The default implementation only provides fixed size limits just like GParted has always done. UDF has a file system specific derived implementation udf::get_filesystem_limits() which computes byte size limits based of the device sector size when creating a new UDF or the existing UDF file system block size ready for when and if resizing UDFs becomes possible. Doesn't try to change any of the other file systems such as EXT2/3/4 or XFS to use the dynamic method to make the limits depend on block sizes. Confirmed that UDF max size is correctly limited to 2TiB on 512 byte sector device and min size is 2MiB on 4K sector device. Both cases the old code with fixes size limits got wrong. From a UDF point of view the most interesting patches are: [P01] Create separate file system limits structure and getter method [P13] Set dynamic UDF file system size limits The rest are a lot of incremental refactoring. Thanks, Mike
Hi Mike & Pali, Patch set v1 in comment #19 looks good to me. I like that we now have the ability to base other file system min/max sizes on device block size. While reviewing the code the only issue I observed was a minor spelling mistake / typo. Following is the change I suggest to the commit message (I can make this change before applying the commits to master). -------------------- [P 11/18] Reorder code in Win_GParted::activate_paste() PROBLEM: Minor typo "resize" s/b "resized" in commit message existing UDF file system being resize and use the device sector size ^^^^^^ -------------------- For testing I mostly looked for regressions in behaviour in addition to testing for the correct message (without [Encrypted]) when trying to reformat a partition with an invalid size file system. If there are no concerns raised, then I will commit patch set v1 in the next few days. Curtis
Looks good. But there is a still problem with rounding with ceil() function as it returns number higher then specified. So this function cannot be used for maximal limits. I fixed it in my patch "[PATCH 3/3] Fix rounding of floating-point numbers".
@Pali, if I recall correctly the reason for the ceiling function is to ensure that sufficient space is kept in the partition for file systems that do not support resizing. This can become relevant when copying and pasting a partition and changing the alignment from cylinder to MiB. If creating a new UDF partition with MiB alignment then in theory the result of the ceil() and floor() functions should be the same. Note that my confidence in the existing code relies on the accuracy of my testing from several years ago (I might have made a mistake) and the fact that the code has been running in the real world without a report of the specific maximum problem you raise. I am reluctant to change this code without extensive testing and proof of the problem. Again if we are going to change this then we should have documented testing of how to set up the tests and the results of the tests. If you wish to undertake such testing then please be my guest. Curtis
> If creating a new UDF partition with MiB alignment then in theory the result of the ceil() and floor() functions should be the same. No it cannot. This is not how current (non-UDF) code is written. Rounding is done on different level. > proof of the problem Take 4TB disk, create a new MBR (msdos) partition layout on it and try to create UDF partition on it with maximal possible size in GUI (with applied all Mike patches). GUI allows to create image of size 2097152 MB, even it is more then maximal allowed size. And this is because of wrong rounding. And if you try to apply changes, it will throw error message.
And same problem happens also with GPT partition layout. And also with FAT32 with GPT, but instead of throwing error message you can find warning from mkfs.fat: "Warning: target too large, space at end will be left unused". So this is a real problem for FAT32, just hidden...
Pali, for testing I need the exact steps used and results. I do not have a 4 TB disk so the steps will need to use virtual disks. I am concerned about regression problems that may be introduced that will cause problems when copying partitions from cylinder alignment to MiB alignment. That testing must also be done.
Create 4TB long file and set it to loopback device. File with holes is working fine too so you do not need to have 4TB free storage on disk. E.g.: $ truncate -s 4TB /tmp/disk $ losetup /dev/loop0 /tmp/disk $ ./src/gpartedbin /dev/loop0 I understand that testing needs to be done. I just want to show that there is a problem in code.
Thank you Pali for providing more detail in the steps. I can confirm the issue you mentioned creating a maximum size UDF file system. In my brief testing of creating maximal UDF file systems I noticed: New Size (MiB): 2097152 # Fails New Size (MiB): 2097151 # Succeeds. Before we make changes to the resizing dialogs much more extensive testing is required. A possible code work-around for now would be to reduce the MAX_UDF_BLOCKS size in src/udf.cc so that the maximum calculated size for 512 byte/sector is 2097151 MiB. Do you think we should hold off on committing patch set v1 (comment #19), or should we handle the minor problem with maximum size in a separate bug report?
It can be also in separate bug as it affects also FAT32 and is not related to UDF, but to GParted core.
Pali, if you could create the separate bug report that would be great. If there are no other issues then I will commit patch set v1 from comment #19 in the next day or so. Curtis
I created https://bugzilla.gnome.org/show_bug.cgi?id=792936
Thank you Pali and Mike for your work on this UDF enhancment. Patch set v1 from comment #19 has been committed to the git repository. The relevant git commits can be viewed at the following links: Create separate file system limits structure and getter method (#787204) https://git.gnome.org/browse/gparted/commit/?id=04535c48b3b41ba8e6b6b79fd51b59523e9753b3 Assign to duplicate FS_Limits (#787204) https://git.gnome.org/browse/gparted/commit/?id=aea0070799e9d52154608d8a0c7008c668a71878 Switch to using struct FS_Limits inside Dialog_Partition_Copy (#787204) https://git.gnome.org/browse/gparted/commit/?id=fc436595fd315b62280224e939745ef3aa749e20 Query and pass struct FS_Limits into Dialog_Partition_Copy (#787204) https://git.gnome.org/browse/gparted/commit/?id=285c24a82a783b6173971c62fa1323d1a469958b Switch to using struct FS_Limits inside Dialog_Partition_Resize_Move (#787204) https://git.gnome.org/browse/gparted/commit/?id=fe7b734792d1a2850e861cd858a7701a9d12bd2a Query and pass struct FS_Limits into Dialog_Partition_Resize_Resize_Move (#787204) https://git.gnome.org/browse/gparted/commit/?id=53b7a7589455f49a9790610ff073ded046da8e96 Switch to using struct FS_Limits inside Dialog_Partition_New (#787204) https://git.gnome.org/browse/gparted/commit/?id=4fa262d7e3c8ffd1cc8508e50b2e026901c04012 Use struct FS_Limits in GParted_Core::create() (#787204) https://git.gnome.org/browse/gparted/commit/?id=8729556778c88bb5c60b82817e916eec23505cb6 Use struct FS_Limits in Win_GParted::activate_format() (#787204) https://git.gnome.org/browse/gparted/commit/?id=d5cd6ca349d87ae4c69e6625bf60fa91525388b0 Remove struct FS members .MIN & .MAX (#787204) https://git.gnome.org/browse/gparted/commit/?id=e234df6b2e924531d6df1bba19db5ee5c718710d Reorder code in Win_GParted::activate_paste() (#787204) https://git.gnome.org/browse/gparted/commit/?id=f8b38b7b310a78393f14f5052a4ee27063ae733a Pass Partition object to get_filesystem_limits() (#787204) https://git.gnome.org/browse/gparted/commit/?id=668957c0a40e74cdcb988d8d837084d459169172 Set dynamic UDF file system size limits (#787204) https://git.gnome.org/browse/gparted/commit/?id=ae2a8723b5b457eb628953dd19e5368ea2f63c2e Extract common code into GParted_Core::get_filesystem_limits() (#787204) https://git.gnome.org/browse/gparted/commit/?id=46bf5a383e56ec3478f45cfb2d644140fad829a1 Fix cannot format error dialog which always reported the file system as encrypted https://git.gnome.org/browse/gparted/commit/?id=32c483c314dd3a7227b237fd0612650bcc2feb8e Rename enum FILESYSTEM to FSType https://git.gnome.org/browse/gparted/commit/?id=175d27c55d44b87ec873a0b09847e8022011d51f Move struct FS and FS_Limits into FileSystem.h https://git.gnome.org/browse/gparted/commit/?id=a3b47ca14a14640ed9fd3aef660a101db761d4dd Add comment about needing to compute encryption overhead in activate_format() https://git.gnome.org/browse/gparted/commit/?id=578ebf133ef073d284bad2f864b38cb6fdebfbc1
This enhancement was included in the GParted 0.31.0 release on March 19, 2018.