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

[Jori Koolstra]

> 
> 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 :)

> 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.

> 
> 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.

> 
> Thanks,
> Slava.
> 

Thank-you for the extended response!

Jori.