Patchwork [03/11] raw-posix: Don't "try harder" for BDRV_TYPE_CDROM

login
register
mail settings
Submitter Markus Armbruster
Date June 30, 2010, 11:55 a.m.
Message ID <1277898942-6501-4-git-send-email-armbru@redhat.com>
Download mbox | patch
Permalink /patch/57394/
State New
Headers show

Comments

Markus Armbruster - June 30, 2010, 11:55 a.m.
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(-)
Kevin Wolf - July 5, 2010, 3:31 p.m.
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
Markus Armbruster - July 5, 2010, 4:15 p.m.
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 ;)
Kevin Wolf - July 5, 2010, 4:35 p.m.
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

Patch

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;
 }