Re: [Qemu-devel] [PATCH -V2 4/6] hw/9pfs: Implement syncfs

[Aneesh Kumar K. V]

On Tue, 1 Mar 2011 15:59:19 +0000, Stefan Hajnoczi <address@hidden> wrote:
> On Tue, Mar 1, 2011 at 3:02 PM, Aneesh Kumar K. V
> <address@hidden> wrote:
> > On Tue, 1 Mar 2011 10:22:07 +0000, Stefan Hajnoczi <address@hidden> wrote:
> >> On Tue, Mar 1, 2011 at 6:38 AM, Aneesh Kumar K.V
> >> <address@hidden> wrote:
> >> > Signed-off-by: Aneesh Kumar K.V <address@hidden>
> >> > ---
> >> >  hw/9pfs/virtio-9p.c |   31 +++++++++++++++++++++++++++++++
> >> >  hw/9pfs/virtio-9p.h |    2 ++
> >> >  2 files changed, 33 insertions(+), 0 deletions(-)
> >> >
> >> > diff --git a/hw/9pfs/virtio-9p.c b/hw/9pfs/virtio-9p.c
> >> > index c4b0198..882f4f3 100644
> >> > --- a/hw/9pfs/virtio-9p.c
> >> > +++ b/hw/9pfs/virtio-9p.c
> >> > @@ -1978,6 +1978,36 @@ static void v9fs_fsync(V9fsState *s, V9fsPDU *pdu)
> >> >     v9fs_post_do_fsync(s, pdu, err);
> >> >  }
> >> >
> >> > +static void v9fs_post_do_syncfs(V9fsState *s, V9fsPDU *pdu, int err)
> >> > +{
> >> > +    if (err == -1) {
> >> > +        err = -errno;
> >> > +    }
> >> > +    complete_pdu(s, pdu, err);
> >> > +}
> >> > +
> >> > +static void v9fs_syncfs(V9fsState *s, V9fsPDU *pdu)
> >> > +{
> >> > +    int err;
> >> > +    int32_t fid;
> >> > +    size_t offset = 7;
> >> > +    V9fsFidState *fidp;
> >> > +
> >> > +    pdu_unmarshal(pdu, offset, "d", &fid);
> >> > +    fidp = lookup_fid(s, fid);
> >> > +    if (fidp == NULL) {
> >> > +        err = -ENOENT;
> >> > +        v9fs_post_do_syncfs(s, pdu, err);
> >> > +        return;
> >> > +    }
> >> > +    /*
> >> > +     * We don't have per file system syncfs
> >> > +     * So just return success
> >> > +     */
> >> > +    err = 0;
> >> > +    v9fs_post_do_syncfs(s, pdu, err);
> >> > +}
> >>
> >> Please explain the semantics of P9_TSYNCFS.  Won't returning success
> >> without doing anything lead to data integrity issues?
> >
> > I should actually do the 9P Operation format as commit message. Will
> > add in the next update. Whether returning here would cause a data
> > integrity issue, it depends what sort of guarantee we want to
> > provide. So calling sync on the guest will cause all the dirty pages in
> > the guest to be flushed to host. Now all those changes are in the host
> > page cache and it would be nice to flush them  as a part of sync but
> > then since we don't have a per file system sync, the above would imply
> > we flush all dirty pages on the host which can result in large
> > performance impact.
> 
> You get the define the semantics of P9_TSYNCFS?  I thought this is
> part of a well-defined protocol :).  If this is a .L extension then
> it's probably a bad design and shouldn't be added to the protocol if
> we can't implement it.

It is a part of .L extension and we can definitely implement it. There
is patch out there which is yet to be merged 

http://thread.gmane.org/gmane.linux.file-systems/44628

> 
> Is this operation supposed to flush the disk write cache too?

I am not sure we need to specify that as a part of 9p operation. I guess
we can only say maximum possible data integrity. Whether a sync will
cause disk write cache flush depends on the file system. For ext* that
can be controlled by mount option barrier. 

> 
> I think virtio-9p has a file descriptor cache.  Would it be possible
> to fsync() those file descriptors?
> 

Ideally we should. But that would involve a large number of fsync calls.

-aneesh