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

[Viacheslav Dubeyko <slava@dubeyko.com>]

On Sat, 2026-10-17 at 12:06 -0400, hengyul@cs.unc.edu wrote:
> From: Hengyu Liang <hengyul@cs.unc.edu>
> 
> hfs_inode_setattr() sets i_size and calls hfs_file_truncate() to
> change
> the allocation. If expansion fails, hfs_file_truncate() restores
> i_size
> to the allocated size, but its void return type leaves setattr
> reporting
> success.
> 
> On a freshly formatted 64 MiB HFS volume, ftruncate() to 128 MiB
> returns
> zero while the file remains empty. When the requested size fits
> within
> the volume but exceeds the free space, it also returns zero, leaving
> the file only partially extended.
> 
> Call generic_cont_expand_simple() before truncate_setsize() when
> growing
> a file, and propagate its error. This makes failed extensions report
> EFBIG or ENOSPC.
> 
> Signed-off-by: Hengyu Liang <hengyul@cs.unc.edu>

Mostly, patch looks good and it mimics the HFS+ logic but I have
several comments.

> ---
> Compared unpatched and patched v7.3-rc7 in KVM with GCC 10.5 and
> identical KASAN configurations:
> - 36 growth/shrink/error cases per kernel on 32/64/128 MiB HFS
> volumes;
>   content, zeroing and read-only remount checks passed.
> - fsx (10,000 operations) and generic/736 (10 runs) passed on both
>   kernels; post-test fsck.hfs -fn was clean.

I don't see any relation of this fix to generic/736. So, this comment
is not very useful. It will be much better to run the whole set of
xfstests and to check that this fix decrease, increase or keep the same
the number of failures. This run results could be really good to have.

> - fsck.hfs -fn was also clean for ordinary and recovery cases.
> 
> No kernel BUG, WARN or KASAN report was observed.
> 
>  fs/hfs/inode.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/fs/hfs/inode.c b/fs/hfs/inode.c
> index 2aef3c36a150..44b1735a54c2 100644
> --- a/fs/hfs/inode.c
> +++ b/fs/hfs/inode.c
> @@ -684,6 +684,12 @@ int hfs_inode_setattr(struct mnt_idmap *idmap,
> struct dentry *dentry,
>  		if (error)
>  			return error;
>  
> +		if (attr->ia_size > inode->i_size) {
> +			error = generic_cont_expand_simple(inode,
> attr->ia_size);
> +			if (error)
> +				return error;
> +		}
> +
>  		truncate_setsize(inode, attr->ia_size);
>  		hfs_file_truncate(inode);

Probably, we don't need to call the hfs_file_truncate() for the case of
attr->ia_size > inode->i_size. We have the same code in HFS+ logic. Do
you think this improvement makes sense?

Thanks,
Slava.

>  		inode_set_mtime_to_ts(inode,
> inode_set_ctime_current(inode));