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 <kbu...@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; > > > } > > > } > > > --