Re: [PATCH v2] block: Refactor get_tmp_filename()
Hi Markus,
On Tue, Sep 27, 2022 at 2:22 PM Markus Armbruster wrote:
>
> Bin Meng writes:
>
> > On Mon, Sep 26, 2022 at 6:13 PM Markus Armbruster wrote:
> >>
> >> Bin Meng writes:
> >>
> >> > From: Bin Meng
> >> >
> >> > At present there are two callers of get_tmp_filename() and they are
> >> > inconsistent.
> >> >
> >> > One does:
> >> >
> >> > /* TODO: extra byte is a hack to ensure MAX_PATH space on Windows. */
> >> > char *tmp_filename = g_malloc0(PATH_MAX + 1);
> >> > ...
> >> > ret = get_tmp_filename(tmp_filename, PATH_MAX + 1);
> >> >
> >> > while the other does:
> >> >
> >> > s->qcow_filename = g_malloc(PATH_MAX);
> >> > ret = get_tmp_filename(s->qcow_filename, PATH_MAX);
> >> >
> >> > As we can see different 'size' arguments are passed. There are also
> >> > platform specific implementations inside the function, and this use
> >> > of snprintf is really undesirable.
> >> >
> >> > Refactor this routine by changing its signature to:
> >> >
> >> > char *get_tmp_filename(void)
> >> >
> >> > and use g_file_open_tmp() for a consistent implementation.
> >> >
> >> > Signed-off-by: Bin Meng
> >> > ---
> >> >
> >> > Changes in v2:
> >> > - Use g_autofree and g_steal_pointer
> >> >
> >> > include/block/block_int-common.h | 2 +-
> >> > block.c | 42 ++--
> >> > block/vvfat.c| 8 +++---
> >> > 3 files changed, 18 insertions(+), 34 deletions(-)
> >> >
> >> > diff --git a/include/block/block_int-common.h
> >> > b/include/block/block_int-common.h
> >> > index 8947abab76..ea69a9349c 100644
> >> > --- a/include/block/block_int-common.h
> >> > +++ b/include/block/block_int-common.h
> >> > @@ -1230,7 +1230,7 @@ static inline BlockDriverState *child_bs(BdrvChild
> >> > *child)
> >> > }
> >> >
> >> > int bdrv_check_request(int64_t offset, int64_t bytes, Error **errp);
> >> > -int get_tmp_filename(char *filename, int size);
> >> > +char *get_tmp_filename(void);
> >> > void bdrv_parse_filename_strip_prefix(const char *filename, const char
> >> > *prefix,
> >> >QDict *options);
> >> >
> >> > diff --git a/block.c b/block.c
> >> > index bc85f46eed..4e7a795566 100644
> >> > --- a/block.c
> >> > +++ b/block.c
> >> > @@ -860,38 +860,23 @@ int bdrv_probe_geometry(BlockDriverState *bs,
> >> > HDGeometry *geo)
> >> >
> >> > /*
> >> > * Create a uniquely-named empty temporary file.
> >> > - * Return 0 upon success, otherwise a negative errno value.
> >> > + * Return the actual name used upon success, otherwise NULL.
> >> > + * The called function is responsible for freeing it.
> >> > */
> >> > -int get_tmp_filename(char *filename, int size)
> >> > +char *get_tmp_filename(void)
> >> > {
> >> > -#ifdef _WIN32
> >> > -char temp_dir[MAX_PATH];
> >> > -/* GetTempFileName requires that its output buffer (4th param)
> >> > - have length MAX_PATH or greater. */
> >> > -assert(size >= MAX_PATH);
> >> > -return (GetTempPath(MAX_PATH, temp_dir)
> >> > -&& GetTempFileName(temp_dir, "qem", 0, filename)
> >> > -? 0 : -GetLastError());
> >> > -#else
> >> > +g_autofree char *filename = NULL;
> >> > int fd;
> >> > -const char *tmpdir;
> >> > -tmpdir = getenv("TMPDIR");
> >> > -if (!tmpdir) {
> >> > -tmpdir = "/var/tmp";
> >> > -}
> >> > -if (snprintf(filename, size, "%s/vl.XX", tmpdir) >= size) {
> >> > -return -EOVERFLOW;
> >> > -}
> >> > -fd = mkstemp(filename);
> >> > +
> >> > +fd = g_file_open_tmp("vl.XX", &filename, NULL);
> >> > if (fd < 0) {
> >> > -return -errno;
> >> > +return NULL;
> >> > }
> >> > if (close(fd) != 0) {
> >> > unlink(filename);
> >> > -return -errno;
> >> > +return NULL;
> >> > }
> >> > -return 0;
> >> > -#endif
> >> > +return g_steal_pointer(&filename);
> >> > }
> >>
> >> Oh my, what a lovely mess you're messing with!
> >>
> >> The function creates a temporary *file*, not just a filename. Makes
> >> sense, as creating a unique filename is inherently racy. The contract
> >> is clear enough ("Create a uniquely-named empty temporary file"), but
> >> the function name is actively misleading.
> >
> > Agreed that the name is misleading.
>
> Care to fix that?
How about create_tmp_file()?
>
> >> Of course, creating a temporary file for the caller to (re)open is also
> >> racy. By the time the caller gets around to it, the filename could name
> >> anything. Return an open file file descriptor is a better idea. It's
> >> basically g_file_open_tmp(). Could we rework the two users of
> >> get_tmp_filename() accept a file descriptor?
> >
> > I looked at the 2 callers, and it looks like we need to do more than
> > these 2 callers to teach them to accept a file descriptor. :(
>
> Looks like it requires surgery to bdrv_create() at least. I'm not
> demanding you do that now.
>
Yes, big su
Re: [PATCH] qemu-nbd: set timeout to qemu-nbd socket
在 2022/9/26 20:44, Vladimir Sementsov-Ogievskiy 写道:
On 9/26/22 14:34, Denis V. Lunev wrote:
On 9/26/22 12:05, Vladimir Sementsov-Ogievskiy wrote:
[+ Den]
On 9/25/22 16:53, luzhipeng wrote:
From: lu zhipeng
Prevent the NBD socket stuck all the time, So
set timeout.
Signed-off-by: lu zhipeng
---
nbd/client.c | 8
1 file changed, 8 insertions(+)
diff --git a/nbd/client.c b/nbd/client.c
index 30d5383cb1..89dde53a0f 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -24,6 +24,8 @@
#include "nbd-internal.h"
#include "qemu/cutils.h"
+#define NBD_DEFAULT_TIMEOUT 30
+
/* Definitions for opaque data types */
static QTAILQ_HEAD(, NBDExport) exports =
QTAILQ_HEAD_INITIALIZER(exports);
@@ -1301,6 +1303,12 @@ int nbd_init(int fd, QIOChannelSocket *sioc,
NBDExportInfo *info,
}
}
+ if (ioctl(fd, NBD_SET_TIMEOUT, NBD_DEFAULT_TIMEOUT) < 0) {
+ int serrno = errno;
+ error_setg(errp, "Failed setting timeout");
+ return -serrno;
+ }
+
trace_nbd_init_finish();
return 0;
Personally, I don't see a problem in enabling timeout by default..
But probably we need a new option instead?
I believe that this should be the same story as we have had with
KEEPALIVE. This should be set as an option and downstream
will change its default when necessary.
It's also interesting, how NBD_SET_TIMEOUT would interfere with
keep-alive options set on the socket. Isn't existing keep-alive option
already enough, do we need both timeouts?
(and yes, if we need both ways for different cases, we definitely should
keep a possibility for the user to enable only one timeout, so now I
agree, that we need an option for this new feature)
Keep alive is only valid for tcp, but not for unix sockets
Re: [PATCH v2] block: Refactor get_tmp_filename()
Bin Meng writes:
> On Mon, Sep 26, 2022 at 6:13 PM Markus Armbruster wrote:
>>
>> Bin Meng writes:
>>
>> > From: Bin Meng
>> >
>> > At present there are two callers of get_tmp_filename() and they are
>> > inconsistent.
>> >
>> > One does:
>> >
>> > /* TODO: extra byte is a hack to ensure MAX_PATH space on Windows. */
>> > char *tmp_filename = g_malloc0(PATH_MAX + 1);
>> > ...
>> > ret = get_tmp_filename(tmp_filename, PATH_MAX + 1);
>> >
>> > while the other does:
>> >
>> > s->qcow_filename = g_malloc(PATH_MAX);
>> > ret = get_tmp_filename(s->qcow_filename, PATH_MAX);
>> >
>> > As we can see different 'size' arguments are passed. There are also
>> > platform specific implementations inside the function, and this use
>> > of snprintf is really undesirable.
>> >
>> > Refactor this routine by changing its signature to:
>> >
>> > char *get_tmp_filename(void)
>> >
>> > and use g_file_open_tmp() for a consistent implementation.
>> >
>> > Signed-off-by: Bin Meng
>> > ---
>> >
>> > Changes in v2:
>> > - Use g_autofree and g_steal_pointer
>> >
>> > include/block/block_int-common.h | 2 +-
>> > block.c | 42 ++--
>> > block/vvfat.c| 8 +++---
>> > 3 files changed, 18 insertions(+), 34 deletions(-)
>> >
>> > diff --git a/include/block/block_int-common.h
>> > b/include/block/block_int-common.h
>> > index 8947abab76..ea69a9349c 100644
>> > --- a/include/block/block_int-common.h
>> > +++ b/include/block/block_int-common.h
>> > @@ -1230,7 +1230,7 @@ static inline BlockDriverState *child_bs(BdrvChild
>> > *child)
>> > }
>> >
>> > int bdrv_check_request(int64_t offset, int64_t bytes, Error **errp);
>> > -int get_tmp_filename(char *filename, int size);
>> > +char *get_tmp_filename(void);
>> > void bdrv_parse_filename_strip_prefix(const char *filename, const char
>> > *prefix,
>> >QDict *options);
>> >
>> > diff --git a/block.c b/block.c
>> > index bc85f46eed..4e7a795566 100644
>> > --- a/block.c
>> > +++ b/block.c
>> > @@ -860,38 +860,23 @@ int bdrv_probe_geometry(BlockDriverState *bs,
>> > HDGeometry *geo)
>> >
>> > /*
>> > * Create a uniquely-named empty temporary file.
>> > - * Return 0 upon success, otherwise a negative errno value.
>> > + * Return the actual name used upon success, otherwise NULL.
>> > + * The called function is responsible for freeing it.
>> > */
>> > -int get_tmp_filename(char *filename, int size)
>> > +char *get_tmp_filename(void)
>> > {
>> > -#ifdef _WIN32
>> > -char temp_dir[MAX_PATH];
>> > -/* GetTempFileName requires that its output buffer (4th param)
>> > - have length MAX_PATH or greater. */
>> > -assert(size >= MAX_PATH);
>> > -return (GetTempPath(MAX_PATH, temp_dir)
>> > -&& GetTempFileName(temp_dir, "qem", 0, filename)
>> > -? 0 : -GetLastError());
>> > -#else
>> > +g_autofree char *filename = NULL;
>> > int fd;
>> > -const char *tmpdir;
>> > -tmpdir = getenv("TMPDIR");
>> > -if (!tmpdir) {
>> > -tmpdir = "/var/tmp";
>> > -}
>> > -if (snprintf(filename, size, "%s/vl.XX", tmpdir) >= size) {
>> > -return -EOVERFLOW;
>> > -}
>> > -fd = mkstemp(filename);
>> > +
>> > +fd = g_file_open_tmp("vl.XX", &filename, NULL);
>> > if (fd < 0) {
>> > -return -errno;
>> > +return NULL;
>> > }
>> > if (close(fd) != 0) {
>> > unlink(filename);
>> > -return -errno;
>> > +return NULL;
>> > }
>> > -return 0;
>> > -#endif
>> > +return g_steal_pointer(&filename);
>> > }
>>
>> Oh my, what a lovely mess you're messing with!
>>
>> The function creates a temporary *file*, not just a filename. Makes
>> sense, as creating a unique filename is inherently racy. The contract
>> is clear enough ("Create a uniquely-named empty temporary file"), but
>> the function name is actively misleading.
>
> Agreed that the name is misleading.
Care to fix that?
>> Of course, creating a temporary file for the caller to (re)open is also
>> racy. By the time the caller gets around to it, the filename could name
>> anything. Return an open file file descriptor is a better idea. It's
>> basically g_file_open_tmp(). Could we rework the two users of
>> get_tmp_filename() accept a file descriptor?
>
> I looked at the 2 callers, and it looks like we need to do more than
> these 2 callers to teach them to accept a file descriptor. :(
Looks like it requires surgery to bdrv_create() at least. I'm not
demanding you do that now.
[...]
Re: [PATCH] qemu-nbd: set timeout to qemu-nbd socket
在 2022/9/26 20:44, Vladimir Sementsov-Ogievskiy 写道:
On 9/26/22 14:34, Denis V. Lunev wrote:
On 9/26/22 12:05, Vladimir Sementsov-Ogievskiy wrote:
[+ Den]
On 9/25/22 16:53, luzhipeng wrote:
From: lu zhipeng
Prevent the NBD socket stuck all the time, So
set timeout.
Signed-off-by: lu zhipeng
---
nbd/client.c | 8
1 file changed, 8 insertions(+)
diff --git a/nbd/client.c b/nbd/client.c
index 30d5383cb1..89dde53a0f 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -24,6 +24,8 @@
#include "nbd-internal.h"
#include "qemu/cutils.h"
+#define NBD_DEFAULT_TIMEOUT 30
+
/* Definitions for opaque data types */
static QTAILQ_HEAD(, NBDExport) exports =
QTAILQ_HEAD_INITIALIZER(exports);
@@ -1301,6 +1303,12 @@ int nbd_init(int fd, QIOChannelSocket *sioc,
NBDExportInfo *info,
}
}
+ if (ioctl(fd, NBD_SET_TIMEOUT, NBD_DEFAULT_TIMEOUT) < 0) {
+ int serrno = errno;
+ error_setg(errp, "Failed setting timeout");
+ return -serrno;
+ }
+
trace_nbd_init_finish();
return 0;
Personally, I don't see a problem in enabling timeout by default..
But probably we need a new option instead?
I believe that this should be the same story as we have had with
KEEPALIVE. This should be set as an option and downstream
will change its default when necessary.
It's also interesting, how NBD_SET_TIMEOUT would interfere with
keep-alive options set on the socket. Isn't existing keep-alive option
already enough, do we need both timeouts?
(and yes, if we need both ways for different cases, we definitely should
keep a possibility for the user to enable only one timeout, so now I
agree, that we need an option for this new feature)
keep-alive is only valid for tcp mode, unix domain is not valid
Re: [PATCH v3 37/54] tests/qtest: {ahci, ide}-test: Use relative path for temporary files for win32
Hi Thomas,
On Tue, Sep 27, 2022 at 12:20 AM Thomas Huth wrote:
>
> On 25/09/2022 13.30, Bin Meng wrote:
> > From: Bin Meng
> >
> > These test cases uses "blkdebug:path/to/config:path/to/image" for
> > testing. On Windows, absolute file paths contain the delimiter ':'
> > which causes the blkdebug filename parser fail to parse filenames.
> >
> > Signed-off-by: Bin Meng
> > Reviewed-by: Marc-André Lureau
> > ---
> >
> > (no changes since v1)
> >
> > tests/qtest/ahci-test.c | 21 ++---
> > tests/qtest/ide-test.c | 20 ++--
> > 2 files changed, 36 insertions(+), 5 deletions(-)
> >
> > diff --git a/tests/qtest/ahci-test.c b/tests/qtest/ahci-test.c
> > index 1d5929d8c3..66652fed04 100644
> > --- a/tests/qtest/ahci-test.c
> > +++ b/tests/qtest/ahci-test.c
> > @@ -1833,7 +1833,7 @@ static void create_ahci_io_test(enum IOMode type,
> > enum AddrMode addr,
> >
> > int main(int argc, char **argv)
> > {
> > -const char *arch;
> > +const char *arch, *base;
> > int ret;
> > int fd;
> > int c;
> > @@ -1871,8 +1871,22 @@ int main(int argc, char **argv)
> > return 0;
> > }
> >
> > +/*
> > + * "base" stores the starting point where we create temporary files.
> > + *
> > + * On Windows, this is set to the relative path of current working
> > + * directory, because the absolute path causes the blkdebug filename
> > + * parser fail to parse "blkdebug:path/to/config:path/to/image".
> > + */
> > +#ifndef _WIN32
> > +base = g_get_tmp_dir();
> > +#else
> > +base = ".";
> > +#endif
> > +
> > /* Create a temporary image */
> > -fd = g_file_open_tmp("qtest.XX", &tmp_path, NULL);
> > +tmp_path = g_strdup_printf("%s/qtest.XX", base);
> > +fd = g_mkstemp(tmp_path);
> > g_assert(fd >= 0);
> > if (have_qemu_img()) {
> > imgfmt = "qcow2";
> > @@ -1889,7 +1903,8 @@ int main(int argc, char **argv)
> > close(fd);
> >
> > /* Create temporary blkdebug instructions */
> > -fd = g_file_open_tmp("qtest-blkdebug.XX", &debug_path, NULL);
> > +debug_path = g_strdup_printf("%s/qtest-blkdebug.XX", base);
> > +fd = g_mkstemp(debug_path);
> > g_assert(fd >= 0);
> > close(fd);
>
> It would maybe make sense to merge this with patch 05 ("tests/qtest:
> ahci-test: Avoid using hardcoded /tmp") ? ... but if you want to keep it
> separate, that's fine for me, too.
I'd prefer to keep these two patches separate as they are resolving
different issues.
> Reviewed-by: Thomas Huth
>
Thanks for the review!
Regards,
Bin
Regards,
Bin
Re: [PATCH v3 37/54] tests/qtest: {ahci, ide}-test: Use relative path for temporary files for win32
On 25/09/2022 13.30, Bin Meng wrote:
From: Bin Meng
These test cases uses "blkdebug:path/to/config:path/to/image" for
testing. On Windows, absolute file paths contain the delimiter ':'
which causes the blkdebug filename parser fail to parse filenames.
Signed-off-by: Bin Meng
Reviewed-by: Marc-André Lureau
---
(no changes since v1)
tests/qtest/ahci-test.c | 21 ++---
tests/qtest/ide-test.c | 20 ++--
2 files changed, 36 insertions(+), 5 deletions(-)
diff --git a/tests/qtest/ahci-test.c b/tests/qtest/ahci-test.c
index 1d5929d8c3..66652fed04 100644
--- a/tests/qtest/ahci-test.c
+++ b/tests/qtest/ahci-test.c
@@ -1833,7 +1833,7 @@ static void create_ahci_io_test(enum IOMode type, enum
AddrMode addr,
int main(int argc, char **argv)
{
-const char *arch;
+const char *arch, *base;
int ret;
int fd;
int c;
@@ -1871,8 +1871,22 @@ int main(int argc, char **argv)
return 0;
}
+/*
+ * "base" stores the starting point where we create temporary files.
+ *
+ * On Windows, this is set to the relative path of current working
+ * directory, because the absolute path causes the blkdebug filename
+ * parser fail to parse "blkdebug:path/to/config:path/to/image".
+ */
+#ifndef _WIN32
+base = g_get_tmp_dir();
+#else
+base = ".";
+#endif
+
/* Create a temporary image */
-fd = g_file_open_tmp("qtest.XX", &tmp_path, NULL);
+tmp_path = g_strdup_printf("%s/qtest.XX", base);
+fd = g_mkstemp(tmp_path);
g_assert(fd >= 0);
if (have_qemu_img()) {
imgfmt = "qcow2";
@@ -1889,7 +1903,8 @@ int main(int argc, char **argv)
close(fd);
/* Create temporary blkdebug instructions */
-fd = g_file_open_tmp("qtest-blkdebug.XX", &debug_path, NULL);
+debug_path = g_strdup_printf("%s/qtest-blkdebug.XX", base);
+fd = g_mkstemp(debug_path);
g_assert(fd >= 0);
close(fd);
It would maybe make sense to merge this with patch 05 ("tests/qtest:
ahci-test: Avoid using hardcoded /tmp") ? ... but if you want to keep it
separate, that's fine for me, too.
Reviewed-by: Thomas Huth
Re: [PATCH 1/1] 9pfs: avoid iterator invalidation in v9fs_mark_fids_unreclaim
On Montag, 26. September 2022 14:42:06 CEST Linus Heckemann wrote:
> Previously, the yielding in v9fs_reopen_fid and put_fid could result
> in other parts of the code modifying the fid table. This would
> invalidate the hash table iterator, causing misbehaviour.
>
> Now we ensure that we complete the iteration before yielding, so that
> the iterator remains valid throughout the loop, and only reopen the
> fids afterwards.
> ---
Ah, you sent this fix as a separate patch on top. I actually just meant that
you would take my already queued patch as the latest version (just because I
had made some minor changes on my end) and adjust that patch further as v4.
Anyway, there are still some things to do here, so maybe you can send your
patch squashed in the next round ...
> @Christian: I still haven't been able to reproduce the issue that this
> commit is supposed to fix (I tried building KDE too, no problems), so
> it's a bit of a shot in the dark. It certainly still runs and I think it
> should fix the issue, but it would be great if you could test it.
No worries about reproduction, I will definitely test this thoroughly. ;-)
> hw/9pfs/9p.c | 46 ++
> 1 file changed, 30 insertions(+), 16 deletions(-)
>
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index f4c1e37202..825c39e122 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -522,33 +522,47 @@ static int coroutine_fn
> v9fs_mark_fids_unreclaim(V9fsPDU *pdu, V9fsPath *path) V9fsFidState *fidp;
> gpointer fid;
> GHashTableIter iter;
> +/*
> + * The most common case is probably that we have exactly one
> + * fid for the given path, so preallocate exactly one.
> + */
> +GArray *to_reopen = g_array_sized_new(FALSE, FALSE,
> sizeof(V9fsFidState*), 1); +gint i;
Please use `g_autoptr(GArray)` instead of `GArray *`, that avoids the need for
explicit calls to g_array_free() below.
>
> g_hash_table_iter_init(&iter, s->fids);
>
> +/*
> + * We iterate over the fid table looking for the entries we need
> + * to reopen, and store them in to_reopen. This is because
> + * reopening them happens asynchronously, allowing the fid table
> + * to be modified in the meantime, invalidating our iterator.
> + */
> while (g_hash_table_iter_next(&iter, &fid, (gpointer *) &fidp)) {
> -/*
> - * Ensure the fid survives a potential clunk request during
> - * v9fs_reopen_fid.
> - */
> -fidp->ref++;
> -
> if (fidp->path.size == path->size &&
> !memcmp(fidp->path.data, path->data, path->size)) {
> -/* Mark the fid non reclaimable. */
> -fidp->flags |= FID_NON_RECLAIMABLE;
Why did you remove that? It should still be marked as FID_NON_RECLAIMABLE, no?
> -
> -/* reopen the file/dir if already closed */
> -err = v9fs_reopen_fid(pdu, fidp);
> -if (err < 0) {
> -put_fid(pdu, fidp);
> -return err;
> -}
> +/*
> + * Ensure the fid survives a potential clunk request during
> + * v9fs_reopen_fid or put_fid.
> + */
> +fidp->ref++;
Hmm, bumping the refcount here makes sense, as the 2nd loop may be interrupted
and the fid otherwise disappear in between, but ...
> +g_array_append_val(to_reopen, fidp);
> }
> +}
>
> -/* We're done with this fid */
> +for (i=0; i < to_reopen->len; i++) {
> +fidp = g_array_index(to_reopen, V9fsFidState*, i);
> +/* reopen the file/dir if already closed */
> +err = v9fs_reopen_fid(pdu, fidp);
> +if (err < 0) {
> +put_fid(pdu, fidp);
> +g_array_free(to_reopen, TRUE);
> +return err;
... this return would then leak all remainder fids that you have bumped the
refcount for above already.
> +}
> put_fid(pdu, fidp);
> }
>
> +g_array_free(to_reopen, TRUE);
> +
With `g_autoptr(GArray)` you can drop both g_array_free() calls.
> return 0;
> }
Also: I noticed that your changes in virtfs_reset() would need the same 2-loop
hack to avoid hash iterator invalidation, as it would also call put_fid()
inside the loop and be prone for hash iterator invalidation otherwise.
Best regards,
Christian Schoenebeck
Re: [PATCH v2] block: Refactor get_tmp_filename()
On Mon, Sep 26, 2022 at 6:13 PM Markus Armbruster wrote:
>
> Bin Meng writes:
>
> > From: Bin Meng
> >
> > At present there are two callers of get_tmp_filename() and they are
> > inconsistent.
> >
> > One does:
> >
> > /* TODO: extra byte is a hack to ensure MAX_PATH space on Windows. */
> > char *tmp_filename = g_malloc0(PATH_MAX + 1);
> > ...
> > ret = get_tmp_filename(tmp_filename, PATH_MAX + 1);
> >
> > while the other does:
> >
> > s->qcow_filename = g_malloc(PATH_MAX);
> > ret = get_tmp_filename(s->qcow_filename, PATH_MAX);
> >
> > As we can see different 'size' arguments are passed. There are also
> > platform specific implementations inside the function, and this use
> > of snprintf is really undesirable.
> >
> > Refactor this routine by changing its signature to:
> >
> > char *get_tmp_filename(void)
> >
> > and use g_file_open_tmp() for a consistent implementation.
> >
> > Signed-off-by: Bin Meng
> > ---
> >
> > Changes in v2:
> > - Use g_autofree and g_steal_pointer
> >
> > include/block/block_int-common.h | 2 +-
> > block.c | 42 ++--
> > block/vvfat.c| 8 +++---
> > 3 files changed, 18 insertions(+), 34 deletions(-)
> >
> > diff --git a/include/block/block_int-common.h
> > b/include/block/block_int-common.h
> > index 8947abab76..ea69a9349c 100644
> > --- a/include/block/block_int-common.h
> > +++ b/include/block/block_int-common.h
> > @@ -1230,7 +1230,7 @@ static inline BlockDriverState *child_bs(BdrvChild
> > *child)
> > }
> >
> > int bdrv_check_request(int64_t offset, int64_t bytes, Error **errp);
> > -int get_tmp_filename(char *filename, int size);
> > +char *get_tmp_filename(void);
> > void bdrv_parse_filename_strip_prefix(const char *filename, const char
> > *prefix,
> >QDict *options);
> >
> > diff --git a/block.c b/block.c
> > index bc85f46eed..4e7a795566 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -860,38 +860,23 @@ int bdrv_probe_geometry(BlockDriverState *bs,
> > HDGeometry *geo)
> >
> > /*
> > * Create a uniquely-named empty temporary file.
> > - * Return 0 upon success, otherwise a negative errno value.
> > + * Return the actual name used upon success, otherwise NULL.
> > + * The called function is responsible for freeing it.
> > */
> > -int get_tmp_filename(char *filename, int size)
> > +char *get_tmp_filename(void)
> > {
> > -#ifdef _WIN32
> > -char temp_dir[MAX_PATH];
> > -/* GetTempFileName requires that its output buffer (4th param)
> > - have length MAX_PATH or greater. */
> > -assert(size >= MAX_PATH);
> > -return (GetTempPath(MAX_PATH, temp_dir)
> > -&& GetTempFileName(temp_dir, "qem", 0, filename)
> > -? 0 : -GetLastError());
> > -#else
> > +g_autofree char *filename = NULL;
> > int fd;
> > -const char *tmpdir;
> > -tmpdir = getenv("TMPDIR");
> > -if (!tmpdir) {
> > -tmpdir = "/var/tmp";
> > -}
> > -if (snprintf(filename, size, "%s/vl.XX", tmpdir) >= size) {
> > -return -EOVERFLOW;
> > -}
> > -fd = mkstemp(filename);
> > +
> > +fd = g_file_open_tmp("vl.XX", &filename, NULL);
> > if (fd < 0) {
> > -return -errno;
> > +return NULL;
> > }
> > if (close(fd) != 0) {
> > unlink(filename);
> > -return -errno;
> > +return NULL;
> > }
> > -return 0;
> > -#endif
> > +return g_steal_pointer(&filename);
> > }
>
> Oh my, what a lovely mess you're messing with!
>
> The function creates a temporary *file*, not just a filename. Makes
> sense, as creating a unique filename is inherently racy. The contract
> is clear enough ("Create a uniquely-named empty temporary file"), but
> the function name is actively misleading.
Agreed that the name is misleading.
> Of course, creating a temporary file for the caller to (re)open is also
> racy. By the time the caller gets around to it, the filename could name
> anything. Return an open file file descriptor is a better idea. It's
> basically g_file_open_tmp(). Could we rework the two users of
> get_tmp_filename() accept a file descriptor?
I looked at the 2 callers, and it looks like we need to do more than
these 2 callers to teach them to accept a file descriptor. :(
>
> Anyway, code after the patch:
>
>/*
> * Create a uniquely-named empty temporary file.
> * Return the actual name used upon success, otherwise NULL.
> * The called function is responsible for freeing it.
> */
>char *get_tmp_filename(void)
>{
>g_autofree char *filename = NULL;
>int fd;
>
>fd = g_file_open_tmp("vl.XX", &filename, NULL);
>if (fd < 0) {
>
> As Philippe wrote, this throws away possibly useful error information.
>
>return NULL;
>}
>if (close(fd) != 0) {
>unlink(filename);
>return NULL;
>}
>re
Re: [PATCH v3 19/54] tests/qtest: virtio-blk-test: Avoid using hardcoded /tmp
On Sun, Sep 25, 2022 at 4:09 PM Bin Meng wrote:
> From: Bin Meng
>
> This case was written to use hardcoded /tmp directory for temporary
> files. Update to use g_file_open_tmp() for a portable implementation.
>
> Signed-off-by: Bin Meng
>
Reviewed-by: Marc-André Lureau
> ---
>
> Changes in v3:
> - Split to a separate patch
>
> tests/qtest/virtio-blk-test.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/tests/qtest/virtio-blk-test.c b/tests/qtest/virtio-blk-test.c
> index dc5eed31c8..19c01f808b 100644
> --- a/tests/qtest/virtio-blk-test.c
> +++ b/tests/qtest/virtio-blk-test.c
> @@ -49,10 +49,10 @@ static void drive_destroy(void *path)
> static char *drive_create(void)
> {
> int fd, ret;
> -char *t_path = g_strdup("/tmp/qtest.XX");
> +char *t_path;
>
> /* Create a temporary raw image */
> -fd = mkstemp(t_path);
> +fd = g_file_open_tmp("qtest.XX", &t_path, NULL);
> g_assert_cmpint(fd, >=, 0);
> ret = ftruncate(fd, TEST_IMAGE_SIZE);
> g_assert_cmpint(ret, ==, 0);
> --
> 2.34.1
>
>
>
--
Marc-André Lureau
Re: [PATCH v3 13/54] tests/qtest: ide-test: Avoid using hardcoded /tmp
On Sun, Sep 25, 2022 at 3:56 PM Bin Meng wrote:
> From: Bin Meng
>
> This case was written to use hardcoded /tmp directory for temporary
> files. Update to use g_file_open_tmp() for a portable implementation.
>
> Signed-off-by: Bin Meng
>
Reviewed-by: Marc-André Lureau
> ---
>
> Changes in v3:
> - Split to a separate patch
>
> tests/qtest/ide-test.c | 10 ++
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/tests/qtest/ide-test.c b/tests/qtest/ide-test.c
> index 5bcb75a7e5..25302be6dc 100644
> --- a/tests/qtest/ide-test.c
> +++ b/tests/qtest/ide-test.c
> @@ -121,8 +121,8 @@ enum {
> static QPCIBus *pcibus = NULL;
> static QGuestAllocator guest_malloc;
>
> -static char tmp_path[] = "/tmp/qtest.XX";
> -static char debug_path[] = "/tmp/qtest-blkdebug.XX";
> +static char *tmp_path;
> +static char *debug_path;
>
> static QTestState *ide_test_start(const char *cmdline_fmt, ...)
> {
> @@ -1015,12 +1015,12 @@ int main(int argc, char **argv)
> int ret;
>
> /* Create temporary blkdebug instructions */
> -fd = mkstemp(debug_path);
> +fd = g_file_open_tmp("qtest-blkdebug.XX", &debug_path, NULL);
> g_assert(fd >= 0);
> close(fd);
>
> /* Create a temporary raw image */
> -fd = mkstemp(tmp_path);
> +fd = g_file_open_tmp("qtest.XX", &tmp_path, NULL);
> g_assert(fd >= 0);
> ret = ftruncate(fd, TEST_IMAGE_SIZE);
> g_assert(ret == 0);
> @@ -1049,7 +1049,9 @@ int main(int argc, char **argv)
>
> /* Cleanup */
> unlink(tmp_path);
> +g_free(tmp_path);
> unlink(debug_path);
> +g_free(debug_path);
>
> return ret;
> }
> --
> 2.34.1
>
>
>
--
Marc-André Lureau
Re: [PATCH v3 09/54] tests/qtest: fdc-test: Avoid using hardcoded /tmp
On Sun, Sep 25, 2022 at 3:49 PM Bin Meng wrote:
> From: Bin Meng
>
> This case was written to use hardcoded /tmp directory for temporary
> files. Update to use g_file_open_tmp() for a portable implementation.
>
> Signed-off-by: Bin Meng
>
Reviewed-by: Marc-André Lureau
> ---
>
> Changes in v3:
> - Split to a separate patch
>
> tests/qtest/fdc-test.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/tests/qtest/fdc-test.c b/tests/qtest/fdc-test.c
> index 52ade90a7d..1f9b99ad6d 100644
> --- a/tests/qtest/fdc-test.c
> +++ b/tests/qtest/fdc-test.c
> @@ -68,7 +68,7 @@ enum {
> DSKCHG = 0x80,
> };
>
> -static char test_image[] = "/tmp/qtest.XX";
> +static char *test_image;
>
> #define assert_bit_set(data, mask) g_assert_cmphex((data) & (mask), ==,
> (mask))
> #define assert_bit_clear(data, mask) g_assert_cmphex((data) & (mask), ==,
> 0)
> @@ -608,7 +608,7 @@ int main(int argc, char **argv)
> int ret;
>
> /* Create a temporary raw image */
> -fd = mkstemp(test_image);
> +fd = g_file_open_tmp("qtest.XX", &test_image, NULL);
> g_assert(fd >= 0);
> ret = ftruncate(fd, TEST_IMAGE_SIZE);
> g_assert(ret == 0);
> @@ -640,6 +640,7 @@ int main(int argc, char **argv)
> /* Cleanup */
> qtest_end();
> unlink(test_image);
> +g_free(test_image);
>
> return ret;
> }
> --
> 2.34.1
>
>
>
--
Marc-André Lureau
[PATCH 1/1] 9pfs: avoid iterator invalidation in v9fs_mark_fids_unreclaim
Previously, the yielding in v9fs_reopen_fid and put_fid could result
in other parts of the code modifying the fid table. This would
invalidate the hash table iterator, causing misbehaviour.
Now we ensure that we complete the iteration before yielding, so that
the iterator remains valid throughout the loop, and only reopen the
fids afterwards.
---
@Christian: I still haven't been able to reproduce the issue that this
commit is supposed to fix (I tried building KDE too, no problems), so
it's a bit of a shot in the dark. It certainly still runs and I think it
should fix the issue, but it would be great if you could test it.
hw/9pfs/9p.c | 46 ++
1 file changed, 30 insertions(+), 16 deletions(-)
diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index f4c1e37202..825c39e122 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -522,33 +522,47 @@ static int coroutine_fn v9fs_mark_fids_unreclaim(V9fsPDU
*pdu, V9fsPath *path)
V9fsFidState *fidp;
gpointer fid;
GHashTableIter iter;
+/*
+ * The most common case is probably that we have exactly one
+ * fid for the given path, so preallocate exactly one.
+ */
+GArray *to_reopen = g_array_sized_new(FALSE, FALSE, sizeof(V9fsFidState*),
1);
+gint i;
g_hash_table_iter_init(&iter, s->fids);
+/*
+ * We iterate over the fid table looking for the entries we need
+ * to reopen, and store them in to_reopen. This is because
+ * reopening them happens asynchronously, allowing the fid table
+ * to be modified in the meantime, invalidating our iterator.
+ */
while (g_hash_table_iter_next(&iter, &fid, (gpointer *) &fidp)) {
-/*
- * Ensure the fid survives a potential clunk request during
- * v9fs_reopen_fid.
- */
-fidp->ref++;
-
if (fidp->path.size == path->size &&
!memcmp(fidp->path.data, path->data, path->size)) {
-/* Mark the fid non reclaimable. */
-fidp->flags |= FID_NON_RECLAIMABLE;
-
-/* reopen the file/dir if already closed */
-err = v9fs_reopen_fid(pdu, fidp);
-if (err < 0) {
-put_fid(pdu, fidp);
-return err;
-}
+/*
+ * Ensure the fid survives a potential clunk request during
+ * v9fs_reopen_fid or put_fid.
+ */
+fidp->ref++;
+g_array_append_val(to_reopen, fidp);
}
+}
-/* We're done with this fid */
+for (i=0; i < to_reopen->len; i++) {
+fidp = g_array_index(to_reopen, V9fsFidState*, i);
+/* reopen the file/dir if already closed */
+err = v9fs_reopen_fid(pdu, fidp);
+if (err < 0) {
+put_fid(pdu, fidp);
+g_array_free(to_reopen, TRUE);
+return err;
+}
put_fid(pdu, fidp);
}
+g_array_free(to_reopen, TRUE);
+
return 0;
}
--
2.36.2
Re: [PATCH] qemu-nbd: set timeout to qemu-nbd socket
On 9/26/22 14:34, Denis V. Lunev wrote:
On 9/26/22 12:05, Vladimir Sementsov-Ogievskiy wrote:
[+ Den]
On 9/25/22 16:53, luzhipeng wrote:
From: lu zhipeng
Prevent the NBD socket stuck all the time, So
set timeout.
Signed-off-by: lu zhipeng
---
nbd/client.c | 8
1 file changed, 8 insertions(+)
diff --git a/nbd/client.c b/nbd/client.c
index 30d5383cb1..89dde53a0f 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -24,6 +24,8 @@
#include "nbd-internal.h"
#include "qemu/cutils.h"
+#define NBD_DEFAULT_TIMEOUT 30
+
/* Definitions for opaque data types */
static QTAILQ_HEAD(, NBDExport) exports = QTAILQ_HEAD_INITIALIZER(exports);
@@ -1301,6 +1303,12 @@ int nbd_init(int fd, QIOChannelSocket *sioc,
NBDExportInfo *info,
}
}
+ if (ioctl(fd, NBD_SET_TIMEOUT, NBD_DEFAULT_TIMEOUT) < 0) {
+ int serrno = errno;
+ error_setg(errp, "Failed setting timeout");
+ return -serrno;
+ }
+
trace_nbd_init_finish();
return 0;
Personally, I don't see a problem in enabling timeout by default.. But probably
we need a new option instead?
I believe that this should be the same story as we have had with
KEEPALIVE. This should be set as an option and downstream
will change its default when necessary.
It's also interesting, how NBD_SET_TIMEOUT would interfere with keep-alive
options set on the socket. Isn't existing keep-alive option already enough, do
we need both timeouts?
(and yes, if we need both ways for different cases, we definitely should keep a
possibility for the user to enable only one timeout, so now I agree, that we
need an option for this new feature)
--
Best regards,
Vladimir
Re: [PATCH v12 21/21] job: remove unused functions
On 9/26/22 12:32, Emanuele Giuseppe Esposito wrote: These public functions are not used anywhere, thus can be dropped. Also, since this is the final job API that doesn't use AioContext lock and replaces it with job_lock, adjust all remaining function documentation to clearly specify if the job lock is taken or not. Also document the locking requirements for a few functions where the second version is not removed. Signed-off-by: Emanuele Giuseppe Esposito Reviewed-by: Stefan Hajnoczi Reviewed-by: Kevin Wolf Reviewed-by: Vladimir Sementsov-Ogievskiy -- Best regards, Vladimir
Re: [PATCH v12 18/21] job.c: enable job lock/unlock and remove Aiocontext locks
On 9/26/22 12:32, Emanuele Giuseppe Esposito wrote:
Change the job_{lock/unlock} and macros to use job_mutex.
Now that they are not nop anymore, remove the aiocontext
to avoid deadlocks.
Therefore:
- when possible, remove completely the aiocontext lock/unlock pair
- if it is used by some other function too, reduce the locking
section as much as possible, leaving the job API outside.
- change AIO_WAIT_WHILE in AIO_WAIT_WHILE_UNLOCKED, since we
are not using the aiocontext lock anymore
The only functions that still need the aiocontext lock are:
- the JobDriver callbacks, already documented in job.h
- job_cancel_sync() in replication.c is called with aio_context_lock
taken, but now job is using AIO_WAIT_WHILE_UNLOCKED so we need to
release the lock.
Reduce the locking section to only cover the callback invocation
and document the functions that take the AioContext lock,
to avoid taking it twice.
Also remove real_job_{lock/unlock}, as they are replaced by the
public functions.
Signed-off-by: Emanuele Giuseppe Esposito
Reviewed-by: Vladimir Sementsov-Ogievskiy
I'm still not sure about aio_poll() call inside dropped aio-context
acquire/release pair in run_block_job(), so that's up to other maintainers.
Anyway I don't expect some real locking/racing problems in qemu-img.
--
Best regards,
Vladimir
Re: [PATCH v3 05/54] tests/qtest: ahci-test: Avoid using hardcoded /tmp
On 25/09/2022 13.29, Bin Meng wrote: From: Bin Meng This case was written to use hardcoded /tmp directory for temporary files. Update to use g_file_open_tmp() for a portable implementation. Signed-off-by: Bin Meng --- Changes in v3: - Split to a separate patch - Ensure g_autofree variable is initialized tests/qtest/ahci-test.c | 19 +++ 1 file changed, 11 insertions(+), 8 deletions(-) Reviewed-by: Thomas Huth
Re: [PATCH v11 18/21] job.c: enable job lock/unlock and remove Aiocontext locks
On 9/18/22 20:12, Emanuele Giuseppe Esposito wrote:
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -911,7 +911,6 @@ static void run_block_job(BlockJob *job, Error
**errp)
AioContext *aio_context = block_job_get_aio_context(job);
int ret = 0;
- aio_context_acquire(aio_context);
job_lock();
job_ref_locked(&job->job);
do {
aio_poll() call here, doesn't require aio_context to be acquired?
On the contrary I think, if you see in AIO_WAIT_WHILE we explicitly
release it because we don't want to allow something else to run with the
aiocontext acquired.
Still, in AIO_WAIT_WHILE() we release ctx_, but do
aio_poll(qemu_get_aio_context(), true), so we poll in other context.
But here in qemu-img.c we drop aiocontext lock exactly for aio_context, which
is an argument of aio_poll()..
--
Best regards,
Vladimir
Re: [PATCH v12 13/21] jobs: protect job.aio_context with BQL and job_mutex
On 9/26/22 12:32, Emanuele Giuseppe Esposito wrote:
In order to make it thread safe, implement a "fake rwlock",
where we allow reads under BQL *or* job_mutex held, but
writes only under BQL *and* job_mutex.
The only write we have is in child_job_set_aio_ctx, which always
happens under drain (so the job is paused).
For this reason, introduce job_set_aio_context and make sure that
the context is set under BQL, job_mutex and drain.
Also make sure all other places where the aiocontext is read
are protected.
The reads in commit.c and mirror.c are actually safe, because always
done under BQL.
Note: at this stage, job_{lock/unlock} and job lock guard macros
are *nop*.
Suggested-by: Paolo Bonzini
Signed-off-by: Emanuele Giuseppe Esposito
Reviewed-by: Vladimir Sementsov-Ogievskiy
---
block/replication.c | 1 +
blockjob.c | 3 ++-
[..]
--- a/include/qemu/job.h
+++ b/include/qemu/job.h
@@ -74,11 +74,17 @@ typedef struct Job {
/* ProgressMeter API is thread-safe */
ProgressMeter progress;
+/**
+ * AioContext to run the job coroutine in.
+ * The job Aiocontext can be read when holding *either*
+ * the BQL (so we are in the main loop) or the job_mutex.
+ * It can only be written when we hold *both* BQL
+ * and the job_mutex.
+ */
+AioContext *aio_context;
-/** Protected by AioContext lock */
-/** AioContext to run the job coroutine in */
-AioContext *aio_context;
+/** Protected by AioContext lock */
/** Reference count of the block job */
int refcnt;
@@ -741,4 +747,15 @@ int job_finish_sync(Job *job, void (*finish)(Job *, Error
**errp),
int job_finish_sync_locked(Job *job, void (*finish)(Job *, Error **errp),
Error **errp);
+/**
+ * Sets the @job->aio_context.
+ * Called with job_mutex *not* held.
+ *
+ * This function must run in the main thread to protect against
+ * concurrent read in job_finish_sync_locked(), takes the job_mutex
+ * lock to protect against the read in job_do_yield_locked(), and must
+ * be called when the job is quiescent.
I'd also add "set only by job_set_aio_context()".
+ */
+void job_set_aio_context(Job *job, AioContext *ctx);
+
#endif
--
Best regards,
Vladimir
Re: [PATCH] qemu-nbd: set timeout to qemu-nbd socket
On 9/26/22 12:05, Vladimir Sementsov-Ogievskiy wrote:
[+ Den]
On 9/25/22 16:53, luzhipeng wrote:
From: lu zhipeng
Prevent the NBD socket stuck all the time, So
set timeout.
Signed-off-by: lu zhipeng
---
nbd/client.c | 8
1 file changed, 8 insertions(+)
diff --git a/nbd/client.c b/nbd/client.c
index 30d5383cb1..89dde53a0f 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -24,6 +24,8 @@
#include "nbd-internal.h"
#include "qemu/cutils.h"
+#define NBD_DEFAULT_TIMEOUT 30
+
/* Definitions for opaque data types */
static QTAILQ_HEAD(, NBDExport) exports =
QTAILQ_HEAD_INITIALIZER(exports);
@@ -1301,6 +1303,12 @@ int nbd_init(int fd, QIOChannelSocket *sioc,
NBDExportInfo *info,
}
}
+ if (ioctl(fd, NBD_SET_TIMEOUT, NBD_DEFAULT_TIMEOUT) < 0) {
+ int serrno = errno;
+ error_setg(errp, "Failed setting timeout");
+ return -serrno;
+ }
+
trace_nbd_init_finish();
return 0;
Personally, I don't see a problem in enabling timeout by default.. But
probably we need a new option instead?
I believe that this should be the same story as we have had with
KEEPALIVE. This should be set as an option and downstream
will change its default when necessary.
Den
Re: [PATCH v2] block: Refactor get_tmp_filename()
Bin Meng writes:
> From: Bin Meng
>
> At present there are two callers of get_tmp_filename() and they are
> inconsistent.
>
> One does:
>
> /* TODO: extra byte is a hack to ensure MAX_PATH space on Windows. */
> char *tmp_filename = g_malloc0(PATH_MAX + 1);
> ...
> ret = get_tmp_filename(tmp_filename, PATH_MAX + 1);
>
> while the other does:
>
> s->qcow_filename = g_malloc(PATH_MAX);
> ret = get_tmp_filename(s->qcow_filename, PATH_MAX);
>
> As we can see different 'size' arguments are passed. There are also
> platform specific implementations inside the function, and this use
> of snprintf is really undesirable.
>
> Refactor this routine by changing its signature to:
>
> char *get_tmp_filename(void)
>
> and use g_file_open_tmp() for a consistent implementation.
>
> Signed-off-by: Bin Meng
> ---
>
> Changes in v2:
> - Use g_autofree and g_steal_pointer
>
> include/block/block_int-common.h | 2 +-
> block.c | 42 ++--
> block/vvfat.c| 8 +++---
> 3 files changed, 18 insertions(+), 34 deletions(-)
>
> diff --git a/include/block/block_int-common.h
> b/include/block/block_int-common.h
> index 8947abab76..ea69a9349c 100644
> --- a/include/block/block_int-common.h
> +++ b/include/block/block_int-common.h
> @@ -1230,7 +1230,7 @@ static inline BlockDriverState *child_bs(BdrvChild
> *child)
> }
>
> int bdrv_check_request(int64_t offset, int64_t bytes, Error **errp);
> -int get_tmp_filename(char *filename, int size);
> +char *get_tmp_filename(void);
> void bdrv_parse_filename_strip_prefix(const char *filename, const char
> *prefix,
>QDict *options);
>
> diff --git a/block.c b/block.c
> index bc85f46eed..4e7a795566 100644
> --- a/block.c
> +++ b/block.c
> @@ -860,38 +860,23 @@ int bdrv_probe_geometry(BlockDriverState *bs,
> HDGeometry *geo)
>
> /*
> * Create a uniquely-named empty temporary file.
> - * Return 0 upon success, otherwise a negative errno value.
> + * Return the actual name used upon success, otherwise NULL.
> + * The called function is responsible for freeing it.
> */
> -int get_tmp_filename(char *filename, int size)
> +char *get_tmp_filename(void)
> {
> -#ifdef _WIN32
> -char temp_dir[MAX_PATH];
> -/* GetTempFileName requires that its output buffer (4th param)
> - have length MAX_PATH or greater. */
> -assert(size >= MAX_PATH);
> -return (GetTempPath(MAX_PATH, temp_dir)
> -&& GetTempFileName(temp_dir, "qem", 0, filename)
> -? 0 : -GetLastError());
> -#else
> +g_autofree char *filename = NULL;
> int fd;
> -const char *tmpdir;
> -tmpdir = getenv("TMPDIR");
> -if (!tmpdir) {
> -tmpdir = "/var/tmp";
> -}
> -if (snprintf(filename, size, "%s/vl.XX", tmpdir) >= size) {
> -return -EOVERFLOW;
> -}
> -fd = mkstemp(filename);
> +
> +fd = g_file_open_tmp("vl.XX", &filename, NULL);
> if (fd < 0) {
> -return -errno;
> +return NULL;
> }
> if (close(fd) != 0) {
> unlink(filename);
> -return -errno;
> +return NULL;
> }
> -return 0;
> -#endif
> +return g_steal_pointer(&filename);
> }
Oh my, what a lovely mess you're messing with!
The function creates a temporary *file*, not just a filename. Makes
sense, as creating a unique filename is inherently racy. The contract
is clear enough ("Create a uniquely-named empty temporary file"), but
the function name is actively misleading.
Of course, creating a temporary file for the caller to (re)open is also
racy. By the time the caller gets around to it, the filename could name
anything. Return an open file file descriptor is a better idea. It's
basically g_file_open_tmp(). Could we rework the two users of
get_tmp_filename() accept a file descriptor?
Anyway, code after the patch:
/*
* Create a uniquely-named empty temporary file.
* Return the actual name used upon success, otherwise NULL.
* The called function is responsible for freeing it.
*/
char *get_tmp_filename(void)
{
g_autofree char *filename = NULL;
int fd;
fd = g_file_open_tmp("vl.XX", &filename, NULL);
if (fd < 0) {
As Philippe wrote, this throws away possibly useful error information.
return NULL;
}
if (close(fd) != 0) {
unlink(filename);
return NULL;
}
return g_steal_pointer(&filename);
}
Other than that, it's an improvement within the limits of a flawed
interface.
[...]
Re: [PATCH] qemu-nbd: set timeout to qemu-nbd socket
[+ Den]
On 9/25/22 16:53, luzhipeng wrote:
From: lu zhipeng
Prevent the NBD socket stuck all the time, So
set timeout.
Signed-off-by: lu zhipeng
---
nbd/client.c | 8
1 file changed, 8 insertions(+)
diff --git a/nbd/client.c b/nbd/client.c
index 30d5383cb1..89dde53a0f 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -24,6 +24,8 @@
#include "nbd-internal.h"
#include "qemu/cutils.h"
+#define NBD_DEFAULT_TIMEOUT 30
+
/* Definitions for opaque data types */
static QTAILQ_HEAD(, NBDExport) exports = QTAILQ_HEAD_INITIALIZER(exports);
@@ -1301,6 +1303,12 @@ int nbd_init(int fd, QIOChannelSocket *sioc,
NBDExportInfo *info,
}
}
+if (ioctl(fd, NBD_SET_TIMEOUT, NBD_DEFAULT_TIMEOUT) < 0) {
+int serrno = errno;
+error_setg(errp, "Failed setting timeout");
+return -serrno;
+}
+
trace_nbd_init_finish();
return 0;
Personally, I don't see a problem in enabling timeout by default.. But probably
we need a new option instead?
--
Best regards,
Vladimir
[PATCH v12 21/21] job: remove unused functions
These public functions are not used anywhere, thus can be dropped. Also, since this is the final job API that doesn't use AioContext lock and replaces it with job_lock, adjust all remaining function documentation to clearly specify if the job lock is taken or not. Also document the locking requirements for a few functions where the second version is not removed. Signed-off-by: Emanuele Giuseppe Esposito Reviewed-by: Stefan Hajnoczi Reviewed-by: Kevin Wolf --- include/qemu/job.h | 110 + job.c | 107 ++-- tests/unit/test-blockjob.c | 4 +- 3 files changed, 46 insertions(+), 175 deletions(-) diff --git a/include/qemu/job.h b/include/qemu/job.h index 8c8c58dada..32ad5622e1 100644 --- a/include/qemu/job.h +++ b/include/qemu/job.h @@ -384,6 +384,8 @@ JobTxn *job_txn_new(void); /** * Release a reference that was previously acquired with job_txn_add_job or * job_txn_new. If it's the last reference to the object, it will be freed. + * + * Called with job lock *not* held. */ void job_txn_unref(JobTxn *txn); @@ -413,21 +415,18 @@ void *job_create(const char *job_id, const JobDriver *driver, JobTxn *txn, /** * Add a reference to Job refcnt, it will be decreased with job_unref, and then * be freed if it comes to be the last reference. + * + * Called with job lock held. */ -void job_ref(Job *job); - -/* Same as job_ref(), but called with job lock held. */ void job_ref_locked(Job *job); /** - * Release a reference that was previously acquired with job_ref() or + * Release a reference that was previously acquired with job_ref_locked() or * job_create(). If it's the last reference to the object, it will be freed. * * Takes AioContext lock internally to invoke a job->driver callback. + * Called with job lock held. */ -void job_unref(Job *job); - -/* Same as job_unref(), but called with job lock held. */ void job_unref_locked(Job *job); /** @@ -473,12 +472,8 @@ void job_progress_increase_remaining(Job *job, uint64_t delta); * Conditionally enter the job coroutine if the job is ready to run, not * already busy and fn() returns true. fn() is called while under the job_lock * critical section. - */ -void job_enter_cond(Job *job, bool(*fn)(Job *job)); - -/* - * Same as job_enter_cond(), but called with job lock held. - * Might release the lock temporarily. + * + * Called with job lock held, but might release it temporarily. */ void job_enter_cond_locked(Job *job, bool(*fn)(Job *job)); @@ -557,11 +552,8 @@ bool job_cancel_requested(Job *job); /** * Returns whether the job is in a completed state. - * Called with job_mutex *not* held. + * Called with job lock held. */ -bool job_is_completed(Job *job); - -/* Same as job_is_completed(), but called with job lock held. */ bool job_is_completed_locked(Job *job); /** @@ -576,14 +568,16 @@ bool job_is_ready_locked(Job *job); /** * Request @job to pause at the next pause point. Must be paired with * job_resume(). If the job is supposed to be resumed by user action, call - * job_user_pause() instead. + * job_user_pause_locked() instead. + * + * Called with job lock *not* held. */ void job_pause(Job *job); /* Same as job_pause(), but called with job lock held. */ void job_pause_locked(Job *job); -/** Resumes a @job paused with job_pause. */ +/** Resumes a @job paused with job_pause. Called with job lock *not* held. */ void job_resume(Job *job); /* @@ -595,27 +589,20 @@ void job_resume_locked(Job *job); /** * Asynchronously pause the specified @job. * Do not allow a resume until a matching call to job_user_resume. + * Called with job lock held. */ -void job_user_pause(Job *job, Error **errp); - -/* Same as job_user_pause(), but called with job lock held. */ void job_user_pause_locked(Job *job, Error **errp); -/** Returns true if the job is user-paused. */ -bool job_user_paused(Job *job); - -/* Same as job_user_paused(), but called with job lock held. */ +/** + * Returns true if the job is user-paused. + * Called with job lock held. + */ bool job_user_paused_locked(Job *job); /** * Resume the specified @job. - * Must be paired with a preceding job_user_pause. - */ -void job_user_resume(Job *job, Error **errp); - -/* - * Same as job_user_resume(), but called with job lock held. - * Might release the lock temporarily. + * Must be paired with a preceding job_user_pause_locked. + * Called with job lock held, but might release it temporarily. */ void job_user_resume_locked(Job *job, Error **errp); @@ -624,6 +611,7 @@ void job_user_resume_locked(Job *job, Error **errp); * first one if @job is %NULL. * * Returns the requested job, or %NULL if there are no more jobs left. + * Called with job lock *not* held. */ Job *job_next(Job *job); @@ -634,20 +622,17 @@ Job *job_next_locked(Job *job); * Get the job identified by @id (which must not be %NULL). * * Returns the requested job, or %NUL
[PATCH v12 14/21] blockjob.h: categorize fields in struct BlockJob
The same job lock is being used also to protect some of blockjob fields.
Categorize them just as done in job.h.
Reviewed-by: Vladimir Sementsov-Ogievskiy
Signed-off-by: Emanuele Giuseppe Esposito
---
include/block/blockjob.h | 32 ++--
1 file changed, 26 insertions(+), 6 deletions(-)
diff --git a/include/block/blockjob.h b/include/block/blockjob.h
index 8b65d3949d..10c24e240a 100644
--- a/include/block/blockjob.h
+++ b/include/block/blockjob.h
@@ -40,21 +40,38 @@ typedef struct BlockJobDriver BlockJobDriver;
* Long-running operation on a BlockDriverState.
*/
typedef struct BlockJob {
-/** Data belonging to the generic Job infrastructure */
+/**
+ * Data belonging to the generic Job infrastructure.
+ * Protected by job mutex.
+ */
Job job;
-/** Status that is published by the query-block-jobs QMP API */
+/**
+ * Status that is published by the query-block-jobs QMP API.
+ * Protected by job mutex.
+ */
BlockDeviceIoStatus iostatus;
-/** Speed that was set with @block_job_set_speed. */
+/**
+ * Speed that was set with @block_job_set_speed.
+ * Always modified and read under QEMU global mutex (GLOBAL_STATE_CODE).
+ */
int64_t speed;
-/** Rate limiting data structure for implementing @speed. */
+/**
+ * Rate limiting data structure for implementing @speed.
+ * RateLimit API is thread-safe.
+ */
RateLimit limit;
-/** Block other operations when block job is running */
+/**
+ * Block other operations when block job is running.
+ * Always modified and read under QEMU global mutex (GLOBAL_STATE_CODE).
+ */
Error *blocker;
+/** All notifiers are set once in block_job_create() and never modified. */
+
/** Called when a cancelled job is finalised. */
Notifier finalize_cancelled_notifier;
@@ -70,7 +87,10 @@ typedef struct BlockJob {
/** Called when the job coroutine yields or terminates */
Notifier idle_notifier;
-/** BlockDriverStates that are involved in this block job */
+/**
+ * BlockDriverStates that are involved in this block job.
+ * Always modified and read under QEMU global mutex (GLOBAL_STATE_CODE).
+ */
GSList *nodes;
} BlockJob;
--
2.31.1
[PATCH v12 19/21] block_job_query: remove atomic read
Not sure what the atomic here was supposed to do, since job.busy is protected by the job lock. Since the whole function is called under job_mutex, just remove the atomic. Signed-off-by: Emanuele Giuseppe Esposito Reviewed-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Stefan Hajnoczi Reviewed-by: Kevin Wolf --- blockjob.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/blockjob.c b/blockjob.c index d04f804001..120c1b7ead 100644 --- a/blockjob.c +++ b/blockjob.c @@ -338,7 +338,7 @@ BlockJobInfo *block_job_query_locked(BlockJob *job, Error **errp) info = g_new0(BlockJobInfo, 1); info->type = g_strdup(job_type_str(&job->job)); info->device= g_strdup(job->job.id); -info->busy = qatomic_read(&job->job.busy); +info->busy = job->job.busy; info->paused= job->job.pause_count > 0; info->offset= progress_current; info->len = progress_total; -- 2.31.1
[PATCH v12 17/21] job.h: categorize JobDriver callbacks that need the AioContext lock
Some callbacks implementation use bdrv_* APIs that assume the
AioContext lock is held. Make sure this invariant is documented.
Signed-off-by: Emanuele Giuseppe Esposito
Reviewed-by: Vladimir Sementsov-Ogievskiy
---
include/qemu/job.h | 27 +--
1 file changed, 25 insertions(+), 2 deletions(-)
diff --git a/include/qemu/job.h b/include/qemu/job.h
index 50a4c06b93..a954f8f992 100644
--- a/include/qemu/job.h
+++ b/include/qemu/job.h
@@ -65,7 +65,11 @@ typedef struct Job {
/** True if this job should automatically dismiss itself */
bool auto_dismiss;
-/** The completion function that will be called when the job completes. */
+/**
+ * The completion function that will be called when the job completes.
+ * Called with AioContext lock held, since many callback implementations
+ * use bdrv_* functions that require to hold the lock.
+ */
BlockCompletionFunc *cb;
/** The opaque value that is passed to the completion function. */
@@ -260,6 +264,9 @@ struct JobDriver {
*
* This callback will not be invoked if the job has already failed.
* If it fails, abort and then clean will be called.
+ *
+ * Called with AioContext lock held, since many callbacs implementations
+ * use bdrv_* functions that require to hold the lock.
*/
int (*prepare)(Job *job);
@@ -270,6 +277,9 @@ struct JobDriver {
*
* All jobs will complete with a call to either .commit() or .abort() but
* never both.
+ *
+ * Called with AioContext lock held, since many callback implementations
+ * use bdrv_* functions that require to hold the lock.
*/
void (*commit)(Job *job);
@@ -280,6 +290,9 @@ struct JobDriver {
*
* All jobs will complete with a call to either .commit() or .abort() but
* never both.
+ *
+ * Called with AioContext lock held, since many callback implementations
+ * use bdrv_* functions that require to hold the lock.
*/
void (*abort)(Job *job);
@@ -288,6 +301,9 @@ struct JobDriver {
* .commit() or .abort(). Regardless of which callback is invoked after
* completion, .clean() will always be called, even if the job does not
* belong to a transaction group.
+ *
+ * Called with AioContext lock held, since many callbacs implementations
+ * use bdrv_* functions that require to hold the lock.
*/
void (*clean)(Job *job);
@@ -302,11 +318,18 @@ struct JobDriver {
* READY).
* (If the callback is NULL, the job is assumed to terminate
* without I/O.)
+ *
+ * Called with AioContext lock held, since many callback implementations
+ * use bdrv_* functions that require to hold the lock.
*/
bool (*cancel)(Job *job, bool force);
-/** Called when the job is freed */
+/**
+ * Called when the job is freed.
+ * Called with AioContext lock held, since many callback implementations
+ * use bdrv_* functions that require to hold the lock.
+ */
void (*free)(Job *job);
};
--
2.31.1
[PATCH v12 11/21] jobs: group together API calls under the same job lock
Now that the API offers also _locked() functions, take advantage
of it and give also the caller control to take the lock and call
_locked functions.
This makes sense especially when we have for loops, because it
makes no sense to have:
for(job = job_next(); ...)
where each job_next() takes the lock internally.
Instead we want
JOB_LOCK_GUARD();
for(job = job_next_locked(); ...)
In addition, protect also direct field accesses, by either creating a
new critical section or widening the existing ones.
Note: at this stage, job_{lock/unlock} and job lock guard macros
are *nop*.
Signed-off-by: Emanuele Giuseppe Esposito
Reviewed-by: Vladimir Sementsov-Ogievskiy
---
block.c| 17 ++---
blockdev.c | 14 ++
blockjob.c | 35 ++-
job-qmp.c | 9 ++---
monitor/qmp-cmds.c | 7 +--
qemu-img.c | 15 ++-
6 files changed, 63 insertions(+), 34 deletions(-)
diff --git a/block.c b/block.c
index bc85f46eed..2c6a4f62c9 100644
--- a/block.c
+++ b/block.c
@@ -4980,8 +4980,8 @@ static void bdrv_close(BlockDriverState *bs)
void bdrv_close_all(void)
{
-assert(job_next(NULL) == NULL);
GLOBAL_STATE_CODE();
+assert(job_next(NULL) == NULL);
/* Drop references from requests still in flight, such as canceled block
* jobs whose AIO context has not been polled yet */
@@ -6167,13 +6167,16 @@ XDbgBlockGraph *bdrv_get_xdbg_block_graph(Error **errp)
}
}
-for (job = block_job_next(NULL); job; job = block_job_next(job)) {
-GSList *el;
+WITH_JOB_LOCK_GUARD() {
+for (job = block_job_next_locked(NULL); job;
+ job = block_job_next_locked(job)) {
+GSList *el;
-xdbg_graph_add_node(gr, job, X_DBG_BLOCK_GRAPH_NODE_TYPE_BLOCK_JOB,
- job->job.id);
-for (el = job->nodes; el; el = el->next) {
-xdbg_graph_add_edge(gr, job, (BdrvChild *)el->data);
+xdbg_graph_add_node(gr, job, X_DBG_BLOCK_GRAPH_NODE_TYPE_BLOCK_JOB,
+job->job.id);
+for (el = job->nodes; el; el = el->next) {
+xdbg_graph_add_edge(gr, job, (BdrvChild *)el->data);
+}
}
}
diff --git a/blockdev.c b/blockdev.c
index 2e941e2979..46090bb0aa 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -150,12 +150,15 @@ void blockdev_mark_auto_del(BlockBackend *blk)
return;
}
-for (job = block_job_next(NULL); job; job = block_job_next(job)) {
+JOB_LOCK_GUARD();
+
+for (job = block_job_next_locked(NULL); job;
+ job = block_job_next_locked(job)) {
if (block_job_has_bdrv(job, blk_bs(blk))) {
AioContext *aio_context = job->job.aio_context;
aio_context_acquire(aio_context);
-job_cancel(&job->job, false);
+job_cancel_locked(&job->job, false);
aio_context_release(aio_context);
}
@@ -3756,7 +3759,10 @@ BlockJobInfoList *qmp_query_block_jobs(Error **errp)
BlockJobInfoList *head = NULL, **tail = &head;
BlockJob *job;
-for (job = block_job_next(NULL); job; job = block_job_next(job)) {
+JOB_LOCK_GUARD();
+
+for (job = block_job_next_locked(NULL); job;
+ job = block_job_next_locked(job)) {
BlockJobInfo *value;
AioContext *aio_context;
@@ -3765,7 +3771,7 @@ BlockJobInfoList *qmp_query_block_jobs(Error **errp)
}
aio_context = block_job_get_aio_context(job);
aio_context_acquire(aio_context);
-value = block_job_query(job, errp);
+value = block_job_query_locked(job, errp);
aio_context_release(aio_context);
if (!value) {
qapi_free_BlockJobInfoList(head);
diff --git a/blockjob.c b/blockjob.c
index 0d59aba439..96fb9d9f73 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -111,8 +111,10 @@ static bool child_job_drained_poll(BdrvChild *c)
/* An inactive or completed job doesn't have any pending requests. Jobs
* with !job->busy are either already paused or have a pause point after
* being reentered, so no job driver code will run before they pause. */
-if (!job->busy || job_is_completed(job)) {
-return false;
+WITH_JOB_LOCK_GUARD() {
+if (!job->busy || job_is_completed_locked(job)) {
+return false;
+}
}
/* Otherwise, assume that it isn't fully stopped yet, but allow the job to
@@ -475,13 +477,15 @@ void *block_job_create(const char *job_id, const
BlockJobDriver *driver,
job->ready_notifier.notify = block_job_event_ready;
job->idle_notifier.notify = block_job_on_idle;
-notifier_list_add(&job->job.on_finalize_cancelled,
- &job->finalize_cancelled_notifier);
-notifier_list_add(&job->job.on_finalize_completed,
- &job->finalize_completed_notifier);
-notifier_list_add(&job->job.on_pending, &job->pending_n
[PATCH v12 10/21] block/mirror.c: use of job helpers in drivers
Once job lock is used and aiocontext is removed, mirror has
to perform job operations under the same critical section,
Note: at this stage, job_{lock/unlock} and job lock guard macros
are *nop*.
Signed-off-by: Emanuele Giuseppe Esposito
Reviewed-by: Vladimir Sementsov-Ogievskiy
---
block/mirror.c | 13 +
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/block/mirror.c b/block/mirror.c
index 3c4ab1159d..c6bf7f40ce 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1152,8 +1152,10 @@ static void mirror_complete(Job *job, Error **errp)
s->should_complete = true;
/* If the job is paused, it will be re-entered when it is resumed */
-if (!job->paused) {
-job_enter(job);
+WITH_JOB_LOCK_GUARD() {
+if (!job->paused) {
+job_enter_cond_locked(job, NULL);
+}
}
}
@@ -1173,8 +1175,11 @@ static bool mirror_drained_poll(BlockJob *job)
* from one of our own drain sections, to avoid a deadlock waiting for
* ourselves.
*/
-if (!s->common.job.paused && !job_is_cancelled(&job->job) && !s->in_drain)
{
-return true;
+WITH_JOB_LOCK_GUARD() {
+if (!s->common.job.paused && !job_is_cancelled_locked(&job->job)
+&& !s->in_drain) {
+return true;
+}
}
return !!s->in_flight;
--
2.31.1
[PATCH v12 12/21] job: detect change of aiocontext within job coroutine
From: Paolo Bonzini
We want to make sure access of job->aio_context is always done
under either BQL or job_mutex. The problem is that using
aio_co_enter(job->aiocontext, job->co) in job_start and job_enter_cond
makes the coroutine immediately resume, so we can't hold the job lock.
And caching it is not safe either, as it might change.
job_start is under BQL, so it can freely read job->aiocontext, but
job_enter_cond is not.
We want to avoid reading job->aio_context in job_enter_cond, therefore:
1) use aio_co_wake(), since it doesn't want an aiocontext as argument
but uses job->co->ctx
2) detect possible discrepancy between job->co->ctx and job->aio_context
by checking right after the coroutine resumes back from yielding if
job->aio_context has changed. If so, reschedule the coroutine to the
new context.
Calling bdrv_try_set_aio_context() will issue the following calls
(simplified):
* in terms of bdrv callbacks:
.drained_begin -> .set_aio_context -> .drained_end
* in terms of child_job functions:
child_job_drained_begin -> child_job_set_aio_context -> child_job_drained_end
* in terms of job functions:
job_pause_locked -> job_set_aio_context -> job_resume_locked
We can see that after setting the new aio_context, job_resume_locked
calls again job_enter_cond, which then invokes aio_co_wake(). But
while job->aiocontext has been set in job_set_aio_context,
job->co->ctx has not changed, so the coroutine would be entering in
the wrong aiocontext.
Using aio_co_schedule in job_resume_locked() might seem as a valid
alternative, but the problem is that the bh resuming the coroutine
is not scheduled immediately, and if in the meanwhile another
bdrv_try_set_aio_context() is run (see test_propagate_mirror() in
test-block-iothread.c), we would have the first schedule in the
wrong aiocontext, and the second set of drains won't even manage
to schedule the coroutine, as job->busy would still be true from
the previous job_resume_locked().
The solution is to stick with aio_co_wake() and detect every time
the coroutine resumes back from yielding if job->aio_context
has changed. If so, we can reschedule it to the new context.
Check for the aiocontext change in job_do_yield_locked because:
1) aio_co_reschedule_self requires to be in the running coroutine
2) since child_job_set_aio_context allows changing the aiocontext only
while the job is paused, this is the exact place where the coroutine
resumes, before running JobDriver's code.
Reviewed-by: Vladimir Sementsov-Ogievskiy
Reviewed-by: Stefan Hajnoczi
Signed-off-by: Paolo Bonzini
---
job.c | 19 +--
1 file changed, 17 insertions(+), 2 deletions(-)
diff --git a/job.c b/job.c
index e336af0c1c..85ae843f03 100644
--- a/job.c
+++ b/job.c
@@ -588,7 +588,7 @@ void job_enter_cond_locked(Job *job, bool(*fn)(Job *job))
job->busy = true;
real_job_unlock();
job_unlock();
-aio_co_enter(job->aio_context, job->co);
+aio_co_wake(job->co);
job_lock();
}
@@ -615,6 +615,8 @@ void job_enter(Job *job)
*/
static void coroutine_fn job_do_yield_locked(Job *job, uint64_t ns)
{
+AioContext *next_aio_context;
+
real_job_lock();
if (ns != -1) {
timer_mod(&job->sleep_timer, ns);
@@ -626,7 +628,20 @@ static void coroutine_fn job_do_yield_locked(Job *job,
uint64_t ns)
qemu_coroutine_yield();
job_lock();
-/* Set by job_enter_cond() before re-entering the coroutine. */
+next_aio_context = job->aio_context;
+/*
+ * Coroutine has resumed, but in the meanwhile the job AioContext
+ * might have changed via bdrv_try_set_aio_context(), so we need to move
+ * the coroutine too in the new aiocontext.
+ */
+while (qemu_get_current_aio_context() != next_aio_context) {
+job_unlock();
+aio_co_reschedule_self(next_aio_context);
+job_lock();
+next_aio_context = job->aio_context;
+}
+
+/* Set by job_enter_cond_locked() before re-entering the coroutine. */
assert(job->busy);
}
--
2.31.1
[PATCH v12 18/21] job.c: enable job lock/unlock and remove Aiocontext locks
Change the job_{lock/unlock} and macros to use job_mutex.
Now that they are not nop anymore, remove the aiocontext
to avoid deadlocks.
Therefore:
- when possible, remove completely the aiocontext lock/unlock pair
- if it is used by some other function too, reduce the locking
section as much as possible, leaving the job API outside.
- change AIO_WAIT_WHILE in AIO_WAIT_WHILE_UNLOCKED, since we
are not using the aiocontext lock anymore
The only functions that still need the aiocontext lock are:
- the JobDriver callbacks, already documented in job.h
- job_cancel_sync() in replication.c is called with aio_context_lock
taken, but now job is using AIO_WAIT_WHILE_UNLOCKED so we need to
release the lock.
Reduce the locking section to only cover the callback invocation
and document the functions that take the AioContext lock,
to avoid taking it twice.
Also remove real_job_{lock/unlock}, as they are replaced by the
public functions.
Signed-off-by: Emanuele Giuseppe Esposito
---
block/replication.c | 2 +
blockdev.c | 72 +++-
include/qemu/job.h | 17 ++---
job-qmp.c| 46 +++--
job.c| 111 +--
qemu-img.c | 2 -
tests/unit/test-bdrv-drain.c | 4 +-
tests/unit/test-block-iothread.c | 2 +-
tests/unit/test-blockjob.c | 19 +++---
9 files changed, 72 insertions(+), 203 deletions(-)
diff --git a/block/replication.c b/block/replication.c
index 5977f7a833..c67f931f37 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -727,7 +727,9 @@ static void replication_stop(ReplicationState *rs, bool
failover, Error **errp)
* disk, secondary disk in backup_job_completed().
*/
if (s->backup_job) {
+aio_context_release(aio_context);
job_cancel_sync(&s->backup_job->job, true);
+aio_context_acquire(aio_context);
}
if (!failover) {
diff --git a/blockdev.c b/blockdev.c
index 46090bb0aa..a32bafc07a 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -155,12 +155,7 @@ void blockdev_mark_auto_del(BlockBackend *blk)
for (job = block_job_next_locked(NULL); job;
job = block_job_next_locked(job)) {
if (block_job_has_bdrv(job, blk_bs(blk))) {
-AioContext *aio_context = job->job.aio_context;
-aio_context_acquire(aio_context);
-
job_cancel_locked(&job->job, false);
-
-aio_context_release(aio_context);
}
}
@@ -1847,14 +1842,7 @@ static void drive_backup_abort(BlkActionState *common)
DriveBackupState *state = DO_UPCAST(DriveBackupState, common, common);
if (state->job) {
-AioContext *aio_context;
-
-aio_context = bdrv_get_aio_context(state->bs);
-aio_context_acquire(aio_context);
-
job_cancel_sync(&state->job->job, true);
-
-aio_context_release(aio_context);
}
}
@@ -1948,14 +1936,7 @@ static void blockdev_backup_abort(BlkActionState *common)
BlockdevBackupState *state = DO_UPCAST(BlockdevBackupState, common,
common);
if (state->job) {
-AioContext *aio_context;
-
-aio_context = bdrv_get_aio_context(state->bs);
-aio_context_acquire(aio_context);
-
job_cancel_sync(&state->job->job, true);
-
-aio_context_release(aio_context);
}
}
@@ -3317,19 +3298,14 @@ out:
}
/*
- * Get a block job using its ID and acquire its AioContext.
- * Called with job_mutex held.
+ * Get a block job using its ID. Called with job_mutex held.
*/
-static BlockJob *find_block_job_locked(const char *id,
- AioContext **aio_context,
- Error **errp)
+static BlockJob *find_block_job_locked(const char *id, Error **errp)
{
BlockJob *job;
assert(id != NULL);
-*aio_context = NULL;
-
job = block_job_get_locked(id);
if (!job) {
@@ -3338,36 +3314,30 @@ static BlockJob *find_block_job_locked(const char *id,
return NULL;
}
-*aio_context = block_job_get_aio_context(job);
-aio_context_acquire(*aio_context);
-
return job;
}
void qmp_block_job_set_speed(const char *device, int64_t speed, Error **errp)
{
-AioContext *aio_context;
BlockJob *job;
JOB_LOCK_GUARD();
-job = find_block_job_locked(device, &aio_context, errp);
+job = find_block_job_locked(device, errp);
if (!job) {
return;
}
block_job_set_speed_locked(job, speed, errp);
-aio_context_release(aio_context);
}
void qmp_block_job_cancel(const char *device,
bool has_force, bool force, Error **errp)
{
-AioContext *aio_context;
BlockJob *job;
JOB_LOCK_GUARD();
-job = find_block_job_locked(device, &aio_context, errp);
+job = find_block_job_locked(device, errp);
if (!job) {
return;
[PATCH v12 15/21] blockjob: rename notifier callbacks as _locked
They all are called with job_lock held, in job_event_*_locked()
Signed-off-by: Emanuele Giuseppe Esposito
Reviewed-by: Vladimir Sementsov-Ogievskiy
Reviewed-by: Stefan Hajnoczi
Reviewed-by: Kevin Wolf
---
blockjob.c | 25 +++--
1 file changed, 15 insertions(+), 10 deletions(-)
diff --git a/blockjob.c b/blockjob.c
index c8919cef9b..d8fb5311c7 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -250,7 +250,8 @@ int block_job_add_bdrv(BlockJob *job, const char *name,
BlockDriverState *bs,
return 0;
}
-static void block_job_on_idle(Notifier *n, void *opaque)
+/* Called with job_mutex lock held. */
+static void block_job_on_idle_locked(Notifier *n, void *opaque)
{
aio_wait_kick();
}
@@ -370,7 +371,8 @@ static void block_job_iostatus_set_err(BlockJob *job, int
error)
}
}
-static void block_job_event_cancelled(Notifier *n, void *opaque)
+/* Called with job_mutex lock held. */
+static void block_job_event_cancelled_locked(Notifier *n, void *opaque)
{
BlockJob *job = opaque;
uint64_t progress_current, progress_total;
@@ -389,7 +391,8 @@ static void block_job_event_cancelled(Notifier *n, void
*opaque)
job->speed);
}
-static void block_job_event_completed(Notifier *n, void *opaque)
+/* Called with job_mutex lock held. */
+static void block_job_event_completed_locked(Notifier *n, void *opaque)
{
BlockJob *job = opaque;
const char *msg = NULL;
@@ -415,7 +418,8 @@ static void block_job_event_completed(Notifier *n, void
*opaque)
msg);
}
-static void block_job_event_pending(Notifier *n, void *opaque)
+/* Called with job_mutex lock held. */
+static void block_job_event_pending_locked(Notifier *n, void *opaque)
{
BlockJob *job = opaque;
@@ -427,7 +431,8 @@ static void block_job_event_pending(Notifier *n, void
*opaque)
job->job.id);
}
-static void block_job_event_ready(Notifier *n, void *opaque)
+/* Called with job_mutex lock held. */
+static void block_job_event_ready_locked(Notifier *n, void *opaque)
{
BlockJob *job = opaque;
uint64_t progress_current, progress_total;
@@ -472,11 +477,11 @@ void *block_job_create(const char *job_id, const
BlockJobDriver *driver,
ratelimit_init(&job->limit);
-job->finalize_cancelled_notifier.notify = block_job_event_cancelled;
-job->finalize_completed_notifier.notify = block_job_event_completed;
-job->pending_notifier.notify = block_job_event_pending;
-job->ready_notifier.notify = block_job_event_ready;
-job->idle_notifier.notify = block_job_on_idle;
+job->finalize_cancelled_notifier.notify = block_job_event_cancelled_locked;
+job->finalize_completed_notifier.notify = block_job_event_completed_locked;
+job->pending_notifier.notify = block_job_event_pending_locked;
+job->ready_notifier.notify = block_job_event_ready_locked;
+job->idle_notifier.notify = block_job_on_idle_locked;
WITH_JOB_LOCK_GUARD() {
notifier_list_add(&job->job.on_finalize_cancelled,
--
2.31.1
[PATCH v12 09/21] jobs: use job locks also in the unit tests
Add missing job synchronization in the unit tests, with
explicit locks.
We are deliberately using _locked functions wrapped by a guard
instead of a normal call because the normal call will be removed
in future, as the only usage is limited to the tests.
In other words, if a function like job_pause() is/will be only used
in tests to avoid:
WITH_JOB_LOCK_GUARD(){
job_pause_locked();
}
then it is not worth keeping job_pause(), and just use the guard.
Note: at this stage, job_{lock/unlock} and job lock guard macros
are *nop*.
Signed-off-by: Emanuele Giuseppe Esposito
Reviewed-by: Vladimir Sementsov-Ogievskiy
Reviewed-by: Kevin Wolf
---
tests/unit/test-bdrv-drain.c | 76
tests/unit/test-block-iothread.c | 8 ++-
tests/unit/test-blockjob-txn.c | 24 ---
tests/unit/test-blockjob.c | 115 +++
4 files changed, 140 insertions(+), 83 deletions(-)
diff --git a/tests/unit/test-bdrv-drain.c b/tests/unit/test-bdrv-drain.c
index 36be84ae55..0db056ea63 100644
--- a/tests/unit/test-bdrv-drain.c
+++ b/tests/unit/test-bdrv-drain.c
@@ -943,61 +943,83 @@ static void test_blockjob_common_drain_node(enum
drain_type drain_type,
}
}
-g_assert_cmpint(job->job.pause_count, ==, 0);
-g_assert_false(job->job.paused);
-g_assert_true(tjob->running);
-g_assert_true(job->job.busy); /* We're in qemu_co_sleep_ns() */
+WITH_JOB_LOCK_GUARD() {
+g_assert_cmpint(job->job.pause_count, ==, 0);
+g_assert_false(job->job.paused);
+g_assert_true(tjob->running);
+g_assert_true(job->job.busy); /* We're in qemu_co_sleep_ns() */
+}
do_drain_begin_unlocked(drain_type, drain_bs);
-if (drain_type == BDRV_DRAIN_ALL) {
-/* bdrv_drain_all() drains both src and target */
-g_assert_cmpint(job->job.pause_count, ==, 2);
-} else {
-g_assert_cmpint(job->job.pause_count, ==, 1);
+WITH_JOB_LOCK_GUARD() {
+if (drain_type == BDRV_DRAIN_ALL) {
+/* bdrv_drain_all() drains both src and target */
+g_assert_cmpint(job->job.pause_count, ==, 2);
+} else {
+g_assert_cmpint(job->job.pause_count, ==, 1);
+}
+g_assert_true(job->job.paused);
+g_assert_false(job->job.busy); /* The job is paused */
}
-g_assert_true(job->job.paused);
-g_assert_false(job->job.busy); /* The job is paused */
do_drain_end_unlocked(drain_type, drain_bs);
if (use_iothread) {
-/* paused is reset in the I/O thread, wait for it */
+/*
+ * Here we are waiting for the paused status to change,
+ * so don't bother protecting the read every time.
+ *
+ * paused is reset in the I/O thread, wait for it
+ */
while (job->job.paused) {
aio_poll(qemu_get_aio_context(), false);
}
}
-g_assert_cmpint(job->job.pause_count, ==, 0);
-g_assert_false(job->job.paused);
-g_assert_true(job->job.busy); /* We're in qemu_co_sleep_ns() */
+WITH_JOB_LOCK_GUARD() {
+g_assert_cmpint(job->job.pause_count, ==, 0);
+g_assert_false(job->job.paused);
+g_assert_true(job->job.busy); /* We're in qemu_co_sleep_ns() */
+}
do_drain_begin_unlocked(drain_type, target);
-if (drain_type == BDRV_DRAIN_ALL) {
-/* bdrv_drain_all() drains both src and target */
-g_assert_cmpint(job->job.pause_count, ==, 2);
-} else {
-g_assert_cmpint(job->job.pause_count, ==, 1);
+WITH_JOB_LOCK_GUARD() {
+if (drain_type == BDRV_DRAIN_ALL) {
+/* bdrv_drain_all() drains both src and target */
+g_assert_cmpint(job->job.pause_count, ==, 2);
+} else {
+g_assert_cmpint(job->job.pause_count, ==, 1);
+}
+g_assert_true(job->job.paused);
+g_assert_false(job->job.busy); /* The job is paused */
}
-g_assert_true(job->job.paused);
-g_assert_false(job->job.busy); /* The job is paused */
do_drain_end_unlocked(drain_type, target);
if (use_iothread) {
-/* paused is reset in the I/O thread, wait for it */
+/*
+ * Here we are waiting for the paused status to change,
+ * so don't bother protecting the read every time.
+ *
+ * paused is reset in the I/O thread, wait for it
+ */
while (job->job.paused) {
aio_poll(qemu_get_aio_context(), false);
}
}
-g_assert_cmpint(job->job.pause_count, ==, 0);
-g_assert_false(job->job.paused);
-g_assert_true(job->job.busy); /* We're in qemu_co_sleep_ns() */
+WITH_JOB_LOCK_GUARD() {
+g_assert_cmpint(job->job.pause_count, ==, 0);
+g_assert_false(job->job.paused);
+g_assert_true(job->job.busy); /* We're in qemu_co_sleep_ns() */
+}
aio_context_acquire(ctx);
-ret = job_complete_sync(&job->job, &error_abort);
+WITH_JOB_LOCK_GUARD() {
+
[PATCH v12 06/21] job: move and update comments from blockjob.c
This comment applies more on job, it was left in blockjob as in the past
the whole job logic was implemented there.
Note: at this stage, job_{lock/unlock} and job lock guard macros
are *nop*.
No functional change intended.
Signed-off-by: Emanuele Giuseppe Esposito
Reviewed-by: Vladimir Sementsov-Ogievskiy
Reviewed-by: Stefan Hajnoczi
---
blockjob.c | 20
job.c | 16
2 files changed, 16 insertions(+), 20 deletions(-)
diff --git a/blockjob.c b/blockjob.c
index 4868453d74..7da59a1f1c 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -36,21 +36,6 @@
#include "qemu/main-loop.h"
#include "qemu/timer.h"
-/*
- * The block job API is composed of two categories of functions.
- *
- * The first includes functions used by the monitor. The monitor is
- * peculiar in that it accesses the block job list with block_job_get, and
- * therefore needs consistency across block_job_get and the actual operation
- * (e.g. block_job_set_speed). The consistency is achieved with
- * aio_context_acquire/release. These functions are declared in blockjob.h.
- *
- * The second includes functions used by the block job drivers and sometimes
- * by the core block layer. These do not care about locking, because the
- * whole coroutine runs under the AioContext lock, and are declared in
- * blockjob_int.h.
- */
-
static bool is_block_job(Job *job)
{
return job_type(job) == JOB_TYPE_BACKUP ||
@@ -433,11 +418,6 @@ static void block_job_event_ready(Notifier *n, void
*opaque)
}
-/*
- * API for block job drivers and the block layer. These functions are
- * declared in blockjob_int.h.
- */
-
void *block_job_create(const char *job_id, const BlockJobDriver *driver,
JobTxn *txn, BlockDriverState *bs, uint64_t perm,
uint64_t shared_perm, int64_t speed, int flags,
diff --git a/job.c b/job.c
index 8967ec671e..e336af0c1c 100644
--- a/job.c
+++ b/job.c
@@ -32,6 +32,22 @@
#include "trace/trace-root.h"
#include "qapi/qapi-events-job.h"
+/*
+ * The job API is composed of two categories of functions.
+ *
+ * The first includes functions used by the monitor. The monitor is
+ * peculiar in that it accesses the job list with job_get, and
+ * therefore needs consistency across job_get and the actual operation
+ * (e.g. job_user_cancel). To achieve this consistency, the caller
+ * calls job_lock/job_unlock itself around the whole operation.
+ *
+ *
+ * The second includes functions used by the job drivers and sometimes
+ * by the core block layer. These delegate the locking to the callee instead.
+ *
+ * TODO Actually make this true
+ */
+
/*
* job_mutex protects the jobs list, but also makes the
* struct job fields thread-safe.
--
2.31.1
[PATCH v12 04/21] aio-wait.h: introduce AIO_WAIT_WHILE_UNLOCKED
Same as AIO_WAIT_WHILE macro, but if we are in the Main loop
do not release and then acquire ctx_ 's aiocontext.
Once all Aiocontext locks go away, this macro will replace
AIO_WAIT_WHILE.
Signed-off-by: Emanuele Giuseppe Esposito
Reviewed-by: Stefan Hajnoczi
Reviewed-by: Vladimir Sementsov-Ogievskiy
---
include/block/aio-wait.h | 17 +
1 file changed, 13 insertions(+), 4 deletions(-)
diff --git a/include/block/aio-wait.h b/include/block/aio-wait.h
index 54840f8622..dd9a7f6461 100644
--- a/include/block/aio-wait.h
+++ b/include/block/aio-wait.h
@@ -59,10 +59,13 @@ typedef struct {
extern AioWait global_aio_wait;
/**
- * AIO_WAIT_WHILE:
+ * AIO_WAIT_WHILE_INTERNAL:
* @ctx: the aio context, or NULL if multiple aio contexts (for which the
* caller does not hold a lock) are involved in the polling condition.
* @cond: wait while this conditional expression is true
+ * @unlock: whether to unlock and then lock again @ctx. This apples
+ * only when waiting for another AioContext from the main loop.
+ * Otherwise it's ignored.
*
* Wait while a condition is true. Use this to implement synchronous
* operations that require event loop activity.
@@ -75,7 +78,7 @@ extern AioWait global_aio_wait;
* wait on conditions between two IOThreads since that could lead to deadlock,
* go via the main loop instead.
*/
-#define AIO_WAIT_WHILE(ctx, cond) ({ \
+#define AIO_WAIT_WHILE_INTERNAL(ctx, cond, unlock) ({ \
bool waited_ = false; \
AioWait *wait_ = &global_aio_wait; \
AioContext *ctx_ = (ctx); \
@@ -92,11 +95,11 @@ extern AioWait global_aio_wait;
assert(qemu_get_current_aio_context() == \
qemu_get_aio_context());\
while ((cond)) { \
-if (ctx_) {\
+if (unlock && ctx_) { \
aio_context_release(ctx_); \
} \
aio_poll(qemu_get_aio_context(), true);\
-if (ctx_) {\
+if (unlock && ctx_) { \
aio_context_acquire(ctx_); \
} \
waited_ = true;\
@@ -105,6 +108,12 @@ extern AioWait global_aio_wait;
qatomic_dec(&wait_->num_waiters); \
waited_; })
+#define AIO_WAIT_WHILE(ctx, cond) \
+AIO_WAIT_WHILE_INTERNAL(ctx, cond, true)
+
+#define AIO_WAIT_WHILE_UNLOCKED(ctx, cond) \
+AIO_WAIT_WHILE_INTERNAL(ctx, cond, false)
+
/**
* aio_wait_kick:
* Wake up the main thread if it is waiting on AIO_WAIT_WHILE(). During
--
2.31.1
[PATCH v12 16/21] blockjob: protect iostatus field in BlockJob struct
iostatus is the only field (together with .job) that needs
protection using the job mutex.
It is set in the main loop (GLOBAL_STATE functions) but read
in I/O code (block_job_error_action).
In order to protect it, change block_job_iostatus_set_err
to block_job_iostatus_set_err_locked(), always called under
job lock.
Signed-off-by: Emanuele Giuseppe Esposito
Reviewed-by: Kevin Wolf
Reviewed-by: Vladimir Sementsov-Ogievskiy
---
block/mirror.c | 7 +--
blockjob.c | 5 +++--
2 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/block/mirror.c b/block/mirror.c
index c6bf7f40ce..7e32ee1d31 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -893,7 +893,7 @@ static int coroutine_fn mirror_run(Job *job, Error **errp)
MirrorBlockJob *s = container_of(job, MirrorBlockJob, common.job);
BlockDriverState *bs = s->mirror_top_bs->backing->bs;
BlockDriverState *target_bs = blk_bs(s->target);
-bool need_drain = true;
+bool need_drain = true, iostatus;
int64_t length;
int64_t target_length;
BlockDriverInfo bdi;
@@ -1016,8 +1016,11 @@ static int coroutine_fn mirror_run(Job *job, Error
**errp)
* We do so every BLKOCK_JOB_SLICE_TIME nanoseconds, or when there is
* an error, or when the source is clean, whichever comes first. */
delta = qemu_clock_get_ns(QEMU_CLOCK_REALTIME) - s->last_pause_ns;
+WITH_JOB_LOCK_GUARD() {
+iostatus = s->common.iostatus;
+}
if (delta < BLOCK_JOB_SLICE_TIME &&
-s->common.iostatus == BLOCK_DEVICE_IO_STATUS_OK) {
+iostatus == BLOCK_DEVICE_IO_STATUS_OK) {
if (s->in_flight >= MAX_IN_FLIGHT || s->buf_free_count == 0 ||
(cnt == 0 && s->in_flight > 0)) {
trace_mirror_yield(s, cnt, s->buf_free_count, s->in_flight);
diff --git a/blockjob.c b/blockjob.c
index d8fb5311c7..d04f804001 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -363,7 +363,8 @@ BlockJobInfo *block_job_query(BlockJob *job, Error **errp)
return block_job_query_locked(job, errp);
}
-static void block_job_iostatus_set_err(BlockJob *job, int error)
+/* Called with job lock held */
+static void block_job_iostatus_set_err_locked(BlockJob *job, int error)
{
if (job->iostatus == BLOCK_DEVICE_IO_STATUS_OK) {
job->iostatus = error == ENOSPC ? BLOCK_DEVICE_IO_STATUS_NOSPACE :
@@ -577,8 +578,8 @@ BlockErrorAction block_job_error_action(BlockJob *job,
BlockdevOnError on_err,
*/
job->job.user_paused = true;
}
+block_job_iostatus_set_err_locked(job, error);
}
-block_job_iostatus_set_err(job, error);
}
return action;
}
--
2.31.1
[PATCH v12 00/21] job: replace AioContext lock with job_mutex
In this series, we want to remove the AioContext lock and instead
use the already existent job_mutex to protect the job structures
and list. This is part of the work to get rid of AioContext lock
usage in favour of smaller granularity locks.
In order to simplify reviewer's job, job lock/unlock functions and
macros are added as empty prototypes (nop) in patch 1.
They are converted to use the actual job mutex only in the last
patch. In this way we can freely create locking sections
without worrying about deadlocks with the aiocontext lock.
Patch 2 defines what fields in the job structure need protection.
Patches 3-6 are in preparation to the job locks, moving functions
from global to static and introducing helpers.
Patch 7-9 introduce the (nop) job lock into the job API and
its users, and patches 10-13 categorize respectively locked and
unlocked functions in the job API.
Patches 14-17 take care of protecting job->aio_context, and
finally patch 18 makes the prototypes in patch 1 use the
job_mutex and removes all aiocontext lock at the same time.
Tested this series by running unit tests, qemu-iotests and qtests
(x86_64).
---
v12:
* Apply Vladimir feedbacks (minor comment adjustments)
* Add aiocontext release + acquire in block/replication.c
v11:
* Apply Kevin and Vladimir feedbacks
* job_set_aio_context: check coroutine is quiescent if job_is_completed
* Rephrased commit message in patch 13
v10:
* protect job->status in unit tests
* patch 11: change commit description and avoid using lock guard for a single
function call
* move patch 19 before patch 15
v9:
* merge patch 6 and 7 to 5.
* additional "taken with job lock/unlock" added and propagated in callers
* protect iostatus field of BlockJobs
* move all blockjob patches torward the end of the serie
v8:
* reorganize patch ordering according with Vladimir proposal
* minor nitpicks
v7:
* s/temporary/temporarly
* double identical locking comment to the same function
* patch 2: add "Protected by AioContext lock" to better categorize fields in
job.h
* use same comment style in all function headers ("Just like {funct}, but
called between job_lock and job_unlock")
v6:
* patch 4 and 6 squashed with patch 19 (enable job lock and
reduce/remove AioContext lock)
* patch 19: job_unref_locked read the aiocontext inside the
job lock.
v5:
* just restructured patches a little bit better, as there were
functions used before they were defined.
* rebased on kwolf/block branch and API split serie
v4:
* move "protected by job_mutex" from patch 2 to 15, where the job_mutex is
actually added.
* s/aio_co_enter/aio_co_schedule in job.c, and adjust tests accordingly.
* remove job_get_aio_context, add job_set_aio_context. Use "fake rwlock"
to protect job->aiocontext.
* get rid of useless getters method, namely:
job_get_status
job_get_pause_count
job_get_paused
job_get_busy
They are all used only by tests, and such getter is pretty useless.
Replace with job_lock(); assert(); job_unlock();
* use job lock macros instead of job lock/unlock in unit tests.
* convert also blockjob functions to have _locked
* put the job_lock/unlock patches before the _locked ones
* replace aio_co_enter in job.c and detect change of context
v3:
* add "_locked" suffix to the functions called under job_mutex lock
* rename _job_lock in real_job_lock
* job_mutex is now public, and drivers like monitor use it directly
* introduce and protect job_get_aio_context
* remove mirror-specific APIs and just use WITH_JOB_GUARD
* more extensive use of WITH_JOB_GUARD and JOB_LOCK_GUARD
RFC v2:
* use JOB_LOCK_GUARD and WITH_JOB_LOCK_GUARD
* mu(u)ltiple typos in commit messages
* job API split patches are sent separately in another series
* use of empty job_{lock/unlock} and JOB_LOCK_GUARD/WITH_JOB_LOCK_GUARD
to avoid deadlocks and simplify the reviewer job
* move patch 11 (block_job_query: remove atomic read) as last
Emanuele Giuseppe Esposito (20):
job.c: make job_mutex and job_lock/unlock() public
job.h: categorize fields in struct Job
job.c: API functions not used outside should be static
aio-wait.h: introduce AIO_WAIT_WHILE_UNLOCKED
job.c: add job_lock/unlock while keeping job.h intact
job: move and update comments from blockjob.c
blockjob: introduce block_job _locked() APIs
jobs: add job lock in find_* functions
jobs: use job locks also in the unit tests
block/mirror.c: use of job helpers in drivers
jobs: group together API calls under the same job lock
jobs: protect job.aio_context with BQL and job_mutex
blockjob.h: categorize fields in struct BlockJob
blockjob: rename notifier callbacks as _locked
blockjob: protect iostatus field in BlockJob struct
job.h: categorize JobDriver callbacks that need the AioContext lock
job.c: enable job lock/unlock and remove Aiocontext locks
block_job_query: remove atomic read
blockjob: remove unused functions
job: remove unused functions
Paolo Bonzini (1):
job: detect change of aiocontext within job cor
[PATCH v12 20/21] blockjob: remove unused functions
These public functions are not used anywhere, thus can be dropped.
Signed-off-by: Emanuele Giuseppe Esposito
Reviewed-by: Stefan Hajnoczi
Reviewed-by: Kevin Wolf
Reviewed-by: Vladimir Sementsov-Ogievskiy
---
blockjob.c | 16 ++--
include/block/blockjob.h | 31 ---
2 files changed, 14 insertions(+), 33 deletions(-)
diff --git a/blockjob.c b/blockjob.c
index 120c1b7ead..bdf20a0e35 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -56,12 +56,6 @@ BlockJob *block_job_next_locked(BlockJob *bjob)
return job ? container_of(job, BlockJob, job) : NULL;
}
-BlockJob *block_job_next(BlockJob *bjob)
-{
-JOB_LOCK_GUARD();
-return block_job_next_locked(bjob);
-}
-
BlockJob *block_job_get_locked(const char *id)
{
Job *job = job_get_locked(id);
@@ -308,7 +302,7 @@ bool block_job_set_speed_locked(BlockJob *job, int64_t
speed, Error **errp)
return true;
}
-bool block_job_set_speed(BlockJob *job, int64_t speed, Error **errp)
+static bool block_job_set_speed(BlockJob *job, int64_t speed, Error **errp)
{
JOB_LOCK_GUARD();
return block_job_set_speed_locked(job, speed, errp);
@@ -357,12 +351,6 @@ BlockJobInfo *block_job_query_locked(BlockJob *job, Error
**errp)
return info;
}
-BlockJobInfo *block_job_query(BlockJob *job, Error **errp)
-{
-JOB_LOCK_GUARD();
-return block_job_query_locked(job, errp);
-}
-
/* Called with job lock held */
static void block_job_iostatus_set_err_locked(BlockJob *job, int error)
{
@@ -525,7 +513,7 @@ void block_job_iostatus_reset_locked(BlockJob *job)
job->iostatus = BLOCK_DEVICE_IO_STATUS_OK;
}
-void block_job_iostatus_reset(BlockJob *job)
+static void block_job_iostatus_reset(BlockJob *job)
{
JOB_LOCK_GUARD();
block_job_iostatus_reset_locked(job);
diff --git a/include/block/blockjob.h b/include/block/blockjob.h
index 10c24e240a..03032b2eca 100644
--- a/include/block/blockjob.h
+++ b/include/block/blockjob.h
@@ -102,17 +102,15 @@ typedef struct BlockJob {
*/
/**
- * block_job_next:
+ * block_job_next_locked:
* @job: A block job, or %NULL.
*
* Get the next element from the list of block jobs after @job, or the
* first one if @job is %NULL.
*
* Returns the requested job, or %NULL if there are no more jobs left.
+ * Called with job lock held.
*/
-BlockJob *block_job_next(BlockJob *job);
-
-/* Same as block_job_next(), but called with job lock held. */
BlockJob *block_job_next_locked(BlockJob *job);
/**
@@ -122,6 +120,7 @@ BlockJob *block_job_next_locked(BlockJob *job);
* Get the block job identified by @id (which must not be %NULL).
*
* Returns the requested job, or %NULL if it doesn't exist.
+ * Called with job lock *not* held.
*/
BlockJob *block_job_get(const char *id);
@@ -161,43 +160,37 @@ void block_job_remove_all_bdrv(BlockJob *job);
bool block_job_has_bdrv(BlockJob *job, BlockDriverState *bs);
/**
- * block_job_set_speed:
+ * block_job_set_speed_locked:
* @job: The job to set the speed for.
* @speed: The new value
* @errp: Error object.
*
* Set a rate-limiting parameter for the job; the actual meaning may
* vary depending on the job type.
- */
-bool block_job_set_speed(BlockJob *job, int64_t speed, Error **errp);
-
-/*
- * Same as block_job_set_speed(), but called with job lock held.
- * Might release the lock temporarily.
+ *
+ * Called with job lock held, but might release it temporarily.
*/
bool block_job_set_speed_locked(BlockJob *job, int64_t speed, Error **errp);
/**
- * block_job_query:
+ * block_job_query_locked:
* @job: The job to get information about.
*
* Return information about a job.
+ *
+ * Called with job lock held.
*/
-BlockJobInfo *block_job_query(BlockJob *job, Error **errp);
-
-/* Same as block_job_query(), but called with job lock held. */
BlockJobInfo *block_job_query_locked(BlockJob *job, Error **errp);
/**
- * block_job_iostatus_reset:
+ * block_job_iostatus_reset_locked:
* @job: The job whose I/O status should be reset.
*
* Reset I/O status on @job and on BlockDriverState objects it uses,
* other than job->blk.
+ *
+ * Called with job lock held.
*/
-void block_job_iostatus_reset(BlockJob *job);
-
-/* Same as block_job_iostatus_reset(), but called with job lock held. */
void block_job_iostatus_reset_locked(BlockJob *job);
/*
--
2.31.1
[PATCH v12 03/21] job.c: API functions not used outside should be static
job_event_* functions can all be static, as they are not used
outside job.c.
Same applies for job_txn_add_job().
Signed-off-by: Emanuele Giuseppe Esposito
Reviewed-by: Stefan Hajnoczi
Reviewed-by: Vladimir Sementsov-Ogievskiy
Reviewed-by: Kevin Wolf
---
include/qemu/job.h | 18 --
job.c | 22 +++---
2 files changed, 19 insertions(+), 21 deletions(-)
diff --git a/include/qemu/job.h b/include/qemu/job.h
index 876e13d549..4b64eb15f7 100644
--- a/include/qemu/job.h
+++ b/include/qemu/job.h
@@ -358,18 +358,6 @@ JobTxn *job_txn_new(void);
*/
void job_txn_unref(JobTxn *txn);
-/**
- * @txn: The transaction (may be NULL)
- * @job: Job to add to the transaction
- *
- * Add @job to the transaction. The @job must not already be in a transaction.
- * The caller must call either job_txn_unref() or job_completed() to release
- * the reference that is automatically grabbed here.
- *
- * If @txn is NULL, the function does nothing.
- */
-void job_txn_add_job(JobTxn *txn, Job *job);
-
/**
* Create a new long-running job and return it.
*
@@ -431,12 +419,6 @@ void job_progress_set_remaining(Job *job, uint64_t
remaining);
*/
void job_progress_increase_remaining(Job *job, uint64_t delta);
-/** To be called when a cancelled job is finalised. */
-void job_event_cancelled(Job *job);
-
-/** To be called when a successfully completed job is finalised. */
-void job_event_completed(Job *job);
-
/**
* Conditionally enter the job coroutine if the job is ready to run, not
* already busy and fn() returns true. fn() is called while under the job_lock
diff --git a/job.c b/job.c
index 2b4ffca9d4..cafd597ba4 100644
--- a/job.c
+++ b/job.c
@@ -125,7 +125,17 @@ void job_txn_unref(JobTxn *txn)
}
}
-void job_txn_add_job(JobTxn *txn, Job *job)
+/**
+ * @txn: The transaction (may be NULL)
+ * @job: Job to add to the transaction
+ *
+ * Add @job to the transaction. The @job must not already be in a transaction.
+ * The caller must call either job_txn_unref() or job_completed() to release
+ * the reference that is automatically grabbed here.
+ *
+ * If @txn is NULL, the function does nothing.
+ */
+static void job_txn_add_job(JobTxn *txn, Job *job)
{
if (!txn) {
return;
@@ -427,12 +437,18 @@ void job_progress_increase_remaining(Job *job, uint64_t
delta)
progress_increase_remaining(&job->progress, delta);
}
-void job_event_cancelled(Job *job)
+/**
+ * To be called when a cancelled job is finalised.
+ */
+static void job_event_cancelled(Job *job)
{
notifier_list_notify(&job->on_finalize_cancelled, job);
}
-void job_event_completed(Job *job)
+/**
+ * To be called when a successfully completed job is finalised.
+ */
+static void job_event_completed(Job *job)
{
notifier_list_notify(&job->on_finalize_completed, job);
}
--
2.31.1
[PATCH v12 05/21] job.c: add job_lock/unlock while keeping job.h intact
With "intact" we mean that all job.h functions implicitly
take the lock. Therefore API callers are unmodified.
This means that:
- many static functions that will be always called with job lock held
become _locked, and call _locked functions
- all public functions take the lock internally if needed, and call _locked
functions
- all public functions called internally by other functions in job.c will have a
_locked counterpart (sometimes public), to avoid deadlocks (job lock already
taken).
These functions are not used for now.
- some public functions called only from exernal files (not job.c) do not
have _locked() counterpart and take the lock inside. Others won't need
the lock at all because use fields only set at initialization and
never modified.
job_{lock/unlock} is independent from real_job_{lock/unlock}.
Note: at this stage, job_{lock/unlock} and job lock guard macros
are *nop*
Signed-off-by: Emanuele Giuseppe Esposito
Reviewed-by: Kevin Wolf
Reviewed-by: Stefan Hajnoczi
Reviewed-by: Vladimir Sementsov-Ogievskiy
---
include/qemu/job.h | 138 +-
job.c | 610 -
2 files changed, 561 insertions(+), 187 deletions(-)
diff --git a/include/qemu/job.h b/include/qemu/job.h
index 4b64eb15f7..5709e8d4a8 100644
--- a/include/qemu/job.h
+++ b/include/qemu/job.h
@@ -358,8 +358,15 @@ JobTxn *job_txn_new(void);
*/
void job_txn_unref(JobTxn *txn);
+/*
+ * Same as job_txn_unref(), but called with job lock held.
+ * Might release the lock temporarily.
+ */
+void job_txn_unref_locked(JobTxn *txn);
+
/**
* Create a new long-running job and return it.
+ * Called with job_mutex *not* held.
*
* @job_id: The id of the newly-created job, or %NULL for internal jobs
* @driver: The class object for the newly-created job.
@@ -380,17 +387,25 @@ void *job_create(const char *job_id, const JobDriver
*driver, JobTxn *txn,
*/
void job_ref(Job *job);
+/* Same as job_ref(), but called with job lock held. */
+void job_ref_locked(Job *job);
+
/**
* Release a reference that was previously acquired with job_ref() or
* job_create(). If it's the last reference to the object, it will be freed.
*/
void job_unref(Job *job);
+/* Same as job_unref(), but called with job lock held. */
+void job_unref_locked(Job *job);
+
/**
* @job: The job that has made progress
* @done: How much progress the job made since the last call
*
* Updates the progress counter of the job.
+ *
+ * May be called with mutex held or not held.
*/
void job_progress_update(Job *job, uint64_t done);
@@ -401,6 +416,8 @@ void job_progress_update(Job *job, uint64_t done);
*
* Sets the expected end value of the progress counter of a job so that a
* completion percentage can be calculated when the progress is updated.
+ *
+ * May be called with mutex held or not held.
*/
void job_progress_set_remaining(Job *job, uint64_t remaining);
@@ -416,6 +433,8 @@ void job_progress_set_remaining(Job *job, uint64_t
remaining);
* length before, and job_progress_update() afterwards.
* (So the operation acts as a parenthesis in regards to the main job
* operation running in background.)
+ *
+ * May be called with mutex held or not held.
*/
void job_progress_increase_remaining(Job *job, uint64_t delta);
@@ -426,11 +445,19 @@ void job_progress_increase_remaining(Job *job, uint64_t
delta);
*/
void job_enter_cond(Job *job, bool(*fn)(Job *job));
+/*
+ * Same as job_enter_cond(), but called with job lock held.
+ * Might release the lock temporarily.
+ */
+void job_enter_cond_locked(Job *job, bool(*fn)(Job *job));
+
/**
* @job: A job that has not yet been started.
*
* Begins execution of a job.
* Takes ownership of one reference to the job object.
+ *
+ * Called with job_mutex *not* held.
*/
void job_start(Job *job);
@@ -438,6 +465,7 @@ void job_start(Job *job);
* @job: The job to enter.
*
* Continue the specified job by entering the coroutine.
+ * Called with job_mutex *not* held.
*/
void job_enter(Job *job);
@@ -446,6 +474,8 @@ void job_enter(Job *job);
*
* Pause now if job_pause() has been called. Jobs that perform lots of I/O
* must call this between requests so that the job can be paused.
+ *
+ * Called with job_mutex *not* held.
*/
void coroutine_fn job_pause_point(Job *job);
@@ -453,6 +483,7 @@ void coroutine_fn job_pause_point(Job *job);
* @job: The job that calls the function.
*
* Yield the job coroutine.
+ * Called with job_mutex *not* held.
*/
void job_yield(Job *job);
@@ -463,6 +494,8 @@ void job_yield(Job *job);
* Put the job to sleep (assuming that it wasn't canceled) for @ns
* %QEMU_CLOCK_REALTIME nanoseconds. Canceling the job will immediately
* interrupt the wait.
+ *
+ * Called with job_mutex *not* held.
*/
void coroutine_fn job_sleep_ns(Job *job, int64_t ns);
@@ -475,21 +508,40 @@ const char *job_type_str(const Job *job);
/** Returns true if the job should not be visible to
[PATCH v12 08/21] jobs: add job lock in find_* functions
Both blockdev.c and job-qmp.c have TOC/TOU conditions, because
they first search for the job and then perform an action on it.
Therefore, we need to do the search + action under the same
job mutex critical section.
Note: at this stage, job_{lock/unlock} and job lock guard macros
are *nop*.
Signed-off-by: Emanuele Giuseppe Esposito
Reviewed-by: Vladimir Sementsov-Ogievskiy
Reviewed-by: Stefan Hajnoczi
Reviewed-by: Kevin Wolf
---
blockdev.c | 67 +-
job-qmp.c | 57 --
2 files changed, 86 insertions(+), 38 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index 392d9476e6..2e941e2979 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3313,9 +3313,13 @@ out:
aio_context_release(aio_context);
}
-/* Get a block job using its ID and acquire its AioContext */
-static BlockJob *find_block_job(const char *id, AioContext **aio_context,
-Error **errp)
+/*
+ * Get a block job using its ID and acquire its AioContext.
+ * Called with job_mutex held.
+ */
+static BlockJob *find_block_job_locked(const char *id,
+ AioContext **aio_context,
+ Error **errp)
{
BlockJob *job;
@@ -3323,7 +3327,7 @@ static BlockJob *find_block_job(const char *id,
AioContext **aio_context,
*aio_context = NULL;
-job = block_job_get(id);
+job = block_job_get_locked(id);
if (!job) {
error_set(errp, ERROR_CLASS_DEVICE_NOT_ACTIVE,
@@ -3340,13 +3344,16 @@ static BlockJob *find_block_job(const char *id,
AioContext **aio_context,
void qmp_block_job_set_speed(const char *device, int64_t speed, Error **errp)
{
AioContext *aio_context;
-BlockJob *job = find_block_job(device, &aio_context, errp);
+BlockJob *job;
+
+JOB_LOCK_GUARD();
+job = find_block_job_locked(device, &aio_context, errp);
if (!job) {
return;
}
-block_job_set_speed(job, speed, errp);
+block_job_set_speed_locked(job, speed, errp);
aio_context_release(aio_context);
}
@@ -3354,7 +3361,10 @@ void qmp_block_job_cancel(const char *device,
bool has_force, bool force, Error **errp)
{
AioContext *aio_context;
-BlockJob *job = find_block_job(device, &aio_context, errp);
+BlockJob *job;
+
+JOB_LOCK_GUARD();
+job = find_block_job_locked(device, &aio_context, errp);
if (!job) {
return;
@@ -3364,14 +3374,14 @@ void qmp_block_job_cancel(const char *device,
force = false;
}
-if (job_user_paused(&job->job) && !force) {
+if (job_user_paused_locked(&job->job) && !force) {
error_setg(errp, "The block job for device '%s' is currently paused",
device);
goto out;
}
trace_qmp_block_job_cancel(job);
-job_user_cancel(&job->job, force, errp);
+job_user_cancel_locked(&job->job, force, errp);
out:
aio_context_release(aio_context);
}
@@ -3379,57 +3389,69 @@ out:
void qmp_block_job_pause(const char *device, Error **errp)
{
AioContext *aio_context;
-BlockJob *job = find_block_job(device, &aio_context, errp);
+BlockJob *job;
+
+JOB_LOCK_GUARD();
+job = find_block_job_locked(device, &aio_context, errp);
if (!job) {
return;
}
trace_qmp_block_job_pause(job);
-job_user_pause(&job->job, errp);
+job_user_pause_locked(&job->job, errp);
aio_context_release(aio_context);
}
void qmp_block_job_resume(const char *device, Error **errp)
{
AioContext *aio_context;
-BlockJob *job = find_block_job(device, &aio_context, errp);
+BlockJob *job;
+
+JOB_LOCK_GUARD();
+job = find_block_job_locked(device, &aio_context, errp);
if (!job) {
return;
}
trace_qmp_block_job_resume(job);
-job_user_resume(&job->job, errp);
+job_user_resume_locked(&job->job, errp);
aio_context_release(aio_context);
}
void qmp_block_job_complete(const char *device, Error **errp)
{
AioContext *aio_context;
-BlockJob *job = find_block_job(device, &aio_context, errp);
+BlockJob *job;
+
+JOB_LOCK_GUARD();
+job = find_block_job_locked(device, &aio_context, errp);
if (!job) {
return;
}
trace_qmp_block_job_complete(job);
-job_complete(&job->job, errp);
+job_complete_locked(&job->job, errp);
aio_context_release(aio_context);
}
void qmp_block_job_finalize(const char *id, Error **errp)
{
AioContext *aio_context;
-BlockJob *job = find_block_job(id, &aio_context, errp);
+BlockJob *job;
+
+JOB_LOCK_GUARD();
+job = find_block_job_locked(id, &aio_context, errp);
if (!job) {
return;
}
trace_qmp_block_job_finalize(job);
-job_ref(&job->job);
-job_finalize(&job->job, errp);
+job_ref_locked(&job->job);
+job_finalize_locked(&job->job, errp);
/*
* Job's con
[PATCH v12 07/21] blockjob: introduce block_job _locked() APIs
Just as done with job.h, create _locked() functions in blockjob.h
These functions will be later useful when caller has already taken
the lock. All blockjob _locked functions call job _locked functions.
Note: at this stage, job_{lock/unlock} and job lock guard macros
are *nop*.
Signed-off-by: Emanuele Giuseppe Esposito
Reviewed-by: Vladimir Sementsov-Ogievskiy
Reviewed-by: Kevin Wolf
Reviewed-by: Stefan Hajnoczi
---
blockjob.c | 52
include/block/blockjob.h | 18 ++
2 files changed, 60 insertions(+), 10 deletions(-)
diff --git a/blockjob.c b/blockjob.c
index 7da59a1f1c..0d59aba439 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -44,21 +44,27 @@ static bool is_block_job(Job *job)
job_type(job) == JOB_TYPE_STREAM;
}
-BlockJob *block_job_next(BlockJob *bjob)
+BlockJob *block_job_next_locked(BlockJob *bjob)
{
Job *job = bjob ? &bjob->job : NULL;
GLOBAL_STATE_CODE();
do {
-job = job_next(job);
+job = job_next_locked(job);
} while (job && !is_block_job(job));
return job ? container_of(job, BlockJob, job) : NULL;
}
-BlockJob *block_job_get(const char *id)
+BlockJob *block_job_next(BlockJob *bjob)
{
-Job *job = job_get(id);
+JOB_LOCK_GUARD();
+return block_job_next_locked(bjob);
+}
+
+BlockJob *block_job_get_locked(const char *id)
+{
+Job *job = job_get_locked(id);
GLOBAL_STATE_CODE();
if (job && is_block_job(job)) {
@@ -68,6 +74,12 @@ BlockJob *block_job_get(const char *id)
}
}
+BlockJob *block_job_get(const char *id)
+{
+JOB_LOCK_GUARD();
+return block_job_get_locked(id);
+}
+
void block_job_free(Job *job)
{
BlockJob *bjob = container_of(job, BlockJob, job);
@@ -256,14 +268,14 @@ static bool job_timer_pending(Job *job)
return timer_pending(&job->sleep_timer);
}
-bool block_job_set_speed(BlockJob *job, int64_t speed, Error **errp)
+bool block_job_set_speed_locked(BlockJob *job, int64_t speed, Error **errp)
{
const BlockJobDriver *drv = block_job_driver(job);
int64_t old_speed = job->speed;
GLOBAL_STATE_CODE();
-if (job_apply_verb(&job->job, JOB_VERB_SET_SPEED, errp) < 0) {
+if (job_apply_verb_locked(&job->job, JOB_VERB_SET_SPEED, errp) < 0) {
return false;
}
if (speed < 0) {
@@ -277,7 +289,9 @@ bool block_job_set_speed(BlockJob *job, int64_t speed,
Error **errp)
job->speed = speed;
if (drv->set_speed) {
+job_unlock();
drv->set_speed(job, speed);
+job_lock();
}
if (speed && speed <= old_speed) {
@@ -285,18 +299,24 @@ bool block_job_set_speed(BlockJob *job, int64_t speed,
Error **errp)
}
/* kick only if a timer is pending */
-job_enter_cond(&job->job, job_timer_pending);
+job_enter_cond_locked(&job->job, job_timer_pending);
return true;
}
+bool block_job_set_speed(BlockJob *job, int64_t speed, Error **errp)
+{
+JOB_LOCK_GUARD();
+return block_job_set_speed_locked(job, speed, errp);
+}
+
int64_t block_job_ratelimit_get_delay(BlockJob *job, uint64_t n)
{
IO_CODE();
return ratelimit_calculate_delay(&job->limit, n);
}
-BlockJobInfo *block_job_query(BlockJob *job, Error **errp)
+BlockJobInfo *block_job_query_locked(BlockJob *job, Error **errp)
{
BlockJobInfo *info;
uint64_t progress_current, progress_total;
@@ -320,7 +340,7 @@ BlockJobInfo *block_job_query(BlockJob *job, Error **errp)
info->len = progress_total;
info->speed = job->speed;
info->io_status = job->iostatus;
-info->ready = job_is_ready(&job->job),
+info->ready = job_is_ready_locked(&job->job),
info->status= job->job.status;
info->auto_finalize = job->job.auto_finalize;
info->auto_dismiss = job->job.auto_dismiss;
@@ -333,6 +353,12 @@ BlockJobInfo *block_job_query(BlockJob *job, Error **errp)
return info;
}
+BlockJobInfo *block_job_query(BlockJob *job, Error **errp)
+{
+JOB_LOCK_GUARD();
+return block_job_query_locked(job, errp);
+}
+
static void block_job_iostatus_set_err(BlockJob *job, int error)
{
if (job->iostatus == BLOCK_DEVICE_IO_STATUS_OK) {
@@ -478,7 +504,7 @@ fail:
return NULL;
}
-void block_job_iostatus_reset(BlockJob *job)
+void block_job_iostatus_reset_locked(BlockJob *job)
{
GLOBAL_STATE_CODE();
if (job->iostatus == BLOCK_DEVICE_IO_STATUS_OK) {
@@ -488,6 +514,12 @@ void block_job_iostatus_reset(BlockJob *job)
job->iostatus = BLOCK_DEVICE_IO_STATUS_OK;
}
+void block_job_iostatus_reset(BlockJob *job)
+{
+JOB_LOCK_GUARD();
+block_job_iostatus_reset_locked(job);
+}
+
void block_job_user_resume(Job *job)
{
BlockJob *bjob = container_of(job, BlockJob, job);
diff --git a/include/block/blockjob.h b/include/block/blockjob.h
index 6525e16fd5..8b65d3949d 100644
--- a/include/block/blockjob.h
+++ b/include/block/blockjob.h
@@ -92,6 +92,9 @@ typedef struct BlockJob {
*/
BlockJob *bl
[PATCH v12 01/21] job.c: make job_mutex and job_lock/unlock() public
job mutex will be used to protect the job struct elements and list,
replacing AioContext locks.
Right now use a shared lock for all jobs, in order to keep things
simple. Once the AioContext lock is gone, we can introduce per-job
locks.
To simplify the switch from aiocontext to job lock, introduce
*nop* lock/unlock functions and macros.
We want to always call job_lock/unlock outside the AioContext locks,
and not vice-versa, otherwise we might get a deadlock. This is not
straightforward to do, and that's why we start with nop functions.
Once everything is protected by job_lock/unlock, we can change the nop into
an actual mutex and remove the aiocontext lock.
Since job_mutex is already being used, add static
real_job_{lock/unlock} for the existing usage.
Signed-off-by: Emanuele Giuseppe Esposito
Reviewed-by: Stefan Hajnoczi
Reviewed-by: Vladimir Sementsov-Ogievskiy
---
include/qemu/job.h | 24
job.c | 35 +++
2 files changed, 47 insertions(+), 12 deletions(-)
diff --git a/include/qemu/job.h b/include/qemu/job.h
index c105b31076..d1192ffd61 100644
--- a/include/qemu/job.h
+++ b/include/qemu/job.h
@@ -303,6 +303,30 @@ typedef enum JobCreateFlags {
JOB_MANUAL_DISMISS = 0x04,
} JobCreateFlags;
+extern QemuMutex job_mutex;
+
+#define JOB_LOCK_GUARD() /* QEMU_LOCK_GUARD(&job_mutex) */
+
+#define WITH_JOB_LOCK_GUARD() /* WITH_QEMU_LOCK_GUARD(&job_mutex) */
+
+/**
+ * job_lock:
+ *
+ * Take the mutex protecting the list of jobs and their status.
+ * Most functions called by the monitor need to call job_lock
+ * and job_unlock manually. On the other hand, function called
+ * by the block jobs themselves and by the block layer will take the
+ * lock for you.
+ */
+void job_lock(void);
+
+/**
+ * job_unlock:
+ *
+ * Release the mutex protecting the list of jobs and their status.
+ */
+void job_unlock(void);
+
/**
* Allocate and return a new job transaction. Jobs can be added to the
* transaction using job_txn_add_job().
diff --git a/job.c b/job.c
index 075c6f3a20..2b4ffca9d4 100644
--- a/job.c
+++ b/job.c
@@ -32,6 +32,12 @@
#include "trace/trace-root.h"
#include "qapi/qapi-events-job.h"
+/*
+ * job_mutex protects the jobs list, but also makes the
+ * struct job fields thread-safe.
+ */
+QemuMutex job_mutex;
+
static QLIST_HEAD(, Job) jobs = QLIST_HEAD_INITIALIZER(jobs);
/* Job State Transition Table */
@@ -74,17 +80,22 @@ struct JobTxn {
int refcnt;
};
-/* Right now, this mutex is only needed to synchronize accesses to job->busy
- * and job->sleep_timer, such as concurrent calls to job_do_yield and
- * job_enter. */
-static QemuMutex job_mutex;
+void job_lock(void)
+{
+/* nop */
+}
+
+void job_unlock(void)
+{
+/* nop */
+}
-static void job_lock(void)
+static void real_job_lock(void)
{
qemu_mutex_lock(&job_mutex);
}
-static void job_unlock(void)
+static void real_job_unlock(void)
{
qemu_mutex_unlock(&job_mutex);
}
@@ -450,21 +461,21 @@ void job_enter_cond(Job *job, bool(*fn)(Job *job))
return;
}
-job_lock();
+real_job_lock();
if (job->busy) {
-job_unlock();
+real_job_unlock();
return;
}
if (fn && !fn(job)) {
-job_unlock();
+real_job_unlock();
return;
}
assert(!job->deferred_to_main_loop);
timer_del(&job->sleep_timer);
job->busy = true;
-job_unlock();
+real_job_unlock();
aio_co_enter(job->aio_context, job->co);
}
@@ -481,13 +492,13 @@ void job_enter(Job *job)
* called explicitly. */
static void coroutine_fn job_do_yield(Job *job, uint64_t ns)
{
-job_lock();
+real_job_lock();
if (ns != -1) {
timer_mod(&job->sleep_timer, ns);
}
job->busy = false;
job_event_idle(job);
-job_unlock();
+real_job_unlock();
qemu_coroutine_yield();
/* Set by job_enter_cond() before re-entering the coroutine. */
--
2.31.1
[PATCH v12 02/21] job.h: categorize fields in struct Job
Categorize the fields in struct Job to understand which ones
need to be protected by the job mutex and which don't.
Signed-off-by: Emanuele Giuseppe Esposito
Reviewed-by: Vladimir Sementsov-Ogievskiy
Reviewed-by: Kevin Wolf
Reviewed-by: Stefan Hajnoczi
---
include/qemu/job.h | 61 +++---
1 file changed, 36 insertions(+), 25 deletions(-)
diff --git a/include/qemu/job.h b/include/qemu/job.h
index d1192ffd61..876e13d549 100644
--- a/include/qemu/job.h
+++ b/include/qemu/job.h
@@ -40,27 +40,52 @@ typedef struct JobTxn JobTxn;
* Long-running operation.
*/
typedef struct Job {
+
+/* Fields set at initialization (job_create), and never modified */
+
/** The ID of the job. May be NULL for internal jobs. */
char *id;
-/** The type of this job. */
+/**
+ * The type of this job.
+ * All callbacks are called with job_mutex *not* held.
+ */
const JobDriver *driver;
-/** Reference count of the block job */
-int refcnt;
-
-/** Current state; See @JobStatus for details. */
-JobStatus status;
-
-/** AioContext to run the job coroutine in */
-AioContext *aio_context;
-
/**
* The coroutine that executes the job. If not NULL, it is reentered when
* busy is false and the job is cancelled.
+ * Initialized in job_start()
*/
Coroutine *co;
+/** True if this job should automatically finalize itself */
+bool auto_finalize;
+
+/** True if this job should automatically dismiss itself */
+bool auto_dismiss;
+
+/** The completion function that will be called when the job completes. */
+BlockCompletionFunc *cb;
+
+/** The opaque value that is passed to the completion function. */
+void *opaque;
+
+/* ProgressMeter API is thread-safe */
+ProgressMeter progress;
+
+
+/** Protected by AioContext lock */
+
+/** AioContext to run the job coroutine in */
+AioContext *aio_context;
+
+/** Reference count of the block job */
+int refcnt;
+
+/** Current state; See @JobStatus for details. */
+JobStatus status;
+
/**
* Timer that is used by @job_sleep_ns. Accessed under job_mutex (in
* job.c).
@@ -112,14 +137,6 @@ typedef struct Job {
/** Set to true when the job has deferred work to the main loop. */
bool deferred_to_main_loop;
-/** True if this job should automatically finalize itself */
-bool auto_finalize;
-
-/** True if this job should automatically dismiss itself */
-bool auto_dismiss;
-
-ProgressMeter progress;
-
/**
* Return code from @run and/or @prepare callback(s).
* Not final until the job has reached the CONCLUDED status.
@@ -134,12 +151,6 @@ typedef struct Job {
*/
Error *err;
-/** The completion function that will be called when the job completes. */
-BlockCompletionFunc *cb;
-
-/** The opaque value that is passed to the completion function. */
-void *opaque;
-
/** Notifiers called when a cancelled job is finalised */
NotifierList on_finalize_cancelled;
@@ -167,6 +178,7 @@ typedef struct Job {
/**
* Callbacks and other information about a Job driver.
+ * All callbacks are invoked with job_mutex *not* held.
*/
struct JobDriver {
@@ -472,7 +484,6 @@ void job_yield(Job *job);
*/
void coroutine_fn job_sleep_ns(Job *job, int64_t ns);
-
/** Returns the JobType of a given Job. */
JobType job_type(const Job *job);
--
2.31.1
[PATCH v12 13/21] jobs: protect job.aio_context with BQL and job_mutex
In order to make it thread safe, implement a "fake rwlock",
where we allow reads under BQL *or* job_mutex held, but
writes only under BQL *and* job_mutex.
The only write we have is in child_job_set_aio_ctx, which always
happens under drain (so the job is paused).
For this reason, introduce job_set_aio_context and make sure that
the context is set under BQL, job_mutex and drain.
Also make sure all other places where the aiocontext is read
are protected.
The reads in commit.c and mirror.c are actually safe, because always
done under BQL.
Note: at this stage, job_{lock/unlock} and job lock guard macros
are *nop*.
Suggested-by: Paolo Bonzini
Signed-off-by: Emanuele Giuseppe Esposito
---
block/replication.c | 1 +
blockjob.c | 3 ++-
include/qemu/job.h | 23 ---
job.c | 12
4 files changed, 35 insertions(+), 4 deletions(-)
diff --git a/block/replication.c b/block/replication.c
index 55c8f894aa..5977f7a833 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -142,6 +142,7 @@ static void replication_close(BlockDriverState *bs)
{
BDRVReplicationState *s = bs->opaque;
Job *commit_job;
+GLOBAL_STATE_CODE();
if (s->stage == BLOCK_REPLICATION_RUNNING) {
replication_stop(s->rs, false, NULL);
diff --git a/blockjob.c b/blockjob.c
index 96fb9d9f73..c8919cef9b 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -162,12 +162,13 @@ static void child_job_set_aio_ctx(BdrvChild *c,
AioContext *ctx,
bdrv_set_aio_context_ignore(sibling->bs, ctx, ignore);
}
-job->job.aio_context = ctx;
+job_set_aio_context(&job->job, ctx);
}
static AioContext *child_job_get_parent_aio_context(BdrvChild *c)
{
BlockJob *job = c->opaque;
+GLOBAL_STATE_CODE();
return job->job.aio_context;
}
diff --git a/include/qemu/job.h b/include/qemu/job.h
index 5709e8d4a8..50a4c06b93 100644
--- a/include/qemu/job.h
+++ b/include/qemu/job.h
@@ -74,11 +74,17 @@ typedef struct Job {
/* ProgressMeter API is thread-safe */
ProgressMeter progress;
+/**
+ * AioContext to run the job coroutine in.
+ * The job Aiocontext can be read when holding *either*
+ * the BQL (so we are in the main loop) or the job_mutex.
+ * It can only be written when we hold *both* BQL
+ * and the job_mutex.
+ */
+AioContext *aio_context;
-/** Protected by AioContext lock */
-/** AioContext to run the job coroutine in */
-AioContext *aio_context;
+/** Protected by AioContext lock */
/** Reference count of the block job */
int refcnt;
@@ -741,4 +747,15 @@ int job_finish_sync(Job *job, void (*finish)(Job *, Error
**errp),
int job_finish_sync_locked(Job *job, void (*finish)(Job *, Error **errp),
Error **errp);
+/**
+ * Sets the @job->aio_context.
+ * Called with job_mutex *not* held.
+ *
+ * This function must run in the main thread to protect against
+ * concurrent read in job_finish_sync_locked(), takes the job_mutex
+ * lock to protect against the read in job_do_yield_locked(), and must
+ * be called when the job is quiescent.
+ */
+void job_set_aio_context(Job *job, AioContext *ctx);
+
#endif
diff --git a/job.c b/job.c
index 85ae843f03..c4ac363f08 100644
--- a/job.c
+++ b/job.c
@@ -396,6 +396,17 @@ Job *job_get(const char *id)
return job_get_locked(id);
}
+void job_set_aio_context(Job *job, AioContext *ctx)
+{
+/* protect against read in job_finish_sync_locked and job_start */
+GLOBAL_STATE_CODE();
+/* protect against read in job_do_yield_locked */
+JOB_LOCK_GUARD();
+/* ensure the job is quiescent while the AioContext is changed */
+assert(job->paused || job_is_completed_locked(job));
+job->aio_context = ctx;
+}
+
/* Called with job_mutex *not* held. */
static void job_sleep_timer_cb(void *opaque)
{
@@ -1379,6 +1390,7 @@ int job_finish_sync_locked(Job *job,
{
Error *local_err = NULL;
int ret;
+GLOBAL_STATE_CODE();
job_ref_locked(job);
--
2.31.1
Re: [PATCH v3] block: Refactor get_tmp_filename()
On Sun, Sep 25, 2022 at 12:32:00AM +0800, Bin Meng wrote:
> From: Bin Meng
>
> At present there are two callers of get_tmp_filename() and they are
> inconsistent.
>
> One does:
>
> /* TODO: extra byte is a hack to ensure MAX_PATH space on Windows. */
> char *tmp_filename = g_malloc0(PATH_MAX + 1);
> ...
> ret = get_tmp_filename(tmp_filename, PATH_MAX + 1);
>
> while the other does:
>
> s->qcow_filename = g_malloc(PATH_MAX);
> ret = get_tmp_filename(s->qcow_filename, PATH_MAX);
>
> As we can see different 'size' arguments are passed. There are also
> platform specific implementations inside the function, and this use
> of snprintf is really undesirable.
>
> Refactor this routine by changing its signature to:
>
> int get_tmp_filename(char **filename)
>
> and use g_file_open_tmp() for a consistent implementation.
>
> Signed-off-by: Bin Meng
> ---
>
> Changes in v3:
> - Do not use errno directly, instead still let get_tmp_filename() return
> a negative number to indicate error
>
> Changes in v2:
> - Use g_autofree and g_steal_pointer
>
> include/block/block_int-common.h | 2 +-
> block.c | 36 ++--
> block/vvfat.c| 3 +--
> 3 files changed, 13 insertions(+), 28 deletions(-)
>
> diff --git a/include/block/block_int-common.h
> b/include/block/block_int-common.h
> index 8947abab76..eb30193198 100644
> --- a/include/block/block_int-common.h
> +++ b/include/block/block_int-common.h
> @@ -1230,7 +1230,7 @@ static inline BlockDriverState *child_bs(BdrvChild
> *child)
> }
>
> int bdrv_check_request(int64_t offset, int64_t bytes, Error **errp);
> -int get_tmp_filename(char *filename, int size);
> +int get_tmp_filename(char **filename);
Change it to:
char *get_tmp_filename(Error **errp);
> @@ -3737,7 +3723,7 @@ static BlockDriverState
> *bdrv_append_temp_snapshot(BlockDriverState *bs,
> }
>
> /* Create the temporary image */
> -ret = get_tmp_filename(tmp_filename, PATH_MAX + 1);
> +ret = get_tmp_filename(&tmp_filename);
> if (ret < 0) {
> error_setg_errno(errp, -ret, "Could not get temporary filename");
And pass 'errp' straight into get_tmp_filename, thus avoid the
different error message text here vs below
eg
tmp_filename = get_tmp_filename(errp);
if (!tmp_filename) {
goto out;
}
> goto out;
> diff --git a/block/vvfat.c b/block/vvfat.c
> index d6dd919683..43f42fd1ea 100644
> --- a/block/vvfat.c
> +++ b/block/vvfat.c
> @@ -3146,8 +3146,7 @@ static int enable_write_target(BlockDriverState *bs,
> Error **errp)
>
> array_init(&(s->commits), sizeof(commit_t));
>
> -s->qcow_filename = g_malloc(PATH_MAX);
> -ret = get_tmp_filename(s->qcow_filename, PATH_MAX);
> +ret = get_tmp_filename(&s->qcow_filename);
> if (ret < 0) {
> error_setg_errno(errp, -ret, "can't create temporary file");
> goto err;
With regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH] block: Refactor get_tmp_filename()
On Sat, Sep 24, 2022 at 04:00:34PM +0800, Bin Meng wrote: > From: Bin Meng > > At present there are two callers of get_tmp_filename() and they are > inconsistent. > > One does: > > /* TODO: extra byte is a hack to ensure MAX_PATH space on Windows. */ > char *tmp_filename = g_malloc0(PATH_MAX + 1); > ... > ret = get_tmp_filename(tmp_filename, PATH_MAX + 1); > > while the other does: > > s->qcow_filename = g_malloc(PATH_MAX); > ret = get_tmp_filename(s->qcow_filename, PATH_MAX); > > As we can see different 'size' arguments are passed. There are also > platform specific implementations inside the function, and this use > of snprintf is really undesirable. > > Refactor this routine by changing its signature to: > > char *get_tmp_filename(void) > > and use g_file_open_tmp() for a consistent implementation. > > Signed-off-by: Bin Meng > --- > > include/block/block_int-common.h | 2 +- > block.c | 42 ++-- > block/vvfat.c| 8 +++--- > 3 files changed, 18 insertions(+), 34 deletions(-) Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
