[Qemu-devel] [PATCH] block: change default for discard and write zeroes to INT_MAX

[Qemu-devel] [PATCH] block: change default for discard and write zeroes to INT_MAX

[Peter Lieven]

do not trim requests if the driver does not supply a limit
through BlockLimits. For write zeroes we still keep a limit
for the unsupported path to avoid allocating a big bounce buffer.

Suggested-by: Kevin Wolf <kwolf@redhat.com>
Suggested-by: Denis V. Lunev <den@openvz.org>
Signed-off-by: Peter Lieven <pl@kamp.de>
---
 block.c | 16 ++++------------
 1 file changed, 4 insertions(+), 12 deletions(-)

diff --git a/block.c b/block.c
index d45e4dd..ee7ff2c 100644
--- a/block.c
+++ b/block.c
@@ -3192,10 +3192,7 @@ int coroutine_fn bdrv_co_copy_on_readv(BlockDriverState *bs,
                             BDRV_REQ_COPY_ON_READ);
 }
 
-/* if no limit is specified in the BlockLimits use a default
- * of 32768 512-byte sectors (16 MiB) per request.
- */
-#define MAX_WRITE_ZEROES_DEFAULT 32768
+#define MAX_WRITE_ZEROES_BOUNCE_BUFFER 32768
 
 static int coroutine_fn bdrv_co_do_write_zeroes(BlockDriverState *bs,
     int64_t sector_num, int nb_sectors, BdrvRequestFlags flags)
@@ -3206,7 +3203,7 @@ static int coroutine_fn bdrv_co_do_write_zeroes(BlockDriverState *bs,
     int ret = 0;
 
     int max_write_zeroes = bs->bl.max_write_zeroes ?
-                           bs->bl.max_write_zeroes : MAX_WRITE_ZEROES_DEFAULT;
+                           bs->bl.max_write_zeroes : INT_MAX;
 
     while (nb_sectors > 0 && !ret) {
         int num = nb_sectors;
@@ -3242,7 +3239,7 @@ static int coroutine_fn bdrv_co_do_write_zeroes(BlockDriverState *bs,
         if (ret == -ENOTSUP) {
             /* Fall back to bounce buffer if write zeroes is unsupported */
             int max_xfer_len = MIN_NON_ZERO(bs->bl.max_transfer_length,
-                                            MAX_WRITE_ZEROES_DEFAULT);
+                                            MAX_WRITE_ZEROES_BOUNCE_BUFFER);
             num = MIN(num, max_xfer_len);
             iov.iov_len = num * BDRV_SECTOR_SIZE;
             if (iov.iov_base == NULL) {
@@ -5097,11 +5094,6 @@ static void coroutine_fn bdrv_discard_co_entry(void *opaque)
     rwco->ret = bdrv_co_discard(rwco->bs, rwco->sector_num, rwco->nb_sectors);
 }
 
-/* if no limit is specified in the BlockLimits use a default
- * of 32768 512-byte sectors (16 MiB) per request.
- */
-#define MAX_DISCARD_DEFAULT 32768
-
 int coroutine_fn bdrv_co_discard(BlockDriverState *bs, int64_t sector_num,
                                  int nb_sectors)
 {
@@ -5126,7 +5118,7 @@ int coroutine_fn bdrv_co_discard(BlockDriverState *bs, int64_t sector_num,
         return 0;
     }
 
-    max_discard = bs->bl.max_discard ?  bs->bl.max_discard : MAX_DISCARD_DEFAULT;
+    max_discard = bs->bl.max_discard ?  bs->bl.max_discard : INT_MAX;
     while (nb_sectors > 0) {
         int ret;
         int num = nb_sectors;
-- 
1.9.1

Re: [Qemu-devel] [PATCH] block: change default for discard and write zeroes to INT_MAX

[Kevin Wolf]

Am 02.02.2015 um 15:48 hat Peter Lieven geschrieben:
> do not trim requests if the driver does not supply a limit
> through BlockLimits. For write zeroes we still keep a limit
> for the unsupported path to avoid allocating a big bounce buffer.
> 
> Suggested-by: Kevin Wolf <kwolf@redhat.com>
> Suggested-by: Denis V. Lunev <den@openvz.org>
> Signed-off-by: Peter Lieven <pl@kamp.de>

Thanks, applied to the block branch (and removed 'block/raw-posix: set
max_write_zeroes to INT_MAX for regular files' from the queue).

Kevin

Re: [Qemu-devel] [PATCH] block: change default for discard and write zeroes to INT_MAX

[Denis V. Lunev]

On 02/02/15 19:13, Kevin Wolf wrote:
> Am 02.02.2015 um 15:48 hat Peter Lieven geschrieben:
>> do not trim requests if the driver does not supply a limit
>> through BlockLimits. For write zeroes we still keep a limit
>> for the unsupported path to avoid allocating a big bounce buffer.
>>
>> Suggested-by: Kevin Wolf <kwolf@redhat.com>
>> Suggested-by: Denis V. Lunev <den@openvz.org>
>> Signed-off-by: Peter Lieven <pl@kamp.de>
> Thanks, applied to the block branch (and removed 'block/raw-posix: set
> max_write_zeroes to INT_MAX for regular files' from the queue).
>
> Kevin
double checked the code.

There are 2 things to patch for discard, write_zeroes is OK for me.
Sorry, for not paying attention for discard branch :(

Den

Re: [Qemu-devel] [PATCH] block: change default for discard and write zeroes to INT_MAX

[Kevin Wolf]

Am 02.02.2015 um 17:25 hat Denis V. Lunev geschrieben:
> On 02/02/15 19:13, Kevin Wolf wrote:
> >Am 02.02.2015 um 15:48 hat Peter Lieven geschrieben:
> >>do not trim requests if the driver does not supply a limit
> >>through BlockLimits. For write zeroes we still keep a limit
> >>for the unsupported path to avoid allocating a big bounce buffer.
> >>
> >>Suggested-by: Kevin Wolf <kwolf@redhat.com>
> >>Suggested-by: Denis V. Lunev <den@openvz.org>
> >>Signed-off-by: Peter Lieven <pl@kamp.de>
> >Thanks, applied to the block branch (and removed 'block/raw-posix: set
> >max_write_zeroes to INT_MAX for regular files' from the queue).
> >
> >Kevin
> double checked the code.
> 
> There are 2 things to patch for discard, write_zeroes is OK for me.
> Sorry, for not paying attention for discard branch :(

Good catch, thanks!

But shouldn't we use the actual limits instead of INT_MAX, i.e. SIZE_MAX
for gluster and UINT32_MAX for nbd?

Kevin

Re: [Qemu-devel] [PATCH] block: change default for discard and write zeroes to INT_MAX

[Denis V. Lunev]

On 02/02/15 19:45, Kevin Wolf wrote:
> Am 02.02.2015 um 17:25 hat Denis V. Lunev geschrieben:
>> On 02/02/15 19:13, Kevin Wolf wrote:
>>> Am 02.02.2015 um 15:48 hat Peter Lieven geschrieben:
>>>> do not trim requests if the driver does not supply a limit
>>>> through BlockLimits. For write zeroes we still keep a limit
>>>> for the unsupported path to avoid allocating a big bounce buffer.
>>>>
>>>> Suggested-by: Kevin Wolf <kwolf@redhat.com>
>>>> Suggested-by: Denis V. Lunev <den@openvz.org>
>>>> Signed-off-by: Peter Lieven <pl@kamp.de>
>>> Thanks, applied to the block branch (and removed 'block/raw-posix: set
>>> max_write_zeroes to INT_MAX for regular files' from the queue).
>>>
>>> Kevin
>> double checked the code.
>>
>> There are 2 things to patch for discard, write_zeroes is OK for me.
>> Sorry, for not paying attention for discard branch :(
> Good catch, thanks!
>
> But shouldn't we use the actual limits instead of INT_MAX, i.e. SIZE_MAX
> for gluster and UINT32_MAX for nbd?
>
> Kevin
You are absolutely correct for NBD case but I do not get the
point about SIZE_MAX for gluster. There is no such definition
in their git at git://github.com/gluster/glusterfs nor in
public API headers in Ubuntu :(

Regards,
     Den

Re: [Qemu-devel] [PATCH] block: change default for discard and write zeroes to INT_MAX

[Paolo Bonzini]

On 02/02/2015 18:00, Denis V. Lunev wrote:
>>
> You are absolutely correct for NBD case but I do not get the
> point about SIZE_MAX for gluster. There is no such definition
> in their git at git://github.com/gluster/glusterfs nor in
> public API headers in Ubuntu :(

SIZE_MAX is defined in stdint.h (provided by the C library).

Paolo

Re: [Qemu-devel] [PATCH] block: change default for discard and write zeroes to INT_MAX

[Denis V. Lunev]

On 02/02/15 20:34, Paolo Bonzini wrote:
>
>
> On 02/02/2015 18:00, Denis V. Lunev wrote:
>>>
>> You are absolutely correct for NBD case but I do not get the
>> point about SIZE_MAX for gluster. There is no such definition
>> in their git at git://github.com/gluster/glusterfs nor in
>> public API headers in Ubuntu :(
>
> SIZE_MAX is defined in stdint.h (provided by the C library).
>
> Paolo
>
In this case the code should avoid overflow on 64bit arch.

OK, re-spinning.

[Qemu-devel] [PATCH 1/2] glusterfs: fix max_discard

[Denis V. Lunev]

qemu_gluster_co_discard calculates size to discard as follows
    size_t size = nb_sectors * BDRV_SECTOR_SIZE;
    ret = glfs_discard_async(s->fd, offset, size, &gluster_finish_aiocb, acb);

glfs_discard_async is declared as follows:
  int glfs_discard_async (glfs_fd_t *fd, off_t length, size_t lent,
                          glfs_io_cbk fn, void *data) __THROW
This is problematic on i686 as sizeof(size_t) == 4.

Set bl_max_discard to INT_MAX >> BDRV_SECTOR_BITS to avoid overflow.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Peter Lieven <pl@kamp.de>
---
 block/gluster.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/block/gluster.c b/block/gluster.c
index 1eb3a8c..ed09691 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -622,6 +622,11 @@ out:
     return ret;
 }
 
+static void qemu_gluster_refresh_limits(BlockDriverState *bs, Error **errp)
+{
+    bs->bl.max_discard = INT_MAX >> BDRV_SECTOR_BITS;
+}
+
 #ifdef CONFIG_GLUSTERFS_DISCARD
 static coroutine_fn int qemu_gluster_co_discard(BlockDriverState *bs,
         int64_t sector_num, int nb_sectors)
@@ -735,6 +740,7 @@ static BlockDriver bdrv_gluster = {
 #ifdef CONFIG_GLUSTERFS_DISCARD
     .bdrv_co_discard              = qemu_gluster_co_discard,
 #endif
+    .bdrv_refresh_limits          = qemu_gluster_refresh_limits,
 #ifdef CONFIG_GLUSTERFS_ZEROFILL
     .bdrv_co_write_zeroes         = qemu_gluster_co_write_zeroes,
 #endif
@@ -762,6 +768,7 @@ static BlockDriver bdrv_gluster_tcp = {
 #ifdef CONFIG_GLUSTERFS_DISCARD
     .bdrv_co_discard              = qemu_gluster_co_discard,
 #endif
+    .bdrv_refresh_limits          = qemu_gluster_refresh_limits,
 #ifdef CONFIG_GLUSTERFS_ZEROFILL
     .bdrv_co_write_zeroes         = qemu_gluster_co_write_zeroes,
 #endif
@@ -789,6 +796,7 @@ static BlockDriver bdrv_gluster_unix = {
 #ifdef CONFIG_GLUSTERFS_DISCARD
     .bdrv_co_discard              = qemu_gluster_co_discard,
 #endif
+    .bdrv_refresh_limits          = qemu_gluster_refresh_limits,
 #ifdef CONFIG_GLUSTERFS_ZEROFILL
     .bdrv_co_write_zeroes         = qemu_gluster_co_write_zeroes,
 #endif
@@ -816,6 +824,7 @@ static BlockDriver bdrv_gluster_rdma = {
 #ifdef CONFIG_GLUSTERFS_DISCARD
     .bdrv_co_discard              = qemu_gluster_co_discard,
 #endif
+    .bdrv_refresh_limits          = qemu_gluster_refresh_limits,
 #ifdef CONFIG_GLUSTERFS_ZEROFILL
     .bdrv_co_write_zeroes         = qemu_gluster_co_write_zeroes,
 #endif
-- 
1.9.1

[Qemu-devel] [PATCH 2/2] nbd: fix max_discard

[Denis V. Lunev]

nbd_co_discard calls nbd_client_session_co_discard which uses uint32_t
as the length in bytes of the data to discard due to the following
definition:

struct nbd_request {
    uint32_t magic;
    uint32_t type;
    uint64_t handle;
    uint64_t from;
    uint32_t len; <-- the length of data to be discarded, in bytes
} QEMU_PACKED;

Thus we should limit bl_max_discard to INT_MAX >> BDRV_SECTOR_BITS to avoid
overflow.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Peter Lieven <pl@kamp.de>
---
 block/nbd.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/block/nbd.c b/block/nbd.c
index 04cc845..008d52e 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -301,6 +301,11 @@ static int nbd_co_flush(BlockDriverState *bs)
     return nbd_client_session_co_flush(&s->client);
 }
 
+static void nbd_refresh_limits(BlockDriverState *bs, Error **errp)
+{
+    bs->bl.max_discard = INT_MAX >> BDRV_SECTOR_BITS;
+}
+
 static int nbd_co_discard(BlockDriverState *bs, int64_t sector_num,
                           int nb_sectors)
 {
@@ -396,6 +401,7 @@ static BlockDriver bdrv_nbd = {
     .bdrv_close                 = nbd_close,
     .bdrv_co_flush_to_os        = nbd_co_flush,
     .bdrv_co_discard            = nbd_co_discard,
+    .bdrv_refresh_limits        = nbd_refresh_limits,
     .bdrv_getlength             = nbd_getlength,
     .bdrv_detach_aio_context    = nbd_detach_aio_context,
     .bdrv_attach_aio_context    = nbd_attach_aio_context,
@@ -413,6 +419,7 @@ static BlockDriver bdrv_nbd_tcp = {
     .bdrv_close                 = nbd_close,
     .bdrv_co_flush_to_os        = nbd_co_flush,
     .bdrv_co_discard            = nbd_co_discard,
+    .bdrv_refresh_limits        = nbd_refresh_limits,
     .bdrv_getlength             = nbd_getlength,
     .bdrv_detach_aio_context    = nbd_detach_aio_context,
     .bdrv_attach_aio_context    = nbd_attach_aio_context,
@@ -430,6 +437,7 @@ static BlockDriver bdrv_nbd_unix = {
     .bdrv_close                 = nbd_close,
     .bdrv_co_flush_to_os        = nbd_co_flush,
     .bdrv_co_discard            = nbd_co_discard,
+    .bdrv_refresh_limits        = nbd_refresh_limits,
     .bdrv_getlength             = nbd_getlength,
     .bdrv_detach_aio_context    = nbd_detach_aio_context,
     .bdrv_attach_aio_context    = nbd_attach_aio_context,
-- 
1.9.1