* [Qemu-devel] [PATCH 1/1] block/file-posix.c: fix not reopened lock file descriptor
@ 2018-03-22 17:20 Dion Bosschieter
2018-03-22 17:39 ` Kevin Wolf
0 siblings, 1 reply; 5+ messages in thread
From: Dion Bosschieter @ 2018-03-22 17:20 UTC (permalink / raw)
To: qemu-devel; +Cc: mreitz, kwolf, Dion Bosschieter
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 <dionbosschieter@gmail.com>
---
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH 1/1] block/file-posix.c: fix not reopened lock file descriptor
2018-03-22 17:20 [Qemu-devel] [PATCH 1/1] block/file-posix.c: fix not reopened lock file descriptor Dion Bosschieter
@ 2018-03-22 17:39 ` Kevin Wolf
2018-03-22 18:08 ` Dion Bosschieter
2018-03-30 6:47 ` Fam Zheng
0 siblings, 2 replies; 5+ messages in thread
From: Kevin Wolf @ 2018-03-22 17:39 UTC (permalink / raw)
To: Dion Bosschieter; +Cc: qemu-devel, mreitz, famz, qemu-block
[ 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 <dionbosschieter@gmail.com>
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
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH 1/1] block/file-posix.c: fix not reopened lock file descriptor
2018-03-22 17:39 ` Kevin Wolf
@ 2018-03-22 18:08 ` Dion Bosschieter
2018-03-30 6:48 ` Fam Zheng
2018-03-30 6:47 ` Fam Zheng
1 sibling, 1 reply; 5+ messages in thread
From: Dion Bosschieter @ 2018-03-22 18:08 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-devel, mreitz, famz, qemu-block
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 <kwolf@redhat.com> 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 <dionbosschieter@gmail.com>
>
> 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
>>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH 1/1] block/file-posix.c: fix not reopened lock file descriptor
2018-03-22 17:39 ` Kevin Wolf
2018-03-22 18:08 ` Dion Bosschieter
@ 2018-03-30 6:47 ` Fam Zheng
1 sibling, 0 replies; 5+ messages in thread
From: Fam Zheng @ 2018-03-30 6:47 UTC (permalink / raw)
To: Kevin Wolf; +Cc: Dion Bosschieter, qemu-devel, mreitz, qemu-block
On Thu, 03/22 18:39, Kevin Wolf wrote:
> [ 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 <dionbosschieter@gmail.com>
>
> 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.
I think (I remember we have discussed a bit before) techinically we can do
reopen just well without a separate s->lock_fd. After all all locks we acquire
are shared lock, so it is possible to handle the lock between old fd and the new
one back and forth freely.
Fam
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH 1/1] block/file-posix.c: fix not reopened lock file descriptor
2018-03-22 18:08 ` Dion Bosschieter
@ 2018-03-30 6:48 ` Fam Zheng
0 siblings, 0 replies; 5+ messages in thread
From: Fam Zheng @ 2018-03-30 6:48 UTC (permalink / raw)
To: Dion Bosschieter; +Cc: Kevin Wolf, qemu-devel, mreitz, qemu-block
On Thu, 03/22 19:08, Dion Bosschieter wrote:
> 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.
What is the point of changing the symlink and checking if FDs are changed?
Fam
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-03-30 6:48 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-22 17:20 [Qemu-devel] [PATCH 1/1] block/file-posix.c: fix not reopened lock file descriptor Dion Bosschieter
2018-03-22 17:39 ` Kevin Wolf
2018-03-22 18:08 ` Dion Bosschieter
2018-03-30 6:48 ` Fam Zheng
2018-03-30 6:47 ` Fam Zheng
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.