Re: [PATCH] block: use the request length for iov alignment

[Kevin Wolf]

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 <kbusch@kernel.org>
> > > > 
> > > > 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