| Message ID | 1277898942-6501-4-git-send-email-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
Am 30.06.2010 13:55, schrieb Markus Armbruster: > raw_pread_aligned() retries up to two times if the block device backs > a virtual CD-ROM. This makes no sense. Whether retrying reads can > correct read errors may depend on what we're reading, not on how the > result gets used. > > Also clean up gratuitous use of goto. > > This reverts what's left of commit 8c05dbf9. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> Are you sure that this won't cause a regression? I mean if there is a patch specifically adding this behaviour, there probably was a problem that made someone touch the code in the first place. Arguably checking for the type hint is nonsense, however I think the case for which this was written is passing through a real CD-ROM to a VM - in which case the condition would be true anyway. So instead of removing the code, the fix to achieve what was probably intended is to check for bs->drv == &bdrv_host_cdrom. Kevin
Kevin Wolf <kwolf@redhat.com> writes: > Am 30.06.2010 13:55, schrieb Markus Armbruster: >> raw_pread_aligned() retries up to two times if the block device backs >> a virtual CD-ROM. This makes no sense. Whether retrying reads can >> correct read errors may depend on what we're reading, not on how the >> result gets used. >> >> Also clean up gratuitous use of goto. >> >> This reverts what's left of commit 8c05dbf9. >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> > > Are you sure that this won't cause a regression? I mean if there is a > patch specifically adding this behaviour, there probably was a problem > that made someone touch the code in the first place. > > Arguably checking for the type hint is nonsense, however I think the > case for which this was written is passing through a real CD-ROM to a VM > - in which case the condition would be true anyway. > > So instead of removing the code, the fix to achieve what was probably > intended is to check for bs->drv == &bdrv_host_cdrom. I can do that. But does it make sense? How can retrying failed reads help? Isn't the OS in a much better position to retry? Keeping the retry code feels like voodoo-programming to me: I have no idea how waving around this dead chicken could help, but we've always done it, so keep waving ;)
Am 05.07.2010 18:15, schrieb Markus Armbruster: > Kevin Wolf <kwolf@redhat.com> writes: > >> Am 30.06.2010 13:55, schrieb Markus Armbruster: >>> raw_pread_aligned() retries up to two times if the block device backs >>> a virtual CD-ROM. This makes no sense. Whether retrying reads can >>> correct read errors may depend on what we're reading, not on how the >>> result gets used. >>> >>> Also clean up gratuitous use of goto. >>> >>> This reverts what's left of commit 8c05dbf9. >>> >>> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> >> Are you sure that this won't cause a regression? I mean if there is a >> patch specifically adding this behaviour, there probably was a problem >> that made someone touch the code in the first place. >> >> Arguably checking for the type hint is nonsense, however I think the >> case for which this was written is passing through a real CD-ROM to a VM >> - in which case the condition would be true anyway. >> >> So instead of removing the code, the fix to achieve what was probably >> intended is to check for bs->drv == &bdrv_host_cdrom. > > I can do that. But does it make sense? How can retrying failed reads > help? Isn't the OS in a much better position to retry? > > Keeping the retry code feels like voodoo-programming to me: I have no > idea how waving around this dead chicken could help, but we've always > done it, so keep waving ;) I would agree that someone tried to be clever without real reason if this was buried in one of those big Fabrice-style commits. But is was added as a commit for itself, and I'd be surprised if someone sent a patch if it didn't change anything for him. Let's try if I've got a valid email address of Ben for CCing him... Ben, do you remember this patch and can you help us? commit 8c05dbf9b68cc8444573116063582e01a0442b0b Author: ths <ths@c046a42c-6fe2-441c-8c8c-71466251a162> Date: Thu Sep 13 12:29:23 2007 +0000 Enhance raw io reliability, by Ben Guthro. Kevin
diff --git a/block/raw-posix.c b/block/raw-posix.c index 3f0701b..2a847aa 100644 --- a/block/raw-posix.c +++ b/block/raw-posix.c @@ -242,15 +242,14 @@ static int raw_pread_aligned(BlockDriverState *bs, int64_t offset, ret = pread(s->fd, buf, count, offset); if (ret == count) - goto label__raw_read__success; + return ret; /* Allow reads beyond the end (needed for pwrite) */ if ((ret == 0) && bs->growable) { int64_t size = raw_getlength(bs); if (offset >= size) { memset(buf, 0, count); - ret = count; - goto label__raw_read__success; + return count; } } @@ -259,23 +258,6 @@ static int raw_pread_aligned(BlockDriverState *bs, int64_t offset, s->fd, bs->filename, offset, buf, count, bs->total_sectors, ret, errno, strerror(errno)); - /* Try harder for CDrom. */ - if (bs->type == BDRV_TYPE_CDROM) { - ret = pread(s->fd, buf, count, offset); - if (ret == count) - goto label__raw_read__success; - ret = pread(s->fd, buf, count, offset); - if (ret == count) - goto label__raw_read__success; - - DEBUG_BLOCK_PRINT("raw_pread(%d:%s, %" PRId64 ", %p, %d) [%" PRId64 - "] retry read failed %d : %d = %s\n", - s->fd, bs->filename, offset, buf, count, - bs->total_sectors, ret, errno, strerror(errno)); - } - -label__raw_read__success: - return (ret < 0) ? -errno : ret; } @@ -298,15 +280,13 @@ static int raw_pwrite_aligned(BlockDriverState *bs, int64_t offset, ret = pwrite(s->fd, buf, count, offset); if (ret == count) - goto label__raw_write__success; + return ret; DEBUG_BLOCK_PRINT("raw_pwrite(%d:%s, %" PRId64 ", %p, %d) [%" PRId64 "] write failed %d : %d = %s\n", s->fd, bs->filename, offset, buf, count, bs->total_sectors, ret, errno, strerror(errno)); -label__raw_write__success: - return (ret < 0) ? -errno : ret; }
raw_pread_aligned() retries up to two times if the block device backs a virtual CD-ROM. This makes no sense. Whether retrying reads can correct read errors may depend on what we're reading, not on how the result gets used. Also clean up gratuitous use of goto. This reverts what's left of commit 8c05dbf9. Signed-off-by: Markus Armbruster <armbru@redhat.com> --- block/raw-posix.c | 26 +++----------------------- 1 files changed, 3 insertions(+), 23 deletions(-)