linux-kernel - Re: [PATCH v2] nvmet: Make blksize

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <2880421.FSEd18e0ET@nailgun>
Date: Thu, 03 Jul 2025 11:29:58 +0200
From: Richard Weinberger <richard@...ma-star.at>
To: Damien Le Moal <dlemoal@...nel.org>, upstream@...ma-star.at
Cc: Richard Weinberger <richard@....at>, linux-nvme@...ts.infradead.org,
 linux-kernel@...r.kernel.org, kch@...dia.com, sagi@...mberg.me, hch@....de,
 upstream+nvme@...ma-star.at, Christoph Hellwig <hch@....de>
Subject: Re: [PATCH v2] nvmet: Make blksize_shift configurable

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