Re: [PATCH] block: use the request length for iov alignment
Am 20.09.2022 um 21:27 hat Keith Busch geschrieben: > On Wed, Sep 14, 2022 at 11:36:14AM +0100, Kevin Wolf wrote: > > Am 13.09.2022 um 15:12 hat Keith Busch geschrieben: > > > On Thu, Sep 08, 2022 at 09:45:26AM -0700, Keith Busch wrote: > > > > From: Keith Busch > > > > > > > > An iov length needs to be aligned to the logical block size, which may > > > > be larger than the memory alignment. > > > > > > [cc'ing some other interested folks] > > > > > > Any thoughts on this patch? It is fixing an observed IO error when > > > running > > > virtio-blk with the default 512b logical block size backed by a drive > > > formatted > > > with 4k logical block. > > > > I need to take a real look after KVM Forum, but my first thought was > > that we might be overloading request_alignment with multiple meanings > > now (file offset alignment and memory address alignment), and the values > > just happen to be the same for files on Linux. > > > > Did you consider a separate iov_alignment or similar and intentionally > > decided against it, or is it something you just didn't think about? > > I've looked again at the request_alignment usage, and I think that > value is exactly what we want. This alignment check is only used with > O_DIRECT backing file descriptors, and the request_alignment in that > case looks like it will always be the logical blocks size. > > Linux direct-io can accept arbitrary memory addrses offsets based on > the backing file's internal block limits, but the individual vector > lengths do need to be lba granularity. [ Coming back from below, the following paragraphs are kind of irrelevant... ] Sorry, I think I was unclear. I wasn't thinking about the file-posix backend on Linux, I have no doubts that you got the right condition there. I was just wondering if the condition would be incorrect on other backends, which would be affected by a change in the generic block layer, too. Checking the source code, it seems that several network protocol backends either set a request_alignment (nbd, iscsi) or get a default of 512 bytes because they don't implement byte granularity callbacks (gluster, ssh), but obviously these limits apply only to the offset and length of requests, not to memory addresses and lengths which won't be visible on the other end of the network connection any more. So in this case, using bs->bl.request_alignment seems too restrictive. [ This is the relevant one: ] But! After writing all of this, I notice that bdrv_qiov_is_aligned() is only ever called by file-posix. So let's move the function there instead of block/io.c, and then I think your approach is fine. :-) > Just in case, though, I could amend this so that the alignment is the > larger of request_alignment and the existing criteria, though I don't > see how request_alignment would ever be the smaller. > > size_t len = max(bs->bl.request_alignment, alignment); Yeah, it shouldn't be. As you can see above, my concern was the opposite case. Kevin
Re: [PATCH] block: use the request length for iov alignment
On Wed, Sep 14, 2022 at 11:36:14AM +0100, Kevin Wolf wrote:
> Am 13.09.2022 um 15:12 hat Keith Busch geschrieben:
> > On Thu, Sep 08, 2022 at 09:45:26AM -0700, Keith Busch wrote:
> > > From: Keith Busch
> > >
> > > An iov length needs to be aligned to the logical block size, which may
> > > be larger than the memory alignment.
> >
> > [cc'ing some other interested folks]
> >
> > Any thoughts on this patch? It is fixing an observed IO error when running
> > virtio-blk with the default 512b logical block size backed by a drive
> > formatted
> > with 4k logical block.
>
> I need to take a real look after KVM Forum, but my first thought was
> that we might be overloading request_alignment with multiple meanings
> now (file offset alignment and memory address alignment), and the values
> just happen to be the same for files on Linux.
>
> Did you consider a separate iov_alignment or similar and intentionally
> decided against it, or is it something you just didn't think about?
I've looked again at the request_alignment usage, and I think that value is
exactly what we want. This alignment check is only used with O_DIRECT backing
file descriptors, and the request_alignment in that case looks like it will
always be the logical blocks size.
Linux direct-io can accept arbitrary memory addrses offsets based on the
backing file's internal block limits, but the individual vector lengths do need
to be lba granularity.
Just in case, though, I could amend this so that the alignment is the larger of
request_alignment and the existing criteria, though I don't see how
request_alignment would ever be the smaller.
size_t len = max(bs->bl.request_alignment, alignment);
> > > @@ -3243,13 +3243,14 @@ bool bdrv_qiov_is_aligned(BlockDriverState *bs,
> > > QEMUIOVector *qiov)
> > > {
> > > int i;
> > > size_t alignment = bdrv_min_mem_align(bs);
> > > +size_t len = bs->bl.request_alignment;
> > > IO_CODE();
> > >
> > > for (i = 0; i < qiov->niov; i++) {
> > > if ((uintptr_t) qiov->iov[i].iov_base % alignment) {
> > > return false;
> > > }
> > > -if (qiov->iov[i].iov_len % alignment) {
> > > +if (qiov->iov[i].iov_len % len) {
> > > return false;
> > > }
> > > }
> > > --
Re: [PATCH] block: use the request length for iov alignment
On Wed, Sep 14, 2022 at 11:36:14AM +0100, Kevin Wolf wrote: > Am 13.09.2022 um 15:12 hat Keith Busch geschrieben: > > On Thu, Sep 08, 2022 at 09:45:26AM -0700, Keith Busch wrote: > > > From: Keith Busch > > > > > > An iov length needs to be aligned to the logical block size, which may > > > be larger than the memory alignment. > > > > [cc'ing some other interested folks] > > > > Any thoughts on this patch? It is fixing an observed IO error when running > > virtio-blk with the default 512b logical block size backed by a drive > > formatted > > with 4k logical block. > > I need to take a real look after KVM Forum, but my first thought was > that we might be overloading request_alignment with multiple meanings > now (file offset alignment and memory address alignment), and the values > just happen to be the same for files on Linux. > > Did you consider a separate iov_alignment or similar and intentionally > decided against it, or is it something you just didn't think about? I thought the request_alignment was indicating the minimum block length, so I did not consider an additional separate value. If it can be overloaded, then yes, I can certainly take on that idea.
Re: [PATCH] block: use the request length for iov alignment
Am 13.09.2022 um 15:12 hat Keith Busch geschrieben:
> On Thu, Sep 08, 2022 at 09:45:26AM -0700, Keith Busch wrote:
> > From: Keith Busch
> >
> > An iov length needs to be aligned to the logical block size, which may
> > be larger than the memory alignment.
>
> [cc'ing some other interested folks]
>
> Any thoughts on this patch? It is fixing an observed IO error when running
> virtio-blk with the default 512b logical block size backed by a drive
> formatted
> with 4k logical block.
I need to take a real look after KVM Forum, but my first thought was
that we might be overloading request_alignment with multiple meanings
now (file offset alignment and memory address alignment), and the values
just happen to be the same for files on Linux.
Did you consider a separate iov_alignment or similar and intentionally
decided against it, or is it something you just didn't think about?
Kevin
> > ---
> > block/io.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/block/io.c b/block/io.c
> > index 0a8cbefe86..296d4b49a7 100644
> > --- a/block/io.c
> > +++ b/block/io.c
> > @@ -3243,13 +3243,14 @@ bool bdrv_qiov_is_aligned(BlockDriverState *bs,
> > QEMUIOVector *qiov)
> > {
> > int i;
> > size_t alignment = bdrv_min_mem_align(bs);
> > +size_t len = bs->bl.request_alignment;
> > IO_CODE();
> >
> > for (i = 0; i < qiov->niov; i++) {
> > if ((uintptr_t) qiov->iov[i].iov_base % alignment) {
> > return false;
> > }
> > -if (qiov->iov[i].iov_len % alignment) {
> > +if (qiov->iov[i].iov_len % len) {
> > return false;
> > }
> > }
> > --
> > 2.30.2
Re: [PATCH] block: use the request length for iov alignment
On Tue, Sep 13, 2022 at 03:20:23PM +0100, Damien Le Moal wrote:
> On 2022/09/13 15:12, Keith Busch wrote:
> > On Thu, Sep 08, 2022 at 09:45:26AM -0700, Keith Busch wrote:
> >> From: Keith Busch
> >>
> >> An iov length needs to be aligned to the logical block size, which may
> >> be larger than the memory alignment.
> >
> > [cc'ing some other interested folks]
> >
> > Any thoughts on this patch? It is fixing an observed IO error when running
> > virtio-blk with the default 512b logical block size backed by a drive
> > formatted
> > with 4k logical block.
>
> The patch look OK to me, but having virtio expose a 512B LBA size for a
> backing
> device that has 4K LBAs will break all IOs if caching is turned off (direct
> IOs
> case), even if this patch is applied. No ?
Oh, as to why that type of setup "works" with O_DIRECT, when the check below
returns 'false', qemu allocates a bounce buffer. We want that to happen if the
guest's virtio driver tries to read/write 512b. The lengths just need to be
checked against the backing store's block size instead of the memory address
alignment.
> >> @@ -3243,13 +3243,14 @@ bool bdrv_qiov_is_aligned(BlockDriverState *bs,
> >> QEMUIOVector *qiov)
> >> {
> >> int i;
> >> size_t alignment = bdrv_min_mem_align(bs);
> >> +size_t len = bs->bl.request_alignment;
> >> IO_CODE();
> >>
> >> for (i = 0; i < qiov->niov; i++) {
> >> if ((uintptr_t) qiov->iov[i].iov_base % alignment) {
> >> return false;
> >> }
> >> -if (qiov->iov[i].iov_len % alignment) {
> >> +if (qiov->iov[i].iov_len % len) {
> >> return false;
> >> }
> >> }
Re: [PATCH] block: use the request length for iov alignment
On 2022/09/13 15:51, Keith Busch wrote:
> On Tue, Sep 13, 2022 at 03:20:23PM +0100, Damien Le Moal wrote:
>> On 2022/09/13 15:12, Keith Busch wrote:
>>> On Thu, Sep 08, 2022 at 09:45:26AM -0700, Keith Busch wrote:
From: Keith Busch
An iov length needs to be aligned to the logical block size, which may
be larger than the memory alignment.
>>>
>>> [cc'ing some other interested folks]
>>>
>>> Any thoughts on this patch? It is fixing an observed IO error when running
>>> virtio-blk with the default 512b logical block size backed by a drive
>>> formatted
>>> with 4k logical block.
>>
>> The patch look OK to me, but having virtio expose a 512B LBA size for a
>> backing
>> device that has 4K LBAs will break all IOs if caching is turned off (direct
>> IOs
>> case), even if this patch is applied. No ?
>
> Oh, as to why that type of setup "works" with O_DIRECT, when the check below
> returns 'false', qemu allocates a bounce buffer. We want that to happen if the
> guest's virtio driver tries to read/write 512b. The lengths just need to be
> checked against the backing store's block size instead of the memory address
> alignment.
Ah ! Got it. Thanks !
>
@@ -3243,13 +3243,14 @@ bool bdrv_qiov_is_aligned(BlockDriverState *bs,
QEMUIOVector *qiov)
{
int i;
size_t alignment = bdrv_min_mem_align(bs);
+size_t len = bs->bl.request_alignment;
IO_CODE();
for (i = 0; i < qiov->niov; i++) {
if ((uintptr_t) qiov->iov[i].iov_base % alignment) {
return false;
}
-if (qiov->iov[i].iov_len % alignment) {
+if (qiov->iov[i].iov_len % len) {
return false;
}
}
--
Damien Le Moal
Western Digital Research
Re: [PATCH] block: use the request length for iov alignment
On 2022/09/13 15:36, Keith Busch wrote: > On Tue, Sep 13, 2022 at 03:20:23PM +0100, Damien Le Moal wrote: >> On 2022/09/13 15:12, Keith Busch wrote: >>> On Thu, Sep 08, 2022 at 09:45:26AM -0700, Keith Busch wrote: From: Keith Busch An iov length needs to be aligned to the logical block size, which may be larger than the memory alignment. >>> >>> [cc'ing some other interested folks] >>> >>> Any thoughts on this patch? It is fixing an observed IO error when running >>> virtio-blk with the default 512b logical block size backed by a drive >>> formatted >>> with 4k logical block. >> >> The patch look OK to me, but having virtio expose a 512B LBA size for a >> backing >> device that has 4K LBAs will break all IOs if caching is turned off (direct >> IOs >> case), even if this patch is applied. No ? > > Yeah, it's just the default. I don't think anyone would do that on purpose > since it's so sub-optimal from a performance stand-point. As a follow up > patch, > perhaps if the logical_block_size parameter is not provided, we should default > to the backing device's block size? That sound like a reasonable approach to me. Note that I was not talking about the sub-optimal read-modify-write pattern for IOs not aligned on the backend device sector boundaries, but rather thinking about no IOs working at all if QEMU is started without caching (cache.direct=on option) -- Damien Le Moal Western Digital Research
Re: [PATCH] block: use the request length for iov alignment
On Tue, Sep 13, 2022 at 03:20:23PM +0100, Damien Le Moal wrote: > On 2022/09/13 15:12, Keith Busch wrote: > > On Thu, Sep 08, 2022 at 09:45:26AM -0700, Keith Busch wrote: > >> From: Keith Busch > >> > >> An iov length needs to be aligned to the logical block size, which may > >> be larger than the memory alignment. > > > > [cc'ing some other interested folks] > > > > Any thoughts on this patch? It is fixing an observed IO error when running > > virtio-blk with the default 512b logical block size backed by a drive > > formatted > > with 4k logical block. > > The patch look OK to me, but having virtio expose a 512B LBA size for a > backing > device that has 4K LBAs will break all IOs if caching is turned off (direct > IOs > case), even if this patch is applied. No ? Yeah, it's just the default. I don't think anyone would do that on purpose since it's so sub-optimal from a performance stand-point. As a follow up patch, perhaps if the logical_block_size parameter is not provided, we should default to the backing device's block size?
Re: [PATCH] block: use the request length for iov alignment
On 9/13/22 8:12 AM, Keith Busch wrote: > On Thu, Sep 08, 2022 at 09:45:26AM -0700, Keith Busch wrote: >> From: Keith Busch >> >> An iov length needs to be aligned to the logical block size, which may >> be larger than the memory alignment. > > [cc'ing some other interested folks] > > Any thoughts on this patch? It is fixing an observed IO error when running > virtio-blk with the default 512b logical block size backed by a drive > formatted > with 4k logical block. I ran into this issue and tested the patch. Works for me! Tested-by: Jens Axboe -- Jens Axboe
Re: [PATCH] block: use the request length for iov alignment
On 2022/09/13 15:12, Keith Busch wrote:
> On Thu, Sep 08, 2022 at 09:45:26AM -0700, Keith Busch wrote:
>> From: Keith Busch
>>
>> An iov length needs to be aligned to the logical block size, which may
>> be larger than the memory alignment.
>
> [cc'ing some other interested folks]
>
> Any thoughts on this patch? It is fixing an observed IO error when running
> virtio-blk with the default 512b logical block size backed by a drive
> formatted
> with 4k logical block.
The patch look OK to me, but having virtio expose a 512B LBA size for a backing
device that has 4K LBAs will break all IOs if caching is turned off (direct IOs
case), even if this patch is applied. No ?
>
>> ---
>> block/io.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/block/io.c b/block/io.c
>> index 0a8cbefe86..296d4b49a7 100644
>> --- a/block/io.c
>> +++ b/block/io.c
>> @@ -3243,13 +3243,14 @@ bool bdrv_qiov_is_aligned(BlockDriverState *bs,
>> QEMUIOVector *qiov)
>> {
>> int i;
>> size_t alignment = bdrv_min_mem_align(bs);
>> +size_t len = bs->bl.request_alignment;
>> IO_CODE();
>>
>> for (i = 0; i < qiov->niov; i++) {
>> if ((uintptr_t) qiov->iov[i].iov_base % alignment) {
>> return false;
>> }
>> -if (qiov->iov[i].iov_len % alignment) {
>> +if (qiov->iov[i].iov_len % len) {
>> return false;
>> }
>> }
>> --
>> 2.30.2
>>
--
Damien Le Moal
Western Digital Research
Re: [PATCH] block: use the request length for iov alignment
On Thu, Sep 08, 2022 at 09:45:26AM -0700, Keith Busch wrote:
> From: Keith Busch
>
> An iov length needs to be aligned to the logical block size, which may
> be larger than the memory alignment.
[cc'ing some other interested folks]
Any thoughts on this patch? It is fixing an observed IO error when running
virtio-blk with the default 512b logical block size backed by a drive formatted
with 4k logical block.
> ---
> block/io.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/block/io.c b/block/io.c
> index 0a8cbefe86..296d4b49a7 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -3243,13 +3243,14 @@ bool bdrv_qiov_is_aligned(BlockDriverState *bs,
> QEMUIOVector *qiov)
> {
> int i;
> size_t alignment = bdrv_min_mem_align(bs);
> +size_t len = bs->bl.request_alignment;
> IO_CODE();
>
> for (i = 0; i < qiov->niov; i++) {
> if ((uintptr_t) qiov->iov[i].iov_base % alignment) {
> return false;
> }
> -if (qiov->iov[i].iov_len % alignment) {
> +if (qiov->iov[i].iov_len % len) {
> return false;
> }
> }
> --
> 2.30.2
>
