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

[Stefan Hajnoczi]

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.

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

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

Stefan