Yeah I have a use case, before a last sync on a storage migration we suspend a VM -> send the last diffs -> mount the new storage server and after that we change a symlink -> call reopen -> check if all file descriptors are changed before resuming the VM.
Dion > Op 22 mrt. 2018 om 18:39 heeft Kevin Wolf <[email protected]> het volgende > geschreven: > > [ Cc: qemu-block ] > > Am 22.03.2018 um 18:20 hat Dion Bosschieter geschrieben: >> In commit 244a5668106297378391b768e7288eb157616f64 another >> file descriptor to BDRVRawState is added. When we try to issue the >> reopen command only s->fd is reopened; lock_fd could still hold an old >> file descriptor "possibly" pointing to another file. >> >> - change raw_reopen_prepare so it checks use_lock from BDRVRawState and >> tries to reopen lock_fd accordingly >> - change raw_reopen_commit so it closes the old lock_fd on use_lock >> >> Signed-off-by: Dion Bosschieter <[email protected]> > > bdrv_reopen() is not meant for opening a different file, it is meant to > change the flags and options of the same file. Do you have a use case > where you would actually need to switch to a different file? > > As far as I know, lock_fd was specifically introduced _because_ it stays > the same across reopen, so we don't need a racy release/reacquire pair. > Fam (CCed) should know more. > > In any case, doesn't your patch drop all the locks without reacquiring > them on the new lock_fd? > > Kevin > >> block/file-posix.c | 25 +++++++++++++++++++++++++ >> 1 file changed, 25 insertions(+) >> >> diff --git a/block/file-posix.c b/block/file-posix.c >> index d7fb772c14..16d83fc49e 100644 >> --- a/block/file-posix.c >> +++ b/block/file-posix.c >> @@ -167,6 +167,7 @@ typedef struct BDRVRawState { >> >> typedef struct BDRVRawReopenState { >> int fd; >> + int lock_fd; >> int open_flags; >> } BDRVRawReopenState; >> >> @@ -795,6 +796,7 @@ static int raw_reopen_prepare(BDRVReopenState *state, >> raw_parse_flags(state->flags, &rs->open_flags); >> >> rs->fd = -1; >> + rs->lock_fd = -1; >> >> int fcntl_flags = O_APPEND | O_NONBLOCK; >> #ifdef O_NOATIME >> @@ -820,6 +822,17 @@ static int raw_reopen_prepare(BDRVReopenState *state, >> rs->fd = -1; >> } >> } >> + >> + if (s->use_lock) { >> + rs->lock_fd = qemu_dup(s->lock_fd); >> + if (rs->lock_fd >= 0) { >> + ret = fcntl_setfl(rs->lock_fd, rs->open_flags); >> + if (ret) { >> + qemu_close(rs->lock_fd); >> + rs->lock_fd = -1; >> + } >> + } >> + } >> } >> >> /* If we cannot use fcntl, or fcntl failed, fall back to qemu_open() */ >> @@ -835,6 +848,14 @@ static int raw_reopen_prepare(BDRVReopenState *state, >> error_setg_errno(errp, errno, "Could not reopen file"); >> ret = -1; >> } >> + >> + if (s->use_lock) { >> + rs->lock_fd = qemu_open(normalized_filename, >> rs->open_flags); >> + if (rs->lock_fd == -1) { >> + error_setg_errno(errp, errno, "Could not reopen file >> for locking"); >> + ret = -1; >> + } >> + } >> } >> } >> >> @@ -861,7 +882,11 @@ static void raw_reopen_commit(BDRVReopenState *state) >> s->open_flags = rs->open_flags; >> >> qemu_close(s->fd); >> + if (s->use_lock) { >> + qemu_close(s->lock_fd); >> + } >> s->fd = rs->fd; >> + s->lock_fd = rs->lock_fd; >> >> g_free(state->opaque); >> state->opaque = NULL; >> -- >> 2.14.2 >>
