| Message ID | 1279639056-20465-1-git-send-email-Jes.Sorensen@redhat.com |
|---|---|
| State | New |
| Headers | show |
Am 20.07.2010 17:17, schrieb Jes.Sorensen@redhat.com: > From: Jes Sorensen <Jes.Sorensen@redhat.com> > > O_DIRECT (cache=none) requires sector alignment, however the physical > sector size of CDROM/DVD drives is 2048, as opposed to most disk > devices which use 512. QEMU is hard coding 512 all over the place, so > allowing O_DIRECT for CDROM/DVD devices does not work. > > Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com> > --- > block/raw-posix.c | 5 +++++ > 1 files changed, 5 insertions(+), 0 deletions(-) > > diff --git a/block/raw-posix.c b/block/raw-posix.c > index 291699f..0ea79b6 100644 > --- a/block/raw-posix.c > +++ b/block/raw-posix.c > @@ -1139,6 +1139,11 @@ static int cdrom_open(BlockDriverState *bs, const char *filename, int flags) > BDRVRawState *s = bs->opaque; > > s->type = FTYPE_CD; > + if (flags & BDRV_O_NOCACHE) { > + fprintf(stderr, "Disabling unsupported O_DIRECT (cache=none) for " > + "CDROM/DVD device (%s)\n", filename); > + flags &= ~BDRV_O_NOCACHE; > + } Good point. Just one detail: We should probably change bs->open_flags, too, to keep things consistent. Kevin
On 07/20/2010 10:17 AM, Jes.Sorensen@redhat.com wrote: > From: Jes Sorensen<Jes.Sorensen@redhat.com> > > O_DIRECT (cache=none) requires sector alignment, however the physical > sector size of CDROM/DVD drives is 2048, as opposed to most disk > devices which use 512. QEMU is hard coding 512 all over the place, so > allowing O_DIRECT for CDROM/DVD devices does not work. > > Signed-off-by: Jes Sorensen<Jes.Sorensen@redhat.com> > Wouldn't a better solution be to have a cdrom_read/cdrom_write hook that did the appropriate bouncing? Silently disabling something a user explicitly asked for is not a good option. In the very least, it should error out entirely. Regards, Anthony Liguori > --- > block/raw-posix.c | 5 +++++ > 1 files changed, 5 insertions(+), 0 deletions(-) > > diff --git a/block/raw-posix.c b/block/raw-posix.c > index 291699f..0ea79b6 100644 > --- a/block/raw-posix.c > +++ b/block/raw-posix.c > @@ -1139,6 +1139,11 @@ static int cdrom_open(BlockDriverState *bs, const char *filename, int flags) > BDRVRawState *s = bs->opaque; > > s->type = FTYPE_CD; > + if (flags& BDRV_O_NOCACHE) { > + fprintf(stderr, "Disabling unsupported O_DIRECT (cache=none) for " > + "CDROM/DVD device (%s)\n", filename); > + flags&= ~BDRV_O_NOCACHE; > + } > > /* open will not fail even if no CD is inserted, so add O_NONBLOCK */ > return raw_open_common(bs, filename, flags, O_NONBLOCK); >
On 07/20/10 17:40, Anthony Liguori wrote: > On 07/20/2010 10:17 AM, Jes.Sorensen@redhat.com wrote: >> From: Jes Sorensen<Jes.Sorensen@redhat.com> >> >> O_DIRECT (cache=none) requires sector alignment, however the physical >> sector size of CDROM/DVD drives is 2048, as opposed to most disk >> devices which use 512. QEMU is hard coding 512 all over the place, so >> allowing O_DIRECT for CDROM/DVD devices does not work. >> >> Signed-off-by: Jes Sorensen<Jes.Sorensen@redhat.com> >> > > Wouldn't a better solution be to have a cdrom_read/cdrom_write hook that > did the appropriate bouncing? > > Silently disabling something a user explicitly asked for is not a good > option. In the very least, it should error out entirely. I thought about this, but it would require basically fixing up or copying all of the pread/pwrite code to use the right block size. This is really more of a band-aid but it should be pretty safe. Cheers, Jes
On 07/20/2010 11:02 AM, Jes Sorensen wrote: > On 07/20/10 17:40, Anthony Liguori wrote: > >> On 07/20/2010 10:17 AM, Jes.Sorensen@redhat.com wrote: >> >>> From: Jes Sorensen<Jes.Sorensen@redhat.com> >>> >>> O_DIRECT (cache=none) requires sector alignment, however the physical >>> sector size of CDROM/DVD drives is 2048, as opposed to most disk >>> devices which use 512. QEMU is hard coding 512 all over the place, so >>> allowing O_DIRECT for CDROM/DVD devices does not work. >>> >>> Signed-off-by: Jes Sorensen<Jes.Sorensen@redhat.com> >>> >>> >> Wouldn't a better solution be to have a cdrom_read/cdrom_write hook that >> did the appropriate bouncing? >> >> Silently disabling something a user explicitly asked for is not a good >> option. In the very least, it should error out entirely. >> > I thought about this, but it would require basically fixing up or > copying all of the pread/pwrite code to use the right block size. This > is really more of a band-aid but it should be pretty safe. > Please throw an error. If a user explicitly asks for something, and we can provide it, we should not continue. Changing it to something else is a bug. That doesn't apply when we're changing a default value, but if the user asks for something, we should give it to them or fail. Regards, Anthony Liguori > Cheers, > Jes >
On 07/20/10 17:40, Kevin Wolf wrote: >> diff --git a/block/raw-posix.c b/block/raw-posix.c >> index 291699f..0ea79b6 100644 >> --- a/block/raw-posix.c >> +++ b/block/raw-posix.c >> @@ -1139,6 +1139,11 @@ static int cdrom_open(BlockDriverState *bs, const char *filename, int flags) >> BDRVRawState *s = bs->opaque; >> >> s->type = FTYPE_CD; >> + if (flags & BDRV_O_NOCACHE) { >> + fprintf(stderr, "Disabling unsupported O_DIRECT (cache=none) for " >> + "CDROM/DVD device (%s)\n", filename); >> + flags &= ~BDRV_O_NOCACHE; >> + } > > Good point. Just one detail: We should probably change bs->open_flags, > too, to keep things consistent. Thats effectively what my patch does. cdrom_open() calls raw_open_common() which has this part: /* Use O_DSYNC for write-through caching, no flags for write-back caching, * and O_DIRECT for no caching. */ if ((bdrv_flags & BDRV_O_NOCACHE)) s->open_flags |= O_DIRECT; Cheers, Jes
On 07/20/10 18:03, Anthony Liguori wrote: > On 07/20/2010 11:02 AM, Jes Sorensen wrote: >> On 07/20/10 17:40, Anthony Liguori wrote: >>> Wouldn't a better solution be to have a cdrom_read/cdrom_write hook that >>> did the appropriate bouncing? >>> >>> Silently disabling something a user explicitly asked for is not a good >>> option. In the very least, it should error out entirely. >>> >> I thought about this, but it would require basically fixing up or >> copying all of the pread/pwrite code to use the right block size. This >> is really more of a band-aid but it should be pretty safe. > > Please throw an error. If a user explicitly asks for something, and we > can provide it, we should not continue. Changing it to something else > is a bug. > > That doesn't apply when we're changing a default value, but if the user > asks for something, we should give it to them or fail. Ok that seems fair enough! I'll post an updated patch in a minute. Cheers, Jes
Am 20.07.2010 17:40, schrieb Anthony Liguori: > On 07/20/2010 10:17 AM, Jes.Sorensen@redhat.com wrote: >> From: Jes Sorensen<Jes.Sorensen@redhat.com> >> >> O_DIRECT (cache=none) requires sector alignment, however the physical >> sector size of CDROM/DVD drives is 2048, as opposed to most disk >> devices which use 512. QEMU is hard coding 512 all over the place, so >> allowing O_DIRECT for CDROM/DVD devices does not work. >> >> Signed-off-by: Jes Sorensen<Jes.Sorensen@redhat.com> >> > > Wouldn't a better solution be to have a cdrom_read/cdrom_write hook that > did the appropriate bouncing? We already have code that does some bouncing, we'd just need to teach it to use different sizes than 512. Duplicating everything wouldn't be a nice solution. > Silently disabling something a user explicitly asked for is not a good > option. In the very least, it should error out entirely. Yeah, that's probably better. Kevin
On 07/20/10 18:09, Kevin Wolf wrote: > Am 20.07.2010 17:40, schrieb Anthony Liguori: >> Wouldn't a better solution be to have a cdrom_read/cdrom_write hook that >> did the appropriate bouncing? > > We already have code that does some bouncing, we'd just need to teach it > to use different sizes than 512. Duplicating everything wouldn't be a > nice solution. The problem is that it is quite hard to detect properly and there are cases where we can get false positives, plus it would require cleaning up the code to handle variable sector sizes. The latter is probably a good thing, and if/when it happens, we can remove this. Cheers, Jes
Am 20.07.2010 18:04, schrieb Jes Sorensen: > On 07/20/10 17:40, Kevin Wolf wrote: >>> diff --git a/block/raw-posix.c b/block/raw-posix.c >>> index 291699f..0ea79b6 100644 >>> --- a/block/raw-posix.c >>> +++ b/block/raw-posix.c >>> @@ -1139,6 +1139,11 @@ static int cdrom_open(BlockDriverState *bs, const char *filename, int flags) >>> BDRVRawState *s = bs->opaque; >>> >>> s->type = FTYPE_CD; >>> + if (flags & BDRV_O_NOCACHE) { >>> + fprintf(stderr, "Disabling unsupported O_DIRECT (cache=none) for " >>> + "CDROM/DVD device (%s)\n", filename); >>> + flags &= ~BDRV_O_NOCACHE; >>> + } >> >> Good point. Just one detail: We should probably change bs->open_flags, >> too, to keep things consistent. > > Thats effectively what my patch does. cdrom_open() calls > raw_open_common() which has this part: > > /* Use O_DSYNC for write-through caching, no flags for write-back > caching, > * and O_DIRECT for no caching. */ > if ((bdrv_flags & BDRV_O_NOCACHE)) > s->open_flags |= O_DIRECT; s and bs both have a field open_flags (and I think they share some more field names, which has caused confusion more than once). I meant the bs one here. Kevin
On 07/20/10 10:02, Jes Sorensen wrote: > On 07/20/10 17:40, Anthony Liguori wrote: >> On 07/20/2010 10:17 AM, Jes.Sorensen@redhat.com wrote: >>> From: Jes Sorensen<Jes.Sorensen@redhat.com> >>> >>> O_DIRECT (cache=none) requires sector alignment, however the physical >>> sector size of CDROM/DVD drives is 2048, as opposed to most disk >>> devices which use 512. QEMU is hard coding 512 all over the place, so >>> allowing O_DIRECT for CDROM/DVD devices does not work. >>> >>> Signed-off-by: Jes Sorensen<Jes.Sorensen@redhat.com> >>> >> >> Wouldn't a better solution be to have a cdrom_read/cdrom_write hook that >> did the appropriate bouncing? >> >> Silently disabling something a user explicitly asked for is not a good >> option. In the very least, it should error out entirely. > > I thought about this, but it would require basically fixing up or > copying all of the pread/pwrite code to use the right block size. This > is really more of a band-aid but it should be pretty safe. > > Cheers, > Jes > > What about setting the logical and physical block size properties? Is the intent of these to handle varying sizes amongst block devices? David
On 07/20/10 18:12, Kevin Wolf wrote: > Am 20.07.2010 18:04, schrieb Jes Sorensen: >> On 07/20/10 17:40, Kevin Wolf wrote: >> Thats effectively what my patch does. cdrom_open() calls >> raw_open_common() which has this part: >> >> /* Use O_DSYNC for write-through caching, no flags for write-back >> caching, >> * and O_DIRECT for no caching. */ >> if ((bdrv_flags & BDRV_O_NOCACHE)) >> s->open_flags |= O_DIRECT; > > s and bs both have a field open_flags (and I think they share some more > field names, which has caused confusion more than once). I meant the bs > one here. > > Kevin Hi, It shows up even that case is handled as one of the first things bdrv_open_common() does is to set bs->open_flags = flags. Anyway throwing an error as Anthony suggested seems safer. Cheers, Jes
On 07/20/10 18:16, David S. Ahern wrote: > What about setting the logical and physical block size properties? Is > the intent of these to handle varying sizes amongst block devices? > > David Well the problem is that we need to know the sector size of the host device, and then start using that within QEMU for anything using O_DIRECT. The way the QEMU code handles it right now is by hard coding it, and cleaning that up could be a bit messy, but it should go on the todo list. Cheers, Jes
El 20/07/2010, a las 16:17, Jes.Sorensen@redhat.com escribió: > From: Jes Sorensen <Jes.Sorensen@redhat.com> > > O_DIRECT (cache=none) requires sector alignment, however the physical > sector size of CDROM/DVD drives is 2048, as opposed to most disk > devices which use 512. QEMU is hard coding 512 all over the place, so > allowing O_DIRECT for CDROM/DVD devices does not work. What about if the device is a 4096 byte/sector hard disk, a 512 byte/sector CD-ROM (IRIX ones), a 2048 byte/sector magnetooptical? We should get rid of that hard codes and use real values. > Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com> > --- > block/raw-posix.c | 5 +++++ > 1 files changed, 5 insertions(+), 0 deletions(-) > > diff --git a/block/raw-posix.c b/block/raw-posix.c > index 291699f..0ea79b6 100644 > --- a/block/raw-posix.c > +++ b/block/raw-posix.c > @@ -1139,6 +1139,11 @@ static int cdrom_open(BlockDriverState *bs, const char *filename, int flags) > BDRVRawState *s = bs->opaque; > > s->type = FTYPE_CD; > + if (flags & BDRV_O_NOCACHE) { > + fprintf(stderr, "Disabling unsupported O_DIRECT (cache=none) for " > + "CDROM/DVD device (%s)\n", filename); > + flags &= ~BDRV_O_NOCACHE; > + } > > /* open will not fail even if no CD is inserted, so add O_NONBLOCK */ > return raw_open_common(bs, filename, flags, O_NONBLOCK); > -- > 1.7.1.1 > >
On 07/20/10 18:45, Natalia Portillo wrote: > El 20/07/2010, a las 16:17, Jes.Sorensen@redhat.com escribió: > >> From: Jes Sorensen <Jes.Sorensen@redhat.com> >> >> O_DIRECT (cache=none) requires sector alignment, however the physical >> sector size of CDROM/DVD drives is 2048, as opposed to most disk >> devices which use 512. QEMU is hard coding 512 all over the place, so >> allowing O_DIRECT for CDROM/DVD devices does not work. > > What about if the device is a 4096 byte/sector hard disk, a 512 byte/sector CD-ROM (IRIX ones), a 2048 byte/sector magnetooptical? > > We should get rid of that hard codes and use real values. I totally agree! However for now my patch throws an error for the CDROM case, which is better than the old situation. We need to fix it in general, but it isn't trivial. The 4096 sector size hard disk going to be the main issue. Jes
On 07/20/2010 11:45 AM, Natalia Portillo wrote: > El 20/07/2010, a las 16:17, Jes.Sorensen@redhat.com escribió: > > >> From: Jes Sorensen<Jes.Sorensen@redhat.com> >> >> O_DIRECT (cache=none) requires sector alignment, however the physical >> sector size of CDROM/DVD drives is 2048, as opposed to most disk >> devices which use 512. QEMU is hard coding 512 all over the place, so >> allowing O_DIRECT for CDROM/DVD devices does not work. >> > What about if the device is a 4096 byte/sector hard disk, a 512 byte/sector CD-ROM (IRIX ones), a 2048 byte/sector magnetooptical? > BlockDriverStates need to handle non-aligned reads/writes. As I mentioned earlier, we need cdrom_pread/cdrom_pwrite functions that do RMWs as necessary. Regards, Anthony Liguori > We should get rid of that hard codes and use real values. > > >> Signed-off-by: Jes Sorensen<Jes.Sorensen@redhat.com> >> --- >> block/raw-posix.c | 5 +++++ >> 1 files changed, 5 insertions(+), 0 deletions(-) >> >> diff --git a/block/raw-posix.c b/block/raw-posix.c >> index 291699f..0ea79b6 100644 >> --- a/block/raw-posix.c >> +++ b/block/raw-posix.c >> @@ -1139,6 +1139,11 @@ static int cdrom_open(BlockDriverState *bs, const char *filename, int flags) >> BDRVRawState *s = bs->opaque; >> >> s->type = FTYPE_CD; >> + if (flags& BDRV_O_NOCACHE) { >> + fprintf(stderr, "Disabling unsupported O_DIRECT (cache=none) for " >> + "CDROM/DVD device (%s)\n", filename); >> + flags&= ~BDRV_O_NOCACHE; >> + } >> >> /* open will not fail even if no CD is inserted, so add O_NONBLOCK */ >> return raw_open_common(bs, filename, flags, O_NONBLOCK); >> -- >> 1.7.1.1 >> >> >> > >
diff --git a/block/raw-posix.c b/block/raw-posix.c index 291699f..0ea79b6 100644 --- a/block/raw-posix.c +++ b/block/raw-posix.c @@ -1139,6 +1139,11 @@ static int cdrom_open(BlockDriverState *bs, const char *filename, int flags) BDRVRawState *s = bs->opaque; s->type = FTYPE_CD; + if (flags & BDRV_O_NOCACHE) { + fprintf(stderr, "Disabling unsupported O_DIRECT (cache=none) for " + "CDROM/DVD device (%s)\n", filename); + flags &= ~BDRV_O_NOCACHE; + } /* open will not fail even if no CD is inserted, so add O_NONBLOCK */ return raw_open_common(bs, filename, flags, O_NONBLOCK);