diff mbox

[PATCHv4,4/4] block: avoid creating oversized writes in multiwrite_merge

Message ID 1413446090-30050-5-git-send-email-pl@kamp.de
State New
Headers show

Commit Message

Peter Lieven Oct. 16, 2014, 7:54 a.m. UTC 
Signed-off-by: Peter Lieven <pl@kamp.de>
Reviewed-by: Ronnie Sahlberg <ronniesahlberg@gmail.com>
---
 block.c |    5 +++++
 1 file changed, 5 insertions(+)

Comments

Max Reitz Oct. 23, 2014, 11:23 a.m. UTC | #1 
On 2014-10-16 at 09:54, Peter Lieven wrote:
> Signed-off-by: Peter Lieven <pl@kamp.de>
> Reviewed-by: Ronnie Sahlberg <ronniesahlberg@gmail.com>
> ---
>   block.c |    5 +++++
>   1 file changed, 5 insertions(+)
>
> diff --git a/block.c b/block.c
> index 0fbf916..9ad2287 100644
> --- a/block.c
> +++ b/block.c
> @@ -4554,6 +4554,11 @@ static int multiwrite_merge(BlockDriverState *bs, BlockRequest *reqs,
>               merge = 0;
>           }
>   
> +        if (bs->bl.max_transfer_length && reqs[outidx].nb_sectors +
> +            reqs[i].nb_sectors > bs->bl.max_transfer_length) {
> +            merge = 0;
> +        }
> +
>           if (merge) {
>               size_t size;
>               QEMUIOVector *qiov = g_malloc0(sizeof(*qiov));

Reviewed-by: Max Reitz <mreitz@redhat.com>

I feel like we should respect max_transfer_length in more than just this 
function, though. Every block device (or block driver) that sets a 
maximum transfer length should check requests against it as well, so we 
don't need to duplicate the same tests in the block layer functions, but 
maybe if at some point there are no things left to be done on the block 
layer, we could do that.

Max
Peter Lieven Oct. 25, 2014, 3:19 p.m. UTC | #2 
Am 23.10.2014 um 13:23 schrieb Max Reitz:
> On 2014-10-16 at 09:54, Peter Lieven wrote:
>> Signed-off-by: Peter Lieven <pl@kamp.de>
>> Reviewed-by: Ronnie Sahlberg <ronniesahlberg@gmail.com>
>> ---
>>   block.c |    5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/block.c b/block.c
>> index 0fbf916..9ad2287 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -4554,6 +4554,11 @@ static int multiwrite_merge(BlockDriverState *bs, BlockRequest *reqs,
>>               merge = 0;
>>           }
>>   +        if (bs->bl.max_transfer_length && reqs[outidx].nb_sectors +
>> +            reqs[i].nb_sectors > bs->bl.max_transfer_length) {
>> +            merge = 0;
>> +        }
>> +
>>           if (merge) {
>>               size_t size;
>>               QEMUIOVector *qiov = g_malloc0(sizeof(*qiov));
>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
>
> I feel like we should respect max_transfer_length in more than just this function, though. Every block device (or block driver) that sets a maximum transfer length should check requests against it as well, so we don't need to duplicate the same tests in the block layer functions, but maybe if at some point there are no things left to be done on the block layer, we could do that.

I have on my todo to add a splitting logic to block.c to 

bdrv_co_do_preadv() and bdrv_co_do_pwritev(), but I agreed with Kevin to introduce this
*after* 2.2.

Peter
diff mbox

Patch

diff --git a/block.c b/block.c
index 0fbf916..9ad2287 100644
--- a/block.c
+++ b/block.c
@@ -4554,6 +4554,11 @@  static int multiwrite_merge(BlockDriverState *bs, BlockRequest *reqs,
             merge = 0;
         }
 
+        if (bs->bl.max_transfer_length && reqs[outidx].nb_sectors +
+            reqs[i].nb_sectors > bs->bl.max_transfer_length) {
+            merge = 0;
+        }
+
         if (merge) {
             size_t size;
             QEMUIOVector *qiov = g_malloc0(sizeof(*qiov));