Re: [PATCH v2] nvmet: Make blksize_shift configurable

[Richard Weinberger <richard@sigma-star.at>]

On Donnerstag, 3. Juli 2025 10:54 Christoph Hellwig wrote:
> On Tue, Jul 01, 2025 at 09:34:00AM +0900, Damien Le Moal wrote:
> > Even if internally you use the block size bit shift, I think it would be better
> > if the user facing interface is the block size as that is much easier to
> > manipulate without having to remember the exponent for powers of 2 values :)
> 
> Yeah, block sizes are probably a nice user interface indeed.

Ok!

> 
> > 		pr_err("Configured blksize needs to be at least %u for device %s\n",
> > 			bdev_logical_block_size(ns->bdev),
> > 			ns->device_path);
> > 		return -EINVAL;
> > 	}
> > 
> > Also, if the backend is an HDD, do we want to allow the user to configure a
> > block size that is less than the *physical* block size ? Performance will
> > suffer on regular HDDs and writes may fail with SMR HDDs.
> 
> I don't think we should babysit the user like that, just like we allow
> creating file systems with block size smaller than the physical block
> size.

I'm fine with either way.

> 
> > > +			if (!vfs_getattr(&ns->file->f_path, &st, STATX_DIOALIGN, 0) &&
> > > +			    (st.result_mask & STATX_DIOALIGN) &&
> > > +			    (1 << ns->blksize_shift) < st.dio_offset_align)
> > > +				return -EINVAL;
> > > +
> > > +			if (sb_bdev && (1 << ns->blksize_shift < bdev_logical_block_size(sb_bdev)))
> > > +				return -EINVAL;
> > 
> > I am confused... This is going to check both... But if you got STATX_DIOALIGN
> > and it is OK, you do not need (and probably should not) do the second if, no ?
> > 
> > Also, the second condition of the second if is essentially the same check as
> > for the block dev case. So maybe reuse that by creating a small helper function ?
> 
> This code is copy and pasted from loop, so it's originally my fault.
> It just missed the comment that explains why it is there:
> 
> 	/*
>          * In a perfect world this wouldn't be needed, but as of Linux 6.13 only
>          * a handful of file systems support the STATX_DIOALIGN flag.
>          */

Well, my code is the other way around. I checks the logical block size of a device
even if STATX_DIOALIGN succeeded, which is a bit too paranoid I guess.

Thanks,
//richard

-- 
​​​​​sigma star gmbh | Eduard-Bodem-Gasse 6, 6020 Innsbruck, AUT UID/VAT Nr:
ATU 66964118 | FN: 374287y