On Wed, 10/11 11:48, Kevin Wolf wrote: > Am 11.10.2017 um 11:38 hat Vladimir Sementsov-Ogievskiy geschrieben: > > 11.10.2017 12:22, Kevin Wolf wrote: > > > [ Cc: Fam ]
Sorry for being slow, now finally getting my hands on email backlog after holiday and preparing for KVM Forum presentation. > > > > > > Am 10.10.2017 um 15:42 hat Vladimir Sementsov-Ogievskiy geschrieben: > > > > We do not reopen lock_fd on bdrv_reopen which leads to problems on > > > > reopen image RO. So, lets make lock_fd be always RO. > > > > This is correct, because qemu_lock_fd always called with exclusive=false > > > > on lock_fd. > > > > > > > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com> > > > > --- > > > > > > > > Hi all! > > > > > > > > We've faced the following problem with our shared-storage migration > > > > scheme. We make an external snapshot and need base image to be reopened > > > > RO. However, bdrv_reopen reopens only .fd of BDRVRawState but not > > > > .lock_fd. So, .lock_fd is left opened RW and this breaks the whole > > > > thing. > > > > > > > > The simple fix is here: let's just open lock_fd as RO always. This > > > > looks fine for current code, as we never try to set write locks > > > > (qemu_lock_fd always called with exclusive=false). > > > > > > > > However it will not work if we are going to use write locks. > > > I was sure that we had discussed this during review, so I just went back > > > and checked. Indeed, Fam originally had an unconditional O_RDONLY in > > > some version of the image locking patches, but I actually found a > > > potential problem with that back then: > > > > > > > Note that with /dev/fdset there can be cases where we can open a file > > > > O_RDWR, but not O_RDONLY. Should we better just use the same flags as > > > > for the s->fd? > > > https://lists.gnu.org/archive/html/qemu-devel/2017-04/msg05107.html > > > > > > However, I'm now wondering whether we really still need a separate > > > s->lock_fd or whether we can just use the normal image fd for this. If I > > > understood the old threads correctly, the original reason for it was > > > that during bdrv_reopen(), we couldn't safely migrate exclusive locks > > > from the old fd to the new one. But as we aren't using exclusive locks > > > any more, this shouldn't be a problem today. > > > > > > Fam, are there more reasons why we need a separate lock_fd? I think your reasoning is right. We needed lock_fd in earlier revisions of the image locking patch because we cannot move exclusive lock from one fd to another transactionally. We don't need that now. Fam