Re: [PATCH v2] hfs: report errors when extending files with setattr

[Viacheslav Dubeyko <slava@dubeyko.com>]

On Thu, 2026-10-22 at 09:51 -0400, Hengyu Liang wrote:
> Hi Slava,
> 
> On Wed, Oct 21, 2026 at 02:22:41PM -0700, Viacheslav Dubeyko wrote:
> > I think now this code [1] becomes dead. And we need to rework it:
> > 
> > void hfs_file_truncate(struct inode *inode)
> > {
> >         ...
> >         hfs_dbg(...);
> > 
> >         if (WARN_ON_ONCE(inode->i_size > HFS_I(inode)->phys_size))
> >                 return;
> >         if (inode->i_size == HFS_I(inode)->phys_size)
> >                 return;
> [...]
> > It will be great if you could share another patch with the fix.
> 
> Thanks for the review. I made this change and ran the whole xfstests
> set, but the grow path is still reached. The WARN_ON_ONCE() fires
> under
> fsstress in generic/013, 076, 476, 650, 750 and 795, every time from
> close() through hfs_file_release().
> 
> It comes from direct writes:
> 
> 1. When an inode is read, phys_size is set to i_size, which does not
>    have to be block aligned, e.g. 100.
> 2. A direct write does not go through cont_write_begin(). When it
>    needs a new block, hfs_get_block() only adds s_blocksize to
>    phys_size. A direct write of 512 bytes at offset 512 makes
> phys_size
>    612 and i_size 1024.
> 3. At the last close, hfs_file_truncate() sees i_size > phys_size and
>    calls hfs_write_begin(). cont_write_begin() then zeroes
>    [phys_size, i_size) in the page cache, which overwrites the data
>    that the direct write has just written.
> 
> So today this path loses data, and removing it would only replace
> that
> with the warning, while phys_size stays behind i_size.
> 
> The issue can be reproduced with:
> 
>     truncate -s 32M hfs.img
>     mkfs.hfsplus -h hfs.img
>     mount -t hfs -o loop hfs.img /mnt
>     printf 'A%.0s' $(seq 100) > /mnt/f
>     umount /mnt
>     mount -t hfs -o loop hfs.img /mnt
>     xfs_io -d -c 'pwrite -S 0x42 512 512' /mnt/f
>     umount /mnt
>     mount -t hfs -o loop hfs.img /mnt
>     od -An -tx1 -j 512 -N 512 /mnt/f
> 
> On a804c5742bff, with and without the v2 patch, the last 412 bytes
> read back as zeros instead of 0x42, and fsck.hfs reports no error.
> With the cleanup applied, the data is intact but the warning fires.
> 
> HFS+ has the same problem. hfsplus_file_truncate() has the same grow
> path, and commit 5f63ac80aef2 ("hfsplus: fix issue of direct writes
> beyond end-of-file") only covers writes that start past EOF. A direct
> write of 1024 bytes at offset 0 to the same 100-byte file loses 924
> bytes on HFS+.
> 
> I think the direct write path should be fixed first, so that i_size
> can no longer get past phys_size. Then the grow path really is dead
> and your cleanup is right. The simplest way I see is what
> fat_direct_IO() does: if a direct write would end past phys_size,
> return 0 so that it falls back to a buffered write, which zeroes and
> accounts through cont_write_begin().
> 
> Would that be fine with you? If so, I will send that fix for HFS with
> your cleanup on top, and a similar fix for HFS+.
> 
> 

Yes, we still have enough issues in HFS and let's fix it step by step.
You are right, we need to fix the direct IO issue before removing this
"dead" code (it's not completely dead yet).

Related to HFS+... I am still working on rebuilding direct IO path for
HFS+ and, potentially, your fix could conflict with my patchset. But,
anyway, you can share your fix and, then, we can manage this conflict
(if we have one finally).

Thanks,
Slava.