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

[Keith Busch]

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.

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;
> > >          }
> > >      }
> > > --