From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48519) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XgUb9-0008Uk-Ak for qemu-devel@nongnu.org; Tue, 21 Oct 2014 04:15:32 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XgUaq-0002Dt-V6 for qemu-devel@nongnu.org; Tue, 21 Oct 2014 04:15:19 -0400 Received: from smtp.citrix.com ([66.165.176.89]:59224) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XgUaq-0002DG-QC for qemu-devel@nongnu.org; Tue, 21 Oct 2014 04:15:00 -0400 Message-ID: <54461601.3040207@citrix.com> Date: Tue, 21 Oct 2014 10:14:57 +0200 From: =?windows-1252?Q?Roger_Pau_Monn=E9?= MIME-Version: 1.0 References: <1413823167-26619-1-git-send-email-roger.pau@citrix.com> <20141020172248.GU3585@noname.redhat.com> In-Reply-To: <20141020172248.GU3585@noname.redhat.com> Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH] block: char devices on FreeBSD are not behind a pager List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: qemu-devel@nongnu.org, Stefan Hajnoczi 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é >> Cc: Kevin Wolf >> Cc: Stefan Hajnoczi >> --- >> 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? Roger.