Xen devel - Dec 2011 - [PATCH] Only retry mapping pages when ENOENT is returned

# HG changeset patch
# User Adin Scannell <adin@scannell.ca>
# Date 1324094033 18000
# Node ID 14d42c7d8e0817040186cd01c46f034999fc5dff
# Parent  9dcc8557a0cb676b84e6788e03ab596bcfda7143
Only retry mapping pages when ENOENT is returned

If the return value from the ioctl() is not ENOENT, it''s possible that
err[i]
will not be updated and libxc will just loop forever.  Although it''s
unlikely
that err[i] would not be updated after the ioctl() gets through at least once,
it''s better to be defensive.

diff -r 9dcc8557a0cb -r 14d42c7d8e08 tools/libxc/xc_linux_osdep.c
--- a/tools/libxc/xc_linux_osdep.c
+++ b/tools/libxc/xc_linux_osdep.c
@@ -232,7 +232,7 @@ static void *linux_privcmd_map_foreign_b
             do {
                 usleep(100);
                 rc = ioctl(fd, IOCTL_PRIVCMD_MMAPBATCH_V2, &ioctlx);
-            } while ( rc < 0 && err[i] == -ENOENT );
+            } while ( rc < 0 && errno == ENOENT && err[i] ==
-ENOENT );
         }
     }

Adin Scannell writes ("[Xen-devel] [PATCH] Only retry mapping pages when
ENOENT is returned"):
> Only retry mapping pages when ENOENT is returned
> 
> If the return value from the ioctl() is not ENOENT, it''s possible
that err[i]
> will not be updated and libxc will just loop forever.  Although
it''s unlikely
> that err[i] would not be updated after the ioctl() gets through at least
once,
> it''s better to be defensive.
I confess I don''t understand the intended error handling here.  AFAICT
this ioctl leaves a separate errno value for each request, in the
array.  Presumably, however, it can also fail entirely and not update
the separate in-array errno values.

So how does one tell which of these is the case ?

CCing Jan since it looks like his code originally, in 20791:0b138a019292

Ian.

> I confess I don''t understand the intended error handling here.
 AFAICT
> this ioctl leaves a separate errno value for each request, in the
> array.  Presumably, however, it can also fail entirely and not update
> the separate in-array errno values.
>
> So how does one tell which of these is the case ?
ENOENT is returned in the case where everything is successful, but at
least one of the entries is ENOENT as the page was not available
(paged-out).  In other words, when ENOENT is returned you can trust
the errno entries were filled in.  It''s a somewhat special case.

As you''ve pointed out, one cannot trust the errno vals if some
arbitrary error (like EINVAL) is returned.  The current code assumes
that after the first ENOENT, subsequent calls will be the same and the
errno vals can be read and trusted.  My patch was intended to fix this
behavior and only read the errno values when ENOENT is returned and
they are meaningful.  If you get back an EINVAL with my patch, the
loop will break and function call will fail.  Without the patch, it''ll
keep looping forever (it''s unlikely that this would ever happen, but I
noticed that the logic was a bit broken...).

Cheers,
-Adin

Adin Scannell writes ("Re: [Xen-devel] [PATCH] Only retry mapping pages
when ENOENT is returned"):
> ENOENT is returned in the case where everything is successful, but at
> least one of the entries is ENOENT as the page was not available
> (paged-out).  In other words, when ENOENT is returned you can trust
> the errno entries were filled in.  It''s a somewhat special case.
Right, thanks, that makes sense.  I would apply it but you didn''t sign
it off.  Also, we should CC Olaf, so doing that now.

Thanks,
Ian.

On Fri, Dec 16, Adin Scannell wrote:
> # HG changeset patch
> # User Adin Scannell <adin@scannell.ca>
> # Date 1324094033 18000
> # Node ID 14d42c7d8e0817040186cd01c46f034999fc5dff
> # Parent  9dcc8557a0cb676b84e6788e03ab596bcfda7143
> Only retry mapping pages when ENOENT is returned
> 
> If the return value from the ioctl() is not ENOENT, it''s possible
that err[i]
> will not be updated and libxc will just loop forever.  Although
it''s unlikely
> that err[i] would not be updated after the ioctl() gets through at least
once,
> it''s better to be defensive.
In linux-2.6.18-xen.hg the ioctl could in theory return -ENOMEM in a
later iteration and maybe even other values if the guest was destroyed
meanwhile. So checking also errno for ENOENT is good, because thats the
return code if any of the requested gfns is still in paged-out state.

Acked-by: Olaf Hering <olaf@aepfle.de>
> diff -r 9dcc8557a0cb -r 14d42c7d8e08 tools/libxc/xc_linux_osdep.c
> --- a/tools/libxc/xc_linux_osdep.c
> +++ b/tools/libxc/xc_linux_osdep.c
> @@ -232,7 +232,7 @@ static void *linux_privcmd_map_foreign_b
>              do {
>                  usleep(100);
>                  rc = ioctl(fd, IOCTL_PRIVCMD_MMAPBATCH_V2, &ioctlx);
> -            } while ( rc < 0 && err[i] == -ENOENT );
> +            } while ( rc < 0 && errno == ENOENT &&
err[i] == -ENOENT );
>          }
>      }
>  
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
>

Olaf Hering writes ("Re: [Xen-devel] [PATCH] Only retry mapping pages when
ENOENT is returned"):
> In linux-2.6.18-xen.hg the ioctl could in theory return -ENOMEM in a
> later iteration and maybe even other values if the guest was destroyed
> meanwhile. So checking also errno for ENOENT is good, because thats the
> return code if any of the requested gfns is still in paged-out state.
> 
> Acked-by: Olaf Hering <olaf@aepfle.de>
Thanks.  Adin, would you care to sign it off, indicating your
confirmation that the Developer''s Certificate of Origin applies  ?
See  http://wiki.xen.org/wiki/SubmittingXenPatches

Thanks,
Ian.

On Thu, Jan 5, 2012 at 12:40, Ian Jackson <Ian.Jackson@eu.citrix.com>
wrote:
> Thanks.  Adin, would you care to sign it off, indicating your
> confirmation that the Developer''s Certificate of Origin applies  ?
> See  http://wiki.xen.org/wiki/SubmittingXenPatches
Absolutely, sorry about that.

Signed-off-by: Adin Scannell <adin@scannell.ca>

tools/libxc/xc_linux_osdep.c |  2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)


If the return value from the ioctl() is not ENOENT, it''s possible that
err[i]
will not be updated and libxc will just loop forever.  Although it''s
unlikely
that err[i] would not be updated after the ioctl() gets through at least once,
it''s better to be defensive.

Signed-off-by: Adin Scannell <adin@scannell.ca>
Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>

diff -r 2a2f3597d156 -r 90f764bf02c3 tools/libxc/xc_linux_osdep.c
--- a/tools/libxc/xc_linux_osdep.c
+++ b/tools/libxc/xc_linux_osdep.c
@@ -232,7 +232,7 @@ static void *linux_privcmd_map_foreign_b
             do {
                 usleep(100);
                 rc = ioctl(fd, IOCTL_PRIVCMD_MMAPBATCH_V2, &ioctlx);
-            } while ( rc < 0 && err[i] == -ENOENT );
+            } while ( rc < 0 && errno == ENOENT && err[i] ==
-ENOENT );
         }
     }

Andres Lagar-Cavilla writes ("[PATCH] Only retry mapping pages when ENOENT
is returned"):
> If the return value from the ioctl() is not ENOENT, it''s possible
that err[i]
> will not be updated and libxc will just loop forever.  Although
it''s unlikely
> that err[i] would not be updated after the ioctl() gets through at least
once,
> it''s better to be defensive.
Committed-by: Ian Jackson <ian.jackson@eu.citrix.com>