I have two questions:

1. how was it tested - was there some corruption case?
2. do you plan to review boot1 code too (as separate patch)?

In D19140#409448, @tsoome wrote:

I have two questions:

1. how was it tested - was there some corruption case?

I did not see any corruption happen. I found this while reading code while root-causing an issue I was having with a machine I had upgraded to 11.2 (the issue described briefly in D19142). On the 11.2 machine, I did instrument vdev_read and found that on that machine, no reads were performed that required the bounce buffer. I imagine the bounce buffer only comes into use with 4K sector drives - none of my gear is 4K.

I tested this patch using a VM booting from a ZFS mirror.

2. do you plan to review boot1 code too (as separate patch)?

I had not yet looked at it, but now I have, and it would appear to require the same fix. I don't see a reasonable way to abstract this new read logic into a reusable function - the abstract version would have to take pointers to alloc, free, and read routines, the read routines would have to be wrappers in both cases, and the free routine would have to be a wrapper in the efi/boot1 case. So I think that patch will largely be an exercise in copy/paste. After this passes review, I can prepare a patch for efi/boot1.

In D19140#409563, @pkelsey wrote:

In D19140#409448, @tsoome wrote:

I have two questions:

1. how was it tested - was there some corruption case?

I did not see any corruption happen. I found this while reading code while root-causing an issue I was having with a machine I had upgraded to 11.2 (the issue described briefly in D19142). On the 11.2 machine, I did instrument vdev_read and found that on that machine, no reads were performed that required the bounce buffer. I imagine the bounce buffer only comes into use with 4K sector drives - none of my gear is 4K.

I tested this patch using a VM booting from a ZFS mirror.

The reason why I am asking is, I am trying to understand if we actually *can* get into the situation with overrun, I got the impression the zfs on disk format should keep things sector aligned, but it really is easy to get confused there... And it does feel safer if we do have proper checks in place.

2. do you plan to review boot1 code too (as separate patch)?

I had not yet looked at it, but now I have, and it would appear to require the same fix. I don't see a reasonable way to abstract this new read logic into a reusable function - the abstract version would have to take pointers to alloc, free, and read routines, the read routines would have to be wrappers in both cases, and the free routine would have to be a wrapper in the efi/boot1 case. So I think that patch will largely be an exercise in copy/paste. After this passes review, I can prepare a patch for efi/boot1.

In D19140#409629, @tsoome wrote:

The reason why I am asking is, I am trying to understand if we actually *can* get into the situation with overrun, I got the impression the zfs on disk format should keep things sector aligned, but it really is easy to get confused there... And it does feel safer if we do have proper checks in place.

If the zfs on-disk format keeps things sector-aligned / sector-multiple, then why was vdev_read complicated with code to handle non-sector aligned / non-sector multiple reads to begin with?

In D19140#409630, @pkelsey wrote:

In D19140#409629, @tsoome wrote:

The reason why I am asking is, I am trying to understand if we actually *can* get into the situation with overrun, I got the impression the zfs on disk format should keep things sector aligned, but it really is easy to get confused there... And it does feel safer if we do have proper checks in place.

If the zfs on-disk format keeps things sector-aligned / sector-multiple, then why was vdev_read complicated with code to handle non-sector aligned / non-sector multiple reads to begin with?

Yea, you are right:)

In D19140#409631, @tsoome wrote:

In D19140#409630, @pkelsey wrote:

In D19140#409629, @tsoome wrote:

The reason why I am asking is, I am trying to understand if we actually *can* get into the situation with overrun, I got the impression the zfs on disk format should keep things sector aligned, but it really is easy to get confused there... And it does feel safer if we do have proper checks in place.

If the zfs on-disk format keeps things sector-aligned / sector-multiple, then why was vdev_read complicated with code to handle non-sector aligned / non-sector multiple reads to begin with?

Yea, you are right:)

I think one way to wind up in the unaligned / non-multiple case is with a vdev having ashift = 9 on a 512e 4K device (either created that way on the 512e 4K device originally, or perhaps created on a 512 device and disk-imaged to a 512e 4K device). I think in that case, on a BIOS system, you will have drives reporting a 4K sector size via int13h ah=48h, so vdev_read() will consider that the sector size, but the metadata layout driving the reads will have been created from the standpoint of having 512 byte sectors and thus may have non-4K sizes and alignments.

Not sure that this works.
I try this on 11.2 and on Hyper-V gen1 system stick at ldelf loading stage on boot from zfs root.

In D19140#411495, @rozhuk.im-gmail.com wrote:

Not sure that this works.
I try this on 11.2 and on Hyper-V gen1 system stick at ldelf loading stage on boot from zfs root.

Can you describe in more detail what you tried? Did you merge this patch into into 11.2 sources (if so, which ones - RELEASE, STABLE, ...), or did you build the bootcode from head and copy the results to your 11.2 system (if so, what did you copy)?

I merge it to stable.
Sorry for noise, problem was with random - not enough entropy on first boot to unblock it. It reproduced even on 11.2 from official site.