* [Qemu-devel] [PATCH] block: change default for discard and write zeroes to INT_MAX
@ 2015-02-02 14:48 Peter Lieven
2015-02-02 16:13 ` Kevin Wolf
2015-02-02 16:23 ` [Qemu-devel] [PATCH 1/2] glusterfs: fix max_discard Denis V. Lunev
0 siblings, 2 replies; 9+ messages in thread
From: Peter Lieven @ 2015-02-02 14:48 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, den, 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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] block: change default for discard and write zeroes to INT_MAX
2015-02-02 14:48 [Qemu-devel] [PATCH] block: change default for discard and write zeroes to INT_MAX Peter Lieven
@ 2015-02-02 16:13 ` Kevin Wolf
2015-02-02 16:25 ` Denis V. Lunev
2015-02-02 16:23 ` [Qemu-devel] [PATCH 1/2] glusterfs: fix max_discard Denis V. Lunev
1 sibling, 1 reply; 9+ messages in thread
From: Kevin Wolf @ 2015-02-02 16:13 UTC (permalink / raw)
To: Peter Lieven; +Cc: den, qemu-devel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] block: change default for discard and write zeroes to INT_MAX
2015-02-02 16:13 ` Kevin Wolf
@ 2015-02-02 16:25 ` Denis V. Lunev
2015-02-02 16:45 ` Kevin Wolf
0 siblings, 1 reply; 9+ messages in thread
From: Denis V. Lunev @ 2015-02-02 16:25 UTC (permalink / raw)
To: Kevin Wolf, Peter Lieven; +Cc: qemu-devel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] block: change default for discard and write zeroes to INT_MAX
2015-02-02 16:25 ` Denis V. Lunev
@ 2015-02-02 16:45 ` Kevin Wolf
2015-02-02 17:00 ` Denis V. Lunev
0 siblings, 1 reply; 9+ messages in thread
From: Kevin Wolf @ 2015-02-02 16:45 UTC (permalink / raw)
To: Denis V. Lunev; +Cc: Peter Lieven, qemu-devel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] block: change default for discard and write zeroes to INT_MAX
2015-02-02 16:45 ` Kevin Wolf
@ 2015-02-02 17:00 ` Denis V. Lunev
2015-02-02 17:34 ` Paolo Bonzini
0 siblings, 1 reply; 9+ messages in thread
From: Denis V. Lunev @ 2015-02-02 17:00 UTC (permalink / raw)
To: Kevin Wolf; +Cc: Peter Lieven, qemu-devel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] block: change default for discard and write zeroes to INT_MAX
2015-02-02 17:00 ` Denis V. Lunev
@ 2015-02-02 17:34 ` Paolo Bonzini
2015-02-02 17:40 ` Denis V. Lunev
0 siblings, 1 reply; 9+ messages in thread
From: Paolo Bonzini @ 2015-02-02 17:34 UTC (permalink / raw)
To: Denis V. Lunev, Kevin Wolf; +Cc: Peter Lieven, qemu-devel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] block: change default for discard and write zeroes to INT_MAX
2015-02-02 17:34 ` Paolo Bonzini
@ 2015-02-02 17:40 ` Denis V. Lunev
0 siblings, 0 replies; 9+ messages in thread
From: Denis V. Lunev @ 2015-02-02 17:40 UTC (permalink / raw)
To: Paolo Bonzini, Denis V. Lunev, Kevin Wolf; +Cc: Peter Lieven, qemu-devel
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* [Qemu-devel] [PATCH 1/2] glusterfs: fix max_discard
2015-02-02 14:48 [Qemu-devel] [PATCH] block: change default for discard and write zeroes to INT_MAX Peter Lieven
2015-02-02 16:13 ` Kevin Wolf
@ 2015-02-02 16:23 ` Denis V. Lunev
2015-02-02 16:23 ` [Qemu-devel] [PATCH 2/2] nbd: " Denis V. Lunev
1 sibling, 1 reply; 9+ messages in thread
From: Denis V. Lunev @ 2015-02-02 16:23 UTC (permalink / raw)
Cc: Kevin Wolf, Denis V. Lunev, Peter Lieven, qemu-devel
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [Qemu-devel] [PATCH 2/2] nbd: fix max_discard
2015-02-02 16:23 ` [Qemu-devel] [PATCH 1/2] glusterfs: fix max_discard Denis V. Lunev
@ 2015-02-02 16:23 ` Denis V. Lunev
0 siblings, 0 replies; 9+ messages in thread
From: Denis V. Lunev @ 2015-02-02 16:23 UTC (permalink / raw)
Cc: Kevin Wolf, Denis V. Lunev, Peter Lieven, qemu-devel
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2015-02-02 17:41 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-02-02 14:48 [Qemu-devel] [PATCH] block: change default for discard and write zeroes to INT_MAX Peter Lieven
2015-02-02 16:13 ` Kevin Wolf
2015-02-02 16:25 ` Denis V. Lunev
2015-02-02 16:45 ` Kevin Wolf
2015-02-02 17:00 ` Denis V. Lunev
2015-02-02 17:34 ` Paolo Bonzini
2015-02-02 17:40 ` Denis V. Lunev
2015-02-02 16:23 ` [Qemu-devel] [PATCH 1/2] glusterfs: fix max_discard Denis V. Lunev
2015-02-02 16:23 ` [Qemu-devel] [PATCH 2/2] nbd: " Denis V. Lunev
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.