RE: [PATCH] hfs: Replace BUG_ON with error handling in hfs_new_inode()

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

On Sat, 2025-11-22 at 20:50 +0100, Jori Koolstra wrote:
> > 
> > I am terribly sorry, I've missed the patch. But, please, please,
> > please, add prefix 'hfs:' to the topic. This is the reason why I've
> > missed the patch. I expected to see something like this:
> > 
> > hfs: Replace BUG_ON with error handling in hfs_new_inode()
> > 
> > I need to process dozens emails every day. So, if I don't see proper
> > keyword in the topic, then I skip the emails.
> 
> My bad, I didn't know this convention. Is this LKML-wide? Because I have
> also been waiting for a few weeks on feedback on jfs patches. Normally,
> it would not matter, but I am in the Linux Foundation Kernel Mentorship
> Program and we need to get several patches in before the deadline to
> succeed :)
> 

You can take a look here [1]. Usually, yes, everybody expects kernel subsystem's
name as the prefix:

Subject: [PATCH 001/123] subsystem: summary phrase

> > OK. I see. You have modified the hfs_new_inode() with the goal to
> > return error code instead of NULL.
> > 
> > Frankly speaking, I am not sure that inode is NULL, then it means
> > always that we are out of memory (-ENOMEM).
> > 
> 
> I think this is correct. See for instance fs/ext4/ialloc.c at __ext4_new_inode.
> 

Probably, you are right. I simply took a look into alloc_inode():

struct inode *alloc_inode(struct super_block *sb)
{
	const struct super_operations *ops = sb->s_op;
	struct inode *inode;

	if (ops->alloc_inode)
		inode = ops->alloc_inode(sb);
	else
		inode = alloc_inode_sb(sb, inode_cachep, GFP_KERNEL);

	if (!inode)
		return NULL;

	if (unlikely(inode_init_always(sb, inode))) {
		if (ops->destroy_inode) {
			ops->destroy_inode(inode);
			if (!ops->free_inode)
				return NULL;
		}
		inode->free_inode = ops->free_inode;
		i_callback(&inode->i_rcu);
		return NULL;
	}

	return inode;
}

Second part of the method could return NULL even if inode has been allocated.
But we can consider -ENOMEM in hfs_new_inode(). I am OK with that.

Thanks,
Slava.

> > 
> > Why do we use -ENOSPC here? If next_id > U32_MAX, then it doesn't mean
> > that volume is full. Probably, we have corrupted volume, then code
> > error should be completely different (maybe, -EIO).
> > 
> 
> ext4 uses EFSCORRUPTED which is defined as EUCLEAN, I can change that.
> 
> I wasn't exactly sure of the limits in place in hfs. But looking more closely,
> there can only be 65,535 allocation blocks, and I think you need at least one
> per inode. But then why are the CNID, max files, max directories 32-bit values
> in the MBD? What limits indicate corruption?
> 
> 
> > The 'hfs:' prefix is not necessary here. It could be not only file but
> > folder ID too. So, maybe, it makes sense to mention "next CNID". The
> > whole comment needs to be on one line. Also, I believe it makes sense
> > to recommend run FSCK tool here.
> > 
> 
> Will fix this.
> 
> > >  
> > >  void hfs_delete_inode(struct inode *inode)
> > > @@ -251,7 +271,6 @@ void hfs_delete_inode(struct inode *inode)
> > >  
> > >  	hfs_dbg("ino %lu\n", inode->i_ino);
> > >  	if (S_ISDIR(inode->i_mode)) {
> > > -		BUG_ON(atomic64_read(&HFS_SB(sb)->folder_count) >
> > > U32_MAX);
> > 
> > I don't agree with complete removal of this check. Because, we could
> > have bugs in file system logic that can increase folder_count
> > wrongfully above U32_MAX limit.
> > 
> > So, I prefer to have this check in some way. Error code sounds good.
> > 
> 
> Will add these back with error handling instead of BUG_ON.
> 
> 
> 

[1]
https://docs.kernel.org/process/submitting-patches.html#the-canonical-patch-format