| Message ID | 20200116093602.4203-4-pdurrant@amazon.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xl/libxl: domid allocation/preservation changes | expand |
Thanks. I think this is the algorithm as we discussed, thanks.
I have some comments about the implementation...
Paul Durrant writes ("[PATCH v3 3/6] libxl: add infrastructure to track and query 'retired' domids"):
> A domid is considered retired if the domain it represents was destroyed
> less than a specified number of seconds ago. The number can be set using
> the environment variable LIBXL_DOMID_MAX_RETIREMENT. If the variable does
> not exist then a default value of 60s is used.
...
I'm afraid I think your update protocol for this file is wrong. In
general it is a bad idea to try to write over a file in-place. Doing
so is full of gotchas. (In this particular case for example I think
an interrupted attempt at cleaning the file can produce a corrupted
file containing nonsense.)
Can we please use the standard write-to-new-file-and-rename ?
Ie, to launder
flock(open("domid-history.lock"))
fopen("domid-history","r")
fopen("domid-history.new","w")
fgets/fputs
fclose
rename
close
(And no uses of ftell, fopen(,"r+"), etc.)
Reading can be done without taking the lock, if you so fancy.
I think there are a lot of missing error checks in this patch, but
since I'm asking for a different approach I won't point them out
individually.
> + fd = open(name, O_RDWR|O_CREAT, 0644);
> + if (fd < 0) {
> + LOGE(ERROR, "unexpected error while trying open %s, errno=%d",
> + name, errno);
> + goto fail;
> + }
> +
> + for (;;) {
> + ret = flock(fd, LOCK_EX);
I looked for a utility function to do this but didn't find one.
I think this is complicated because it needs to be a `carefd' in libxl
terms because of concurrent forking by other threads in the
application.
I suggest generalising libxl__lock_domain_userdata, which has all the
necessary code (and which also would permit removing the file in the
future).
I feel responsible for this inconvenience. If this is too tiresome
for you, I could do that part for you...
> +/* Write a domid retirement record */
> +static void libxl__retire_domid(libxl__gc *gc, uint32_t domid)
> +{
...
> + while (fgets(line, sizeof(line), f)) {
> + unsigned long sec;
> + unsigned int ignored;
> +
> + roff = ftell(f);
> +
> + if (sscanf(line, "%lu %u", &sec, &ignored) != 2)
> + continue; /* Purge malformed lines */
I'm not sure why you bother with fgets into a buffer, when you could
just use fscanf rather than sscanf. Your code doesn't need to take
much care about weird syntax which might occur (and indeed your code
here doesn't take such care).
> @@ -1331,6 +1462,7 @@ static void devices_destroy_cb(libxl__egc *egc,
> if (!ctx->xch) goto badchild;
>
> if (!dis->soft_reset) {
> + libxl__retire_domid(gc, domid);
I wonder if a better term than "retired" would be possible. I
initially found this patch a bit confusing because I thought a retired
domid would be one which had *not* been used recently. Maybe
"recent", "mark recent", etc. ? Ultimately this is a bikeshed issue
which I will leave this up to you, though.
I don't much like the environment variable to configure this. I don't
object to keeping it but can we have a comment saying this is not
intended for use in production ? Personally I would rather it was
hardcoded, or failing that, written to some config file.
Finally, I think this patch needs an addition to xen-init-dom0 to
remove or empty the record file. This is because while /run is
usually a tmpfs, this is not *necessarily* true.
Ian.
> -----Original Message----- > From: Ian Jackson <ian.jackson@citrix.com> > Sent: 16 January 2020 19:28 > To: Durrant, Paul <pdurrant@amazon.co.uk> > Cc: xen-devel@lists.xenproject.org; Wei Liu <wl@xen.org>; Anthony Perard > <anthony.perard@citrix.com> > Subject: Re: [PATCH v3 3/6] libxl: add infrastructure to track and query > 'retired' domids > > Thanks. I think this is the algorithm as we discussed, thanks. > I have some comments about the implementation... > > Paul Durrant writes ("[PATCH v3 3/6] libxl: add infrastructure to track > and query 'retired' domids"): > > A domid is considered retired if the domain it represents was destroyed > > less than a specified number of seconds ago. The number can be set using > > the environment variable LIBXL_DOMID_MAX_RETIREMENT. If the variable > does > > not exist then a default value of 60s is used. > ... > > I'm afraid I think your update protocol for this file is wrong. In > general it is a bad idea to try to write over a file in-place. Doing > so is full of gotchas. (In this particular case for example I think > an interrupted attempt at cleaning the file can produce a corrupted > file containing nonsense.) > > Can we please use the standard write-to-new-file-and-rename ? Ok, fair enough. I'd not really considered interruption as being too much of a risk but I guess I should. > Ie, to launder > flock(open("domid-history.lock")) > fopen("domid-history","r") > fopen("domid-history.new","w") > fgets/fputs > fclose > rename > close > > (And no uses of ftell, fopen(,"r+"), etc.) > > Reading can be done without taking the lock, if you so fancy. > > I think there are a lot of missing error checks in this patch, but > since I'm asking for a different approach I won't point them out > individually. > Ok. > > + fd = open(name, O_RDWR|O_CREAT, 0644); > > + if (fd < 0) { > > + LOGE(ERROR, "unexpected error while trying open %s, errno=%d", > > + name, errno); > > + goto fail; > > + } > > + > > + for (;;) { > > + ret = flock(fd, LOCK_EX); > > I looked for a utility function to do this but didn't find one. > I think this is complicated because it needs to be a `carefd' in libxl > terms because of concurrent forking by other threads in the > application. > > I suggest generalising libxl__lock_domain_userdata, which has all the > necessary code (and which also would permit removing the file in the > future). > > I feel responsible for this inconvenience. If this is too tiresome > for you, I could do that part for you... > That's ok; I'll insert a preceding generalization patch, unless it turns into a total can of worms... which I doubt it will. > > +/* Write a domid retirement record */ > > +static void libxl__retire_domid(libxl__gc *gc, uint32_t domid) > > +{ > ... > > + while (fgets(line, sizeof(line), f)) { > > + unsigned long sec; > > + unsigned int ignored; > > + > > + roff = ftell(f); > > + > > + if (sscanf(line, "%lu %u", &sec, &ignored) != 2) > > + continue; /* Purge malformed lines */ > > I'm not sure why you bother with fgets into a buffer, when you could > just use fscanf rather than sscanf. Your code doesn't need to take > much care about weird syntax which might occur (and indeed your code > here doesn't take such care). Well, I need to pull the line into a buffer if I'm going to write it out again, but otherwise I could indeed use fscanf(). > > > @@ -1331,6 +1462,7 @@ static void devices_destroy_cb(libxl__egc *egc, > > if (!ctx->xch) goto badchild; > > > > if (!dis->soft_reset) { > > + libxl__retire_domid(gc, domid); > > I wonder if a better term than "retired" would be possible. I > initially found this patch a bit confusing because I thought a retired > domid would be one which had *not* been used recently. Maybe > "recent", "mark recent", etc. ? Ultimately this is a bikeshed issue > which I will leave this up to you, though. > Ok, 'recent' is probably clearer. I'll s/retired/recent/g. > > I don't much like the environment variable to configure this. I don't > object to keeping it but can we have a comment saying this is not > intended for use in production ? Personally I would rather it was > hardcoded, or failing that, written to some config file. > The problem is that libxl has no config file. Env variables seem to be used for other things so I followed suit. I'd rather keep the override for debug purposes; I'll stick a comment in the header saying that's what it's for though, as you suggest. > > Finally, I think this patch needs an addition to xen-init-dom0 to > remove or empty the record file. This is because while /run is > usually a tmpfs, this is not *necessarily* true. > Ok, if we cannot rely on it being tmpfs then I will do that. Paul
Durrant, Paul writes ("RE: [PATCH v3 3/6] libxl: add infrastructure to track and query 'retired' domids"):
> [Ian;]
> > I'm not sure why you bother with fgets into a buffer, when you could
> > just use fscanf rather than sscanf. Your code doesn't need to take
> > much care about weird syntax which might occur (and indeed your code
> > here doesn't take such care).
>
> Well, I need to pull the line into a buffer if I'm going to write it out again, but otherwise I could indeed use fscanf().
Well, you could just fprintf the information.
> Ok, 'recent' is probably clearer. I'll s/retired/recent/g.
Thanks.
> > I don't much like the environment variable to configure this. I don't
> > object to keeping it but can we have a comment saying this is not
> > intended for use in production ? Personally I would rather it was
> > hardcoded, or failing that, written to some config file.
>
> The problem is that libxl has no config file. Env variables seem to be used for other things so I followed suit. I'd rather keep the override for debug purposes; I'll stick a comment in the header saying that's what it's for though, as you suggest.
OK. You are right about the lack of a config file being a problem.
> > Finally, I think this patch needs an addition to xen-init-dom0 to
> > remove or empty the record file. This is because while /run is
> > usually a tmpfs, this is not *necessarily* true.
>
> Ok, if we cannot rely on it being tmpfs then I will do that.
Thanks.
Thanks for the rest of your reply, too, which I snipped as I had
nothing more to say than `thanks'.
Regards,
Ian.
diff --git a/tools/libxl/libxl_domain.c b/tools/libxl/libxl_domain.c index 5714501778..7f255f184c 100644 --- a/tools/libxl/libxl_domain.c +++ b/tools/libxl/libxl_domain.c @@ -1268,6 +1268,137 @@ static void dm_destroy_cb(libxl__egc *egc, libxl__devices_destroy(egc, &dis->drs); } +static unsigned int libxl__get_max_retirement(void) +{ + const char *env_max_retirement = getenv("LIBXL_DOMID_MAX_RETIREMENT"); + + return env_max_retirement ? strtol(env_max_retirement, NULL, 0) : + LIBXL_DOMID_MAX_RETIREMENT; +} + +static int libxl__open_domid_history(libxl__gc *gc) +{ + const char *name; + int fd; + int ret; + + name = GCSPRINTF("%s/domid-history", libxl__run_dir_path()); + + fd = open(name, O_RDWR|O_CREAT, 0644); + if (fd < 0) { + LOGE(ERROR, "unexpected error while trying open %s, errno=%d", + name, errno); + goto fail; + } + + for (;;) { + ret = flock(fd, LOCK_EX); + if (!ret) + break; + if (errno != EINTR) { + /* All other errno: EBADF, EINVAL, ENOLCK, EWOULDBLOCK */ + LOGE(ERROR, + "unexpected error while trying to lock %s, fd=%d, errno=%d", + name, fd, errno); + goto fail; + } + } + + return fd; + +fail: + if (fd >= 0) + close(fd); + + return -1; +} + +/* Write a domid retirement record */ +static void libxl__retire_domid(libxl__gc *gc, uint32_t domid) +{ + long max_retirement = libxl__get_max_retirement(); + int fd; + FILE *f; + long roff, woff; + char line[64]; + struct timespec ts; + + fd = libxl__open_domid_history(gc); + if (fd < 0) + return; + + clock_gettime(CLOCK_MONOTONIC, &ts); + + /* Purge old retirement records */ + + f = fdopen(fd, "r+"); + woff = ftell(f); + + while (fgets(line, sizeof(line), f)) { + unsigned long sec; + unsigned int ignored; + + roff = ftell(f); + + if (sscanf(line, "%lu %u", &sec, &ignored) != 2) + continue; /* Purge malformed lines */ + + if (ts.tv_sec - sec > max_retirement) + continue; + + fseek(f, woff, SEEK_SET); + fputs(line, f); + woff = ftell(f); + + fseek(f, roff, SEEK_SET); + } + + fseek(f, woff, SEEK_SET); + fprintf(f, "%lu %u\n", ts.tv_sec, domid); + woff = ftell(f); + fflush(f); + + ftruncate(fd, woff); /* may now be fewer records */ + + close(fd); +} + +bool libxl__is_retired_domid(libxl__gc *gc, uint32_t domid) +{ + long max_retirement = libxl__get_max_retirement(); + bool retired = false; + int fd; + FILE *f; + char line[64]; + struct timespec ts; + + fd = libxl__open_domid_history(gc); + if (fd < 0) + return false; + + clock_gettime(CLOCK_MONOTONIC, &ts); + + f = fdopen(fd, "r"); + + while (fgets(line, sizeof(line), f)) { + unsigned long sec; + unsigned int check; + + if (sscanf(line, "%lu %u", &sec, &check) != 2) + continue; + + if (check == domid && + ts.tv_sec - sec <= max_retirement) { + retired = true; + break; + } + } + + close(fd); + + return retired; +} + static void devices_destroy_cb(libxl__egc *egc, libxl__devices_remove_state *drs, int rc) @@ -1331,6 +1462,7 @@ static void devices_destroy_cb(libxl__egc *egc, if (!ctx->xch) goto badchild; if (!dis->soft_reset) { + libxl__retire_domid(gc, domid); rc = xc_domain_destroy(ctx->xch, domid); } else { rc = xc_domain_pause(ctx->xch, domid); diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h index cb23490c59..fcac8a93c5 100644 --- a/tools/libxl/libxl_internal.h +++ b/tools/libxl/libxl_internal.h @@ -4770,6 +4770,16 @@ _hidden int libxl__domain_pvcontrol(libxl__egc *egc, libxl__xswait_state *pvcontrol, domid_t domid, const char *cmd); +/* + * Maximum number of seconds a domid remains in retirement after domain + * destruction. This can be overidden by the environment variable of the + * same name. + */ +#define LIBXL_DOMID_MAX_RETIREMENT 60 + +/* Check whether a domid is in retirement */ +bool libxl__is_retired_domid(libxl__gc *gc, uint32_t domid); + #endif /*
A domid is considered retired if the domain it represents was destroyed less than a specified number of seconds ago. The number can be set using the environment variable LIBXL_DOMID_MAX_RETIREMENT. If the variable does not exist then a default value of 60s is used. Whenever a domain is destroyed, a time-stamped record will be written into a history file (/var/run/xen/domid-history). To avoid the history file growing too large, any records with time-stamps that indicate that the domid has exceeded maximum retirement will also be purged. A new utility function, libxl__is_retired_domid(), has been added. This function reads the same history file checking whether a specified domid has a record that does not exceed maximum retirement. Since this utility function does not write to the file, no records are actually purged by it. NOTE: Since the history file is hosted by a tmpfs file system, it is automatically purged on boot thus allowing safe use of CLOCK_MONOTONIC as a time source. Signed-off-by: Paul Durrant <pdurrant@amazon.com> --- Cc: Ian Jackson <ian.jackson@eu.citrix.com> Cc: Wei Liu <wl@xen.org> Cc: Anthony PERARD <anthony.perard@citrix.com> v2: - New in v2 --- tools/libxl/libxl_domain.c | 132 +++++++++++++++++++++++++++++++++++ tools/libxl/libxl_internal.h | 10 +++ 2 files changed, 142 insertions(+)