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

[Kevin Wolf]

Am 11.10.2017 um 11:38 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 11.10.2017 12:22, Kevin Wolf wrote:
> > [ Cc: Fam ]
> > 
> > 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?
> > 
> > Kevin
> 
> If I understand correctly, posix lock will be lost on fd close anyway, so
> other app will have an opportunity of taking this lock, so it's unsafe.

With the OFD locks we're using, you just need to take the lock on the
new fd before you close the old fd, then it should be safe.

With normal POSIX locks, bdrv_reopen() is hopeless anyway, you will
always lose the lock, even with a separate lock_fd. This is why we only
make use of POSIX locks if OFD isn't available, if locking=on is
explicitly requested and only after printing a warning.

Kevin