Re: [Virtio-fs] [PATCH] virtiofsd: Reset O_DIRECT flag during file open

[Vivek Goyal <vgoyal@redhat.com>]

On Tue, Aug 20, 2019 at 10:33:29PM +0800, piaojun wrote:
> Hi Vivek & Aneesh,
> 
> On 2019/8/20 21:45, Vivek Goyal wrote:
> > If an application wants to do direct IO and opens a file with O_DIRECT
> > in guest, that does not necessarily mean that we need to bypass page
> > cache on host as well. So reset this flag on host.
> 
> I have a different understanding about this. The I/O mode should be kept
> same both in Guest and Host. When doing direct io in Guest, the user may
> want the data go to disk rather than kept in Host page cache which means
> volatile. So it seems more reasonable that Guest and Host have the same
> file I/0 semantics.

I look at it other way. If we open a file O_DIRECT, it should bypass
page cache on kernel it is running on. It does not guarantee anything
on underlying storage stack. For example, virtual block device or physical
disks could still be caching data with O_DIRECT.

To me same analogy should apply for caching on host. Atleast by default
O_DIRECT should only mean bypassing page cache in guest. If there is
an application which requires bypassing page cache on host as well, then
it should be a server option to control this behavior. But we can do when
there is a real user.

> 
> > 
> > If somebody needs to bypass page cache on host as well (and it is safe to
> > do so), we can add a knob in daemon later to control this behavior.
> 
> This discussion has some relationship with my RFC PATCH "virtiofsd: do
> not fall back to buffer io when cache=auto/always", and we could involve
> the issue mentioned in that email if you do not mind.
> 

I responded to your patch separately and that seems completely unrelated
to this patch.

[..]
> > +	/*
> > +	 * 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.
> > +	 */
> > +	fi->flags &= ~O_DIRECT;
> > +
> 
> We need do the same thing for lo_create().

That's a good point. I have fixed it and will post V2.

Vivek