diff mbox

[2/5] block: implemented bdrv_lock_image for raw file

Message ID 1450856816-9816-3-git-send-email-den@openvz.org
State New
Headers show

Commit Message

Denis V. Lunev Dec. 23, 2015, 7:46 a.m. UTC
From: Olga Krishtal <okrishtal@virtuozzo.com>

To lock the image file flock (LockFileEx) is used.
We lock file handle/descriptor. If lock is failed -
an error is returned.

In win32 realization we can lock reagion of bytes within the file.
For this reason we at first have to get file size and only than lock it.

Signed-off-by: Olga Krishtal <okrishtal@virtuozzo.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Max Reitz <mreitz@redhat.com>
CC: Eric Blake <eblake@redhat.com>
CC: Fam Zheng <famz@redhat.com>
---
 block/raw-posix.c | 15 +++++++++++++++
 block/raw-win32.c | 19 +++++++++++++++++++
 2 files changed, 34 insertions(+)

Comments

Daniel P. Berrangé Dec. 23, 2015, 12:40 p.m. UTC | #1
On Wed, Dec 23, 2015 at 10:46:53AM +0300, Denis V. Lunev wrote:
> From: Olga Krishtal <okrishtal@virtuozzo.com>
> 
> To lock the image file flock (LockFileEx) is used.
> We lock file handle/descriptor. If lock is failed -
> an error is returned.
> 
> In win32 realization we can lock reagion of bytes within the file.
> For this reason we at first have to get file size and only than lock it.
> 
> Signed-off-by: Olga Krishtal <okrishtal@virtuozzo.com>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Max Reitz <mreitz@redhat.com>
> CC: Eric Blake <eblake@redhat.com>
> CC: Fam Zheng <famz@redhat.com>
> ---
>  block/raw-posix.c | 15 +++++++++++++++
>  block/raw-win32.c | 19 +++++++++++++++++++
>  2 files changed, 34 insertions(+)
> 
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index 076d070..6226a5c 100644
> --- a/block/raw-posix.c
> +++ b/block/raw-posix.c
> @@ -33,6 +33,7 @@
>  #include "raw-aio.h"
>  #include "qapi/util.h"
>  #include "qapi/qmp/qstring.h"
> +#include <sys/file.h>
>  
>  #if defined(__APPLE__) && (__MACH__)
>  #include <paths.h>
> @@ -576,6 +577,19 @@ fail:
>      return ret;
>  }
>  
> +static int raw_lock_image(BlockDriverState *bs, BdrvLockImage lock)
> +{
> +    int ret;
> +    if (lock != BDRV_LOCK_IMAGE_LOCKFILE) {
> +        return -ENOTSUP;
> +    }
> +    ret = flock(((BDRVRawState *)(bs->opaque))->fd, LOCK_EX|LOCK_NB);
> +    if (ret != 0) {
> +        return -ret;
> +    }
> +    return ret;
> +}

flock() is a pretty bad choice wrt to NFS. Historically it was often
a no-op. Some impls treat it as a client-local lock and not a server
side lock. Linux apparently now converts flock locks to fcntl locks,
but on NFS only. As a result they'll suffer from the problem that
a close() on any file descriptor pointing to the file will release
the lock. So IMHO both flock() and fcntl() are unusable in practice
from within QEMU, as I don't think it is practical to guarantee
QEMU won't accidentally release the lock by closing another file
descriptor pointing to the same file. If you want to use flock or
fcntl() and provide a strong safety guarantee the only option is to
acquire the locks in a dedicated process prior to giving QEMU access
to the files, which is what libvirt does with its virtlockd daemon.

Regards,
Daniel

[1] http://0pointer.de/blog/projects/locking.html
diff mbox

Patch

diff --git a/block/raw-posix.c b/block/raw-posix.c
index 076d070..6226a5c 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -33,6 +33,7 @@ 
 #include "raw-aio.h"
 #include "qapi/util.h"
 #include "qapi/qmp/qstring.h"
+#include <sys/file.h>
 
 #if defined(__APPLE__) && (__MACH__)
 #include <paths.h>
@@ -576,6 +577,19 @@  fail:
     return ret;
 }
 
+static int raw_lock_image(BlockDriverState *bs, BdrvLockImage lock)
+{
+    int ret;
+    if (lock != BDRV_LOCK_IMAGE_LOCKFILE) {
+        return -ENOTSUP;
+    }
+    ret = flock(((BDRVRawState *)(bs->opaque))->fd, LOCK_EX|LOCK_NB);
+    if (ret != 0) {
+        return -ret;
+    }
+    return ret;
+}
+
 static int raw_open(BlockDriverState *bs, QDict *options, int flags,
                     Error **errp)
 {
@@ -1946,6 +1960,7 @@  BlockDriver bdrv_file = {
     .bdrv_co_get_block_status = raw_co_get_block_status,
     .bdrv_co_write_zeroes = raw_co_write_zeroes,
 
+    .bdrv_lock_image = raw_lock_image,
     .bdrv_aio_readv = raw_aio_readv,
     .bdrv_aio_writev = raw_aio_writev,
     .bdrv_aio_flush = raw_aio_flush,
diff --git a/block/raw-win32.c b/block/raw-win32.c
index 2d0907a..d05160a 100644
--- a/block/raw-win32.c
+++ b/block/raw-win32.c
@@ -370,6 +370,24 @@  fail:
     return ret;
 }
 
+static int raw_lock_image(BlockDriverState *bs, BdrvLockImage lock)
+{
+    DWORD size_high = 0, size_low = 0;
+    BDRVRawState *s = bs->opaque;
+    if (lock != BDRV_LOCK_IMAGE_LOCKFILE) {
+        return -ENOTSUP;
+    }
+    size_low = GetFileSize(s->hfile, &size_high);
+    if (GetLastError() != 0) {
+        return -EINVAL;
+    }
+    if (!LockFileEx(s->hfile, LOCKFILE_EXCLUSIVE_LOCK|LOCKFILE_FAIL_IMMEDIATELY,
+                        0, size_high, size_low, NULL)) {
+            return -EINVAL;
+    }
+    return 0;
+}
+
 static BlockAIOCB *raw_aio_readv(BlockDriverState *bs,
                          int64_t sector_num, QEMUIOVector *qiov, int nb_sectors,
                          BlockCompletionFunc *cb, void *opaque)
@@ -552,6 +570,7 @@  BlockDriver bdrv_file = {
     .bdrv_create        = raw_create,
     .bdrv_has_zero_init = bdrv_has_zero_init_1,
 
+    .bdrv_lock_image    = raw_lock_image,
     .bdrv_aio_readv     = raw_aio_readv,
     .bdrv_aio_writev    = raw_aio_writev,
     .bdrv_aio_flush     = raw_aio_flush,