qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/7] virtiofsd: Few cleanups in virtio_send_data_iov()
@ 2021-05-11 21:37 Vivek Goyal
  2021-05-11 21:37 ` [PATCH 1/7] virtiofsd: Check for EINTR in preadv() and retry Vivek Goyal
                   ` (8 more replies)
  0 siblings, 9 replies; 21+ messages in thread
From: Vivek Goyal @ 2021-05-11 21:37 UTC (permalink / raw)
  To: qemu-devel, virtio-fs; +Cc: dgilbert, vgoyal

Hi,

Code in virtio_send_data_iov() little twisted and complicated. This
patch series just tries to simplify it a bit to make it little easier
to read this piece of code.

Thanks
Vivek

Vivek Goyal (7):
  virtiofsd: Check for EINTR in preadv() and retry
  virtiofsd: Get rid of unreachable code in read
  virtiofsd: Use iov_discard_front() to skip bytes
  virtiofsd: get rid of in_sg_left variable
  virtiofsd: Simplify skip byte logic
  virtiofsd: Check EOF before short read
  virtiofsd: Set req->reply_sent right after sending reply

 tools/virtiofsd/fuse_virtio.c | 67 +++++++++++------------------------
 1 file changed, 21 insertions(+), 46 deletions(-)

-- 
2.25.4



^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH 1/7] virtiofsd: Check for EINTR in preadv() and retry
  2021-05-11 21:37 [PATCH 0/7] virtiofsd: Few cleanups in virtio_send_data_iov() Vivek Goyal
@ 2021-05-11 21:37 ` Vivek Goyal
  2021-05-18 11:59   ` Dr. David Alan Gilbert
  2021-05-11 21:37 ` [PATCH 2/7] virtiofsd: Get rid of unreachable code in read Vivek Goyal
                   ` (7 subsequent siblings)
  8 siblings, 1 reply; 21+ messages in thread
From: Vivek Goyal @ 2021-05-11 21:37 UTC (permalink / raw)
  To: qemu-devel, virtio-fs; +Cc: dgilbert, vgoyal

We don't seem to check for EINTR and retry. There are other places
in code where we check for EINTR. So lets add a check.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 tools/virtiofsd/fuse_virtio.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/virtiofsd/fuse_virtio.c b/tools/virtiofsd/fuse_virtio.c
index 1170f375a5..32914f7e95 100644
--- a/tools/virtiofsd/fuse_virtio.c
+++ b/tools/virtiofsd/fuse_virtio.c
@@ -421,6 +421,9 @@ int virtio_send_data_iov(struct fuse_session *se, struct fuse_chan *ch,
 
         if (ret == -1) {
             ret = errno;
+            if (ret == EINTR) {
+                continue;
+            }
             fuse_log(FUSE_LOG_DEBUG, "%s: preadv failed (%m) len=%zd\n",
                      __func__, len);
             goto err;
-- 
2.25.4



^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 2/7] virtiofsd: Get rid of unreachable code in read
  2021-05-11 21:37 [PATCH 0/7] virtiofsd: Few cleanups in virtio_send_data_iov() Vivek Goyal
  2021-05-11 21:37 ` [PATCH 1/7] virtiofsd: Check for EINTR in preadv() and retry Vivek Goyal
@ 2021-05-11 21:37 ` Vivek Goyal
  2021-05-18 12:01   ` Dr. David Alan Gilbert
  2021-05-11 21:37 ` [PATCH 3/7] virtiofsd: Use iov_discard_front() to skip bytes Vivek Goyal
                   ` (6 subsequent siblings)
  8 siblings, 1 reply; 21+ messages in thread
From: Vivek Goyal @ 2021-05-11 21:37 UTC (permalink / raw)
  To: qemu-devel, virtio-fs; +Cc: dgilbert, vgoyal

pvreadv() can return following.

- error
- 0 in case of EOF
- short read

We seem to handle all the cases already. We are retrying read in case
of short read. So another check for short read seems like dead code.
Get rid of it.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 tools/virtiofsd/fuse_virtio.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/tools/virtiofsd/fuse_virtio.c b/tools/virtiofsd/fuse_virtio.c
index 32914f7e95..5dcd08fccb 100644
--- a/tools/virtiofsd/fuse_virtio.c
+++ b/tools/virtiofsd/fuse_virtio.c
@@ -446,11 +446,6 @@ int virtio_send_data_iov(struct fuse_session *se, struct fuse_chan *ch,
                      in_sg_left);
             break;
         }
-        if (ret != len) {
-            fuse_log(FUSE_LOG_DEBUG, "%s: ret!=len\n", __func__);
-            ret = EIO;
-            goto err;
-        }
         in_sg_left -= ret;
         len -= ret;
     } while (in_sg_left);
-- 
2.25.4



^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 3/7] virtiofsd: Use iov_discard_front() to skip bytes
  2021-05-11 21:37 [PATCH 0/7] virtiofsd: Few cleanups in virtio_send_data_iov() Vivek Goyal
  2021-05-11 21:37 ` [PATCH 1/7] virtiofsd: Check for EINTR in preadv() and retry Vivek Goyal
  2021-05-11 21:37 ` [PATCH 2/7] virtiofsd: Get rid of unreachable code in read Vivek Goyal
@ 2021-05-11 21:37 ` Vivek Goyal
  2021-05-18 12:10   ` Dr. David Alan Gilbert
  2021-05-11 21:37 ` [PATCH 4/7] virtiofsd: get rid of in_sg_left variable Vivek Goyal
                   ` (5 subsequent siblings)
  8 siblings, 1 reply; 21+ messages in thread
From: Vivek Goyal @ 2021-05-11 21:37 UTC (permalink / raw)
  To: qemu-devel, virtio-fs; +Cc: dgilbert, vgoyal

There are places where we need to skip few bytes from front of the iovec
array. We have our own custom code for that. Looks like iov_discard_front()
can do same thing. So use that helper instead.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 tools/virtiofsd/fuse_virtio.c | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/tools/virtiofsd/fuse_virtio.c b/tools/virtiofsd/fuse_virtio.c
index 5dcd08fccb..d56b225800 100644
--- a/tools/virtiofsd/fuse_virtio.c
+++ b/tools/virtiofsd/fuse_virtio.c
@@ -389,23 +389,15 @@ int virtio_send_data_iov(struct fuse_session *se, struct fuse_chan *ch,
     memcpy(in_sg_cpy, in_sg, sizeof(struct iovec) * in_num);
     /* These get updated as we skip */
     struct iovec *in_sg_ptr = in_sg_cpy;
-    int in_sg_cpy_count = in_num;
+    unsigned int in_sg_cpy_count = in_num;
 
     /* skip over parts of in_sg that contained the header iov */
     size_t skip_size = iov_len;
 
     size_t in_sg_left = 0;
     do {
-        while (skip_size != 0 && in_sg_cpy_count) {
-            if (skip_size >= in_sg_ptr[0].iov_len) {
-                skip_size -= in_sg_ptr[0].iov_len;
-                in_sg_ptr++;
-                in_sg_cpy_count--;
-            } else {
-                in_sg_ptr[0].iov_len -= skip_size;
-                in_sg_ptr[0].iov_base += skip_size;
-                break;
-            }
+        if (skip_size != 0) {
+	    iov_discard_front(&in_sg_ptr, &in_sg_cpy_count, skip_size);
         }
 
         int i;
-- 
2.25.4



^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 4/7] virtiofsd: get rid of in_sg_left variable
  2021-05-11 21:37 [PATCH 0/7] virtiofsd: Few cleanups in virtio_send_data_iov() Vivek Goyal
                   ` (2 preceding siblings ...)
  2021-05-11 21:37 ` [PATCH 3/7] virtiofsd: Use iov_discard_front() to skip bytes Vivek Goyal
@ 2021-05-11 21:37 ` Vivek Goyal
  2021-05-18 12:19   ` Dr. David Alan Gilbert
  2021-05-11 21:37 ` [PATCH 5/7] virtiofsd: Simplify skip byte logic Vivek Goyal
                   ` (4 subsequent siblings)
  8 siblings, 1 reply; 21+ messages in thread
From: Vivek Goyal @ 2021-05-11 21:37 UTC (permalink / raw)
  To: qemu-devel, virtio-fs; +Cc: dgilbert, vgoyal

in_sg_left seems to be being used primarly for debugging purpose. It is
keeping track of how many bytes are left in the scatter list we are
reading into.

We already have another variable "len" which keeps track how many bytes
are left to be read. And in_sg_left is greater than or equal to len. We
have already ensured that in the beginning of function.

    if (in_len < tosend_len) {
        fuse_log(FUSE_LOG_ERR, "%s: elem %d too small for data len %zd\n",
                 __func__, elem->index, tosend_len);
        ret = E2BIG;
        goto err;
    }

So in_sg_left seems like a redundant variable. It probably was useful for
debugging when code was being developed. Get rid of it. It helps simplify
this function.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 tools/virtiofsd/fuse_virtio.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/tools/virtiofsd/fuse_virtio.c b/tools/virtiofsd/fuse_virtio.c
index d56b225800..ccad2b3f8a 100644
--- a/tools/virtiofsd/fuse_virtio.c
+++ b/tools/virtiofsd/fuse_virtio.c
@@ -394,20 +394,16 @@ int virtio_send_data_iov(struct fuse_session *se, struct fuse_chan *ch,
     /* skip over parts of in_sg that contained the header iov */
     size_t skip_size = iov_len;
 
-    size_t in_sg_left = 0;
     do {
         if (skip_size != 0) {
 	    iov_discard_front(&in_sg_ptr, &in_sg_cpy_count, skip_size);
         }
 
-        int i;
-        for (i = 0, in_sg_left = 0; i < in_sg_cpy_count; i++) {
-            in_sg_left += in_sg_ptr[i].iov_len;
-        }
         fuse_log(FUSE_LOG_DEBUG,
                  "%s: after skip skip_size=%zd in_sg_cpy_count=%d "
-                 "in_sg_left=%zd\n",
-                 __func__, skip_size, in_sg_cpy_count, in_sg_left);
+                 "len remaining=%zd\n", __func__, skip_size, in_sg_cpy_count,
+                 len);
+
         ret = preadv(buf->buf[0].fd, in_sg_ptr, in_sg_cpy_count,
                      buf->buf[0].pos);
 
@@ -434,13 +430,12 @@ int virtio_send_data_iov(struct fuse_session *se, struct fuse_chan *ch,
         }
         if (!ret) {
             /* EOF case? */
-            fuse_log(FUSE_LOG_DEBUG, "%s: !ret in_sg_left=%zd\n", __func__,
-                     in_sg_left);
+            fuse_log(FUSE_LOG_DEBUG, "%s: !ret len remaining=%zd\n", __func__,
+                     len);
             break;
         }
-        in_sg_left -= ret;
         len -= ret;
-    } while (in_sg_left);
+    } while (len);
 
     /* Need to fix out->len on EOF */
     if (len) {
-- 
2.25.4



^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 5/7] virtiofsd: Simplify skip byte logic
  2021-05-11 21:37 [PATCH 0/7] virtiofsd: Few cleanups in virtio_send_data_iov() Vivek Goyal
                   ` (3 preceding siblings ...)
  2021-05-11 21:37 ` [PATCH 4/7] virtiofsd: get rid of in_sg_left variable Vivek Goyal
@ 2021-05-11 21:37 ` Vivek Goyal
  2021-05-18 12:26   ` Dr. David Alan Gilbert
  2021-05-11 21:37 ` [PATCH 6/7] virtiofsd: Check EOF before short read Vivek Goyal
                   ` (3 subsequent siblings)
  8 siblings, 1 reply; 21+ messages in thread
From: Vivek Goyal @ 2021-05-11 21:37 UTC (permalink / raw)
  To: qemu-devel, virtio-fs; +Cc: dgilbert, vgoyal

We need to skip bytes in two cases.

a. Before we start reading into in_sg, we need to skip iov_len bytes
   in the beginning which typically will have fuse_out_header.

b. If preadv() does a short read, then we need to retry preadv() with
   remainig bytes and skip the bytes preadv() read in short read.

For case a, there is no reason that skipping logic be inside the while
loop. Move it outside. And only retain logic "b" inside while loop.

Also get rid of variable "skip_size". Looks like we can do without it.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 tools/virtiofsd/fuse_virtio.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/tools/virtiofsd/fuse_virtio.c b/tools/virtiofsd/fuse_virtio.c
index ccad2b3f8a..434fe401cf 100644
--- a/tools/virtiofsd/fuse_virtio.c
+++ b/tools/virtiofsd/fuse_virtio.c
@@ -392,17 +392,11 @@ int virtio_send_data_iov(struct fuse_session *se, struct fuse_chan *ch,
     unsigned int in_sg_cpy_count = in_num;
 
     /* skip over parts of in_sg that contained the header iov */
-    size_t skip_size = iov_len;
+    iov_discard_front(&in_sg_ptr, &in_sg_cpy_count, iov_len);
 
     do {
-        if (skip_size != 0) {
-	    iov_discard_front(&in_sg_ptr, &in_sg_cpy_count, skip_size);
-        }
-
-        fuse_log(FUSE_LOG_DEBUG,
-                 "%s: after skip skip_size=%zd in_sg_cpy_count=%d "
-                 "len remaining=%zd\n", __func__, skip_size, in_sg_cpy_count,
-                 len);
+        fuse_log(FUSE_LOG_DEBUG, "%s: in_sg_cpy_count=%d len remaining=%zd\n",
+                 __func__, in_sg_cpy_count, len);
 
         ret = preadv(buf->buf[0].fd, in_sg_ptr, in_sg_cpy_count,
                      buf->buf[0].pos);
@@ -421,7 +415,7 @@ int virtio_send_data_iov(struct fuse_session *se, struct fuse_chan *ch,
         if (ret < len && ret) {
             fuse_log(FUSE_LOG_DEBUG, "%s: ret < len\n", __func__);
             /* Skip over this much next time around */
-            skip_size = ret;
+            iov_discard_front(&in_sg_ptr, &in_sg_cpy_count, ret);
             buf->buf[0].pos += ret;
             len -= ret;
 
-- 
2.25.4



^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 6/7] virtiofsd: Check EOF before short read
  2021-05-11 21:37 [PATCH 0/7] virtiofsd: Few cleanups in virtio_send_data_iov() Vivek Goyal
                   ` (4 preceding siblings ...)
  2021-05-11 21:37 ` [PATCH 5/7] virtiofsd: Simplify skip byte logic Vivek Goyal
@ 2021-05-11 21:37 ` Vivek Goyal
  2021-05-18 12:31   ` Dr. David Alan Gilbert
  2021-05-11 21:37 ` [PATCH 7/7] virtiofsd: Set req->reply_sent right after sending reply Vivek Goyal
                   ` (2 subsequent siblings)
  8 siblings, 1 reply; 21+ messages in thread
From: Vivek Goyal @ 2021-05-11 21:37 UTC (permalink / raw)
  To: qemu-devel, virtio-fs; +Cc: dgilbert, vgoyal

In virtio_send_data_iov() we are checking first for short read and then
EOF condition. Change the order. Basically check for error and EOF first
and last remaining piece is short ready which will lead to retry
automatically at the end of while loop.

Just that it is little simpler to read to the code. There is no need
to call "continue" and also one less call of "len-=ret".

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 tools/virtiofsd/fuse_virtio.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/tools/virtiofsd/fuse_virtio.c b/tools/virtiofsd/fuse_virtio.c
index 434fe401cf..aa53808ef9 100644
--- a/tools/virtiofsd/fuse_virtio.c
+++ b/tools/virtiofsd/fuse_virtio.c
@@ -410,25 +410,24 @@ int virtio_send_data_iov(struct fuse_session *se, struct fuse_chan *ch,
                      __func__, len);
             goto err;
         }
-        fuse_log(FUSE_LOG_DEBUG, "%s: preadv ret=%d len=%zd\n", __func__,
-                 ret, len);
-        if (ret < len && ret) {
-            fuse_log(FUSE_LOG_DEBUG, "%s: ret < len\n", __func__);
-            /* Skip over this much next time around */
-            iov_discard_front(&in_sg_ptr, &in_sg_cpy_count, ret);
-            buf->buf[0].pos += ret;
-            len -= ret;
 
-            /* Lets do another read */
-            continue;
-        }
         if (!ret) {
             /* EOF case? */
             fuse_log(FUSE_LOG_DEBUG, "%s: !ret len remaining=%zd\n", __func__,
                      len);
             break;
         }
+        fuse_log(FUSE_LOG_DEBUG, "%s: preadv ret=%d len=%zd\n", __func__,
+                 ret, len);
+
         len -= ret;
+        /* Short read. Retry reading remaining bytes */
+        if (len) {
+            fuse_log(FUSE_LOG_DEBUG, "%s: ret < len\n", __func__);
+            /* Skip over this much next time around */
+            iov_discard_front(&in_sg_ptr, &in_sg_cpy_count, ret);
+            buf->buf[0].pos += ret;
+        }
     } while (len);
 
     /* Need to fix out->len on EOF */
-- 
2.25.4



^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 7/7] virtiofsd: Set req->reply_sent right after sending reply
  2021-05-11 21:37 [PATCH 0/7] virtiofsd: Few cleanups in virtio_send_data_iov() Vivek Goyal
                   ` (5 preceding siblings ...)
  2021-05-11 21:37 ` [PATCH 6/7] virtiofsd: Check EOF before short read Vivek Goyal
@ 2021-05-11 21:37 ` Vivek Goyal
  2021-05-13 20:50   ` [Virtio-fs] " Connor Kuehl
  2021-05-18 12:32   ` Dr. David Alan Gilbert
  2021-05-11 21:48 ` [PATCH 0/7] virtiofsd: Few cleanups in virtio_send_data_iov() no-reply
  2021-05-13 20:50 ` [Virtio-fs] " Connor Kuehl
  8 siblings, 2 replies; 21+ messages in thread
From: Vivek Goyal @ 2021-05-11 21:37 UTC (permalink / raw)
  To: qemu-devel, virtio-fs; +Cc: dgilbert, vgoyal

There is no reason to set it in label "err". We should be able to set
it right after sending reply. It is easier to read.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 tools/virtiofsd/fuse_virtio.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/tools/virtiofsd/fuse_virtio.c b/tools/virtiofsd/fuse_virtio.c
index aa53808ef9..b1767dd5b9 100644
--- a/tools/virtiofsd/fuse_virtio.c
+++ b/tools/virtiofsd/fuse_virtio.c
@@ -446,12 +446,9 @@ int virtio_send_data_iov(struct fuse_session *se, struct fuse_chan *ch,
     vu_queue_notify(dev, q);
     pthread_mutex_unlock(&qi->vq_lock);
     vu_dispatch_unlock(qi->virtio_dev);
+    req->reply_sent = true;
 
 err:
-    if (ret == 0) {
-        req->reply_sent = true;
-    }
-
     return ret;
 }
 
-- 
2.25.4



^ permalink raw reply related	[flat|nested] 21+ messages in thread

* Re: [PATCH 0/7] virtiofsd: Few cleanups in virtio_send_data_iov()
  2021-05-11 21:37 [PATCH 0/7] virtiofsd: Few cleanups in virtio_send_data_iov() Vivek Goyal
                   ` (6 preceding siblings ...)
  2021-05-11 21:37 ` [PATCH 7/7] virtiofsd: Set req->reply_sent right after sending reply Vivek Goyal
@ 2021-05-11 21:48 ` no-reply
  2021-05-13 20:50 ` [Virtio-fs] " Connor Kuehl
  8 siblings, 0 replies; 21+ messages in thread
From: no-reply @ 2021-05-11 21:48 UTC (permalink / raw)
  To: vgoyal; +Cc: virtio-fs, qemu-devel, vgoyal, dgilbert

Patchew URL: https://patchew.org/QEMU/20210511213736.281016-1-vgoyal@redhat.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20210511213736.281016-1-vgoyal@redhat.com
Subject: [PATCH 0/7] virtiofsd: Few cleanups in virtio_send_data_iov()

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 - [tag update]      patchew/20210511132641.1022161-1-berrange@redhat.com -> patchew/20210511132641.1022161-1-berrange@redhat.com
 * [new tag]         patchew/20210511213736.281016-1-vgoyal@redhat.com -> patchew/20210511213736.281016-1-vgoyal@redhat.com
Switched to a new branch 'test'
b6ced9e virtiofsd: Set req->reply_sent right after sending reply
c866cc2 virtiofsd: Check EOF before short read
8d45f07 virtiofsd: Simplify skip byte logic
b7afa17 virtiofsd: get rid of in_sg_left variable
37ba755 virtiofsd: Use iov_discard_front() to skip bytes
f87fbd3 virtiofsd: Get rid of unreachable code in read
cc6fb4a virtiofsd: Check for EINTR in preadv() and retry

=== OUTPUT BEGIN ===
1/7 Checking commit cc6fb4a9cf99 (virtiofsd: Check for EINTR in preadv() and retry)
2/7 Checking commit f87fbd364bcd (virtiofsd: Get rid of unreachable code in read)
3/7 Checking commit 37ba755df99d (virtiofsd: Use iov_discard_front() to skip bytes)
ERROR: code indent should never use tabs
#44: FILE: tools/virtiofsd/fuse_virtio.c:400:
+^I    iov_discard_front(&in_sg_ptr, &in_sg_cpy_count, skip_size);$

total: 1 errors, 0 warnings, 26 lines checked

Patch 3/7 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

4/7 Checking commit b7afa17acdc2 (virtiofsd: get rid of in_sg_left variable)
5/7 Checking commit 8d45f07e917d (virtiofsd: Simplify skip byte logic)
6/7 Checking commit c866cc21d91c (virtiofsd: Check EOF before short read)
7/7 Checking commit b6ced9e49587 (virtiofsd: Set req->reply_sent right after sending reply)
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20210511213736.281016-1-vgoyal@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Virtio-fs] [PATCH 7/7] virtiofsd: Set req->reply_sent right after sending reply
  2021-05-11 21:37 ` [PATCH 7/7] virtiofsd: Set req->reply_sent right after sending reply Vivek Goyal
@ 2021-05-13 20:50   ` Connor Kuehl
  2021-05-17 13:08     ` Vivek Goyal
  2021-05-18 12:32   ` Dr. David Alan Gilbert
  1 sibling, 1 reply; 21+ messages in thread
From: Connor Kuehl @ 2021-05-13 20:50 UTC (permalink / raw)
  To: Vivek Goyal, qemu-devel, virtio-fs

On 5/11/21 4:37 PM, Vivek Goyal wrote:
> There is no reason to set it in label "err". We should be able to set
> it right after sending reply. It is easier to read.
> 
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  tools/virtiofsd/fuse_virtio.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/tools/virtiofsd/fuse_virtio.c b/tools/virtiofsd/fuse_virtio.c
> index aa53808ef9..b1767dd5b9 100644
> --- a/tools/virtiofsd/fuse_virtio.c
> +++ b/tools/virtiofsd/fuse_virtio.c
> @@ -446,12 +446,9 @@ int virtio_send_data_iov(struct fuse_session *se, struct fuse_chan *ch,
>      vu_queue_notify(dev, q);
>      pthread_mutex_unlock(&qi->vq_lock);
>      vu_dispatch_unlock(qi->virtio_dev);
> +    req->reply_sent = true;
>  
>  err:

Just a really minor comment: after all these changes, I would venture
that "out" is a more appropriate label name than "err" at this point.

> -    if (ret == 0) {
> -        req->reply_sent = true;
> -    }
> -
>      return ret;
>  }
>  
> 



^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Virtio-fs] [PATCH 0/7] virtiofsd: Few cleanups in virtio_send_data_iov()
  2021-05-11 21:37 [PATCH 0/7] virtiofsd: Few cleanups in virtio_send_data_iov() Vivek Goyal
                   ` (7 preceding siblings ...)
  2021-05-11 21:48 ` [PATCH 0/7] virtiofsd: Few cleanups in virtio_send_data_iov() no-reply
@ 2021-05-13 20:50 ` Connor Kuehl
  2021-05-17 13:08   ` Vivek Goyal
  8 siblings, 1 reply; 21+ messages in thread
From: Connor Kuehl @ 2021-05-13 20:50 UTC (permalink / raw)
  To: Vivek Goyal, qemu-devel, virtio-fs

On 5/11/21 4:37 PM, Vivek Goyal wrote:
> Hi,
> 
> Code in virtio_send_data_iov() little twisted and complicated. This
> patch series just tries to simplify it a bit to make it little easier
> to read this piece of code.
> 
> Thanks
> Vivek
> 
> Vivek Goyal (7):
>   virtiofsd: Check for EINTR in preadv() and retry
>   virtiofsd: Get rid of unreachable code in read
>   virtiofsd: Use iov_discard_front() to skip bytes
>   virtiofsd: get rid of in_sg_left variable
>   virtiofsd: Simplify skip byte logic
>   virtiofsd: Check EOF before short read
>   virtiofsd: Set req->reply_sent right after sending reply
> 
>  tools/virtiofsd/fuse_virtio.c | 67 +++++++++++------------------------
>  1 file changed, 21 insertions(+), 46 deletions(-)
> 

With the codestyle fix to appease the bot:

Reviewed-by: Connor Kuehl <ckuehl@redhat.com>

(For the series)



^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Virtio-fs] [PATCH 7/7] virtiofsd: Set req->reply_sent right after sending reply
  2021-05-13 20:50   ` [Virtio-fs] " Connor Kuehl
@ 2021-05-17 13:08     ` Vivek Goyal
  2021-05-18 12:34       ` Dr. David Alan Gilbert
  0 siblings, 1 reply; 21+ messages in thread
From: Vivek Goyal @ 2021-05-17 13:08 UTC (permalink / raw)
  To: Connor Kuehl; +Cc: virtio-fs, qemu-devel

On Thu, May 13, 2021 at 03:50:13PM -0500, Connor Kuehl wrote:
> On 5/11/21 4:37 PM, Vivek Goyal wrote:
> > There is no reason to set it in label "err". We should be able to set
> > it right after sending reply. It is easier to read.
> > 
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > ---
> >  tools/virtiofsd/fuse_virtio.c | 5 +----
> >  1 file changed, 1 insertion(+), 4 deletions(-)
> > 
> > diff --git a/tools/virtiofsd/fuse_virtio.c b/tools/virtiofsd/fuse_virtio.c
> > index aa53808ef9..b1767dd5b9 100644
> > --- a/tools/virtiofsd/fuse_virtio.c
> > +++ b/tools/virtiofsd/fuse_virtio.c
> > @@ -446,12 +446,9 @@ int virtio_send_data_iov(struct fuse_session *se, struct fuse_chan *ch,
> >      vu_queue_notify(dev, q);
> >      pthread_mutex_unlock(&qi->vq_lock);
> >      vu_dispatch_unlock(qi->virtio_dev);
> > +    req->reply_sent = true;
> >  
> >  err:
> 
> Just a really minor comment: after all these changes, I would venture
> that "out" is a more appropriate label name than "err" at this point.

May be. This path is used both by error path as well as success path. Just
that value of "ret" changes. I am not particular about it. So I will
change this to "out".

Thanks
Vivek
> 
> > -    if (ret == 0) {
> > -        req->reply_sent = true;
> > -    }
> > -
> >      return ret;
> >  }
> >  
> > 
> 



^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Virtio-fs] [PATCH 0/7] virtiofsd: Few cleanups in virtio_send_data_iov()
  2021-05-13 20:50 ` [Virtio-fs] " Connor Kuehl
@ 2021-05-17 13:08   ` Vivek Goyal
  0 siblings, 0 replies; 21+ messages in thread
From: Vivek Goyal @ 2021-05-17 13:08 UTC (permalink / raw)
  To: Connor Kuehl; +Cc: virtio-fs, qemu-devel

On Thu, May 13, 2021 at 03:50:33PM -0500, Connor Kuehl wrote:
> On 5/11/21 4:37 PM, Vivek Goyal wrote:
> > Hi,
> > 
> > Code in virtio_send_data_iov() little twisted and complicated. This
> > patch series just tries to simplify it a bit to make it little easier
> > to read this piece of code.
> > 
> > Thanks
> > Vivek
> > 
> > Vivek Goyal (7):
> >   virtiofsd: Check for EINTR in preadv() and retry
> >   virtiofsd: Get rid of unreachable code in read
> >   virtiofsd: Use iov_discard_front() to skip bytes
> >   virtiofsd: get rid of in_sg_left variable
> >   virtiofsd: Simplify skip byte logic
> >   virtiofsd: Check EOF before short read
> >   virtiofsd: Set req->reply_sent right after sending reply
> > 
> >  tools/virtiofsd/fuse_virtio.c | 67 +++++++++++------------------------
> >  1 file changed, 21 insertions(+), 46 deletions(-)
> > 
> 
> With the codestyle fix to appease the bot:
> 
> Reviewed-by: Connor Kuehl <ckuehl@redhat.com>
> 
> (For the series)

Thanks. I will fix the coding style issue and repost the patch series.

Vivek



^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 1/7] virtiofsd: Check for EINTR in preadv() and retry
  2021-05-11 21:37 ` [PATCH 1/7] virtiofsd: Check for EINTR in preadv() and retry Vivek Goyal
@ 2021-05-18 11:59   ` Dr. David Alan Gilbert
  0 siblings, 0 replies; 21+ messages in thread
From: Dr. David Alan Gilbert @ 2021-05-18 11:59 UTC (permalink / raw)
  To: Vivek Goyal; +Cc: virtio-fs, qemu-devel

* Vivek Goyal (vgoyal@redhat.com) wrote:
> We don't seem to check for EINTR and retry. There are other places
> in code where we check for EINTR. So lets add a check.
> 
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  tools/virtiofsd/fuse_virtio.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/tools/virtiofsd/fuse_virtio.c b/tools/virtiofsd/fuse_virtio.c
> index 1170f375a5..32914f7e95 100644
> --- a/tools/virtiofsd/fuse_virtio.c
> +++ b/tools/virtiofsd/fuse_virtio.c
> @@ -421,6 +421,9 @@ int virtio_send_data_iov(struct fuse_session *se, struct fuse_chan *ch,
>  
>          if (ret == -1) {
>              ret = errno;
> +            if (ret == EINTR) {
> +                continue;
> +            }
>              fuse_log(FUSE_LOG_DEBUG, "%s: preadv failed (%m) len=%zd\n",
>                       __func__, len);
>              goto err;
> -- 
> 2.25.4
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK



^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 2/7] virtiofsd: Get rid of unreachable code in read
  2021-05-11 21:37 ` [PATCH 2/7] virtiofsd: Get rid of unreachable code in read Vivek Goyal
@ 2021-05-18 12:01   ` Dr. David Alan Gilbert
  0 siblings, 0 replies; 21+ messages in thread
From: Dr. David Alan Gilbert @ 2021-05-18 12:01 UTC (permalink / raw)
  To: Vivek Goyal; +Cc: virtio-fs, qemu-devel

* Vivek Goyal (vgoyal@redhat.com) wrote:
> pvreadv() can return following.
> 
> - error
> - 0 in case of EOF
> - short read
> 
> We seem to handle all the cases already. We are retrying read in case
> of short read. So another check for short read seems like dead code.
> Get rid of it.
> 
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  tools/virtiofsd/fuse_virtio.c | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/tools/virtiofsd/fuse_virtio.c b/tools/virtiofsd/fuse_virtio.c
> index 32914f7e95..5dcd08fccb 100644
> --- a/tools/virtiofsd/fuse_virtio.c
> +++ b/tools/virtiofsd/fuse_virtio.c
> @@ -446,11 +446,6 @@ int virtio_send_data_iov(struct fuse_session *se, struct fuse_chan *ch,
>                       in_sg_left);
>              break;
>          }
> -        if (ret != len) {
> -            fuse_log(FUSE_LOG_DEBUG, "%s: ret!=len\n", __func__);
> -            ret = EIO;
> -            goto err;
> -        }
>          in_sg_left -= ret;
>          len -= ret;
>      } while (in_sg_left);
> -- 
> 2.25.4
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK



^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 3/7] virtiofsd: Use iov_discard_front() to skip bytes
  2021-05-11 21:37 ` [PATCH 3/7] virtiofsd: Use iov_discard_front() to skip bytes Vivek Goyal
@ 2021-05-18 12:10   ` Dr. David Alan Gilbert
  0 siblings, 0 replies; 21+ messages in thread
From: Dr. David Alan Gilbert @ 2021-05-18 12:10 UTC (permalink / raw)
  To: Vivek Goyal; +Cc: virtio-fs, qemu-devel

* Vivek Goyal (vgoyal@redhat.com) wrote:
> There are places where we need to skip few bytes from front of the iovec
> array. We have our own custom code for that. Looks like iov_discard_front()
> can do same thing. So use that helper instead.
> 
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>

Oh nice; I hadn't noticed that file; I bet there are loads of other
places that can use it (and I still don't get why iov functions aren't
part of libc)


Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  tools/virtiofsd/fuse_virtio.c | 14 +++-----------
>  1 file changed, 3 insertions(+), 11 deletions(-)
> 
> diff --git a/tools/virtiofsd/fuse_virtio.c b/tools/virtiofsd/fuse_virtio.c
> index 5dcd08fccb..d56b225800 100644
> --- a/tools/virtiofsd/fuse_virtio.c
> +++ b/tools/virtiofsd/fuse_virtio.c
> @@ -389,23 +389,15 @@ int virtio_send_data_iov(struct fuse_session *se, struct fuse_chan *ch,
>      memcpy(in_sg_cpy, in_sg, sizeof(struct iovec) * in_num);
>      /* These get updated as we skip */
>      struct iovec *in_sg_ptr = in_sg_cpy;
> -    int in_sg_cpy_count = in_num;
> +    unsigned int in_sg_cpy_count = in_num;
>  
>      /* skip over parts of in_sg that contained the header iov */
>      size_t skip_size = iov_len;
>  
>      size_t in_sg_left = 0;
>      do {
> -        while (skip_size != 0 && in_sg_cpy_count) {
> -            if (skip_size >= in_sg_ptr[0].iov_len) {
> -                skip_size -= in_sg_ptr[0].iov_len;
> -                in_sg_ptr++;
> -                in_sg_cpy_count--;
> -            } else {
> -                in_sg_ptr[0].iov_len -= skip_size;
> -                in_sg_ptr[0].iov_base += skip_size;
> -                break;
> -            }
> +        if (skip_size != 0) {
> +	    iov_discard_front(&in_sg_ptr, &in_sg_cpy_count, skip_size);
>          }
>  
>          int i;
> -- 
> 2.25.4
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK



^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 4/7] virtiofsd: get rid of in_sg_left variable
  2021-05-11 21:37 ` [PATCH 4/7] virtiofsd: get rid of in_sg_left variable Vivek Goyal
@ 2021-05-18 12:19   ` Dr. David Alan Gilbert
  0 siblings, 0 replies; 21+ messages in thread
From: Dr. David Alan Gilbert @ 2021-05-18 12:19 UTC (permalink / raw)
  To: Vivek Goyal; +Cc: virtio-fs, qemu-devel

* Vivek Goyal (vgoyal@redhat.com) wrote:
> in_sg_left seems to be being used primarly for debugging purpose. It is
> keeping track of how many bytes are left in the scatter list we are
> reading into.
> 
> We already have another variable "len" which keeps track how many bytes
> are left to be read. And in_sg_left is greater than or equal to len. We
> have already ensured that in the beginning of function.
> 
>     if (in_len < tosend_len) {
>         fuse_log(FUSE_LOG_ERR, "%s: elem %d too small for data len %zd\n",
>                  __func__, elem->index, tosend_len);
>         ret = E2BIG;
>         goto err;
>     }
> 
> So in_sg_left seems like a redundant variable. It probably was useful for
> debugging when code was being developed. Get rid of it. It helps simplify
> this function.
> 
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  tools/virtiofsd/fuse_virtio.c | 17 ++++++-----------
>  1 file changed, 6 insertions(+), 11 deletions(-)
> 
> diff --git a/tools/virtiofsd/fuse_virtio.c b/tools/virtiofsd/fuse_virtio.c
> index d56b225800..ccad2b3f8a 100644
> --- a/tools/virtiofsd/fuse_virtio.c
> +++ b/tools/virtiofsd/fuse_virtio.c
> @@ -394,20 +394,16 @@ int virtio_send_data_iov(struct fuse_session *se, struct fuse_chan *ch,
>      /* skip over parts of in_sg that contained the header iov */
>      size_t skip_size = iov_len;
>  
> -    size_t in_sg_left = 0;
>      do {
>          if (skip_size != 0) {
>  	    iov_discard_front(&in_sg_ptr, &in_sg_cpy_count, skip_size);
>          }
>  
> -        int i;
> -        for (i = 0, in_sg_left = 0; i < in_sg_cpy_count; i++) {
> -            in_sg_left += in_sg_ptr[i].iov_len;
> -        }
>          fuse_log(FUSE_LOG_DEBUG,
>                   "%s: after skip skip_size=%zd in_sg_cpy_count=%d "
> -                 "in_sg_left=%zd\n",
> -                 __func__, skip_size, in_sg_cpy_count, in_sg_left);
> +                 "len remaining=%zd\n", __func__, skip_size, in_sg_cpy_count,
> +                 len);
> +
>          ret = preadv(buf->buf[0].fd, in_sg_ptr, in_sg_cpy_count,
>                       buf->buf[0].pos);
>  
> @@ -434,13 +430,12 @@ int virtio_send_data_iov(struct fuse_session *se, struct fuse_chan *ch,
>          }
>          if (!ret) {
>              /* EOF case? */
> -            fuse_log(FUSE_LOG_DEBUG, "%s: !ret in_sg_left=%zd\n", __func__,
> -                     in_sg_left);
> +            fuse_log(FUSE_LOG_DEBUG, "%s: !ret len remaining=%zd\n", __func__,
> +                     len);
>              break;
>          }
> -        in_sg_left -= ret;
>          len -= ret;
> -    } while (in_sg_left);
> +    } while (len);
>  
>      /* Need to fix out->len on EOF */
>      if (len) {
> -- 
> 2.25.4
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK



^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 5/7] virtiofsd: Simplify skip byte logic
  2021-05-11 21:37 ` [PATCH 5/7] virtiofsd: Simplify skip byte logic Vivek Goyal
@ 2021-05-18 12:26   ` Dr. David Alan Gilbert
  0 siblings, 0 replies; 21+ messages in thread
From: Dr. David Alan Gilbert @ 2021-05-18 12:26 UTC (permalink / raw)
  To: Vivek Goyal; +Cc: virtio-fs, qemu-devel

* Vivek Goyal (vgoyal@redhat.com) wrote:
> We need to skip bytes in two cases.
> 
> a. Before we start reading into in_sg, we need to skip iov_len bytes
>    in the beginning which typically will have fuse_out_header.
> 
> b. If preadv() does a short read, then we need to retry preadv() with
>    remainig bytes and skip the bytes preadv() read in short read.
> 
> For case a, there is no reason that skipping logic be inside the while
> loop. Move it outside. And only retain logic "b" inside while loop.
> 
> Also get rid of variable "skip_size". Looks like we can do without it.
> 
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>

Yep, now iov_discard_front makes it easy to skip.


Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  tools/virtiofsd/fuse_virtio.c | 14 ++++----------
>  1 file changed, 4 insertions(+), 10 deletions(-)
> 
> diff --git a/tools/virtiofsd/fuse_virtio.c b/tools/virtiofsd/fuse_virtio.c
> index ccad2b3f8a..434fe401cf 100644
> --- a/tools/virtiofsd/fuse_virtio.c
> +++ b/tools/virtiofsd/fuse_virtio.c
> @@ -392,17 +392,11 @@ int virtio_send_data_iov(struct fuse_session *se, struct fuse_chan *ch,
>      unsigned int in_sg_cpy_count = in_num;
>  
>      /* skip over parts of in_sg that contained the header iov */
> -    size_t skip_size = iov_len;
> +    iov_discard_front(&in_sg_ptr, &in_sg_cpy_count, iov_len);
>  
>      do {
> -        if (skip_size != 0) {
> -	    iov_discard_front(&in_sg_ptr, &in_sg_cpy_count, skip_size);
> -        }
> -
> -        fuse_log(FUSE_LOG_DEBUG,
> -                 "%s: after skip skip_size=%zd in_sg_cpy_count=%d "
> -                 "len remaining=%zd\n", __func__, skip_size, in_sg_cpy_count,
> -                 len);
> +        fuse_log(FUSE_LOG_DEBUG, "%s: in_sg_cpy_count=%d len remaining=%zd\n",
> +                 __func__, in_sg_cpy_count, len);
>  
>          ret = preadv(buf->buf[0].fd, in_sg_ptr, in_sg_cpy_count,
>                       buf->buf[0].pos);
> @@ -421,7 +415,7 @@ int virtio_send_data_iov(struct fuse_session *se, struct fuse_chan *ch,
>          if (ret < len && ret) {
>              fuse_log(FUSE_LOG_DEBUG, "%s: ret < len\n", __func__);
>              /* Skip over this much next time around */
> -            skip_size = ret;
> +            iov_discard_front(&in_sg_ptr, &in_sg_cpy_count, ret);
>              buf->buf[0].pos += ret;
>              len -= ret;
>  
> -- 
> 2.25.4
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK



^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 6/7] virtiofsd: Check EOF before short read
  2021-05-11 21:37 ` [PATCH 6/7] virtiofsd: Check EOF before short read Vivek Goyal
@ 2021-05-18 12:31   ` Dr. David Alan Gilbert
  0 siblings, 0 replies; 21+ messages in thread
From: Dr. David Alan Gilbert @ 2021-05-18 12:31 UTC (permalink / raw)
  To: Vivek Goyal; +Cc: virtio-fs, qemu-devel

* Vivek Goyal (vgoyal@redhat.com) wrote:
> In virtio_send_data_iov() we are checking first for short read and then
> EOF condition. Change the order. Basically check for error and EOF first
> and last remaining piece is short ready which will lead to retry
> automatically at the end of while loop.
> 
> Just that it is little simpler to read to the code. There is no need
> to call "continue" and also one less call of "len-=ret".
> 
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  tools/virtiofsd/fuse_virtio.c | 21 ++++++++++-----------
>  1 file changed, 10 insertions(+), 11 deletions(-)
> 
> diff --git a/tools/virtiofsd/fuse_virtio.c b/tools/virtiofsd/fuse_virtio.c
> index 434fe401cf..aa53808ef9 100644
> --- a/tools/virtiofsd/fuse_virtio.c
> +++ b/tools/virtiofsd/fuse_virtio.c
> @@ -410,25 +410,24 @@ int virtio_send_data_iov(struct fuse_session *se, struct fuse_chan *ch,
>                       __func__, len);
>              goto err;
>          }
> -        fuse_log(FUSE_LOG_DEBUG, "%s: preadv ret=%d len=%zd\n", __func__,
> -                 ret, len);
> -        if (ret < len && ret) {
> -            fuse_log(FUSE_LOG_DEBUG, "%s: ret < len\n", __func__);
> -            /* Skip over this much next time around */
> -            iov_discard_front(&in_sg_ptr, &in_sg_cpy_count, ret);
> -            buf->buf[0].pos += ret;
> -            len -= ret;
>  
> -            /* Lets do another read */
> -            continue;
> -        }
>          if (!ret) {
>              /* EOF case? */
>              fuse_log(FUSE_LOG_DEBUG, "%s: !ret len remaining=%zd\n", __func__,
>                       len);
>              break;
>          }
> +        fuse_log(FUSE_LOG_DEBUG, "%s: preadv ret=%d len=%zd\n", __func__,
> +                 ret, len);
> +
>          len -= ret;
> +        /* Short read. Retry reading remaining bytes */
> +        if (len) {
> +            fuse_log(FUSE_LOG_DEBUG, "%s: ret < len\n", __func__);
> +            /* Skip over this much next time around */
> +            iov_discard_front(&in_sg_ptr, &in_sg_cpy_count, ret);
> +            buf->buf[0].pos += ret;
> +        }
>      } while (len);
>  
>      /* Need to fix out->len on EOF */
> -- 
> 2.25.4
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK



^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 7/7] virtiofsd: Set req->reply_sent right after sending reply
  2021-05-11 21:37 ` [PATCH 7/7] virtiofsd: Set req->reply_sent right after sending reply Vivek Goyal
  2021-05-13 20:50   ` [Virtio-fs] " Connor Kuehl
@ 2021-05-18 12:32   ` Dr. David Alan Gilbert
  1 sibling, 0 replies; 21+ messages in thread
From: Dr. David Alan Gilbert @ 2021-05-18 12:32 UTC (permalink / raw)
  To: Vivek Goyal; +Cc: virtio-fs, qemu-devel

* Vivek Goyal (vgoyal@redhat.com) wrote:
> There is no reason to set it in label "err". We should be able to set
> it right after sending reply. It is easier to read.
> 
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  tools/virtiofsd/fuse_virtio.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/tools/virtiofsd/fuse_virtio.c b/tools/virtiofsd/fuse_virtio.c
> index aa53808ef9..b1767dd5b9 100644
> --- a/tools/virtiofsd/fuse_virtio.c
> +++ b/tools/virtiofsd/fuse_virtio.c
> @@ -446,12 +446,9 @@ int virtio_send_data_iov(struct fuse_session *se, struct fuse_chan *ch,
>      vu_queue_notify(dev, q);
>      pthread_mutex_unlock(&qi->vq_lock);
>      vu_dispatch_unlock(qi->virtio_dev);
> +    req->reply_sent = true;
>  
>  err:
> -    if (ret == 0) {
> -        req->reply_sent = true;
> -    }
> -
>      return ret;
>  }
>  
> -- 
> 2.25.4
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK



^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Virtio-fs] [PATCH 7/7] virtiofsd: Set req->reply_sent right after sending reply
  2021-05-17 13:08     ` Vivek Goyal
@ 2021-05-18 12:34       ` Dr. David Alan Gilbert
  0 siblings, 0 replies; 21+ messages in thread
From: Dr. David Alan Gilbert @ 2021-05-18 12:34 UTC (permalink / raw)
  To: Vivek Goyal; +Cc: Connor Kuehl, virtio-fs, qemu-devel

* Vivek Goyal (vgoyal@redhat.com) wrote:
> On Thu, May 13, 2021 at 03:50:13PM -0500, Connor Kuehl wrote:
> > On 5/11/21 4:37 PM, Vivek Goyal wrote:
> > > There is no reason to set it in label "err". We should be able to set
> > > it right after sending reply. It is easier to read.
> > > 
> > > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > > ---
> > >  tools/virtiofsd/fuse_virtio.c | 5 +----
> > >  1 file changed, 1 insertion(+), 4 deletions(-)
> > > 
> > > diff --git a/tools/virtiofsd/fuse_virtio.c b/tools/virtiofsd/fuse_virtio.c
> > > index aa53808ef9..b1767dd5b9 100644
> > > --- a/tools/virtiofsd/fuse_virtio.c
> > > +++ b/tools/virtiofsd/fuse_virtio.c
> > > @@ -446,12 +446,9 @@ int virtio_send_data_iov(struct fuse_session *se, struct fuse_chan *ch,
> > >      vu_queue_notify(dev, q);
> > >      pthread_mutex_unlock(&qi->vq_lock);
> > >      vu_dispatch_unlock(qi->virtio_dev);
> > > +    req->reply_sent = true;
> > >  
> > >  err:
> > 
> > Just a really minor comment: after all these changes, I would venture
> > that "out" is a more appropriate label name than "err" at this point.
> 
> May be. This path is used both by error path as well as success path. Just
> that value of "ret" changes. I am not particular about it. So I will
> change this to "out".

Well, if it only does 'return ret' we can get rid of the
label and just do return's at the places that did the goto's.

Dave

> Thanks
> Vivek
> > 
> > > -    if (ret == 0) {
> > > -        req->reply_sent = true;
> > > -    }
> > > -
> > >      return ret;
> > >  }
> > >  
> > > 
> > 
> 
> _______________________________________________
> Virtio-fs mailing list
> Virtio-fs@redhat.com
> https://listman.redhat.com/mailman/listinfo/virtio-fs
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK



^ permalink raw reply	[flat|nested] 21+ messages in thread

end of thread, other threads:[~2021-05-18 12:50 UTC | newest]

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-11 21:37 [PATCH 0/7] virtiofsd: Few cleanups in virtio_send_data_iov() Vivek Goyal
2021-05-11 21:37 ` [PATCH 1/7] virtiofsd: Check for EINTR in preadv() and retry Vivek Goyal
2021-05-18 11:59   ` Dr. David Alan Gilbert
2021-05-11 21:37 ` [PATCH 2/7] virtiofsd: Get rid of unreachable code in read Vivek Goyal
2021-05-18 12:01   ` Dr. David Alan Gilbert
2021-05-11 21:37 ` [PATCH 3/7] virtiofsd: Use iov_discard_front() to skip bytes Vivek Goyal
2021-05-18 12:10   ` Dr. David Alan Gilbert
2021-05-11 21:37 ` [PATCH 4/7] virtiofsd: get rid of in_sg_left variable Vivek Goyal
2021-05-18 12:19   ` Dr. David Alan Gilbert
2021-05-11 21:37 ` [PATCH 5/7] virtiofsd: Simplify skip byte logic Vivek Goyal
2021-05-18 12:26   ` Dr. David Alan Gilbert
2021-05-11 21:37 ` [PATCH 6/7] virtiofsd: Check EOF before short read Vivek Goyal
2021-05-18 12:31   ` Dr. David Alan Gilbert
2021-05-11 21:37 ` [PATCH 7/7] virtiofsd: Set req->reply_sent right after sending reply Vivek Goyal
2021-05-13 20:50   ` [Virtio-fs] " Connor Kuehl
2021-05-17 13:08     ` Vivek Goyal
2021-05-18 12:34       ` Dr. David Alan Gilbert
2021-05-18 12:32   ` Dr. David Alan Gilbert
2021-05-11 21:48 ` [PATCH 0/7] virtiofsd: Few cleanups in virtio_send_data_iov() no-reply
2021-05-13 20:50 ` [Virtio-fs] " Connor Kuehl
2021-05-17 13:08   ` Vivek Goyal

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).