Re: [Qemu-devel] [PATCH v2] block: char devices on FreeBSD are not behind a pager

[Max Reitz <mreitz@redhat.com>]

On 2014-10-21 at 12:39, Roger Pau Monne wrote:
> Introduce a new flag to mark devices that require requests to be aligned and
> replace the usage of BDRV_O_NOCACHE and O_DIRECT with this flag when
> appropriate.
>
> If a character device is used as a backend on a FreeBSD host set this flag
> unconditionally.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Cc: Kevin Wolf <kwolf@redhat.com>
> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> ---
> Changes since v1:
>   - Intead of appending BDRV_O_NOCACHE and O_DIRECT when a char dev is used
>     on FreeBSD introduce a new flag that is used to mark if a device needs
>     requests to be aligned.
> ---
>   block/raw-posix.c | 25 ++++++++++++++++++++-----
>   1 file changed, 20 insertions(+), 5 deletions(-)
>
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index 86ce4f2..b5361f7 100644
> --- a/block/raw-posix.c
> +++ b/block/raw-posix.c
> @@ -150,6 +150,7 @@ typedef struct BDRVRawState {
>       bool has_discard:1;
>       bool has_write_zeroes:1;
>       bool discard_zeroes:1;
> +    bool needs_alignement:1;

First, it's "alignment". Second, urgh, do you really want to use a bit 
field here? :-/

I know and see that there is pre-existing code here which does it, but I 
personally find it pretty ugly. There is no need to use one whatsoever.

>   #ifdef CONFIG_FIEMAP
>       bool skip_fiemap;
>   #endif
> @@ -230,7 +231,7 @@ static void raw_probe_alignment(BlockDriverState *bs, int fd, Error **errp)
>   
>       /* For /dev/sg devices the alignment is not really used.
>          With buffered I/O, we don't have any restrictions. */
> -    if (bs->sg || !(s->open_flags & O_DIRECT)) {
> +    if (bs->sg || !s->needs_alignement) {
>           bs->request_alignment = 1;
>           s->buf_align = 1;
>           return;
> @@ -446,6 +447,8 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
>   
>       s->has_discard = true;
>       s->has_write_zeroes = true;
> +    if ((bs->open_flags & BDRV_O_NOCACHE) != 0)
> +        s->needs_alignement = true;

This is not conforming to the qemu coding style (always put {} around 
blocks).

>       if (fstat(s->fd, &st) < 0) {
>           error_setg_errno(errp, errno, "Could not stat file");
> @@ -472,6 +475,17 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
>           }
>   #endif
>       }
> +#ifdef __FreeBSD__
> +    if (S_ISCHR(st.st_mode)) {
> +        /*
> +         * The file is a char device (disk), which on FreeBSD isn't behind
> +         * a pager, so force all requests to be aligned. This is needed
> +         * so Qemu makes sure all IO operations on the device are aligned
> +         * to sector size, or else FreeBSD will reject them with EINVAL.
> +         */
> +        s->needs_alignement = true;
> +    }
> +#endif
>   
>   #ifdef CONFIG_XFS
>       if (platform_test_xfs_fd(s->fd)) {
> @@ -1076,11 +1090,12 @@ static BlockDriverAIOCB *raw_aio_submit(BlockDriverState *bs,
>           return NULL;
>   
>       /*
> -     * If O_DIRECT is used the buffer needs to be aligned on a sector
> -     * boundary.  Check if this is the case or tell the low-level
> -     * driver that it needs to copy the buffer.
> +     * Check if the underlying device requires requests to be aligned,
> +     * and if the request we are trying to submit is aligned or not.
> +     * If this is the case tell the low-level driver that it needs
> +     * to copy the buffer.
>        */
> -    if ((bs->open_flags & BDRV_O_NOCACHE)) {
> +    if (s->needs_alignement) {
>           if (!bdrv_qiov_is_aligned(bs, qiov)) {
>               type |= QEMU_AIO_MISALIGNED;
>   #ifdef CONFIG_LINUX_AIO

With s/alignement/alignment/ and the conditional block changed to 
conform to the qemu coding style:

Reviewed-by: Max Reitz <mreitz@redhat.com>

You are free to keep needs_alignment a bit field or not (the R-b stands 
either way). I wouldn't make it a bit field but I leave it up to you.

Max