Re: [PATCH v5 1/3] fs/file.c: remove sanity_check and add likely/unlikely in alloc_fd()

[Ma, Yu]

On 8/15/2024 5:38 AM, Al Viro wrote:
> On Wed, Jul 17, 2024 at 10:50:16AM -0400, Yu Ma wrote:
> > alloc_fd() has a sanity check inside to make sure the struct file mapping to the
> > allocated fd is NULL. Remove this sanity check since it can be assured by
> > exisitng zero initilization and NULL set when recycling fd. Meanwhile, add
> > likely/unlikely and expand_file() call avoidance to reduce the work under
> > file_lock.
> > +	if (unlikely(fd >= fdt->max_fds)) {
> > +		error = expand_files(files, fd);
> > +		if (error < 0)
> > +			goto out;
> >   
> > -	/*
> > -	 * If we needed to expand the fs array we
> > -	 * might have blocked - try again.
> > -	 */
> > -	if (error)
> > -		goto repeat;
> > +		/*
> > +		 * If we needed to expand the fs array we
> > +		 * might have blocked - try again.
> > +		 */
> > +		if (error)
> > +			goto repeat;
> With that change you can't get 0 from expand_files() here, so the
> last goto should be unconditional.  The only case when expand_files()
> returns 0 is when it has found the descriptor already being covered
> by fdt; since fdt->max_fds is stabilized by ->files_lock we are
> holding here, comparison in expand_files() will give the same
> result as it just had.
>
> IOW, that goto repeat should be unconditional.  The fun part here is
> that this was the only caller that distinguished between 0 and 1...

Yes, thanks Al, fully agree with you. The if (error) could be removed 
here as the above unlikely would make sure no 0 return here. Should I 
submit another version of patch set to update it, or you may help to 
update it directly during merge? I'm not very familiar with the rules, 
please let me know if I'd update and I'll take action soon. Thanks again 
for your careful check here.