Re: [Qemu-block] [PATCH RFC] file-posix: make lock_fd read-only

[Fam Zheng]

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 <address@hidden>
> > > > ---
> > > > 
> > > > 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