[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20250703085451.GA4459@lst.de>
Date: Thu, 3 Jul 2025 10:54:51 +0200
From: Christoph Hellwig <hch@....de>
To: Damien Le Moal <dlemoal@...nel.org>
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
Subject: Re: [PATCH v2] nvmet: Make blksize_shift configurable
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?
Powered by blists - more mailing lists