Re: [PULL 18/28] file-posix: try BLKSECTGET on block devices too, do not round to power of 2

2021-09-23 Thread Paolo Bonzini

On 23/09/21 14:13, Halil Pasic wrote:

On Thu, 23 Sep 2021 12:57:38 +0200
Paolo Bonzini  wrote:


On 22/09/21 21:51, Halil Pasic wrote:

We have figured out what is going on here. The problem seems to be
specific to linux aio, which seems to limit the size of the iovec to
1024 (UIO_MAXIOV).


Hi Halil,

I'll send a patch shortly to fix this issue.  Sorry about the delay as I
was busy with KVM Forum and all that. :(


Hi Paolo!

With some guidance I could cook up a patch myself. But if you prefer to
do it yourself, I will be glad to test the fix (please put me on cc).
Thanks!


NP, the patch already existed but I was busy and didn't send it out.

Paolo




Re: [PULL 18/28] file-posix: try BLKSECTGET on block devices too, do not round to power of 2

2021-09-23 Thread Halil Pasic
On Thu, 23 Sep 2021 12:57:38 +0200
Paolo Bonzini  wrote:

> On 22/09/21 21:51, Halil Pasic wrote:
> > We have figured out what is going on here. The problem seems to be
> > specific to linux aio, which seems to limit the size of the iovec to
> > 1024 (UIO_MAXIOV).  
> 
> Hi Halil,
> 
> I'll send a patch shortly to fix this issue.  Sorry about the delay as I 
> was busy with KVM Forum and all that. :(

Hi Paolo!

With some guidance I could cook up a patch myself. But if you prefer to
do it yourself, I will be glad to test the fix (please put me on cc).
Thanks!

Regards,
Halil




Re: [PULL 18/28] file-posix: try BLKSECTGET on block devices too, do not round to power of 2

2021-09-23 Thread Paolo Bonzini

On 22/09/21 21:51, Halil Pasic wrote:

We have figured out what is going on here. The problem seems to be
specific to linux aio, which seems to limit the size of the iovec to
1024 (UIO_MAXIOV).


Hi Halil,

I'll send a patch shortly to fix this issue.  Sorry about the delay as I 
was busy with KVM Forum and all that. :(


Paolo



Re: [PULL 18/28] file-posix: try BLKSECTGET on block devices too, do not round to power of 2

2021-09-22 Thread Halil Pasic
On Mon, 6 Sep 2021 16:24:20 +0200
Halil Pasic  wrote:

> On Fri, 25 Jun 2021 16:18:12 +0200
> Paolo Bonzini  wrote:
> 
> > bs->sg is only true for character devices, but block devices can also
> > be used with scsi-block and scsi-generic.  Unfortunately BLKSECTGET
> > returns bytes in an int for /dev/sgN devices, and sectors in a short
> > for block devices, so account for that in the code.
> > 
> > The maximum transfer also need not be a power of 2 (for example I have
> > seen disks with 1280 KiB maximum transfer) so there's no need to pass
> > the result through pow2floor.
> > 
> > Signed-off-by: Paolo Bonzini   
> 
> We have found that this patch leads to in guest I/O errors when DASD
> is used as a source device. I.e. libvirt domain xml wise something like:
> 
> 
>   
>   
>   
>   
>   
> 
> 
> I don't think it is the fault of this patch: it LGTM. But it correlates
> 100%, furthermore the problem seems to be related to the value of
> bl.max_iov which now comes from sysfs. 
> 
> We are still investigating what is actually wrong. Just wanted to give
> everybody a heads-up that this does seem to cause a nasty regression on
> s390x, even if the code itself is perfect.
> 

We have figured out what is going on here. The problem seems to be
specific to linux aio, which seems to limit the size of the iovec to
1024 (UIO_MAXIOV).

Because of this requests get rejected with -EINVAL by the io_submit()
syscall. Here comes a real world example:

(gdb) p *laiocb
$5 = {co = 0x3ff700072c0, ctx = 0x3fe60002650, iocb = {data = 0x0, aio_rw_flags 
= 0, key = 0, 
aio_lio_opcode = 8, aio_reqprio = 0, aio_fildes = 38, u = {c = {buf = 
0x3ff70055bc0, 
nbytes = 1274, offset = 19977953280, __pad3 = 0, flags = 1, resfd = 
43}, v = {
vec = 0x3ff70055bc0, nr = 0, offset = 19977953280}, poll = {__pad1 = 
1023, 
events = 1879399360}, saddr = {addr = 0x3ff70055bc0, len = 0}}}, ret = 
-22, 
  nbytes = 20586496, qiov = 0x3ff700382f8, is_read = false, next = {sqe_next = 
0x0}}

On the host kernel side, the -EINVAL comes from this line:
https://elixir.bootlin.com/linux/v5.15-rc2/source/lib/iov_iter.c#L1863
in iovec_from_user() roughly via: __do_sys_io_submit()->
io_submit_one() -> aio_write() -> aio_setup_rw() -> __import_iovec().

Based on the offline discussion with the DASD maintainers, and on the
linux in source tree documentation (Documentation/block/queue-sysfs.rst
und Documentation/block/biodoc.rst), I believe that the DASD driver is
not wrong when advertising the value 65535 for queue/max_segments.

I believe QEMU jumps to the wrong conclusion in blk_get_max_iov() or
in virtio_blk_submit_multireq(), I can't really tell because I'm not
sure what the semantic of blk_get_max_iov() is. But if, I had to, I would
bet that blk_get_max_iov() returns the wrong value, when linux aio is
used. I'm not sure what is the exact relationship between max_segments
and max_iov.

One idea on how to fix this would be, to cap max_iov to UIO_MAXIOV
(unconditionally, or when linux aio is used). But I have to admit, I
don't have clarity. I couldn't even find documentation that states
this limitation of linux aio (I didn't look for it too hard though), so
I can not even be sure this is a QEMU problem.

That is why I decided to write this up first, and start a discussion on
who is playing foul with the most relevant people posted. I intend to try
myself fixing the problem once my understanding of it reaches a
sufficient level.

Thanks in advance for your contribution!

Regards,
Halil



Re: [PULL 18/28] file-posix: try BLKSECTGET on block devices too, do not round to power of 2

2021-09-06 Thread Halil Pasic
On Fri, 25 Jun 2021 16:18:12 +0200
Paolo Bonzini  wrote:

> bs->sg is only true for character devices, but block devices can also
> be used with scsi-block and scsi-generic.  Unfortunately BLKSECTGET
> returns bytes in an int for /dev/sgN devices, and sectors in a short
> for block devices, so account for that in the code.
> 
> The maximum transfer also need not be a power of 2 (for example I have
> seen disks with 1280 KiB maximum transfer) so there's no need to pass
> the result through pow2floor.
> 
> Signed-off-by: Paolo Bonzini 

We have found that this patch leads to in guest I/O errors when DASD
is used as a source device. I.e. libvirt domain xml wise something like:


  
  
  
  
  


I don't think it is the fault of this patch: it LGTM. But it correlates
100%, furthermore the problem seems to be related to the value of
bl.max_iov which now comes from sysfs. 

We are still investigating what is actually wrong. Just wanted to give
everybody a heads-up that this does seem to cause a nasty regression on
s390x, even if the code itself is perfect.

Regards,
Halil