Re: [RFC PATCH 1/1] minix: unify the v1 and v2/v3 itree code paths

[ctdk]

On Tue, 22 Sep 2026 at 06:01:38 -0700, Christoph Hellwig wrote:
> I think this is a good idea, and should make the code more maintainable
> forward, as well as simply your iomap conversion.

It should help immensely with both of those things, I think. More than
one reviewer of the iomap conversion has been left scratching their
heads over the weird tricks both required for the iomap conversion and
simply already present in the module.
 
> A few comments:
> 
> > +	/* Set the direct block and indirect block depth now that the rest of
> > +	 * the version-specific settings have been set.
> > +	 */
> 
> Please use the normal kernel comment style:
> 
> 	/*
> 	 * Set the direct block and indirect block depth now that the rest of
> 	 * the version-specific settings have been set.
> 	 */
> 
> > +	sbi->s_direct = MINIX_DIRECT; /* Always the same. */
> 
> No need for the comment I think.

As I believe I said somewhere, I explicitly loaded this RFC patch to
explain what I was doing since I was looking for comments on it. I
erred on the side of verbosity because I was looking to undo something
that had been this way for at least 21 years, and probably much longer.
I was already planning on stripping the excess comments out, but thank
you for the reminders about style and line length. This applies to all
the comment and style comments below, to save space.

> >  	generic_fillattr(&nop_mnt_idmap, request_mask, inode, stat);
> >  	if (INODE_VERSION(inode) == MINIX_V1)
> > -		stat->blocks = (BLOCK_SIZE / 512) * V1_minix_blocks(stat->size, sb);
> > +		stat->blocks = (BLOCK_SIZE / 512) * minix_blocks(stat->size, sb);
> >  	else
> > -		stat->blocks = (sb->s_blocksize / 512) * V2_minix_blocks(stat->size, sb);
> > +		stat->blocks = (sb->s_blocksize / 512) * minix_blocks(stat->size, sb);
> 
> v1 always sets s_blocksize to BLOCK_SIZE, so this can simply become and
> unconditional:
> 
> 	stat->blocks = (sb->s_blocksize / 512) * minix_blocks(stat->size, sb);

Noted.
 
> > +extern int minix_get_block(struct inode *inode, sector_t block,
> > +		struct buffer_head *bh, int create);
> > +extern unsigned int minix_blocks(loff_t size, struct super_block *sb);
> 
> Please drop the extern for all function declarations that you touch.

I figured out that we no longer need to have 'extern' in the header
declarations after I submitted this patch, but thank you for the
reminder.

Somehow this reply hasn't shown up in my inbox yet, but fortunately I
was able to find it. I'll get this turned around and send out the new
and improved versio shortly.

Thanks again,

-j