Re: [PATCH v2 2/6] file-posix: try BLKSECTGET on block devices too, do not round to power of 2

[Paolo Bonzini <pbonzini@redhat.com>]

On 27/05/21 17:51, Kevin Wolf wrote:
> Am 24.05.2021 um 18:36 hat Paolo Bonzini geschrieben:
>> 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 <pbonzini@redhat.com>
> 
> Looks like this is more or less a revert of Maxim's commit 867eccfe. If
> this is what we want, should this old commit be mentioned in one way or
> another in the commit message?

It is (but it is not intentional).

> Apparently the motivation for Maxim's patch was, if I'm reading the
> description correctly, that it affected non-sg cases by imposing
> unnecessary restrictions. I see that patch 1 changed the max_iov part so
> that it won't affect non-sg cases any more, but max_transfer could still
> be more restricted than necessary, no?

Indeed the kernel puts no limit at all, but especially with O_DIRECT we 
probably benefit from avoiding the moral equivalent of "bufferbloat".

> For convenience, the bug report fixed with that patch is here:
> https://bugzilla.redhat.com/show_bug.cgi?id=1647104
> 
> Are we really trying to describe different things (limits for SG_IO and
> for normal I/O) in one value with max_transfer, even though it could be
> two different numbers for the same block device?

>> -static int sg_get_max_transfer_length(int fd)
>> +static int sg_get_max_transfer_length(int fd, struct stat *st)
> 
> This is now a misnomer. Should we revert to the pre-867eccfe name
> hdev_get_max_transfer_length()?

Yes.

>>   static void raw_refresh_limits(BlockDriverState *bs, Error **errp)
>>   {
>>       BDRVRawState *s = bs->opaque;
>> +    struct stat st;
>> +
>> +    if (fstat(s->fd, &st)) {
>> +        return;
> 
> Don't we want to set errp? Or do you intentionally ignore the error?

Yes, since we ignore errors from the ioctl I figured it's the same for 
fstat (just do not do the ioctls).

However, skipping raw_probe_alignment is wrong.

Thanks for the review!  Should I wait for you to go through the other 
patches?

Paolo