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

udiskslinuxfilesystem: Use ID_FS_SIZE for filesystems that are not known to report ID_FS_LASTBLOCK #1162

Merged
merged 5 commits into from
Sep 4, 2023

Conversation

tbzatek
Copy link
Member

@tbzatek tbzatek commented Aug 4, 2023

Looking at the util-linux-2.39.1 sources only xfs, ext and btrfs probes do have support for reporting filesystem last block. There are many others however that do report filesystem size that would still be beneficial for UDisks to use.

However, for filesystems that we know the two values differ and that are proven to work fine with bd_fs_get_size(), we should still avoid using ID_FS_SIZE. Let's maintain a short internal whitelist.


See the previous discussion in #1114 and the feedback in #1139.

Taking vfat and ntfs as an example, the filesystem size computation looks roughly similar. For the moment it looks like a best effort. The whitelist introduced serves also a blacklist for ID_FS_SIZE for some filesystems.

Fixes #1139
Cc: @mvollmer

Copy link
Member

@vojtechtrefny vojtechtrefny left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@mvollmer
Copy link
Contributor

I don't see how this fixes #1139, tbh, and I can't say whether using ID_FS_SIZE is ever correct for the Filesystem property. Are these udev properties documented somewhere?

@tbzatek
Copy link
Member Author

tbzatek commented Aug 11, 2023

I don't see how this fixes #1139, tbh, and I can't say whether using ID_FS_SIZE is ever correct for the Filesystem property.

So the point is, for all supported filesystems, is bd_fs_get_size() even correct / suitable for this UDisks property? Apart from the whitelist of course. As noted above, the computation for vfat and ntfs looks roughly similar. For these two this however doesn't guarantee the real filesystem border is reported. So even libblockdev mixes the two concepts together.

Some references to the libblkid implementation:

Manpage only mentions this:

- FSSIZE - size of filesystem. Note that for XFS this will return the same value
  as lsblk (without XFS's metadata), but for ext4 it will return the size with
  metadata and for BTRFS will not count overhead of RAID configuration
  (redundant data).

- FSLASTBLOCK - last fsblock/total number of fsblocks

- FSBLOCKSIZE - file system block size

This is what we want to mimic in the first place, right?

So let's ask the experts @alberand and @karelzak the ultimate question:

  • for filesystems that don't implement FSLASTBLOCK natively, shall we ignore FSSIZE as there are no guarantees the value represents the very last block on the device? Concerning mostly simple filesystems like vfat, exfat, ntfs and others without journal or extra metadata.

@tbzatek
Copy link
Member Author

tbzatek commented Aug 11, 2023

@mvollmer Out of curiosity, how does Cockpit decide on minimal shrink size now that UDisks doesn't provide such info for ntfs? cockpit-project/bots#5082 (comment)
Related to both storaged-project/libblockdev#952 and lvmteam/lvm2#123

@alberand
Copy link

alberand commented Aug 18, 2023

So let's ask the experts @alberand and @karelzak the ultimate question:

  • for filesystems that don't implement FSLASTBLOCK natively, shall we ignore FSSIZE as there are no guarantees the value represents the very last block on the device? Concerning mostly simple filesystems like vfat, exfat, ntfs and others without journal or extra metadata.

yup, FSLASTBLOCK is the very last block and FSSIZE is more like "total data blocks", but not always. For example, for FSSIZE btrfs doesn't take into account blocks used for redundant data and ext4 also counts fs metadata blocks. See comments here https://github.com/util-linux/util-linux/pull/1702/files. So, for ext4 FSLASTBLOCK would probably always correspond to FSSIZE. Looking at the code of fs's, you mention, this would be probably also true.

…rieval

Just another quirk discovered by Cockpit tests. Looks like
the superblock gets written on-disk at some later point while
the mounted filesystem structures keep the new geometry in-memory.
This is an obvious limitation of blkid that only reads data
from the block device.
We've done many tweaks and introduced limits to some
filesystem types, let's document that.
Protective part table involved, duplicate identifiers leading
to random match each time. Until we have a proper fix, just
mark them as unstable.
@tbzatek
Copy link
Member Author

tbzatek commented Aug 22, 2023

Thanks @alberand. This is not really reassuring so I'll ditch any use of FSSIZE now. Once libblkid gets proper support for FSLASTBLOCK for other filesystems (ntfs, vfat, etc.), UDisks will pick it up.

I've pushed just another iteration of the computation rules in the meantime, rather restricted.

@tbzatek tbzatek linked an issue Aug 22, 2023 that may be closed by this pull request
@mvollmer
Copy link
Contributor

Out of curiosity, how does Cockpit decide on minimal shrink size now that UDisks doesn't provide such info for ntfs?

It doesn't. Cockpit only uses the Filesystem.Size property to detect filesystems that are too small for their block devices and to allow the user to fix the situation. When resizing a block device directly in Cockpit, the Filesystem.Size property is not used.

Copy link
Member

@vojtechtrefny vojtechtrefny left a comment

Choose a reason for hiding this comment

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

Still looks good to me.

@tbzatek tbzatek merged commit 6527ea3 into storaged-project:master Sep 4, 2023
11 checks passed
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.

XFS size is not updated after expanding Issue with kde plasma and udisks 2.10
4 participants