| 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
| ||
|
Message-ID: <132c1bdf-e100-4e3a-883f-27f9e9b78020@kernel.org>
Date: Tue, 1 Jul 2025 09:34:00 +0900
From: Damien Le Moal <dlemoal@...nel.org>
To: Richard Weinberger <richard@....at>, linux-nvme@...ts.infradead.org
Cc: linux-kernel@...r.kernel.org, kch@...dia.com, sagi@...mberg.me,
hch@....de, upstream+nvme@...ma-star.at
Subject: Re: [PATCH v2] nvmet: Make blksize_shift configurable
On 7/1/25 4:13 AM, Richard Weinberger wrote:
> Currently, the block size is automatically configured, and for
> file-backed namespaces it is likely to be 4K.
> While this is a reasonable default for modern storage, it can
> cause confusion if someone wants to export a pre-created disk image
> that uses a 512-byte block size.
> As a result, partition parsing will fail.
>
> So, just like we already do for the loop block device, let the user
> configure the block size if they know better.
Hmm... That fine with me but this explanation does not match what the patch
does: you allow configuring the block size bit shift, not the size. That is not
super user friendly :)
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 :)
>
> Signed-off-by: Richard Weinberger <richard@....at>
> ---
> Changes since v1 (RFC)[0]:
>
> - Make sure blksize_shift is in general within reason
> - In the bdev case and when using direct IO, blksize_shift has to be
> smaller than the logical block it the device
> - In the file case and when using direct IO try to use STATX_DIOALIGN,
> just like the loop device does
>
> [0] https://lore.kernel.org/linux-nvme/20250418090834.2755289-1-richard@nod.at/
>
> Thanks,
> //richard
> ---
> drivers/nvme/target/configfs.c | 37 +++++++++++++++++++++++++++++++
> drivers/nvme/target/io-cmd-bdev.c | 13 ++++++++++-
> drivers/nvme/target/io-cmd-file.c | 28 ++++++++++++++++++-----
> 3 files changed, 71 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
> index e44ef69dffc24..26175c37374ab 100644
> --- a/drivers/nvme/target/configfs.c
> +++ b/drivers/nvme/target/configfs.c
> @@ -797,6 +797,42 @@ static ssize_t nvmet_ns_resv_enable_store(struct config_item *item,
> }
> CONFIGFS_ATTR(nvmet_ns_, resv_enable);
>
> +static ssize_t nvmet_ns_blksize_shift_show(struct config_item *item, char *page)
As mentioned above, I think this should be nvmet_ns_blksize_show().
> +{
> + return sysfs_emit(page, "%u\n", to_nvmet_ns(item)->blksize_shift);
And you can do:
return sysfs_emit(page, "%u\n",
1U << to_nvmet_ns(item)->blksize_shift);
> +}
> +
> +static ssize_t nvmet_ns_blksize_shift_store(struct config_item *item,
> + const char *page, size_t count)
Similar here: nvmet_ns_blksize_store()
> +{
> + struct nvmet_ns *ns = to_nvmet_ns(item);
> + u32 shift;
> + int ret;
> +
> + ret = kstrtou32(page, 0, &shift);
> + if (ret)
> + return ret;
> +
> + /*
> + * Make sure block size is within reason, something between 512 and
> + * BLK_MAX_BLOCK_SIZE.
> + */
> + if (shift < 9 || shift > 16)
> + return -EINVAL;
And this would be simpler:
if (blksz < SECTOR_SIZE || blksz > BLK_MAX_BLOCK_SIZE ||
!is_power_of_2(blksz))
return -EINVAL;
> +
> + mutex_lock(&ns->subsys->lock);
> + if (ns->enabled) {
> + pr_err("the ns:%d is already enabled.\n", ns->nsid);
> + mutex_unlock(&ns->subsys->lock);
> + return -EINVAL;
> + }
> + ns->blksize_shift = shift;
and here:
ns->blksize_shift = ilog2(blksz);
> + mutex_unlock(&ns->subsys->lock);
> +
> + return count;
> +}
> +CONFIGFS_ATTR(nvmet_ns_, blksize_shift);
> +
> static struct configfs_attribute *nvmet_ns_attrs[] = {
> &nvmet_ns_attr_device_path,
> &nvmet_ns_attr_device_nguid,
> @@ -806,6 +842,7 @@ static struct configfs_attribute *nvmet_ns_attrs[] = {
> &nvmet_ns_attr_buffered_io,
> &nvmet_ns_attr_revalidate_size,
> &nvmet_ns_attr_resv_enable,
> + &nvmet_ns_attr_blksize_shift,
> #ifdef CONFIG_PCI_P2PDMA
> &nvmet_ns_attr_p2pmem,
> #endif
> diff --git a/drivers/nvme/target/io-cmd-bdev.c b/drivers/nvme/target/io-cmd-bdev.c
> index eba42df2f8215..be39837d4d792 100644
> --- a/drivers/nvme/target/io-cmd-bdev.c
> +++ b/drivers/nvme/target/io-cmd-bdev.c
> @@ -77,6 +77,7 @@ static void nvmet_bdev_ns_enable_integrity(struct nvmet_ns *ns)
>
> int nvmet_bdev_ns_enable(struct nvmet_ns *ns)
> {
> + int bdev_blksize_shift;
> int ret;
>
> /*
> @@ -100,7 +101,17 @@ int nvmet_bdev_ns_enable(struct nvmet_ns *ns)
> }
> ns->bdev = file_bdev(ns->bdev_file);
> ns->size = bdev_nr_bytes(ns->bdev);
> - ns->blksize_shift = blksize_bits(bdev_logical_block_size(ns->bdev));
> + bdev_blksize_shift = blksize_bits(bdev_logical_block_size(ns->bdev));
> +
> + if (ns->blksize_shift) {
> + if (ns->blksize_shift < bdev_blksize_shift) {
> + pr_err("Configured blksize_shift needs to be at least %d for device %s\n",
> + bdev_blksize_shift, ns->device_path);
> + return -EINVAL;
> + }
> + } else {
> + ns->blksize_shift = bdev_blksize_shift;
> + }
Nit: to avoid the indented if, may be write this like this: ?
if (!ns->blksize_shift)
ns->blksize_shift = bdev_blksize_shift;
if (ns->blksize_shift < bdev_blksize_shift) {
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.
>
> ns->pi_type = 0;
> ns->metadata_size = 0;
> diff --git a/drivers/nvme/target/io-cmd-file.c b/drivers/nvme/target/io-cmd-file.c
> index 2d068439b129c..a4066b5a1dc97 100644
> --- a/drivers/nvme/target/io-cmd-file.c
> +++ b/drivers/nvme/target/io-cmd-file.c
> @@ -49,12 +49,28 @@ int nvmet_file_ns_enable(struct nvmet_ns *ns)
>
> nvmet_file_ns_revalidate(ns);
>
> - /*
> - * i_blkbits can be greater than the universally accepted upper bound,
> - * so make sure we export a sane namespace lba_shift.
> - */
> - ns->blksize_shift = min_t(u8,
> - file_inode(ns->file)->i_blkbits, 12);
> + if (ns->blksize_shift) {
> + if (!ns->buffered_io) {
> + struct block_device *sb_bdev = ns->file->f_mapping->host->i_sb->s_bdev;
> + struct kstat st;
> +
> + 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 ?
> + }
> + } else {
> + /*
> + * i_blkbits can be greater than the universally accepted
> + * upper bound, so make sure we export a sane namespace
> + * lba_shift.
> + */
> + ns->blksize_shift = min_t(u8,
> + file_inode(ns->file)->i_blkbits, 12);
> + }
It feels like this entire hunk should be a helper function as that would allow
making it a lot more readable with early returns. This code here whould be
something like:
ret = nvmet_file_set_ns_blksize_shift(ns);
if (ret)
return ret;
>
> ns->bvec_pool = mempool_create(NVMET_MIN_MPOOL_OBJ, mempool_alloc_slab,
> mempool_free_slab, nvmet_bvec_cache);
--
Damien Le Moal
Western Digital Research
Powered by blists - more mailing lists