RE: [PATCH] hfs: add logic of correcting a next unused CNID

[Viacheslav Dubeyko <Slava.Dubeyko@ibm.com>]

Hi Yangtao,

On Thu, 2025-06-26 at 15:42 +0800, Yangtao Li wrote:
> Hi Slava,
> 
> 在 2025/6/11 07:16, Viacheslav Dubeyko 写道:
> > The generic/736 xfstest fails for HFS case:
> > 
> > BEGIN TEST default (1 test): hfs Mon May 5 03:18:32 UTC 2025
> > DEVICE: /dev/vdb
> > HFS_MKFS_OPTIONS:
> > MOUNT_OPTIONS: MOUNT_OPTIONS
> > FSTYP -- hfs
> > PLATFORM -- Linux/x86_64 kvm-xfstests 6.15.0-rc4-xfstests-g00b827f0cffa #1 SMP PREEMPT_DYNAMIC Fri May 25
> > MKFS_OPTIONS -- /dev/vdc
> > MOUNT_OPTIONS -- /dev/vdc /vdc
> > 
> > generic/736 [03:18:33][ 3.510255] run fstests generic/736 at 2025-05-05 03:18:33
> > _check_generic_filesystem: filesystem on /dev/vdb is inconsistent
> > (see /results/hfs/results-default/generic/736.full for details)
> > Ran: generic/736
> > Failures: generic/736
> > Failed 1 of 1 tests
> > 
> > The HFS volume becomes corrupted after the test run:
> > 
> > sudo fsck.hfs -d /dev/loop50
> > ** /dev/loop50
> > Using cacheBlockSize=32K cacheTotalBlock=1024 cacheSize=32768K.
> > Executing fsck_hfs (version 540.1-Linux).
> > ** Checking HFS volume.
> > The volume name is untitled
> > ** Checking extents overflow file.
> > ** Checking catalog file.
> > ** Checking catalog hierarchy.
> > ** Checking volume bitmap.
> > ** Checking volume information.
> > invalid MDB drNxtCNID
> > Master Directory Block needs minor repair
> > (1, 0)
> > Verify Status: VIStat = 0x8000, ABTStat = 0x0000 EBTStat = 0x0000
> > CBTStat = 0x0000 CatStat = 0x00000000
> > ** Repairing volume.
> > ** Rechecking volume.
> > ** Checking HFS volume.
> > The volume name is untitled
> > ** Checking extents overflow file.
> > ** Checking catalog file.
> > ** Checking catalog hierarchy.
> > ** Checking volume bitmap.
> > ** Checking volume information.
> > ** The volume untitled was repaired successfully.
> > 
> > The main reason of the issue is the absence of logic that
> > corrects mdb->drNxtCNID/HFS_SB(sb)->next_id (next unused
> > CNID) after deleting a record in Catalog File. This patch
> > introduces a hfs_correct_next_unused_CNID() method that
> > implements the necessary logic. In the case of Catalog File's
> > record delete operation, the function logic checks that
> > (deleted_CNID + 1) == next_unused_CNID and it finds/sets the new
> > value of next_unused_CNID.
> 
> Sorry for the late reply.
> 
> I got you now, and I did some research. And It's a problem of CNID 
> usage. Catalog tree identification number is a type of u32.
> 
> And there're some ways to reuse cnid.
> If cnid reachs U32_MAX, kHFSCatalogNodeIDsReusedMask(apple open source 
> code) is marked to reuse unused cnid.
> And we can use HFSIOC_CHANGE_NEXTCNID ioctl to make use of unused cnid.
> 
> 
> What confused me is that fsck for hfsplus ignore those unused cnid[1], 
> but fsck for hfs only ignore those unused cnid if mdbP->drNxtCNID <= 
> (vcb->vcbNextCatalogID + 4096(which means over 4096 unused cnid)[2]?
> 
> And I didn't find code logic of changind cnid in apple source code when
> romove file.
> 
> So I think your idea is good, but it looks like that's not what the 
> original code did? If I'm wrong, please correct me.
> 
> 

I think you missed what is the problem here. It's not about reaching U32_MAX
threshold. The generic/736 test simply creates some number of files and, then,
deletes it. We increment mdb->drNxtCNID/HFS_SB(sb)->next_id on every creation of
file or folder because we assign the next unused CNID to the created file or
folder. But when we delete the file or folder, then we never correct the mdb-
>drNxtCNID/HFS_SB(sb)->next_id. And fsck tool expects that next unused CNID
should be equal to the last allocated/used CNID + 1. Let's imagine that we
create four files, then file1 has CNID 16, file2 has CNID 17, file3 has CNID 18,
file4 has CNID 19, and next unused CNID should be 20. If we delete file1, then
next unused CNID should be 20 because file4 still exists. And if we deleted all
files, then next unused CNID should be 16 again. This is what fsck tool expects
to see.

Thanks,
Slava.