Re: [Qemu-devel] [PATCH for-2.7 v2 04/17] block: Introduce image file locking

[Denis V. Lunev]

On 04/15/2016 06:27 AM, Fam Zheng wrote:
> Block drivers can implement this new operation .bdrv_lockf to actually lock the
> image in the protocol specific way.
>
> Signed-off-by: Fam Zheng <address@hidden>
> ---
>   block.c                   | 42 ++++++++++++++++++++++++++++++++++++++++++
>   include/block/block_int.h | 12 ++++++++++++
>   2 files changed, 54 insertions(+)
>
> diff --git a/block.c b/block.c
> index 1c575e4..7971a25 100644
> --- a/block.c
> +++ b/block.c
> @@ -846,6 +846,34 @@ out:
>       g_free(gen_node_name);
>   }
>   
> +static int bdrv_lock_unlock_image_do(BlockDriverState *bs, bool lock_image)
> +{
> +    int cmd = BDRV_LOCKF_UNLOCK;
> +
> +    if (bs->image_locked == lock_image) {
> +        return 0;
> +    } else if (!bs->drv) {
> +        return -ENOMEDIUM;
> +    } else if (!bs->drv->bdrv_lockf) {
> +        return 0;
> +    }
> +    if (lock_image) {
> +        cmd = bs->open_flags & BDRV_O_RDWR ? BDRV_LOCKF_RWLOCK :
> +                                             BDRV_LOCKF_ROLOCK;
> +    }
> +    return bs->drv->bdrv_lockf(bs, cmd);
should we handle ENOTSUP specially?
f.e. this would fire with raw-posix.c on a filesystem which does not 
support locking.

from my POW this situations is equivalent to  the absence of 
bs->drv->bdrv_lockf

> +}
> +
> +static int bdrv_lock_image(BlockDriverState *bs)
> +{
> +    return bdrv_lock_unlock_image_do(bs, true);
> +}
> +
> +static int bdrv_unlock_image(BlockDriverState *bs)
> +{
> +    return bdrv_lock_unlock_image_do(bs, false);
> +}
> +
>   static QemuOptsList bdrv_runtime_opts = {
>       .name = "bdrv_common",
>       .head = QTAILQ_HEAD_INITIALIZER(bdrv_runtime_opts.head),
> @@ -995,6 +1023,14 @@ static int bdrv_open_common(BlockDriverState *bs, 
> BdrvChild *file,
>           goto free_and_fail;
>       }
>   
> +    if (!(open_flags & (BDRV_O_NO_LOCK | BDRV_O_INACTIVE))) {
> +        ret = bdrv_lock_image(bs);
> +        if (ret) {
> +            error_setg(errp, "Failed to lock image");
> +            goto free_and_fail;
> +        }
> +    }
> +
>       ret = refresh_total_sectors(bs, bs->total_sectors);
>       if (ret < 0) {
>           error_setg_errno(errp, -ret, "Could not refresh total sector count");
> @@ -2144,6 +2180,7 @@ static void bdrv_close(BlockDriverState *bs)
>       if (bs->drv) {
>           BdrvChild *child, *next;
>   
> +        bdrv_unlock_image(bs);
>           bs->drv->bdrv_close(bs);
>           bs->drv = NULL;
>   
> @@ -3230,6 +3267,9 @@ void bdrv_invalidate_cache(BlockDriverState *bs, Error **errp)
>           error_setg_errno(errp, -ret, "Could not refresh total sector count");
>           return;
>       }
> +    if (!(bs->open_flags & BDRV_O_NO_LOCK)) {
> +        bdrv_lock_image(bs);
> +    }
>   }
>   
>   void bdrv_invalidate_cache_all(Error **errp)
> @@ -3262,6 +3302,7 @@ static int bdrv_inactivate(BlockDriverState *bs)
>       }
>   
>       bs->open_flags |= BDRV_O_INACTIVE;
> +    ret = bdrv_unlock_image(bs);
I'd better move unlock a line above.
Though this is personal. This could be useful
for debugging purposes.


>       return 0;
>   }
>   
> @@ -3981,3 +4022,4 @@ void bdrv_refresh_filename(BlockDriverState *bs)
>           QDECREF(json);
>       }
>   }
> +
this hunk is extra

> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 10d8759..ffa30b0 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -85,6 +85,12 @@ typedef struct BdrvTrackedRequest {
>       struct BdrvTrackedRequest *waiting_for;
>   } BdrvTrackedRequest;
>   
> +typedef enum {
> +    BDRV_LOCKF_RWLOCK,
> +    BDRV_LOCKF_ROLOCK,
> +    BDRV_LOCKF_UNLOCK,
> +} BdrvLockfCmd;
> +
>   struct BlockDriver {
>       const char *format_name;
>       int instance_size;
> @@ -317,6 +323,11 @@ struct BlockDriver {
>        */
>       void (*bdrv_drain)(BlockDriverState *bs);
>   
> +    /**
> +     * Lock/unlock the image.
> +     */
> +    int (*bdrv_lockf)(BlockDriverState *bs, BdrvLockfCmd cmd);
> +
>       QLIST_ENTRY(BlockDriver) list;
>   };
>   
> @@ -485,6 +496,7 @@ struct BlockDriverState {
>       NotifierWithReturn write_threshold_notifier;
>   
>       int quiesce_counter;
> +    bool image_locked;
>   };
>   
>   struct BlockBackendRootState {