[PATCH] virtiofsd: add -o allow_directio|no_directio option

Jiachen Zhang posted 1 patch 3 years, 8 months ago
Test docker-quick@centos7 passed
Test docker-mingw@fedora passed
Test checkpatch passed
Test FreeBSD passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20200821034126.8004-1-zhangjiachen.jaycee@bytedance.com
Maintainers: "Dr. David Alan Gilbert" <dgilbert@redhat.com>, Stefan Hajnoczi <stefanha@redhat.com>
tools/virtiofsd/helper.c         |  4 ++++
tools/virtiofsd/passthrough_ll.c | 20 ++++++++++++++------
2 files changed, 18 insertions(+), 6 deletions(-)
[PATCH] virtiofsd: add -o allow_directio|no_directio option
Posted by Jiachen Zhang 3 years, 8 months ago
Due to the commit 65da4539803373ec4eec97ffc49ee90083e56efd, the O_DIRECT
open flag of guest applications will be discarded by virtiofsd. While
this behavior makes it consistent with the virtio-9p scheme when guest
applications using direct I/O, we no longer have any chance to bypass
the host page cache.

Therefore, we add a flag 'allow_directio' to lo_data. If '-o no_directio'
option is added, or none of '-o no_directio' or '-o allow_directio' is
added, the 'allow_directio' will be set to 0, and virtiofsd discards
O_DIRECT as before. If '-o allow_directio' is added to the stariting
command-line, 'allow_directio' will be set to 1, so that the O_DIRECT
flags will be retained and host page cache can be bypassed.

Signed-off-by: Jiachen Zhang <zhangjiachen.jaycee@bytedance.com>
---
 tools/virtiofsd/helper.c         |  4 ++++
 tools/virtiofsd/passthrough_ll.c | 20 ++++++++++++++------
 2 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/tools/virtiofsd/helper.c b/tools/virtiofsd/helper.c
index 3105b6c23a..534ff52c64 100644
--- a/tools/virtiofsd/helper.c
+++ b/tools/virtiofsd/helper.c
@@ -180,6 +180,10 @@ void fuse_cmdline_help(void)
            "                               (0 leaves rlimit unchanged)\n"
            "                               default: min(1000000, fs.file-max - 16384)\n"
            "                                        if the current rlimit is lower\n"
+           "    -o allow_directio|no_directio\n"
+           "                               retain/discard O_DIRECT flags passed down\n"
+           "                               to virtiofsd from guest applications.\n"
+           "                               default: no_directio\n"
            );
 }
 
diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index 94e0de2d2b..1c5ea27821 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -151,6 +151,7 @@ struct lo_data {
     int timeout_set;
     int readdirplus_set;
     int readdirplus_clear;
+    int allow_directio;
     struct lo_inode root;
     GHashTable *inodes; /* protected by lo->mutex */
     struct lo_map ino_map; /* protected by lo->mutex */
@@ -179,6 +180,8 @@ static const struct fuse_opt lo_opts[] = {
     { "cache=always", offsetof(struct lo_data, cache), CACHE_ALWAYS },
     { "readdirplus", offsetof(struct lo_data, readdirplus_set), 1 },
     { "no_readdirplus", offsetof(struct lo_data, readdirplus_clear), 1 },
+    { "allow_directio", offsetof(struct lo_data, allow_directio), 1 },
+    { "no_directio", offsetof(struct lo_data, allow_directio), 0 },
     FUSE_OPT_END
 };
 static bool use_syslog = false;
@@ -1516,7 +1519,8 @@ static void lo_releasedir(fuse_req_t req, fuse_ino_t ino,
     fuse_reply_err(req, 0);
 }
 
-static void update_open_flags(int writeback, struct fuse_file_info *fi)
+static void update_open_flags(int writeback, int allow_directio,
+                              struct fuse_file_info *fi)
 {
     /*
      * With writeback cache, kernel may send read requests even
@@ -1541,10 +1545,13 @@ static void update_open_flags(int writeback, struct fuse_file_info *fi)
 
     /*
      * O_DIRECT in guest should not necessarily mean bypassing page
-     * cache on host as well. If somebody needs that behavior, it
-     * probably should be a configuration knob in daemon.
+     * cache on host as well. Therefore, we discard it by default
+     * ('-o no_directio'). If somebody needs that behavior, the
+     * '-o allow_directio' option should be set.
      */
-    fi->flags &= ~O_DIRECT;
+    if (!allow_directio) {
+        fi->flags &= ~O_DIRECT;
+    }
 }
 
 static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name,
@@ -1576,7 +1583,7 @@ static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name,
         goto out;
     }
 
-    update_open_flags(lo->writeback, fi);
+    update_open_flags(lo->writeback, lo->allow_directio, fi);
 
     fd = openat(parent_inode->fd, name, (fi->flags | O_CREAT) & ~O_NOFOLLOW,
                 mode);
@@ -1786,7 +1793,7 @@ static void lo_open(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi)
     fuse_log(FUSE_LOG_DEBUG, "lo_open(ino=%" PRIu64 ", flags=%d)\n", ino,
              fi->flags);
 
-    update_open_flags(lo->writeback, fi);
+    update_open_flags(lo->writeback, lo->allow_directio, fi);
 
     sprintf(buf, "%i", lo_fd(req, ino));
     fd = openat(lo->proc_self_fd, buf, fi->flags & ~O_NOFOLLOW);
@@ -2824,6 +2831,7 @@ int main(int argc, char *argv[])
         .debug = 0,
         .writeback = 0,
         .posix_lock = 1,
+        .allow_directio = 0,
         .proc_self_fd = -1,
     };
     struct lo_map_elem *root_elem;
-- 
2.11.0


Re: [PATCH] virtiofsd: add -o allow_directio|no_directio option
Posted by Philippe Mathieu-Daudé 3 years, 8 months ago
On 8/21/20 5:41 AM, Jiachen Zhang wrote:
> Due to the commit 65da4539803373ec4eec97ffc49ee90083e56efd, the O_DIRECT
> open flag of guest applications will be discarded by virtiofsd. While
> this behavior makes it consistent with the virtio-9p scheme when guest
> applications using direct I/O, we no longer have any chance to bypass
> the host page cache.
> 
> Therefore, we add a flag 'allow_directio' to lo_data. If '-o no_directio'
> option is added, or none of '-o no_directio' or '-o allow_directio' is
> added, the 'allow_directio' will be set to 0, and virtiofsd discards
> O_DIRECT as before. If '-o allow_directio' is added to the stariting
> command-line, 'allow_directio' will be set to 1, so that the O_DIRECT
> flags will be retained and host page cache can be bypassed.
> 
> Signed-off-by: Jiachen Zhang <zhangjiachen.jaycee@bytedance.com>
> ---
>  tools/virtiofsd/helper.c         |  4 ++++
>  tools/virtiofsd/passthrough_ll.c | 20 ++++++++++++++------
>  2 files changed, 18 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/virtiofsd/helper.c b/tools/virtiofsd/helper.c
> index 3105b6c23a..534ff52c64 100644
> --- a/tools/virtiofsd/helper.c
> +++ b/tools/virtiofsd/helper.c
> @@ -180,6 +180,10 @@ void fuse_cmdline_help(void)
>             "                               (0 leaves rlimit unchanged)\n"
>             "                               default: min(1000000, fs.file-max - 16384)\n"
>             "                                        if the current rlimit is lower\n"
> +           "    -o allow_directio|no_directio\n"
> +           "                               retain/discard O_DIRECT flags passed down\n"
> +           "                               to virtiofsd from guest applications.\n"
> +           "                               default: no_directio\n"
>             );
>  }
>  
> diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> index 94e0de2d2b..1c5ea27821 100644
> --- a/tools/virtiofsd/passthrough_ll.c
> +++ b/tools/virtiofsd/passthrough_ll.c
> @@ -151,6 +151,7 @@ struct lo_data {
>      int timeout_set;
>      int readdirplus_set;
>      int readdirplus_clear;
> +    int allow_directio;

Can we use 'allow_direct_io'?

>      struct lo_inode root;
>      GHashTable *inodes; /* protected by lo->mutex */
>      struct lo_map ino_map; /* protected by lo->mutex */
> @@ -179,6 +180,8 @@ static const struct fuse_opt lo_opts[] = {
>      { "cache=always", offsetof(struct lo_data, cache), CACHE_ALWAYS },
>      { "readdirplus", offsetof(struct lo_data, readdirplus_set), 1 },
>      { "no_readdirplus", offsetof(struct lo_data, readdirplus_clear), 1 },
> +    { "allow_directio", offsetof(struct lo_data, allow_directio), 1 },
> +    { "no_directio", offsetof(struct lo_data, allow_directio), 0 },

Here too, split 'direct_io'?

>      FUSE_OPT_END
>  };
>  static bool use_syslog = false;
> @@ -1516,7 +1519,8 @@ static void lo_releasedir(fuse_req_t req, fuse_ino_t ino,
>      fuse_reply_err(req, 0);
>  }
>  
> -static void update_open_flags(int writeback, struct fuse_file_info *fi)
> +static void update_open_flags(int writeback, int allow_directio,
> +                              struct fuse_file_info *fi)
>  {
>      /*
>       * With writeback cache, kernel may send read requests even
> @@ -1541,10 +1545,13 @@ static void update_open_flags(int writeback, struct fuse_file_info *fi)
>  
>      /*
>       * O_DIRECT in guest should not necessarily mean bypassing page
> -     * cache on host as well. If somebody needs that behavior, it
> -     * probably should be a configuration knob in daemon.
> +     * cache on host as well. Therefore, we discard it by default
> +     * ('-o no_directio'). If somebody needs that behavior, the
> +     * '-o allow_directio' option should be set.
>       */
> -    fi->flags &= ~O_DIRECT;
> +    if (!allow_directio) {
> +        fi->flags &= ~O_DIRECT;
> +    }
>  }
>  
>  static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name,
> @@ -1576,7 +1583,7 @@ static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name,
>          goto out;
>      }
>  
> -    update_open_flags(lo->writeback, fi);
> +    update_open_flags(lo->writeback, lo->allow_directio, fi);
>  
>      fd = openat(parent_inode->fd, name, (fi->flags | O_CREAT) & ~O_NOFOLLOW,
>                  mode);
> @@ -1786,7 +1793,7 @@ static void lo_open(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi)
>      fuse_log(FUSE_LOG_DEBUG, "lo_open(ino=%" PRIu64 ", flags=%d)\n", ino,
>               fi->flags);
>  
> -    update_open_flags(lo->writeback, fi);
> +    update_open_flags(lo->writeback, lo->allow_directio, fi);
>  
>      sprintf(buf, "%i", lo_fd(req, ino));
>      fd = openat(lo->proc_self_fd, buf, fi->flags & ~O_NOFOLLOW);
> @@ -2824,6 +2831,7 @@ int main(int argc, char *argv[])
>          .debug = 0,
>          .writeback = 0,
>          .posix_lock = 1,
> +        .allow_directio = 0,
>          .proc_self_fd = -1,
>      };
>      struct lo_map_elem *root_elem;
> 


Re: [PATCH] virtiofsd: add -o allow_directio|no_directio option
Posted by Stefan Hajnoczi 3 years, 8 months ago
On Fri, Aug 21, 2020 at 11:41:26AM +0800, Jiachen Zhang wrote:
> Due to the commit 65da4539803373ec4eec97ffc49ee90083e56efd, the O_DIRECT
> open flag of guest applications will be discarded by virtiofsd. While
> this behavior makes it consistent with the virtio-9p scheme when guest
> applications using direct I/O, we no longer have any chance to bypass
> the host page cache.
> 
> Therefore, we add a flag 'allow_directio' to lo_data. If '-o no_directio'
> option is added, or none of '-o no_directio' or '-o allow_directio' is
> added, the 'allow_directio' will be set to 0, and virtiofsd discards
> O_DIRECT as before. If '-o allow_directio' is added to the stariting
> command-line, 'allow_directio' will be set to 1, so that the O_DIRECT
> flags will be retained and host page cache can be bypassed.
> 
> Signed-off-by: Jiachen Zhang <zhangjiachen.jaycee@bytedance.com>
> ---
>  tools/virtiofsd/helper.c         |  4 ++++
>  tools/virtiofsd/passthrough_ll.c | 20 ++++++++++++++------
>  2 files changed, 18 insertions(+), 6 deletions(-)

Nice, thanks! This will be useful for benchmarking.

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Re: [PATCH] virtiofsd: add -o allow_directio|no_directio option
Posted by Daniel P. Berrangé 3 years, 8 months ago
On Fri, Aug 21, 2020 at 11:41:26AM +0800, Jiachen Zhang wrote:
> Due to the commit 65da4539803373ec4eec97ffc49ee90083e56efd, the O_DIRECT
> open flag of guest applications will be discarded by virtiofsd. While
> this behavior makes it consistent with the virtio-9p scheme when guest
> applications using direct I/O, we no longer have any chance to bypass
> the host page cache.
> 
> Therefore, we add a flag 'allow_directio' to lo_data. If '-o no_directio'
> option is added, or none of '-o no_directio' or '-o allow_directio' is
> added, the 'allow_directio' will be set to 0, and virtiofsd discards
> O_DIRECT as before. If '-o allow_directio' is added to the stariting
> command-line, 'allow_directio' will be set to 1, so that the O_DIRECT
> flags will be retained and host page cache can be bypassed.
> 
> Signed-off-by: Jiachen Zhang <zhangjiachen.jaycee@bytedance.com>
> ---
>  tools/virtiofsd/helper.c         |  4 ++++
>  tools/virtiofsd/passthrough_ll.c | 20 ++++++++++++++------
>  2 files changed, 18 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/virtiofsd/helper.c b/tools/virtiofsd/helper.c
> index 3105b6c23a..534ff52c64 100644
> --- a/tools/virtiofsd/helper.c
> +++ b/tools/virtiofsd/helper.c
> @@ -180,6 +180,10 @@ void fuse_cmdline_help(void)
>             "                               (0 leaves rlimit unchanged)\n"
>             "                               default: min(1000000, fs.file-max - 16384)\n"
>             "                                        if the current rlimit is lower\n"
> +           "    -o allow_directio|no_directio\n"
> +           "                               retain/discard O_DIRECT flags passed down\n"
> +           "                               to virtiofsd from guest applications.\n"
> +           "                               default: no_directio\n"
>             );

The standard naming convention from existing options is to use
$OPTNAME and no_$OPTNAME.

IOW, don't use the "allow_" prefix. The options should be just
"directio" and "no_directio"


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|


Re: [PATCH] virtiofsd: add -o allow_directio|no_directio option
Posted by Philippe Mathieu-Daudé 3 years, 8 months ago
On 8/21/20 1:58 PM, Daniel P. Berrangé wrote:
> On Fri, Aug 21, 2020 at 11:41:26AM +0800, Jiachen Zhang wrote:
>> Due to the commit 65da4539803373ec4eec97ffc49ee90083e56efd, the O_DIRECT
>> open flag of guest applications will be discarded by virtiofsd. While
>> this behavior makes it consistent with the virtio-9p scheme when guest
>> applications using direct I/O, we no longer have any chance to bypass
>> the host page cache.
>>
>> Therefore, we add a flag 'allow_directio' to lo_data. If '-o no_directio'
>> option is added, or none of '-o no_directio' or '-o allow_directio' is
>> added, the 'allow_directio' will be set to 0, and virtiofsd discards
>> O_DIRECT as before. If '-o allow_directio' is added to the stariting
>> command-line, 'allow_directio' will be set to 1, so that the O_DIRECT
>> flags will be retained and host page cache can be bypassed.
>>
>> Signed-off-by: Jiachen Zhang <zhangjiachen.jaycee@bytedance.com>
>> ---
>>  tools/virtiofsd/helper.c         |  4 ++++
>>  tools/virtiofsd/passthrough_ll.c | 20 ++++++++++++++------
>>  2 files changed, 18 insertions(+), 6 deletions(-)
>>
>> diff --git a/tools/virtiofsd/helper.c b/tools/virtiofsd/helper.c
>> index 3105b6c23a..534ff52c64 100644
>> --- a/tools/virtiofsd/helper.c
>> +++ b/tools/virtiofsd/helper.c
>> @@ -180,6 +180,10 @@ void fuse_cmdline_help(void)
>>             "                               (0 leaves rlimit unchanged)\n"
>>             "                               default: min(1000000, fs.file-max - 16384)\n"
>>             "                                        if the current rlimit is lower\n"
>> +           "    -o allow_directio|no_directio\n"
>> +           "                               retain/discard O_DIRECT flags passed down\n"
>> +           "                               to virtiofsd from guest applications.\n"
>> +           "                               default: no_directio\n"
>>             );
> 
> The standard naming convention from existing options is to use
> $OPTNAME and no_$OPTNAME.
> 
> IOW, don't use the "allow_" prefix. The options should be just
> "directio" and "no_directio"

As we have 'max_idle_threads' (and not maxidlethreads), can we
use 'direct_io' instead?

> 
> Regards,
> Daniel
> 


Re: [External] Re: [PATCH] virtiofsd: add -o allow_directio|no_directio option
Posted by 张佳辰 3 years, 8 months ago
On Fri, Aug 21, 2020 at 9:40 PM Philippe Mathieu-Daudé <philmd@redhat.com>
wrote:

> On 8/21/20 1:58 PM, Daniel P. Berrangé wrote:
> > On Fri, Aug 21, 2020 at 11:41:26AM +0800, Jiachen Zhang wrote:
> >> Due to the commit 65da4539803373ec4eec97ffc49ee90083e56efd, the O_DIRECT
> >> open flag of guest applications will be discarded by virtiofsd. While
> >> this behavior makes it consistent with the virtio-9p scheme when guest
> >> applications using direct I/O, we no longer have any chance to bypass
> >> the host page cache.
> >>
> >> Therefore, we add a flag 'allow_directio' to lo_data. If '-o
> no_directio'
> >> option is added, or none of '-o no_directio' or '-o allow_directio' is
> >> added, the 'allow_directio' will be set to 0, and virtiofsd discards
> >> O_DIRECT as before. If '-o allow_directio' is added to the stariting
> >> command-line, 'allow_directio' will be set to 1, so that the O_DIRECT
> >> flags will be retained and host page cache can be bypassed.
> >>
> >> Signed-off-by: Jiachen Zhang <zhangjiachen.jaycee@bytedance.com>
> >> ---
> >>  tools/virtiofsd/helper.c         |  4 ++++
> >>  tools/virtiofsd/passthrough_ll.c | 20 ++++++++++++++------
> >>  2 files changed, 18 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/tools/virtiofsd/helper.c b/tools/virtiofsd/helper.c
> >> index 3105b6c23a..534ff52c64 100644
> >> --- a/tools/virtiofsd/helper.c
> >> +++ b/tools/virtiofsd/helper.c
> >> @@ -180,6 +180,10 @@ void fuse_cmdline_help(void)
> >>             "                               (0 leaves rlimit
> unchanged)\n"
> >>             "                               default: min(1000000,
> fs.file-max - 16384)\n"
> >>             "                                        if the current
> rlimit is lower\n"
> >> +           "    -o allow_directio|no_directio\n"
> >> +           "                               retain/discard O_DIRECT
> flags passed down\n"
> >> +           "                               to virtiofsd from guest
> applications.\n"
> >> +           "                               default: no_directio\n"
> >>             );
> >
> > The standard naming convention from existing options is to use
> > $OPTNAME and no_$OPTNAME.
> >
> > IOW, don't use the "allow_" prefix. The options should be just
> > "directio" and "no_directio"
>
> As we have 'max_idle_threads' (and not maxidlethreads), can we
> use 'direct_io' instead?
>
> Thanks. I will split them in the next version.

Jiachen

>
> > Regards,
> > Daniel
> >
>
>
Re: [External] Re: [PATCH] virtiofsd: add -o allow_directio|no_directio option
Posted by 张佳辰 3 years, 8 months ago
On Fri, Aug 21, 2020 at 7:58 PM Daniel P. Berrangé <berrange@redhat.com>
wrote:

> On Fri, Aug 21, 2020 at 11:41:26AM +0800, Jiachen Zhang wrote:
> > Due to the commit 65da4539803373ec4eec97ffc49ee90083e56efd, the O_DIRECT
> > open flag of guest applications will be discarded by virtiofsd. While
> > this behavior makes it consistent with the virtio-9p scheme when guest
> > applications using direct I/O, we no longer have any chance to bypass
> > the host page cache.
> >
> > Therefore, we add a flag 'allow_directio' to lo_data. If '-o no_directio'
> > option is added, or none of '-o no_directio' or '-o allow_directio' is
> > added, the 'allow_directio' will be set to 0, and virtiofsd discards
> > O_DIRECT as before. If '-o allow_directio' is added to the stariting
> > command-line, 'allow_directio' will be set to 1, so that the O_DIRECT
> > flags will be retained and host page cache can be bypassed.
> >
> > Signed-off-by: Jiachen Zhang <zhangjiachen.jaycee@bytedance.com>
> > ---
> >  tools/virtiofsd/helper.c         |  4 ++++
> >  tools/virtiofsd/passthrough_ll.c | 20 ++++++++++++++------
> >  2 files changed, 18 insertions(+), 6 deletions(-)
> >
> > diff --git a/tools/virtiofsd/helper.c b/tools/virtiofsd/helper.c
> > index 3105b6c23a..534ff52c64 100644
> > --- a/tools/virtiofsd/helper.c
> > +++ b/tools/virtiofsd/helper.c
> > @@ -180,6 +180,10 @@ void fuse_cmdline_help(void)
> >             "                               (0 leaves rlimit
> unchanged)\n"
> >             "                               default: min(1000000,
> fs.file-max - 16384)\n"
> >             "                                        if the current
> rlimit is lower\n"
> > +           "    -o allow_directio|no_directio\n"
> > +           "                               retain/discard O_DIRECT
> flags passed down\n"
> > +           "                               to virtiofsd from guest
> applications.\n"
> > +           "                               default: no_directio\n"
> >             );
>
> The standard naming convention from existing options is to use
> $OPTNAME and no_$OPTNAME.
>
> IOW, don't use the "allow_" prefix. The options should be just
> "directio" and "no_directio"
>
> Thanks, Daniel. I did consider using "directio" instead of "allow_directio"
before I send out this patch. Although "-o directio" makes it consistent
with other option names, it may confuse the users of virtiofsd.
Because currently, virtiofsd will not add an O_DIRECT to the open flag,
it will just retain or discard the O_DIRECT added by guest applications.
But "-o direct" may make the users think that virtiofsd will do direct IO
all
the time.

Jiachen

>
> Regards,
> Daniel
> --
> |: https://berrange.com      -o-
> https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-
> https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-
> https://www.instagram.com/dberrange :|
>
Re: [External] Re: [PATCH] virtiofsd: add -o allow_directio|no_directio option
Posted by Daniel P. Berrangé 3 years, 8 months ago
On Sat, Aug 22, 2020 at 01:51:04AM +0800, 张佳辰 wrote:
> On Fri, Aug 21, 2020 at 7:58 PM Daniel P. Berrangé <berrange@redhat.com>
> wrote:
> 
> > On Fri, Aug 21, 2020 at 11:41:26AM +0800, Jiachen Zhang wrote:
> > > Due to the commit 65da4539803373ec4eec97ffc49ee90083e56efd, the O_DIRECT
> > > open flag of guest applications will be discarded by virtiofsd. While
> > > this behavior makes it consistent with the virtio-9p scheme when guest
> > > applications using direct I/O, we no longer have any chance to bypass
> > > the host page cache.
> > >
> > > Therefore, we add a flag 'allow_directio' to lo_data. If '-o no_directio'
> > > option is added, or none of '-o no_directio' or '-o allow_directio' is
> > > added, the 'allow_directio' will be set to 0, and virtiofsd discards
> > > O_DIRECT as before. If '-o allow_directio' is added to the stariting
> > > command-line, 'allow_directio' will be set to 1, so that the O_DIRECT
> > > flags will be retained and host page cache can be bypassed.
> > >
> > > Signed-off-by: Jiachen Zhang <zhangjiachen.jaycee@bytedance.com>
> > > ---
> > >  tools/virtiofsd/helper.c         |  4 ++++
> > >  tools/virtiofsd/passthrough_ll.c | 20 ++++++++++++++------
> > >  2 files changed, 18 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/tools/virtiofsd/helper.c b/tools/virtiofsd/helper.c
> > > index 3105b6c23a..534ff52c64 100644
> > > --- a/tools/virtiofsd/helper.c
> > > +++ b/tools/virtiofsd/helper.c
> > > @@ -180,6 +180,10 @@ void fuse_cmdline_help(void)
> > >             "                               (0 leaves rlimit
> > unchanged)\n"
> > >             "                               default: min(1000000,
> > fs.file-max - 16384)\n"
> > >             "                                        if the current
> > rlimit is lower\n"
> > > +           "    -o allow_directio|no_directio\n"
> > > +           "                               retain/discard O_DIRECT
> > flags passed down\n"
> > > +           "                               to virtiofsd from guest
> > applications.\n"
> > > +           "                               default: no_directio\n"
> > >             );
> >
> > The standard naming convention from existing options is to use
> > $OPTNAME and no_$OPTNAME.
> >
> > IOW, don't use the "allow_" prefix. The options should be just
> > "directio" and "no_directio"
> >
> > Thanks, Daniel. I did consider using "directio" instead of "allow_directio"
> before I send out this patch. Although "-o directio" makes it consistent
> with other option names, it may confuse the users of virtiofsd.
> Because currently, virtiofsd will not add an O_DIRECT to the open flag,
> it will just retain or discard the O_DIRECT added by guest applications.
> But "-o direct" may make the users think that virtiofsd will do direct IO
> all
> the time.

Then -o allow_direct_io   and  -o no_allow_direct_io


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|


Re: [External] Re: [PATCH] virtiofsd: add -o allow_directio|no_directio option
Posted by Jiachen Zhang 3 years, 8 months ago
On Mon, Aug 24, 2020 at 5:39 PM Daniel P. Berrangé <berrange@redhat.com>
wrote:

> On Sat, Aug 22, 2020 at 01:51:04AM +0800, 张佳辰 wrote:
> > On Fri, Aug 21, 2020 at 7:58 PM Daniel P. Berrangé <berrange@redhat.com>
> > wrote:
> >
> > > On Fri, Aug 21, 2020 at 11:41:26AM +0800, Jiachen Zhang wrote:
> > > > Due to the commit 65da4539803373ec4eec97ffc49ee90083e56efd, the
> O_DIRECT
> > > > open flag of guest applications will be discarded by virtiofsd. While
> > > > this behavior makes it consistent with the virtio-9p scheme when
> guest
> > > > applications using direct I/O, we no longer have any chance to bypass
> > > > the host page cache.
> > > >
> > > > Therefore, we add a flag 'allow_directio' to lo_data. If '-o
> no_directio'
> > > > option is added, or none of '-o no_directio' or '-o allow_directio'
> is
> > > > added, the 'allow_directio' will be set to 0, and virtiofsd discards
> > > > O_DIRECT as before. If '-o allow_directio' is added to the stariting
> > > > command-line, 'allow_directio' will be set to 1, so that the O_DIRECT
> > > > flags will be retained and host page cache can be bypassed.
> > > >
> > > > Signed-off-by: Jiachen Zhang <zhangjiachen.jaycee@bytedance.com>
> > > > ---
> > > >  tools/virtiofsd/helper.c         |  4 ++++
> > > >  tools/virtiofsd/passthrough_ll.c | 20 ++++++++++++++------
> > > >  2 files changed, 18 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/tools/virtiofsd/helper.c b/tools/virtiofsd/helper.c
> > > > index 3105b6c23a..534ff52c64 100644
> > > > --- a/tools/virtiofsd/helper.c
> > > > +++ b/tools/virtiofsd/helper.c
> > > > @@ -180,6 +180,10 @@ void fuse_cmdline_help(void)
> > > >             "                               (0 leaves rlimit
> > > unchanged)\n"
> > > >             "                               default: min(1000000,
> > > fs.file-max - 16384)\n"
> > > >             "                                        if the current
> > > rlimit is lower\n"
> > > > +           "    -o allow_directio|no_directio\n"
> > > > +           "                               retain/discard O_DIRECT
> > > flags passed down\n"
> > > > +           "                               to virtiofsd from guest
> > > applications.\n"
> > > > +           "                               default: no_directio\n"
> > > >             );
> > >
> > > The standard naming convention from existing options is to use
> > > $OPTNAME and no_$OPTNAME.
> > >
> > > IOW, don't use the "allow_" prefix. The options should be just
> > > "directio" and "no_directio"
> > >
> > > Thanks, Daniel. I did consider using "directio" instead of
> "allow_directio"
> > before I send out this patch. Although "-o directio" makes it consistent
> > with other option names, it may confuse the users of virtiofsd.
> > Because currently, virtiofsd will not add an O_DIRECT to the open flag,
> > it will just retain or discard the O_DIRECT added by guest applications.
> > But "-o direct" may make the users think that virtiofsd will do direct IO
> > all
> > the time.
>
> Then -o allow_direct_io   and  -o no_allow_direct_io
>
>
OK, thanks. I will use these names in the final version.

Jiachen


>
> Regards,
> Daniel
> --
> |: https://berrange.com      -o-
> https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-
> https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-
> https://www.instagram.com/dberrange :|
>
>