Re: [Qemu-block] [Qemu-devel] [PATCH v15 19/21] osdep: Add qemu_lock_fd and qemu_unlock_fd

[Daniel P. Berrange]

On Wed, Apr 26, 2017 at 09:20:28PM +0800, Fam Zheng wrote:
> On Wed, 04/26 14:57, Kevin Wolf wrote:
> > Am 26.04.2017 um 05:34 hat Fam Zheng geschrieben:
> > > They are wrappers of POSIX fcntl "file private locking", with a
> > > convenient "try lock" wrapper implemented with F_OFD_GETLK.
> > > 
> > > Signed-off-by: Fam Zheng <address@hidden>
> > > Reviewed-by: Max Reitz <address@hidden>
> > > ---
> > >  include/qemu/osdep.h |  3 +++
> > >  util/osdep.c         | 48 
> > > ++++++++++++++++++++++++++++++++++++++++++++++++
> > >  2 files changed, 51 insertions(+)
> > > 
> > > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> > > index 122ff06..1c9f5e2 100644
> > > --- a/include/qemu/osdep.h
> > > +++ b/include/qemu/osdep.h
> > > @@ -341,6 +341,9 @@ int qemu_close(int fd);
> > >  #ifndef _WIN32
> > >  int qemu_dup(int fd);
> > >  #endif
> > > +int qemu_lock_fd(int fd, int64_t start, int64_t len, bool exclusive);
> > > +int qemu_unlock_fd(int fd, int64_t start, int64_t len);
> > > +int qemu_lock_fd_test(int fd, int64_t start, int64_t len, bool 
> > > exclusive);
> > 
> > For the record: On IRC, I proposed adding something like the following:
> > 
> >     #ifndef F_OFD_SETLK
> >     #define F_OFD_SETLK F_SETLK
> >     #define F_OFD_GETLK F_GETLK
> >     #endif
> > 
> > F_OFD_* are still relatively new and e.g. RHEL 7 doesn't support it yet.
> > Using process-based locks is suboptimal because we can easily lose them
> > earlier than we want, but it's still better than nothing and covers the
> > common simple cases.
> 
> Yes, we should add that. But I'd prefer:
> 
>     #ifdef F_OFD_SETLK
>     #define QEMU_SETLK F_OFD_SETLK
>     #define QEMU_GETLK F_OFD_GETLK
>     #else
>     #define QEMU_SETLK F_SETLK
>     #define QEMU_GETLK F_GETLK
>     #endif
> 
> to avoid "abusing" the macro name.
> 
> Another question is whether we should print a warning to make users aware? 
> Even
> the test case in patch 21 can see three "lock losses" on RHEL with posix lock,
> and there are way more corner cases, I believe.

Is it possible to use F_GETLK to detect when we have a missing lock, then
add assertions in various key places that call F_GETLK to validate hat
the lock still exists & print an warning.

I don't like the idea that downstream would be running with locking enabled,
but silently loosing locks with no indication at all that this happened,
especially when upstream development won't be testing this since those devs
will use the F_OFD_SETLK instead.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|