Re: [Qemu-block] [PATCH v6 04/22] block: Introduce image file locking

[Kevin Wolf]

Am 22.06.2016 um 09:23 hat Fam Zheng geschrieben:
> On Fri, 06/17 13:34, Kevin Wolf wrote:
> > Am 03.06.2016 um 10:48 hat Fam Zheng geschrieben:
> > > Block drivers can implement this new operation .bdrv_lockf to actually 
> > > lock the
> > > image in the protocol specific way.
> > > 
> > > Signed-off-by: Fam Zheng <address@hidden>
> > > ---
> > >  block.c                   | 57 
> > > +++++++++++++++++++++++++++++++++++++++++++++++
> > >  include/block/block.h     | 11 ++++++++-
> > >  include/block/block_int.h |  5 +++++
> > >  3 files changed, 72 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/block.c b/block.c
> > > index 736432f..4c2a3ff 100644
> > > --- a/block.c
> > > +++ b/block.c
> > > @@ -854,6 +854,50 @@ out:
> > >      g_free(gen_node_name);
> > >  }
> > >  
> > > +BdrvLockfCmd bdrv_get_locking_cmd(int flags)
> > > +{
> > > +    if (flags & BDRV_O_NO_LOCK) {
> > > +        return BDRV_LOCKF_UNLOCK;
> > > +    } else if (flags & BDRV_O_SHARED_LOCK) {
> > > +        return BDRV_LOCKF_SHARED;
> > > +    } else if (flags & BDRV_O_RDWR) {
> > > +        return BDRV_LOCKF_EXCLUSIVE;
> > > +    } else {
> > > +        return BDRV_LOCKF_SHARED;
> > > +    }
> > > +}
> > 
> > It feels a bit counterintuitive to use the very same operation for
> > "start of critical section, but don't actually lock" and "end of
> > critical section".
> > 
> > Is there a specific reason why you chose this instead of separate
> > .bdrv_lock/bdrv_unlock callbacks?
> 
> Because unlike open(2)/close(2), locking and unlocking are typically
> implemented with one parameterized operation (fcntl(2)) underneath, I followed
> that naturally.

Is it really "typically", or is this an idiosyncrasy of POSIX and we end
up modeling our interface for one particular block driver even though
for others a different interface might be preferable? Did you look at
more interfaces?

Apart from that, we generally make our internal interfaces so that they
are easy to use and code is easy to read rather than directly mapping
POSIX everywhere. For example, our I/O function never do short
reads/writes even though POSIX does that.

> > > @@ -3187,6 +3240,9 @@ void bdrv_invalidate_cache(BlockDriverState *bs, 
> > > Error **errp)
> > >          error_setg_errno(errp, -ret, "Could not refresh total sector 
> > > count");
> > >          return;
> > >      }
> > > +    if (!(bs->open_flags & BDRV_O_NO_LOCK)) {
> > > +        bdrv_lock_image(bs);
> > > +    }
> > >  }
> > 
> > I think the if is unnecessary, bdrv_lock_image() already looks at
> > BDRV_O_NO_LOCK.
> 
> I intentionally made enum BDRV_O_LOCK_* start from non-zero, so 
> bdrv_lock_image
> will call drv->bdrv_lockf even for BDRV_O_NO_LOCK. In the case of
> lock-mode=off, I think we should skip that.

Fair enough.

Kevin