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

[Kevin Wolf]

Am 21.10.2014 um 10:14 hat Roger Pau Monné geschrieben:
> El 20/10/14 a les 19.22, Kevin Wolf ha escrit:
> > Am 20.10.2014 um 18:39 hat Roger Pau Monne geschrieben:
> >> Acknowledge this and forcefully set BDRV_O_NOCACHE and O_DIRECT in order to
> >> force QEMU to use aligned buffers.
> >>
> >> Signed-off-by: Roger Pau Monné <address@hidden>
> >> Cc: Kevin Wolf <address@hidden>
> >> Cc: Stefan Hajnoczi <address@hidden>
> >> ---
> >>  block/raw-posix.c | 12 ++++++++++++
> >>  1 file changed, 12 insertions(+)
> >>
> >> diff --git a/block/raw-posix.c b/block/raw-posix.c
> >> index 86ce4f2..63841dd 100644
> >> --- a/block/raw-posix.c
> >> +++ b/block/raw-posix.c
> >> @@ -472,6 +472,18 @@ 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 set BDRV_O_NOCACHE unconditionally. 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.
> >> +         */
> >> +        bs->open_flags |= BDRV_O_NOCACHE;
> >> +        s->open_flags |= O_DIRECT;
> >> +    }
> >> +#endif
> > 
> > No, this doesn't look right. Block drivers must not modify the options
> > that they get. (Yes, the Linux AIO case is broken in this respect.
> > Hopefully we'll be able to fix it soon.)
> > 
> > Depending on what the real requirements are, I can see two options:
> > 
> > 1. Require cache.direct=on (i.e. O_DIRECT) for char devices on FreeBSD.
> >    If the user didn't set the option, print a nice error message telling
> >    them what option to set.
> > 
> > 2. If O_DIRECT isn't actually required to open the file, but you only
> >    need to make sure to use the right alignment, modify
> >    raw_probe_alignment() so that it returns an alignment > 1 even for
> >    non-O_DIRECT files on FreeBSD if they are character devices.
> > 
> > I don't know FreeBSD good enough, but if it fulfills the requirements,
> > option 2 is certainly the more elegant one.
> 
> Thanks for the review. O_DIRECT is not required to open the file, so
> option 2 seems sensible.
> 
> I've added a new flag to BDRVRawState that's used to check if underlying
> device needs requests to be aligned. This flag is set by default if
> BDRV_O_NOCACHE is used, or if the OS is FreeBSD and the underlying
> device is a char dev. This new flag is used as a replacement of the
> O_DIRECT and BDRV_O_NOCACHE checks that were used in raw_probe_alignment
> and raw_aio_submit. Does this sound OK?

Yes, this sounds reasonable to me.

Kevin