From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38471) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XgVrs-0004wG-FW for qemu-devel@nongnu.org; Tue, 21 Oct 2014 05:36:45 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XgVrm-0005OY-TE for qemu-devel@nongnu.org; Tue, 21 Oct 2014 05:36:40 -0400 Received: from mx1.redhat.com ([209.132.183.28]:51147) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XgVrm-0005OQ-Hx for qemu-devel@nongnu.org; Tue, 21 Oct 2014 05:36:34 -0400 Date: Tue, 21 Oct 2014 11:36:26 +0200 From: Kevin Wolf Message-ID: <20141021093626.GD4409@noname.redhat.com> References: <1413823167-26619-1-git-send-email-roger.pau@citrix.com> <20141020172248.GU3585@noname.redhat.com> <54461601.3040207@citrix.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <54461601.3040207@citrix.com> Content-Transfer-Encoding: quoted-printable 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: Roger Pau =?iso-8859-1?Q?Monn=E9?= Cc: qemu-devel@nongnu.org, Stefan Hajnoczi Am 21.10.2014 um 10:14 hat Roger Pau Monn=E9 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 o= rder to > >> force QEMU to use aligned buffers. > >> > >> Signed-off-by: Roger Pau Monn=E9 > >> 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 a= ligned > >> + * to sector size, or else FreeBSD will reject them with EI= NVAL. > >> + */ > >> + bs->open_flags |=3D BDRV_O_NOCACHE; > >> + s->open_flags |=3D O_DIRECT; > >> + } > >> +#endif > >=20 > > No, this doesn't look right. Block drivers must not modify the option= s > > that they get. (Yes, the Linux AIO case is broken in this respect. > > Hopefully we'll be able to fix it soon.) > >=20 > > Depending on what the real requirements are, I can see two options: > >=20 > > 1. Require cache.direct=3Don (i.e. O_DIRECT) for char devices on Free= BSD. > > If the user didn't set the option, print a nice error message tell= ing > > them what option to set. > >=20 > > 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. > >=20 > > I don't know FreeBSD good enough, but if it fulfills the requirements= , > > option 2 is certainly the more elegant one. >=20 > Thanks for the review. O_DIRECT is not required to open the file, so > option 2 seems sensible. >=20 > I've added a new flag to BDRVRawState that's used to check if underlyin= g > 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_alignmen= t > and raw_aio_submit. Does this sound OK? Yes, this sounds reasonable to me. Kevin