Re: [Qemu-block] [PATCH v2 7/8] file-posix: account discard operations

[Alberto Garcia]

On Mon 12 Feb 2018 05:19:57 PM CET, Anton Nefedov wrote:
>>> @@ -158,6 +158,11 @@ typedef struct BDRVRawState {
>>>   bool page_cache_inconsistent:1;
>>>   bool has_fallocate;
>>>   bool needs_alignment;
>>> +struct {
>>> +int64_t discard_nb_ok;
>>> +int64_t discard_nb_failed;
>>> +int64_t discard_bytes_ok;
>>> +} stats;
>> 
>> Shouldn't this new structure be defined in a header file so other
>> drivers can use it? Or did you define it here because you don't see that
>> happening soon?
>> 
>
> I guess there's no reason to burden the common header files as long as
> it's not really used anywhere else.

Fair enough,

Reviewed-by: Alberto Garcia 

Berto

Re: [Qemu-block] [PATCH v2 7/8] file-posix: account discard operations

[Anton Nefedov]

On 7/2/2018 6:10 PM, Alberto Garcia wrote:

On Fri 19 Jan 2018 01:50:06 PM CET, Anton Nefedov wrote:

This will help to identify how many of the user-issued discard operations
(accounted on a device level) have actually suceeded down on the host file
(even though the numbers will not be exactly the same if non-raw format
driver is used (e.g. qcow2 sending metadata discards)).

Signed-off-by: Anton Nefedov 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---
  block/file-posix.c | 21 +++--
  1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index 36ee89e..544ae58 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -158,6 +158,11 @@ typedef struct BDRVRawState {
  bool page_cache_inconsistent:1;
  bool has_fallocate;
  bool needs_alignment;
+struct {
+int64_t discard_nb_ok;
+int64_t discard_nb_failed;
+int64_t discard_bytes_ok;
+} stats;


Shouldn't this new structure be defined in a header file so other
drivers can use it? Or did you define it here because you don't see that
happening soon?



I guess there's no reason to burden the common header files as long as
it's not really used anywhere else.

Re: [Qemu-block] [PATCH v2 7/8] file-posix: account discard operations

[Alberto Garcia]

On Fri 19 Jan 2018 01:50:06 PM CET, Anton Nefedov wrote:
> This will help to identify how many of the user-issued discard operations
> (accounted on a device level) have actually suceeded down on the host file
> (even though the numbers will not be exactly the same if non-raw format
> driver is used (e.g. qcow2 sending metadata discards)).
>
> Signed-off-by: Anton Nefedov 
> Reviewed-by: Vladimir Sementsov-Ogievskiy 
> ---
>  block/file-posix.c | 21 +++--
>  1 file changed, 19 insertions(+), 2 deletions(-)
>
> diff --git a/block/file-posix.c b/block/file-posix.c
> index 36ee89e..544ae58 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -158,6 +158,11 @@ typedef struct BDRVRawState {
>  bool page_cache_inconsistent:1;
>  bool has_fallocate;
>  bool needs_alignment;
> +struct {
> +int64_t discard_nb_ok;
> +int64_t discard_nb_failed;
> +int64_t discard_bytes_ok;
> +} stats;

Shouldn't this new structure be defined in a header file so other
drivers can use it? Or did you define it here because you don't see that
happening soon?

The rest of the patch looks good.

Berto

[Qemu-block] [PATCH v2 7/8] file-posix: account discard operations

[Anton Nefedov]

This will help to identify how many of the user-issued discard operations
(accounted on a device level) have actually suceeded down on the host file
(even though the numbers will not be exactly the same if non-raw format
driver is used (e.g. qcow2 sending metadata discards)).

Signed-off-by: Anton Nefedov 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---
 block/file-posix.c | 21 +++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index 36ee89e..544ae58 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -158,6 +158,11 @@ typedef struct BDRVRawState {
 bool page_cache_inconsistent:1;
 bool has_fallocate;
 bool needs_alignment;
+struct {
+int64_t discard_nb_ok;
+int64_t discard_nb_failed;
+int64_t discard_bytes_ok;
+} stats;
 
 PRManager *pr_mgr;
 } BDRVRawState;
@@ -1458,6 +1463,16 @@ static ssize_t handle_aiocb_discard(RawPosixAIOData 
*aiocb)
 return ret;
 }
 
+static void raw_account_discard(BDRVRawState *s, uint64_t nbytes, int ret)
+{
+if (ret) {
+s->stats.discard_nb_failed++;
+} else {
+s->stats.discard_nb_ok++;
+s->stats.discard_bytes_ok += nbytes;
+}
+}
+
 static int aio_worker(void *arg)
 {
 RawPosixAIOData *aiocb = arg;
@@ -1494,6 +1509,7 @@ static int aio_worker(void *arg)
 break;
 case QEMU_AIO_DISCARD:
 ret = handle_aiocb_discard(aiocb);
+raw_account_discard(aiocb->bs->opaque, aiocb->aio_nbytes, ret);
 break;
 case QEMU_AIO_WRITE_ZEROES:
 ret = handle_aiocb_write_zeroes(aiocb);
@@ -2654,8 +2670,9 @@ static coroutine_fn BlockAIOCB 
*hdev_aio_pdiscard(BlockDriverState *bs,
 BlockCompletionFunc *cb, void *opaque)
 {
 BDRVRawState *s = bs->opaque;
-
-if (fd_open(bs) < 0) {
+int ret = fd_open(bs);
+if (ret < 0) {
+raw_account_discard(s, bytes, ret);
 return NULL;
 }
 return paio_submit(bs, s->fd, offset, NULL, bytes,
-- 
2.7.4