[PATCH 0/7] virtiofsd: Few cleanups in virtio_send_data_iov()

[PATCH 0/7] virtiofsd: Few cleanups in virtio_send_data_iov()

[Vivek Goyal]

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

[PATCH 1/7] virtiofsd: Check for EINTR in preadv() and retry

[Vivek Goyal]

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

Re: [PATCH 1/7] virtiofsd: Check for EINTR in preadv() and retry

[Dr. David Alan Gilbert]

* 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

[PATCH 2/7] virtiofsd: Get rid of unreachable code in read

[Vivek Goyal]

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

Re: [PATCH 2/7] virtiofsd: Get rid of unreachable code in read

[Dr. David Alan Gilbert]

* 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

[PATCH 3/7] virtiofsd: Use iov_discard_front() to skip bytes

[Vivek Goyal]

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

Re: [PATCH 3/7] virtiofsd: Use iov_discard_front() to skip bytes

[Dr. David Alan Gilbert]

* 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

[PATCH 4/7] virtiofsd: get rid of in_sg_left variable

[Vivek Goyal]

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

Re: [PATCH 4/7] virtiofsd: get rid of in_sg_left variable

[Dr. David Alan Gilbert]

* 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

[PATCH 5/7] virtiofsd: Simplify skip byte logic

[Vivek Goyal]

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

Re: [PATCH 5/7] virtiofsd: Simplify skip byte logic

[Dr. David Alan Gilbert]

* 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

[PATCH 6/7] virtiofsd: Check EOF before short read

[Vivek Goyal]

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

Re: [PATCH 6/7] virtiofsd: Check EOF before short read

[Dr. David Alan Gilbert]

* 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

[PATCH 7/7] virtiofsd: Set req->reply_sent right after sending reply

[Vivek Goyal]

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

Re: [Virtio-fs] [PATCH 7/7] virtiofsd: Set req->reply_sent right after sending reply

[Connor Kuehl]

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;
>  }
>  
> 

Re: [Virtio-fs] [PATCH 7/7] virtiofsd: Set req->reply_sent right after sending reply

[Vivek Goyal]

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;
> >  }
> >  
> > 
> 

Re: [Virtio-fs] [PATCH 7/7] virtiofsd: Set req->reply_sent right after sending reply

[Dr. David Alan Gilbert]

* 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

Re: [PATCH 7/7] virtiofsd: Set req->reply_sent right after sending reply

[Dr. David Alan Gilbert]

* 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

Re: [PATCH 0/7] virtiofsd: Few cleanups in virtio_send_data_iov()

[no-reply]

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

Re: [Virtio-fs] [PATCH 0/7] virtiofsd: Few cleanups in virtio_send_data_iov()

[Connor Kuehl]

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)

Re: [Virtio-fs] [PATCH 0/7] virtiofsd: Few cleanups in virtio_send_data_iov()

[Vivek Goyal]

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