Patchwork [virtio-9p] Add datasync to server side TFSYNC/RFSYNC for dotl

login
register
mail settings
Submitter jvrao
Date Oct. 22, 2010, 7:15 p.m.
Message ID <1287774922-19033-1-git-send-email-jvrao@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/68938/
State New
Headers show

Comments

jvrao - Oct. 22, 2010, 7:15 p.m.
SYNOPSIS
    size[4] Tfsync tag[2] fid[4] datasync[4]

    size[4] Rfsync tag[2]

DESCRIPTION

    The Tfsync transaction transfers ("flushes") all modified in-core data of
    file identified by fid to the disk device (or other  permanent  storage
    device)  where that  file  resides.

    If datasync flag is specified data will be fleshed but does not flush
    modified metadata unless  that  metadata  is  needed  in order to allow a
    subsequent data retrieval to be correctly handled.

Signed-off-by: Venkateswararao Jujjuri <jvrao@linux.vnet.ibm.com>
---
 hw/file-op-9p.h      |    2 +-
 hw/virtio-9p-local.c |    8 ++++++--
 hw/virtio-9p.c       |   11 ++++++-----
 3 files changed, 13 insertions(+), 8 deletions(-)
Blue Swirl - Oct. 23, 2010, 12:44 p.m.
On Fri, Oct 22, 2010 at 7:15 PM, Venkateswararao Jujjuri (JV)
<jvrao@linux.vnet.ibm.com> wrote:
> SYNOPSIS
>    size[4] Tfsync tag[2] fid[4] datasync[4]
>
>    size[4] Rfsync tag[2]
>
> DESCRIPTION
>
>    The Tfsync transaction transfers ("flushes") all modified in-core data of
>    file identified by fid to the disk device (or other  permanent  storage
>    device)  where that  file  resides.
>
>    If datasync flag is specified data will be fleshed but does not flush
>    modified metadata unless  that  metadata  is  needed  in order to allow a
>    subsequent data retrieval to be correctly handled.
>
> Signed-off-by: Venkateswararao Jujjuri <jvrao@linux.vnet.ibm.com>
> ---
>  hw/file-op-9p.h      |    2 +-
>  hw/virtio-9p-local.c |    8 ++++++--
>  hw/virtio-9p.c       |   11 ++++++-----
>  3 files changed, 13 insertions(+), 8 deletions(-)
>
> diff --git a/hw/file-op-9p.h b/hw/file-op-9p.h
> index 21d60b5..c7731c2 100644
> --- a/hw/file-op-9p.h
> +++ b/hw/file-op-9p.h
> @@ -86,7 +86,7 @@ typedef struct FileOperations
>     int (*fstat)(FsContext *, int, struct stat *);
>     int (*rename)(FsContext *, const char *, const char *);
>     int (*truncate)(FsContext *, const char *, off_t);
> -    int (*fsync)(FsContext *, int);
> +    int (*fsync)(FsContext *, int, int);
>     int (*statfs)(FsContext *s, const char *path, struct statfs *stbuf);
>     ssize_t (*lgetxattr)(FsContext *, const char *,
>                          const char *, void *, size_t);
> diff --git a/hw/virtio-9p-local.c b/hw/virtio-9p-local.c
> index 0d52020..d0c79e4 100644
> --- a/hw/virtio-9p-local.c
> +++ b/hw/virtio-9p-local.c
> @@ -490,9 +490,13 @@ static int local_remove(FsContext *ctx, const char *path)
>     return remove(rpath(ctx, path));
>  }
>
> -static int local_fsync(FsContext *ctx, int fd)
> +static int local_fsync(FsContext *ctx, int fd, int datasync)
>  {
> -    return fsync(fd);
> +    if (datasync) {
> +        return fdatasync(fd);

Please use qemu_fdatasync().
jvrao - Oct. 23, 2010, 3:39 p.m.
On 10/23/2010 5:44 AM, Blue Swirl wrote:
> On Fri, Oct 22, 2010 at 7:15 PM, Venkateswararao Jujjuri (JV)
> <jvrao@linux.vnet.ibm.com> wrote:
>> SYNOPSIS
>>    size[4] Tfsync tag[2] fid[4] datasync[4]
>>
>>    size[4] Rfsync tag[2]
>>
>> DESCRIPTION
>>
>>    The Tfsync transaction transfers ("flushes") all modified in-core data of
>>    file identified by fid to the disk device (or other  permanent  storage
>>    device)  where that  file  resides.
>>
>>    If datasync flag is specified data will be fleshed but does not flush
>>    modified metadata unless  that  metadata  is  needed  in order to allow a
>>    subsequent data retrieval to be correctly handled.
>>
>> Signed-off-by: Venkateswararao Jujjuri <jvrao@linux.vnet.ibm.com>
>> ---
>>  hw/file-op-9p.h      |    2 +-
>>  hw/virtio-9p-local.c |    8 ++++++--
>>  hw/virtio-9p.c       |   11 ++++++-----
>>  3 files changed, 13 insertions(+), 8 deletions(-)
>>
>> diff --git a/hw/file-op-9p.h b/hw/file-op-9p.h
>> index 21d60b5..c7731c2 100644
>> --- a/hw/file-op-9p.h
>> +++ b/hw/file-op-9p.h
>> @@ -86,7 +86,7 @@ typedef struct FileOperations
>>     int (*fstat)(FsContext *, int, struct stat *);
>>     int (*rename)(FsContext *, const char *, const char *);
>>     int (*truncate)(FsContext *, const char *, off_t);
>> -    int (*fsync)(FsContext *, int);
>> +    int (*fsync)(FsContext *, int, int);
>>     int (*statfs)(FsContext *s, const char *path, struct statfs *stbuf);
>>     ssize_t (*lgetxattr)(FsContext *, const char *,
>>                          const char *, void *, size_t);
>> diff --git a/hw/virtio-9p-local.c b/hw/virtio-9p-local.c
>> index 0d52020..d0c79e4 100644
>> --- a/hw/virtio-9p-local.c
>> +++ b/hw/virtio-9p-local.c
>> @@ -490,9 +490,13 @@ static int local_remove(FsContext *ctx, const char *path)
>>     return remove(rpath(ctx, path));
>>  }
>>
>> -static int local_fsync(FsContext *ctx, int fd)
>> +static int local_fsync(FsContext *ctx, int fd, int datasync)
>>  {
>> -    return fsync(fd);
>> +    if (datasync) {
>> +        return fdatasync(fd);
> 
> Please use qemu_fdatasync().

This file and functions in it are intended as wrappers of syscalls
which can be used in threadlets. Per Anthony's statement about avoiding
any QEMU specific code in threadlets, l I am planning to avoid any QEMU specific
calls
in the virtio-9p-local.c library.

While these are just wrappers for now, what is the future direction of this kind
of calls like
qemu_fdatasync() qemu_malloc() etc? Will they be OK to call in threadlets
without any locks?
If so, may be we can use them in this function.

Thanks,
JV
Blue Swirl - Oct. 23, 2010, 6:27 p.m.
On Sat, Oct 23, 2010 at 3:39 PM, Venkateswararao Jujjuri (JV)
<jvrao@linux.vnet.ibm.com> wrote:
> On 10/23/2010 5:44 AM, Blue Swirl wrote:
>> On Fri, Oct 22, 2010 at 7:15 PM, Venkateswararao Jujjuri (JV)
>> <jvrao@linux.vnet.ibm.com> wrote:
>>> SYNOPSIS
>>>    size[4] Tfsync tag[2] fid[4] datasync[4]
>>>
>>>    size[4] Rfsync tag[2]
>>>
>>> DESCRIPTION
>>>
>>>    The Tfsync transaction transfers ("flushes") all modified in-core data of
>>>    file identified by fid to the disk device (or other  permanent  storage
>>>    device)  where that  file  resides.
>>>
>>>    If datasync flag is specified data will be fleshed but does not flush
>>>    modified metadata unless  that  metadata  is  needed  in order to allow a
>>>    subsequent data retrieval to be correctly handled.
>>>
>>> Signed-off-by: Venkateswararao Jujjuri <jvrao@linux.vnet.ibm.com>
>>> ---
>>>  hw/file-op-9p.h      |    2 +-
>>>  hw/virtio-9p-local.c |    8 ++++++--
>>>  hw/virtio-9p.c       |   11 ++++++-----
>>>  3 files changed, 13 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/hw/file-op-9p.h b/hw/file-op-9p.h
>>> index 21d60b5..c7731c2 100644
>>> --- a/hw/file-op-9p.h
>>> +++ b/hw/file-op-9p.h
>>> @@ -86,7 +86,7 @@ typedef struct FileOperations
>>>     int (*fstat)(FsContext *, int, struct stat *);
>>>     int (*rename)(FsContext *, const char *, const char *);
>>>     int (*truncate)(FsContext *, const char *, off_t);
>>> -    int (*fsync)(FsContext *, int);
>>> +    int (*fsync)(FsContext *, int, int);
>>>     int (*statfs)(FsContext *s, const char *path, struct statfs *stbuf);
>>>     ssize_t (*lgetxattr)(FsContext *, const char *,
>>>                          const char *, void *, size_t);
>>> diff --git a/hw/virtio-9p-local.c b/hw/virtio-9p-local.c
>>> index 0d52020..d0c79e4 100644
>>> --- a/hw/virtio-9p-local.c
>>> +++ b/hw/virtio-9p-local.c
>>> @@ -490,9 +490,13 @@ static int local_remove(FsContext *ctx, const char *path)
>>>     return remove(rpath(ctx, path));
>>>  }
>>>
>>> -static int local_fsync(FsContext *ctx, int fd)
>>> +static int local_fsync(FsContext *ctx, int fd, int datasync)
>>>  {
>>> -    return fsync(fd);
>>> +    if (datasync) {
>>> +        return fdatasync(fd);
>>
>> Please use qemu_fdatasync().
>
> This file and functions in it are intended as wrappers of syscalls
> which can be used in threadlets. Per Anthony's statement about avoiding
> any QEMU specific code in threadlets, l I am planning to avoid any QEMU specific
> calls
> in the virtio-9p-local.c library.

That way would lead to code duplication, reduced functionality and
portability. If the problem is threading and that the current
functions are not thread safe, then we should add new functions to
QEMU instead.
jvrao - Oct. 24, 2010, 4:30 a.m.
On 10/23/2010 11:27 AM, Blue Swirl wrote:
> On Sat, Oct 23, 2010 at 3:39 PM, Venkateswararao Jujjuri (JV)
> <jvrao@linux.vnet.ibm.com> wrote:
>> On 10/23/2010 5:44 AM, Blue Swirl wrote:
>>> On Fri, Oct 22, 2010 at 7:15 PM, Venkateswararao Jujjuri (JV)
>>> <jvrao@linux.vnet.ibm.com> wrote:
>>>> SYNOPSIS
>>>>    size[4] Tfsync tag[2] fid[4] datasync[4]
>>>>
>>>>    size[4] Rfsync tag[2]
>>>>
>>>> DESCRIPTION
>>>>
>>>>    The Tfsync transaction transfers ("flushes") all modified in-core data of
>>>>    file identified by fid to the disk device (or other  permanent  storage
>>>>    device)  where that  file  resides.
>>>>
>>>>    If datasync flag is specified data will be fleshed but does not flush
>>>>    modified metadata unless  that  metadata  is  needed  in order to allow a
>>>>    subsequent data retrieval to be correctly handled.
>>>>
>>>> Signed-off-by: Venkateswararao Jujjuri <jvrao@linux.vnet.ibm.com>
>>>> ---
>>>>  hw/file-op-9p.h      |    2 +-
>>>>  hw/virtio-9p-local.c |    8 ++++++--
>>>>  hw/virtio-9p.c       |   11 ++++++-----
>>>>  3 files changed, 13 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/hw/file-op-9p.h b/hw/file-op-9p.h
>>>> index 21d60b5..c7731c2 100644
>>>> --- a/hw/file-op-9p.h
>>>> +++ b/hw/file-op-9p.h
>>>> @@ -86,7 +86,7 @@ typedef struct FileOperations
>>>>     int (*fstat)(FsContext *, int, struct stat *);
>>>>     int (*rename)(FsContext *, const char *, const char *);
>>>>     int (*truncate)(FsContext *, const char *, off_t);
>>>> -    int (*fsync)(FsContext *, int);
>>>> +    int (*fsync)(FsContext *, int, int);
>>>>     int (*statfs)(FsContext *s, const char *path, struct statfs *stbuf);
>>>>     ssize_t (*lgetxattr)(FsContext *, const char *,
>>>>                          const char *, void *, size_t);
>>>> diff --git a/hw/virtio-9p-local.c b/hw/virtio-9p-local.c
>>>> index 0d52020..d0c79e4 100644
>>>> --- a/hw/virtio-9p-local.c
>>>> +++ b/hw/virtio-9p-local.c
>>>> @@ -490,9 +490,13 @@ static int local_remove(FsContext *ctx, const char *path)
>>>>     return remove(rpath(ctx, path));
>>>>  }
>>>>
>>>> -static int local_fsync(FsContext *ctx, int fd)
>>>> +static int local_fsync(FsContext *ctx, int fd, int datasync)
>>>>  {
>>>> -    return fsync(fd);
>>>> +    if (datasync) {
>>>> +        return fdatasync(fd);
>>>
>>> Please use qemu_fdatasync().
>>
>> This file and functions in it are intended as wrappers of syscalls
>> which can be used in threadlets. Per Anthony's statement about avoiding
>> any QEMU specific code in threadlets, l I am planning to avoid any QEMU specific
>> calls
>> in the virtio-9p-local.c library.
> 
> That way would lead to code duplication, reduced functionality and
> portability. If the problem is threading and that the current
> functions are not thread safe, then we should add new functions to
> QEMU instead.

Thanks, Sent another patch with qemu_fdatasync.
The function in its current form has no issues with threading.

Thanks,
JV
>

Patch

diff --git a/hw/file-op-9p.h b/hw/file-op-9p.h
index 21d60b5..c7731c2 100644
--- a/hw/file-op-9p.h
+++ b/hw/file-op-9p.h
@@ -86,7 +86,7 @@  typedef struct FileOperations
     int (*fstat)(FsContext *, int, struct stat *);
     int (*rename)(FsContext *, const char *, const char *);
     int (*truncate)(FsContext *, const char *, off_t);
-    int (*fsync)(FsContext *, int);
+    int (*fsync)(FsContext *, int, int);
     int (*statfs)(FsContext *s, const char *path, struct statfs *stbuf);
     ssize_t (*lgetxattr)(FsContext *, const char *,
                          const char *, void *, size_t);
diff --git a/hw/virtio-9p-local.c b/hw/virtio-9p-local.c
index 0d52020..d0c79e4 100644
--- a/hw/virtio-9p-local.c
+++ b/hw/virtio-9p-local.c
@@ -490,9 +490,13 @@  static int local_remove(FsContext *ctx, const char *path)
     return remove(rpath(ctx, path));
 }
 
-static int local_fsync(FsContext *ctx, int fd)
+static int local_fsync(FsContext *ctx, int fd, int datasync)
 {
-    return fsync(fd);
+    if (datasync) {
+        return fdatasync(fd);
+    } else {
+        return fsync(fd);
+    }
 }
 
 static int local_statfs(FsContext *s, const char *path, struct statfs *stbuf)
diff --git a/hw/virtio-9p.c b/hw/virtio-9p.c
index daade77..7c59988 100644
--- a/hw/virtio-9p.c
+++ b/hw/virtio-9p.c
@@ -248,9 +248,9 @@  static int v9fs_do_remove(V9fsState *s, V9fsString *path)
     return s->ops->remove(&s->ctx, path->data);
 }
 
-static int v9fs_do_fsync(V9fsState *s, int fd)
+static int v9fs_do_fsync(V9fsState *s, int fd, int datasync)
 {
-    return s->ops->fsync(&s->ctx, fd);
+    return s->ops->fsync(&s->ctx, fd, datasync);
 }
 
 static int v9fs_do_statfs(V9fsState *s, V9fsString *path, struct statfs *stbuf)
@@ -1868,16 +1868,17 @@  static void v9fs_fsync(V9fsState *s, V9fsPDU *pdu)
     int32_t fid;
     size_t offset = 7;
     V9fsFidState *fidp;
+    int datasync;
     int err;
 
-    pdu_unmarshal(pdu, offset, "d", &fid);
+    pdu_unmarshal(pdu, offset, "dd", &fid, &datasync);
     fidp = lookup_fid(s, fid);
     if (fidp == NULL) {
         err = -ENOENT;
         v9fs_post_do_fsync(s, pdu, err);
         return;
     }
-    err = v9fs_do_fsync(s, fidp->fs.fd);
+    err = v9fs_do_fsync(s, fidp->fs.fd, datasync);
     v9fs_post_do_fsync(s, pdu, err);
 }
 
@@ -3001,7 +3002,7 @@  static void v9fs_wstat(V9fsState *s, V9fsPDU *pdu)
 
     /* do we need to sync the file? */
     if (donttouch_stat(&vs->v9stat)) {
-        err = v9fs_do_fsync(s, vs->fidp->fs.fd);
+        err = v9fs_do_fsync(s, vs->fidp->fs.fd, 0);
         v9fs_wstat_post_fsync(s, vs, err);
         return;
     }