Re: [PATCH v2] nvmet: Make blksize_shift configurable

[Christoph Hellwig <hch@lst.de>]

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.

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

> > +			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.
         */

The situation has unfortunately not improved since 6.13.  Maybe we
just need to do a sweep and fix this up?