Page MenuHomeFreeBSD

Increase UFS/FFS maximum link count from 32767 to 65530
ClosedPublic

Authored by mckusick on Nov 25 2023, 4:01 AM.
Tags
None
Referenced Files
Unknown Object (File)
Nov 28 2024, 5:46 AM
Unknown Object (File)
Nov 28 2024, 5:45 AM
Unknown Object (File)
Nov 28 2024, 5:45 AM
Unknown Object (File)
Nov 28 2024, 5:44 AM
Unknown Object (File)
Nov 28 2024, 5:25 AM
Unknown Object (File)
Nov 26 2024, 5:27 PM
Unknown Object (File)
Nov 14 2024, 3:44 AM
Unknown Object (File)
Nov 14 2024, 3:42 AM
Subscribers

Details

Summary

The link count for a UFS/FFS inode is stored in a signed 16-bit integer. Thus the maximum link count has been 32767.

This limit has been recently hit by the poudriere build system when doing a ports build as it needs one directory per port and the number of ports recently passed 32767.

A long-term solution would be to use one of the spare 32-bit fields in the inode to store the link count. However, the UFS1 format does not have a spare and adding the spare in UFS2 would make it hard to make it compatible when running on older kernels that use the original link count field. So this patch uses the much simpler approach of changing the existing link count field from a signed 16-bit value to an unsigned 16-bit value. It has the fewest lines of code changes. The only thing that changes is the type in the dinode and inode structures and the definition of UFS_LINK_MAX. It has the added benefit that it works with both UFS1 and UFS2.

It allows easy backward compatibility. Indeed it is backward compatibility that is the primary reason to go with this approach. If a filesystem with the new organization is mounted on an older kernel, it still needs to work. Thus if we move the new link count to a new field, we still need to maintain the old link count as best as possible even when running on a kernel that knows about the larger link counts. And we would have to carry this overhead for the indefinite future.

If we have a new link-count field, we will have to add a new filesystem flag to indicate that we are running with larger link counts. We will also need to add of one of the new-feature flags to say that we have larger link counts. Older kernels clear the new-feature flags that they do not know about, so when a filesystem is used on an older kernel and then moved back to a newer one, the newer one will know that the new link counts have not been maintained and that it will be necessary to run a full fsck on the filesystem to correct the link counts before it can be mounted.

With this change, older kernels will generally work with the bigger counts. While it will not itself allow the link count to exceed 32767, it will have no problem working with inodes that have a link count greater than 32767. Since it tests that i_nlink <= UFS_LINK_MAX, counts that are bigger than 32767 will appear negative, so will still pass the test. Of course, if they ever drop below 32767, they will no longer be able to exceed 32767. The one issue is if the link count ever exceeds 65535 then it will wrap to zero and the older kernel will be none the wiser. But this corner case is likely to be very rare since these kernels and the applications running on them do not expect to be able to get link counts over 32767. And over time, the use of new filesystems on older kernels will become rarer and rarer.

Test Plan

Testing by Mark Millard on poudriere. Testing by Peter Holm with his UFS/FFS and filesystems test suites,

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

ffs_softdep.c has several lines of kind

sys/ufs/ffs/ffs_softdep.c:    KASSERT(ip->i_nlink >= 0, ("handle_workitem_remove: file ino "

which no longer make sense.

There are less critical comparisons like ip->i_nlink <= 0 at least in ufs_inode.c and ufs_vnops.c which could be cleaned up.

In D42767#975487, @kib wrote:

ffs_softdep.c has several lines of kind

sys/ufs/ffs/ffs_softdep.c:    KASSERT(ip->i_nlink >= 0, ("handle_workitem_remove: file ino "

which no longer make sense.

There are less critical comparisons like ip->i_nlink <= 0 at least in ufs_inode.c and ufs_vnops.c which could be cleaned up.

Actually i_nlink is a signed 32-bit value where the nlink count is maintained while the inode is in memory. So it can in fact be checked for negative values which of course should never be copied to the 16-bit unsigned value that is maintained on the disk.

In D42767#975487, @kib wrote:

ffs_softdep.c has several lines of kind

sys/ufs/ffs/ffs_softdep.c:    KASSERT(ip->i_nlink >= 0, ("handle_workitem_remove: file ino "

which no longer make sense.

There are less critical comparisons like ip->i_nlink <= 0 at least in ufs_inode.c and ufs_vnops.c which could be cleaned up.

Actually i_nlink is a signed 32-bit value where the nlink count is maintained while the inode is in memory. So it can in fact be checked for negative values which of course should never be copied to the 16-bit unsigned value that is maintained on the disk.

Commenting on my own comment. The fix is that i_nlink should be a signed 32-bit value just as i_effnlink is so that the above tests can be made.

Change in-memory i_nlink to a signed 32-bit value so that can check that it has not gone negative before assigning it to the on-disk unsigned 16-bit di_nlink field.

This revision is now accepted and ready to land.Nov 28 2023, 8:10 PM
olce added a subscriber: olce.

Hi, generally OK with the approach.

I can understand why you chose to make i_nlink a 32-bit signed int (less code churn) but I'd prefer to make it a 16-bit unsigned int and move/adapt the KASSERT() checking for underflow. Would you be OK to review a subsequent patch of mine doing that (in another differential revision)?

sys/ufs/ufs/inode.h
245

Given that DIP_SET_NLINK() is used only with the on-disk inode di_nlink field, I would suggest to hardcode it and remove the field argument, even if this makes it less similar to DIP_SET(), since the KASSERT() message it contains already depends on that.

I would be happy to review a subsequent patch to make i_nlink a 16-bit unsigned int and move/adapt the KASSERT() checking for underflow.

sys/ufs/ufs/inode.h
245

I made your suggested change to drop the field argument.