[Virtio-fs] [PATCH][RFC] viriofsd: do not fall back to buffer io when cache=auto

[Virtio-fs] [PATCH][RFC] viriofsd: do not fall back to buffer io when cache=auto

[piaojun]

Direct io flags will be tured off even if cache=auto, which seems a
little bit strange. It's better to keep the open flags set by user. If
I missed the discussion about this issue, please let me know, thanks.

Signed-off-by: Jun Piao <piaojun@huawei.com>
---
 contrib/virtiofsd/passthrough_ll.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/contrib/virtiofsd/passthrough_ll.c b/contrib/virtiofsd/passthrough_ll.c
index 321bbb2..a080f0d 100644
--- a/contrib/virtiofsd/passthrough_ll.c
+++ b/contrib/virtiofsd/passthrough_ll.c
@@ -1687,6 +1687,8 @@ static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name,
 		fi->direct_io = 1;
 	else if (lo->cache == CACHE_ALWAYS)
 		fi->keep_cache = 1;
+	else
+		fi->direct_io = !!(fi->flags & O_DIRECT);

 out:
 	lo_inode_put(lo, &parent_inode);
@@ -1899,6 +1901,8 @@ static void lo_open(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi)
 		fi->direct_io = 1;
 	else if (lo->cache == CACHE_ALWAYS)
 		fi->keep_cache = 1;
+	else
+		fi->direct_io = !!(fi->flags & O_DIRECT);
 	fuse_reply_open(req, fi);
 }

-- 

Re: [Virtio-fs] [PATCH][RFC] viriofsd: do not fall back to buffer io when cache=auto

[piaojun]

Ping, any comments on this patch?

On 2019/8/14 17:46, piaojun wrote:
> Direct io flags will be tured off even if cache=auto, which seems a
> little bit strange. It's better to keep the open flags set by user. If
> I missed the discussion about this issue, please let me know, thanks.
> 
> Signed-off-by: Jun Piao <piaojun@huawei.com>
> ---
>  contrib/virtiofsd/passthrough_ll.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/contrib/virtiofsd/passthrough_ll.c b/contrib/virtiofsd/passthrough_ll.c
> index 321bbb2..a080f0d 100644
> --- a/contrib/virtiofsd/passthrough_ll.c
> +++ b/contrib/virtiofsd/passthrough_ll.c
> @@ -1687,6 +1687,8 @@ static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name,
>  		fi->direct_io = 1;
>  	else if (lo->cache == CACHE_ALWAYS)
>  		fi->keep_cache = 1;
> +	else
> +		fi->direct_io = !!(fi->flags & O_DIRECT);
> 
>  out:
>  	lo_inode_put(lo, &parent_inode);
> @@ -1899,6 +1901,8 @@ static void lo_open(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi)
>  		fi->direct_io = 1;
>  	else if (lo->cache == CACHE_ALWAYS)
>  		fi->keep_cache = 1;
> +	else
> +		fi->direct_io = !!(fi->flags & O_DIRECT);
>  	fuse_reply_open(req, fi);
>  }
> 

Re: [Virtio-fs] [PATCH][RFC] viriofsd: do not fall back to buffer io when cache=auto

[Eryu Guan]

On Wed, Aug 14, 2019 at 05:46:42PM +0800, piaojun wrote:
> Direct io flags will be tured off even if cache=auto, which seems a
> little bit strange. It's better to keep the open flags set by user. If
> I missed the discussion about this issue, please let me know, thanks.
> 
> Signed-off-by: Jun Piao <piaojun@huawei.com>
> ---
>  contrib/virtiofsd/passthrough_ll.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/contrib/virtiofsd/passthrough_ll.c b/contrib/virtiofsd/passthrough_ll.c
> index 321bbb2..a080f0d 100644
> --- a/contrib/virtiofsd/passthrough_ll.c
> +++ b/contrib/virtiofsd/passthrough_ll.c
> @@ -1687,6 +1687,8 @@ static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name,
>  		fi->direct_io = 1;
>  	else if (lo->cache == CACHE_ALWAYS)
>  		fi->keep_cache = 1;
> +	else
> +		fi->direct_io = !!(fi->flags & O_DIRECT);
> 
>  out:
>  	lo_inode_put(lo, &parent_inode);
> @@ -1899,6 +1901,8 @@ static void lo_open(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi)
>  		fi->direct_io = 1;
>  	else if (lo->cache == CACHE_ALWAYS)
>  		fi->keep_cache = 1;
> +	else
> +		fi->direct_io = !!(fi->flags & O_DIRECT);
>  	fuse_reply_open(req, fi);

IMHO, this is necessary, it seems that in the case cache=auto/always,
virtiofsd already follows what guest requests, i.e.  virtiofsd only does
direct I/O when guest opens file with O_DIRECT.

If I understand the code correctly, fi->direct_io = 1 is used to force
guest to use direct I/O even if guest opens file without O_DIRECT.

Thanks,
Eryu

>  }
> 
> -- 
> 
> _______________________________________________
> Virtio-fs mailing list
> Virtio-fs@redhat.com
> https://www.redhat.com/mailman/listinfo/virtio-fs

Re: [Virtio-fs] [PATCH][RFC] viriofsd: do not fall back to buffer io when cache=auto

[piaojun]

On 2019/8/19 11:15, Eryu Guan wrote:
> On Wed, Aug 14, 2019 at 05:46:42PM +0800, piaojun wrote:
>> Direct io flags will be tured off even if cache=auto, which seems a
>> little bit strange. It's better to keep the open flags set by user. If
>> I missed the discussion about this issue, please let me know, thanks.
>>
>> Signed-off-by: Jun Piao <piaojun@huawei.com>
>> ---
>>  contrib/virtiofsd/passthrough_ll.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/contrib/virtiofsd/passthrough_ll.c b/contrib/virtiofsd/passthrough_ll.c
>> index 321bbb2..a080f0d 100644
>> --- a/contrib/virtiofsd/passthrough_ll.c
>> +++ b/contrib/virtiofsd/passthrough_ll.c
>> @@ -1687,6 +1687,8 @@ static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name,
>>  		fi->direct_io = 1;
>>  	else if (lo->cache == CACHE_ALWAYS)
>>  		fi->keep_cache = 1;
>> +	else
>> +		fi->direct_io = !!(fi->flags & O_DIRECT);
>>
>>  out:
>>  	lo_inode_put(lo, &parent_inode);
>> @@ -1899,6 +1901,8 @@ static void lo_open(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi)
>>  		fi->direct_io = 1;
>>  	else if (lo->cache == CACHE_ALWAYS)
>>  		fi->keep_cache = 1;
>> +	else
>> +		fi->direct_io = !!(fi->flags & O_DIRECT);
>>  	fuse_reply_open(req, fi);
> 
> IMHO, this is necessary, it seems that in the case cache=auto/always,
> virtiofsd already follows what guest requests, i.e.  virtiofsd only does
> direct I/O when guest opens file with O_DIRECT.

>From my testing, virtiofsd won't do direct io even if guest opens file
with O_DIRECT when cache=auto/alway. It's weird to convert direct io to
buffer io at host side.

> 
> If I understand the code correctly, fi->direct_io = 1 is used to force
> guest to use direct I/O even if guest opens file without O_DIRECT.

But in fact, direct I/O will be forced to buffer I/O even if with
O_DIRECT. I will look into this problem by further testing, and very
glad to share my test result.

Jun

Re: [Virtio-fs] [PATCH][RFC] viriofsd: do not fall back to buffer io when cache=auto

[Eryu Guan]

On Mon, Aug 19, 2019 at 11:51:51AM +0800, piaojun wrote:
> 
> 
> On 2019/8/19 11:15, Eryu Guan wrote:
> > On Wed, Aug 14, 2019 at 05:46:42PM +0800, piaojun wrote:
> >> Direct io flags will be tured off even if cache=auto, which seems a
> >> little bit strange. It's better to keep the open flags set by user. If
> >> I missed the discussion about this issue, please let me know, thanks.
> >>
> >> Signed-off-by: Jun Piao <piaojun@huawei.com>
> >> ---
> >>  contrib/virtiofsd/passthrough_ll.c | 4 ++++
> >>  1 file changed, 4 insertions(+)
> >>
> >> diff --git a/contrib/virtiofsd/passthrough_ll.c b/contrib/virtiofsd/passthrough_ll.c
> >> index 321bbb2..a080f0d 100644
> >> --- a/contrib/virtiofsd/passthrough_ll.c
> >> +++ b/contrib/virtiofsd/passthrough_ll.c
> >> @@ -1687,6 +1687,8 @@ static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name,
> >>  		fi->direct_io = 1;
> >>  	else if (lo->cache == CACHE_ALWAYS)
> >>  		fi->keep_cache = 1;
> >> +	else
> >> +		fi->direct_io = !!(fi->flags & O_DIRECT);
> >>
> >>  out:
> >>  	lo_inode_put(lo, &parent_inode);
> >> @@ -1899,6 +1901,8 @@ static void lo_open(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi)
> >>  		fi->direct_io = 1;
> >>  	else if (lo->cache == CACHE_ALWAYS)
> >>  		fi->keep_cache = 1;
> >> +	else
> >> +		fi->direct_io = !!(fi->flags & O_DIRECT);
> >>  	fuse_reply_open(req, fi);
> > 
> > IMHO, this is necessary, it seems that in the case cache=auto/always,

Huge typo here, I meant "not necessary" here..

> > virtiofsd already follows what guest requests, i.e.  virtiofsd only does
> > direct I/O when guest opens file with O_DIRECT.
> 
> From my testing, virtiofsd won't do direct io even if guest opens file
> with O_DIRECT when cache=auto/alway. It's weird to convert direct io to
> buffer io at host side.

That doesn't match what's in my mind, I think FUSE_OPEN will pass guest
open flags to virtiofsd and virtiofsd saves the fd (which is opened with
O_DIRECT) in fd map, then subsequent FUSE_WRITE does direct write to
that fd.

> 
> > 
> > If I understand the code correctly, fi->direct_io = 1 is used to force
> > guest to use direct I/O even if guest opens file without O_DIRECT.
> 
> But in fact, direct I/O will be forced to buffer I/O even if with
> O_DIRECT. I will look into this problem by further testing, and very
> glad to share my test result.

Thanks!

Eryu

Re: [Virtio-fs] [PATCH][RFC] viriofsd: do not fall back to buffer io when cache=auto

[piaojun]

On 2019/8/19 13:07, Eryu Guan wrote:
> On Mon, Aug 19, 2019 at 11:51:51AM +0800, piaojun wrote:
>>
>>
>> On 2019/8/19 11:15, Eryu Guan wrote:
>>> On Wed, Aug 14, 2019 at 05:46:42PM +0800, piaojun wrote:
>>>> Direct io flags will be tured off even if cache=auto, which seems a
>>>> little bit strange. It's better to keep the open flags set by user. If
>>>> I missed the discussion about this issue, please let me know, thanks.
>>>>
>>>> Signed-off-by: Jun Piao <piaojun@huawei.com>
>>>> ---
>>>>  contrib/virtiofsd/passthrough_ll.c | 4 ++++
>>>>  1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/contrib/virtiofsd/passthrough_ll.c b/contrib/virtiofsd/passthrough_ll.c
>>>> index 321bbb2..a080f0d 100644
>>>> --- a/contrib/virtiofsd/passthrough_ll.c
>>>> +++ b/contrib/virtiofsd/passthrough_ll.c
>>>> @@ -1687,6 +1687,8 @@ static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name,
>>>>  		fi->direct_io = 1;
>>>>  	else if (lo->cache == CACHE_ALWAYS)
>>>>  		fi->keep_cache = 1;
>>>> +	else
>>>> +		fi->direct_io = !!(fi->flags & O_DIRECT);
>>>>
>>>>  out:
>>>>  	lo_inode_put(lo, &parent_inode);
>>>> @@ -1899,6 +1901,8 @@ static void lo_open(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi)
>>>>  		fi->direct_io = 1;
>>>>  	else if (lo->cache == CACHE_ALWAYS)
>>>>  		fi->keep_cache = 1;
>>>> +	else
>>>> +		fi->direct_io = !!(fi->flags & O_DIRECT);
>>>>  	fuse_reply_open(req, fi);
>>>
>>> IMHO, this is necessary, it seems that in the case cache=auto/always,
> 
> Huge typo here, I meant "not necessary" here..
> 
>>> virtiofsd already follows what guest requests, i.e.  virtiofsd only does
>>> direct I/O when guest opens file with O_DIRECT.
>>
>> From my testing, virtiofsd won't do direct io even if guest opens file
>> with O_DIRECT when cache=auto/alway. It's weird to convert direct io to
>> buffer io at host side.
> 
> That doesn't match what's in my mind, I think FUSE_OPEN will pass guest
> open flags to virtiofsd and virtiofsd saves the fd (which is opened with
> O_DIRECT) in fd map, then subsequent FUSE_WRITE does direct write to
> that fd.

Yes, I agree with this. And my comment was a little confuing, so I have
to resend the patch later to make it more clear.

Jun

[Virtio-fs] [PATCH RESEND][RFC] virtiofsd: do not fall back to buffer io when cache=auto/always

[piaojun]

When O_DIRECT flags is set by Guest, virtiofsd will open file with
O_DIRECT, but unset 'fi->direct_io' which makes Guest go buffer io path.
That causes inconsistency between Guest and Host.

The cache option in virtiofsd should not affect the file io mode, so
set 'fi->direct_io' according to 'fi->flags'.

Signed-off-by: Jun Piao <piaojun@huawei.com>
---
 contrib/virtiofsd/passthrough_ll.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/contrib/virtiofsd/passthrough_ll.c b/contrib/virtiofsd/passthrough_ll.c
index ca11764..a9c98d0 100644
--- a/contrib/virtiofsd/passthrough_ll.c
+++ b/contrib/virtiofsd/passthrough_ll.c
@@ -1774,8 +1774,10 @@ static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name,
 		fi->fh = fh;
 		err = lo_do_lookup(req, parent, name, &e);
 	}
-	if (lo->cache == CACHE_NONE)
+	if (fi->flags & O_DIRECT)
 		fi->direct_io = 1;
+	if (lo->cache == CACHE_NONE)
+		fi->keep_cache = 0;
 	else if (lo->cache == CACHE_ALWAYS)
 		fi->keep_cache = 1;

@@ -1982,8 +1984,11 @@ static void lo_open(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi)
 	}

 	fi->fh = fh;
-	if (lo->cache == CACHE_NONE)
+
+	if (fi->flags & O_DIRECT)
 		fi->direct_io = 1;
+	if (lo->cache == CACHE_NONE)
+		fi->keep_cache = 0;
 	else if (lo->cache == CACHE_ALWAYS)
 		fi->keep_cache = 1;
 	fuse_reply_open(req, fi);
-- 

Re: [Virtio-fs] [PATCH RESEND][RFC] virtiofsd: do not fall back to buffer io when cache=auto/always

[Eryu Guan]

On Mon, Aug 19, 2019 at 02:49:54PM +0800, piaojun wrote:
> When O_DIRECT flags is set by Guest, virtiofsd will open file with
> O_DIRECT, but unset 'fi->direct_io' which makes Guest go buffer io path.
> That causes inconsistency between Guest and Host.
> 
> The cache option in virtiofsd should not affect the file io mode, so
> set 'fi->direct_io' according to 'fi->flags'.
> 
> Signed-off-by: Jun Piao <piaojun@huawei.com>
> ---
>  contrib/virtiofsd/passthrough_ll.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/contrib/virtiofsd/passthrough_ll.c b/contrib/virtiofsd/passthrough_ll.c
> index ca11764..a9c98d0 100644
> --- a/contrib/virtiofsd/passthrough_ll.c
> +++ b/contrib/virtiofsd/passthrough_ll.c
> @@ -1774,8 +1774,10 @@ static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name,
>  		fi->fh = fh;
>  		err = lo_do_lookup(req, parent, name, &e);
>  	}
> -	if (lo->cache == CACHE_NONE)
> +	if (fi->flags & O_DIRECT)

Looks like this changes cache=none behavior. Previously, we set
direct_io when cache=none and guest will go to dio path even when the
file is opened for buffer io.

But with this change, guest could do buffer io in cache=none case.

>  		fi->direct_io = 1;
> +	if (lo->cache == CACHE_NONE)
> +		fi->keep_cache = 0;

And this only causes guest to invalidate page cache at open(2) time, but
still go buffer io path on cache=none.

>  	else if (lo->cache == CACHE_ALWAYS)
>  		fi->keep_cache = 1;

I'm thinking about something like (totally untested):

--- a/contrib/virtiofsd/passthrough_ll.c
+++ b/contrib/virtiofsd/passthrough_ll.c
@@ -1774,7 +1774,7 @@ static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name,
                fi->fh = fh;
                err = lo_do_lookup(req, parent, name, &e);
        }
-       if (lo->cache == CACHE_NONE)
+       if (lo->cache == CACHE_NONE || fi->flags & O_DIRECT)
                fi->direct_io = 1;
        else if (lo->cache == CACHE_ALWAYS)
                fi->keep_cache = 1;
@@ -1982,7 +1982,7 @@ static void lo_open(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi)
        }

        fi->fh = fh;
-       if (lo->cache == CACHE_NONE)
+       if (lo->cache == CACHE_NONE || fi->flags & O_DIRECT)
                fi->direct_io = 1;
        else if (lo->cache == CACHE_ALWAYS)
                fi->keep_cache = 1;

> 
> @@ -1982,8 +1984,11 @@ static void lo_open(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi)
>  	}
> 
>  	fi->fh = fh;
> -	if (lo->cache == CACHE_NONE)
> +
> +	if (fi->flags & O_DIRECT)
>  		fi->direct_io = 1;
> +	if (lo->cache == CACHE_NONE)
> +		fi->keep_cache = 0;
>  	else if (lo->cache == CACHE_ALWAYS)
>  		fi->keep_cache = 1;

And this code snippet duplicates with lo_create(), maybe we could factor
it into a new helper function.

Thanks,
Eryu

Re: [Virtio-fs] [PATCH RESEND][RFC] virtiofsd: do not fall back to buffer io when cache=auto/always

[piaojun]

On 2019/8/19 15:34, Eryu Guan wrote:
> On Mon, Aug 19, 2019 at 02:49:54PM +0800, piaojun wrote:
>> When O_DIRECT flags is set by Guest, virtiofsd will open file with
>> O_DIRECT, but unset 'fi->direct_io' which makes Guest go buffer io path.
>> That causes inconsistency between Guest and Host.
>>
>> The cache option in virtiofsd should not affect the file io mode, so
>> set 'fi->direct_io' according to 'fi->flags'.
>>
>> Signed-off-by: Jun Piao <piaojun@huawei.com>
>> ---
>>  contrib/virtiofsd/passthrough_ll.c | 9 +++++++--
>>  1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/contrib/virtiofsd/passthrough_ll.c b/contrib/virtiofsd/passthrough_ll.c
>> index ca11764..a9c98d0 100644
>> --- a/contrib/virtiofsd/passthrough_ll.c
>> +++ b/contrib/virtiofsd/passthrough_ll.c
>> @@ -1774,8 +1774,10 @@ static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name,
>>  		fi->fh = fh;
>>  		err = lo_do_lookup(req, parent, name, &e);
>>  	}
>> -	if (lo->cache == CACHE_NONE)
>> +	if (fi->flags & O_DIRECT)
> 
> Looks like this changes cache=none behavior. Previously, we set
> direct_io when cache=none and guest will go to dio path even when the
> file is opened for buffer io.
> 
> But with this change, guest could do buffer io in cache=none case.
> 

IMO, the cache option is only used for determining whether using the
last inode's cache when do open(). The user could choose direct or
buffer io no matter if the cache exists.

Moreover, the cache option only works for the first time buffer io, and
has little effect on subsequent ones.

>>  		fi->direct_io = 1;
>> +	if (lo->cache == CACHE_NONE)
>> +		fi->keep_cache = 0;
> 
> And this only causes guest to invalidate page cache at open(2) time, but
> still go buffer io path on cache=none.

As my comments above, io path has nothing to do with inode's current
page cache.

> 
>>  	else if (lo->cache == CACHE_ALWAYS)
>>  		fi->keep_cache = 1;
> 
> I'm thinking about something like (totally untested):
> 
> --- a/contrib/virtiofsd/passthrough_ll.c
> +++ b/contrib/virtiofsd/passthrough_ll.c
> @@ -1774,7 +1774,7 @@ static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name,
>                 fi->fh = fh;
>                 err = lo_do_lookup(req, parent, name, &e);
>         }
> -       if (lo->cache == CACHE_NONE)
> +       if (lo->cache == CACHE_NONE || fi->flags & O_DIRECT)
>                 fi->direct_io = 1;
>         else if (lo->cache == CACHE_ALWAYS)
>                 fi->keep_cache = 1;
> @@ -1982,7 +1982,7 @@ static void lo_open(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi)
>         }
> 
>         fi->fh = fh;
> -       if (lo->cache == CACHE_NONE)
> +       if (lo->cache == CACHE_NONE || fi->flags & O_DIRECT)
>                 fi->direct_io = 1;
>         else if (lo->cache == CACHE_ALWAYS)
>                 fi->keep_cache = 1;

This will mix cache option with io flags which seems weird.

> 
>>
>> @@ -1982,8 +1984,11 @@ static void lo_open(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi)
>>  	}
>>
>>  	fi->fh = fh;
>> -	if (lo->cache == CACHE_NONE)
>> +
>> +	if (fi->flags & O_DIRECT)
>>  		fi->direct_io = 1;
>> +	if (lo->cache == CACHE_NONE)
>> +		fi->keep_cache = 0;
>>  	else if (lo->cache == CACHE_ALWAYS)
>>  		fi->keep_cache = 1;
> 
> And this code snippet duplicates with lo_create(), maybe we could factor
> it into a new helper function.

It makes sense and I will consider adding a helper.

Jun

Re: [Virtio-fs] [PATCH RESEND][RFC] virtiofsd: do not fall back to buffer io when cache=auto/always

[Vivek Goyal]

On Mon, Aug 19, 2019 at 02:49:54PM +0800, piaojun wrote:
> When O_DIRECT flags is set by Guest, virtiofsd will open file with
> O_DIRECT, but unset 'fi->direct_io' which makes Guest go buffer io path.

Where is virtiofsd unset fi->direct_io flag? All I can see is that it
sets this flag if cache=none?

Thanks
Vivek

> That causes inconsistency between Guest and Host.
> 
> The cache option in virtiofsd should not affect the file io mode, so
> set 'fi->direct_io' according to 'fi->flags'.
> 
> Signed-off-by: Jun Piao <piaojun@huawei.com>
> ---
>  contrib/virtiofsd/passthrough_ll.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/contrib/virtiofsd/passthrough_ll.c b/contrib/virtiofsd/passthrough_ll.c
> index ca11764..a9c98d0 100644
> --- a/contrib/virtiofsd/passthrough_ll.c
> +++ b/contrib/virtiofsd/passthrough_ll.c
> @@ -1774,8 +1774,10 @@ static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name,
>  		fi->fh = fh;
>  		err = lo_do_lookup(req, parent, name, &e);
>  	}
> -	if (lo->cache == CACHE_NONE)
> +	if (fi->flags & O_DIRECT)
>  		fi->direct_io = 1;
> +	if (lo->cache == CACHE_NONE)
> +		fi->keep_cache = 0;
>  	else if (lo->cache == CACHE_ALWAYS)
>  		fi->keep_cache = 1;
> 
> @@ -1982,8 +1984,11 @@ static void lo_open(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi)
>  	}
> 
>  	fi->fh = fh;
> -	if (lo->cache == CACHE_NONE)
> +
> +	if (fi->flags & O_DIRECT)
>  		fi->direct_io = 1;
> +	if (lo->cache == CACHE_NONE)
> +		fi->keep_cache = 0;
>  	else if (lo->cache == CACHE_ALWAYS)
>  		fi->keep_cache = 1;
>  	fuse_reply_open(req, fi);
> -- 
> 
> _______________________________________________
> Virtio-fs mailing list
> Virtio-fs@redhat.com
> https://www.redhat.com/mailman/listinfo/virtio-fs

Re: [Virtio-fs] [PATCH RESEND][RFC] virtiofsd: do not fall back to buffer io when cache=auto/always

[Vivek Goyal]

On Tue, Aug 20, 2019 at 11:39:26AM -0400, Vivek Goyal wrote:
> On Mon, Aug 19, 2019 at 02:49:54PM +0800, piaojun wrote:
> > When O_DIRECT flags is set by Guest, virtiofsd will open file with
> > O_DIRECT, but unset 'fi->direct_io' which makes Guest go buffer io path.
> 
> Where is virtiofsd unset fi->direct_io flag? All I can see is that it
> sets this flag if cache=none?
> 

Ok, I looked at it little bit more closely and I don't think it is a
problem. With cache=always/auto, we don't set fi->direct_io. That means
server is not enforcing a direct I/O policy on client.

Now if client decides to direct I/O by opening file with O_DIRECT, it
will still take direct I/O path and call fuse_direct_IO(). Look at
fuse_file_aops->fuse_direct_IO().

Thanks
Vivek

> Thanks
> Vivek
> 
> > That causes inconsistency between Guest and Host.
> > 
> > The cache option in virtiofsd should not affect the file io mode, so
> > set 'fi->direct_io' according to 'fi->flags'.
> > 
> > Signed-off-by: Jun Piao <piaojun@huawei.com>
> > ---
> >  contrib/virtiofsd/passthrough_ll.c | 9 +++++++--
> >  1 file changed, 7 insertions(+), 2 deletions(-)
> > 
> > diff --git a/contrib/virtiofsd/passthrough_ll.c b/contrib/virtiofsd/passthrough_ll.c
> > index ca11764..a9c98d0 100644
> > --- a/contrib/virtiofsd/passthrough_ll.c
> > +++ b/contrib/virtiofsd/passthrough_ll.c
> > @@ -1774,8 +1774,10 @@ static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name,
> >  		fi->fh = fh;
> >  		err = lo_do_lookup(req, parent, name, &e);
> >  	}
> > -	if (lo->cache == CACHE_NONE)
> > +	if (fi->flags & O_DIRECT)
> >  		fi->direct_io = 1;
> > +	if (lo->cache == CACHE_NONE)
> > +		fi->keep_cache = 0;
> >  	else if (lo->cache == CACHE_ALWAYS)
> >  		fi->keep_cache = 1;
> > 
> > @@ -1982,8 +1984,11 @@ static void lo_open(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi)
> >  	}
> > 
> >  	fi->fh = fh;
> > -	if (lo->cache == CACHE_NONE)
> > +
> > +	if (fi->flags & O_DIRECT)
> >  		fi->direct_io = 1;
> > +	if (lo->cache == CACHE_NONE)
> > +		fi->keep_cache = 0;
> >  	else if (lo->cache == CACHE_ALWAYS)
> >  		fi->keep_cache = 1;
> >  	fuse_reply_open(req, fi);
> > -- 
> > 
> > _______________________________________________
> > Virtio-fs mailing list
> > Virtio-fs@redhat.com
> > https://www.redhat.com/mailman/listinfo/virtio-fs

Re: [Virtio-fs] [PATCH RESEND][RFC] virtiofsd: do not fall back to buffer io when cache=auto/always

[piaojun]

On 2019/8/21 1:56, Vivek Goyal wrote:
> On Tue, Aug 20, 2019 at 11:39:26AM -0400, Vivek Goyal wrote:
>> On Mon, Aug 19, 2019 at 02:49:54PM +0800, piaojun wrote:
>>> When O_DIRECT flags is set by Guest, virtiofsd will open file with
>>> O_DIRECT, but unset 'fi->direct_io' which makes Guest go buffer io path.
>>
>> Where is virtiofsd unset fi->direct_io flag? All I can see is that it
>> sets this flag if cache=none?
>>
> 
> Ok, I looked at it little bit more closely and I don't think it is a
> problem. With cache=always/auto, we don't set fi->direct_io. That means
> server is not enforcing a direct I/O policy on client.
> 
> Now if client decides to direct I/O by opening file with O_DIRECT, it
> will still take direct I/O path and call fuse_direct_IO(). Look at
> fuse_file_aops->fuse_direct_IO().

The problem is that when Guest open with O_DIRECT, the Host won't set
open_flags with FOPEN_DIRECT_IO if CACHE_AUTO is set. It makes Guest
goes into fuse_cache_write_iter() rather than fuse_direct_write_iter().
In other words, the Guest's I/O mode will be changed by virtiofsd.

You cleared my first doubt that 'fi->direct_io' and 'fi->keep_cache'
are used to control Guest I/O mode together, and there may be different
I/O modes between Guest and Host if I undersatnd it correctly. I admit
this offers a more flexible config to user, but also introduces more
complex option. Maybe we could just use *keep_cache* to handle all
these options.

My second doubt is that *keep_cache* is used to control whether reusing
the cache of last open(). IMO, it has nothing to do with the following
I/O path.

Jun

> 
> Thanks
> Vivek
> 
>> Thanks
>> Vivek
>>
>>> That causes inconsistency between Guest and Host.
>>>
>>> The cache option in virtiofsd should not affect the file io mode, so
>>> set 'fi->direct_io' according to 'fi->flags'.
>>>
>>> Signed-off-by: Jun Piao <piaojun@huawei.com>
>>> ---
>>>  contrib/virtiofsd/passthrough_ll.c | 9 +++++++--
>>>  1 file changed, 7 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/contrib/virtiofsd/passthrough_ll.c b/contrib/virtiofsd/passthrough_ll.c
>>> index ca11764..a9c98d0 100644
>>> --- a/contrib/virtiofsd/passthrough_ll.c
>>> +++ b/contrib/virtiofsd/passthrough_ll.c
>>> @@ -1774,8 +1774,10 @@ static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name,
>>>  		fi->fh = fh;
>>>  		err = lo_do_lookup(req, parent, name, &e);
>>>  	}
>>> -	if (lo->cache == CACHE_NONE)
>>> +	if (fi->flags & O_DIRECT)
>>>  		fi->direct_io = 1;
>>> +	if (lo->cache == CACHE_NONE)
>>> +		fi->keep_cache = 0;
>>>  	else if (lo->cache == CACHE_ALWAYS)
>>>  		fi->keep_cache = 1;
>>>
>>> @@ -1982,8 +1984,11 @@ static void lo_open(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi)
>>>  	}
>>>
>>>  	fi->fh = fh;
>>> -	if (lo->cache == CACHE_NONE)
>>> +
>>> +	if (fi->flags & O_DIRECT)
>>>  		fi->direct_io = 1;
>>> +	if (lo->cache == CACHE_NONE)
>>> +		fi->keep_cache = 0;
>>>  	else if (lo->cache == CACHE_ALWAYS)
>>>  		fi->keep_cache = 1;
>>>  	fuse_reply_open(req, fi);
>>> -- 
>>>
>>> _______________________________________________
>>> Virtio-fs mailing list
>>> Virtio-fs@redhat.com
>>> https://www.redhat.com/mailman/listinfo/virtio-fs
> .
> 

Re: [Virtio-fs] [PATCH RESEND][RFC] virtiofsd: do not fall back to buffer io when cache=auto/always

[Vivek Goyal]

On Wed, Aug 21, 2019 at 09:48:18AM +0800, piaojun wrote:
> 
> 
> On 2019/8/21 1:56, Vivek Goyal wrote:
> > On Tue, Aug 20, 2019 at 11:39:26AM -0400, Vivek Goyal wrote:
> >> On Mon, Aug 19, 2019 at 02:49:54PM +0800, piaojun wrote:
> >>> When O_DIRECT flags is set by Guest, virtiofsd will open file with
> >>> O_DIRECT, but unset 'fi->direct_io' which makes Guest go buffer io path.
> >>
> >> Where is virtiofsd unset fi->direct_io flag? All I can see is that it
> >> sets this flag if cache=none?
> >>
> > 
> > Ok, I looked at it little bit more closely and I don't think it is a
> > problem. With cache=always/auto, we don't set fi->direct_io. That means
> > server is not enforcing a direct I/O policy on client.
> > 
> > Now if client decides to direct I/O by opening file with O_DIRECT, it
> > will still take direct I/O path and call fuse_direct_IO(). Look at
> > fuse_file_aops->fuse_direct_IO().
> 
> The problem is that when Guest open with O_DIRECT, the Host won't set
> open_flags with FOPEN_DIRECT_IO if CACHE_AUTO is set. It makes Guest
> goes into fuse_cache_write_iter() rather than fuse_direct_write_iter().
> In other words, the Guest's I/O mode will be changed by virtiofsd.

I think there are two direct I/O paths. One is where server requests
that direct I/O be done on the file and in that case FOPEN_DIRECT_IO
is set.

Other case is where client initiates direct I/O and in that case
FOPEN_DIRECT_IO is not set and we take regular path which is
capable of doing both direct and buffered I/O. For example, for
reads we do following.

fuse_file_read_iter()
   fuse_cache_read_iter()
      generic_file_read_iter()
	mapping->a_ops->direct_IO() <--- (If iocb->ki_flags & IOCB_DIRECT)
	  fuse_direct_IO()

Similary for write path.

fuse_file_write_iter()
  fuse_cache_write_iter()
     generic_file_direct_write() <----- (If iocb->ki_flags & IOCB_DIRECT)
        mapping->a_ops->direct_IO()
	   fuse_direct_IO()

Can you give it a try. I don't think we have the problem what you
are describing.

> 
> You cleared my first doubt that 'fi->direct_io' and 'fi->keep_cache'
> are used to control Guest I/O mode together, and there may be different
> I/O modes between Guest and Host if I undersatnd it correctly. I admit
> this offers a more flexible config to user, but also introduces more
> complex option. Maybe we could just use *keep_cache* to handle all
> these options.
> 
> My second doubt is that *keep_cache* is used to control whether reusing
> the cache of last open(). IMO, it has nothing to do with the following
> I/O path.

FOPEN_KEEP_CACHE is already part of fuse protocol. cache=always is just
making use of that option so that cache contents are not discarded upon
next open. 

Thanks
Vivek

Re: [Virtio-fs] [PATCH RESEND][RFC] virtiofsd: do not fall back to buffer io when cache=auto/always

[piaojun]

On 2019/8/21 21:11, Vivek Goyal wrote:
> On Wed, Aug 21, 2019 at 09:48:18AM +0800, piaojun wrote:
>>
>>
>> On 2019/8/21 1:56, Vivek Goyal wrote:
>>> On Tue, Aug 20, 2019 at 11:39:26AM -0400, Vivek Goyal wrote:
>>>> On Mon, Aug 19, 2019 at 02:49:54PM +0800, piaojun wrote:
>>>>> When O_DIRECT flags is set by Guest, virtiofsd will open file with
>>>>> O_DIRECT, but unset 'fi->direct_io' which makes Guest go buffer io path.
>>>>
>>>> Where is virtiofsd unset fi->direct_io flag? All I can see is that it
>>>> sets this flag if cache=none?
>>>>
>>>
>>> Ok, I looked at it little bit more closely and I don't think it is a
>>> problem. With cache=always/auto, we don't set fi->direct_io. That means
>>> server is not enforcing a direct I/O policy on client.
>>>
>>> Now if client decides to direct I/O by opening file with O_DIRECT, it
>>> will still take direct I/O path and call fuse_direct_IO(). Look at
>>> fuse_file_aops->fuse_direct_IO().
>>
>> The problem is that when Guest open with O_DIRECT, the Host won't set
>> open_flags with FOPEN_DIRECT_IO if CACHE_AUTO is set. It makes Guest
>> goes into fuse_cache_write_iter() rather than fuse_direct_write_iter().
>> In other words, the Guest's I/O mode will be changed by virtiofsd.
> 
> I think there are two direct I/O paths. One is where server requests
> that direct I/O be done on the file and in that case FOPEN_DIRECT_IO
> is set.
> 
> Other case is where client initiates direct I/O and in that case
> FOPEN_DIRECT_IO is not set and we take regular path which is
> capable of doing both direct and buffered I/O. For example, for
> reads we do following.
> 
> fuse_file_read_iter()
>    fuse_cache_read_iter()
>       generic_file_read_iter()
> 	mapping->a_ops->direct_IO() <--- (If iocb->ki_flags & IOCB_DIRECT)
> 	  fuse_direct_IO()
> 
> Similary for write path.
> 
> fuse_file_write_iter()
>   fuse_cache_write_iter()
>      generic_file_direct_write() <----- (If iocb->ki_flags & IOCB_DIRECT)
>         mapping->a_ops->direct_IO()
> 	   fuse_direct_IO()
> 
> Can you give it a try. I don't think we have the problem what you
> are describing.

Thanks for your detailed explanation, and perhaps I missed the regular
path you mentioned above, so I will have a try to confirm this later.

> 
>>
>> You cleared my first doubt that 'fi->direct_io' and 'fi->keep_cache'
>> are used to control Guest I/O mode together, and there may be different
>> I/O modes between Guest and Host if I undersatnd it correctly. I admit
>> this offers a more flexible config to user, but also introduces more
>> complex option. Maybe we could just use *keep_cache* to handle all
>> these options.
>>
>> My second doubt is that *keep_cache* is used to control whether reusing
>> the cache of last open(). IMO, it has nothing to do with the following
>> I/O path.
> 
> FOPEN_KEEP_CACHE is already part of fuse protocol. cache=always is just
> making use of that option so that cache contents are not discarded upon
> next open.

It still confused me that probably we needn't set fi->direct_io if
CACHE_NONE is set to indicate Guest go direct I/O path. As Guest could
also go direct I/O through regular path you have mentioned above.

Jun

Re: [Virtio-fs] [PATCH RESEND][RFC] virtiofsd: do not fall back to buffer io when cache=auto/always

[Vivek Goyal]

On Wed, Aug 21, 2019 at 10:38:39PM +0800, piaojun wrote:

[..]
> >>
> >> You cleared my first doubt that 'fi->direct_io' and 'fi->keep_cache'
> >> are used to control Guest I/O mode together, and there may be different
> >> I/O modes between Guest and Host if I undersatnd it correctly. I admit
> >> this offers a more flexible config to user, but also introduces more
> >> complex option. Maybe we could just use *keep_cache* to handle all
> >> these options.
> >>
> >> My second doubt is that *keep_cache* is used to control whether reusing
> >> the cache of last open(). IMO, it has nothing to do with the following
> >> I/O path.
> > 
> > FOPEN_KEEP_CACHE is already part of fuse protocol. cache=always is just
> > making use of that option so that cache contents are not discarded upon
> > next open.
> 
> It still confused me that probably we needn't set fi->direct_io if
> CACHE_NONE is set to indicate Guest go direct I/O path. As Guest could
> also go direct I/O through regular path you have mentioned above.

fi->direct_io is needed so that server can *enforce* direct I/O on guest.
If guest applications are not doing direct I/O, without fi->direct_io,
you can't force client to do direct I/O.

Vivek

Re: [Virtio-fs] [PATCH RESEND][RFC] virtiofsd: do not fall back to buffer io when cache=auto/always

[piaojun]

On 2019/8/21 22:47, Vivek Goyal wrote:
> On Wed, Aug 21, 2019 at 10:38:39PM +0800, piaojun wrote:
> 
> [..]
>>>>
>>>> You cleared my first doubt that 'fi->direct_io' and 'fi->keep_cache'
>>>> are used to control Guest I/O mode together, and there may be different
>>>> I/O modes between Guest and Host if I undersatnd it correctly. I admit
>>>> this offers a more flexible config to user, but also introduces more
>>>> complex option. Maybe we could just use *keep_cache* to handle all
>>>> these options.
>>>>
>>>> My second doubt is that *keep_cache* is used to control whether reusing
>>>> the cache of last open(). IMO, it has nothing to do with the following
>>>> I/O path.
>>>
>>> FOPEN_KEEP_CACHE is already part of fuse protocol. cache=always is just
>>> making use of that option so that cache contents are not discarded upon
>>> next open.
>>
>> It still confused me that probably we needn't set fi->direct_io if
>> CACHE_NONE is set to indicate Guest go direct I/O path. As Guest could
>> also go direct I/O through regular path you have mentioned above.
> 
> fi->direct_io is needed so that server can *enforce* direct I/O on guest.
> If guest applications are not doing direct I/O, without fi->direct_io,
> you can't force client to do direct I/O.

I get yout point, it seems nothing wrong with it, but it indeed made me
puzzled once. Maybe we'd better wait for some response from real user in
the future.

Thanks,
Jun

> 
> Vivek
> 

Re: [Virtio-fs] [PATCH RESEND][RFC] virtiofsd: do not fall back to buffer io when cache=auto/always

[piaojun]

On 2019/8/21 21:11, Vivek Goyal wrote:
> On Wed, Aug 21, 2019 at 09:48:18AM +0800, piaojun wrote:
>>
>>
>> On 2019/8/21 1:56, Vivek Goyal wrote:
>>> On Tue, Aug 20, 2019 at 11:39:26AM -0400, Vivek Goyal wrote:
>>>> On Mon, Aug 19, 2019 at 02:49:54PM +0800, piaojun wrote:
>>>>> When O_DIRECT flags is set by Guest, virtiofsd will open file with
>>>>> O_DIRECT, but unset 'fi->direct_io' which makes Guest go buffer io path.
>>>>
>>>> Where is virtiofsd unset fi->direct_io flag? All I can see is that it
>>>> sets this flag if cache=none?
>>>>
>>>
>>> Ok, I looked at it little bit more closely and I don't think it is a
>>> problem. With cache=always/auto, we don't set fi->direct_io. That means
>>> server is not enforcing a direct I/O policy on client.
>>>
>>> Now if client decides to direct I/O by opening file with O_DIRECT, it
>>> will still take direct I/O path and call fuse_direct_IO(). Look at
>>> fuse_file_aops->fuse_direct_IO().
>>
>> The problem is that when Guest open with O_DIRECT, the Host won't set
>> open_flags with FOPEN_DIRECT_IO if CACHE_AUTO is set. It makes Guest
>> goes into fuse_cache_write_iter() rather than fuse_direct_write_iter().
>> In other words, the Guest's I/O mode will be changed by virtiofsd.
> 
> I think there are two direct I/O paths. One is where server requests
> that direct I/O be done on the file and in that case FOPEN_DIRECT_IO
> is set.
> 
> Other case is where client initiates direct I/O and in that case
> FOPEN_DIRECT_IO is not set and we take regular path which is
> capable of doing both direct and buffered I/O. For example, for
> reads we do following.
> 
> fuse_file_read_iter()
>    fuse_cache_read_iter()
>       generic_file_read_iter()
> 	mapping->a_ops->direct_IO() <--- (If iocb->ki_flags & IOCB_DIRECT)
> 	  fuse_direct_IO()
> 
> Similary for write path.
> 
> fuse_file_write_iter()
>   fuse_cache_write_iter()
>      generic_file_direct_write() <----- (If iocb->ki_flags & IOCB_DIRECT)
>         mapping->a_ops->direct_IO()
> 	   fuse_direct_IO()
> 
> Can you give it a try. I don't think we have the problem what you
> are describing.
> 

I make a try as you said, and it indeed goes direct path in guest even
if cache=always. So my doubt has been cleared.

Thanks,
Jun

Re: [Virtio-fs] [PATCH][RFC] viriofsd: do not fall back to buffer io when cache=auto

[Vivek Goyal]

On Wed, Aug 14, 2019 at 05:46:42PM +0800, piaojun wrote:
> Direct io flags will be tured off even if cache=auto, which seems a
> little bit strange. It's better to keep the open flags set by user. If
> I missed the discussion about this issue, please let me know, thanks.

fi->direct_io and fi->keep_cache are controlling hints for guest kernel.
So for cache=none, we want to enforce direct I/O in guest hence we set
fi->direct_io (FOPEN_DIRECT_IO) in reply to guest.

For cache=always, we want to cache data and metadata hence we don't
set fi->direct_io. Also, we don't want to invalidate current cache
contents of file in guest hence we set fi->keep_cache (FOPEN_KEEP_CACHE)

For cache=auto, want guest kernel to cache data and metadata hence we
don't set fi->direct_io. We want guest kernel to invalidate data on 
next open hence we don't set fi->keep_cache. 

If guest is doing direct IO on file and fi->flags has O_DIRECT set, that
should not do anything to fi->direct_io. That's a hint for guest kernel
enforced by virtiofsd.

I am not sure what problem you are trying to solve with this patch.

Thanks
Vivek

> 
> Signed-off-by: Jun Piao <piaojun@huawei.com>
> ---
>  contrib/virtiofsd/passthrough_ll.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/contrib/virtiofsd/passthrough_ll.c b/contrib/virtiofsd/passthrough_ll.c
> index 321bbb2..a080f0d 100644
> --- a/contrib/virtiofsd/passthrough_ll.c
> +++ b/contrib/virtiofsd/passthrough_ll.c
> @@ -1687,6 +1687,8 @@ static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name,
>  		fi->direct_io = 1;
>  	else if (lo->cache == CACHE_ALWAYS)
>  		fi->keep_cache = 1;
> +	else
> +		fi->direct_io = !!(fi->flags & O_DIRECT);
> 
>  out:
>  	lo_inode_put(lo, &parent_inode);
> @@ -1899,6 +1901,8 @@ static void lo_open(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi)
>  		fi->direct_io = 1;
>  	else if (lo->cache == CACHE_ALWAYS)
>  		fi->keep_cache = 1;
> +	else
> +		fi->direct_io = !!(fi->flags & O_DIRECT);
>  	fuse_reply_open(req, fi);
>  }
> 
> -- 
> 
> _______________________________________________
> Virtio-fs mailing list
> Virtio-fs@redhat.com
> https://www.redhat.com/mailman/listinfo/virtio-fs