Re: [Qemu-devel] [PATCH for-2.7 v2 05/17] raw-posix: Implement .bdrv_lockf

[Denis V. Lunev]

On 04/18/2016 04:12 AM, Fam Zheng wrote:
> On Sat, 04/16 16:29, Denis V. Lunev wrote:
> > On 04/15/2016 06:27 AM, Fam Zheng wrote:
> > > virtlockd in libvirt locks the first byte, we lock byte 1 to avoid
> > > the intervene.
> > >
> > > Suggested-by: "Daniel P. Berrange" <address@hidden>
> > > Signed-off-by: Fam Zheng <address@hidden>
> > > ---
> > >   block/raw-posix.c | 35 +++++++++++++++++++++++++++++++++++
> > >   1 file changed, 35 insertions(+)
> > >
> > > diff --git a/block/raw-posix.c b/block/raw-posix.c
> > > index 906d5c9..3a2c17f 100644
> > > --- a/block/raw-posix.c
> > > +++ b/block/raw-posix.c
> > > @@ -35,6 +35,7 @@
> > >   #include "raw-aio.h"
> > >   #include "qapi/util.h"
> > >   #include "qapi/qmp/qstring.h"
> > > +#include "glib.h"
> > >   #if defined(__APPLE__) && (__MACH__)
> > >   #include <paths.h>
> > > @@ -397,6 +398,38 @@ static void raw_attach_aio_context(BlockDriverState *bs,
> > >   #endif
> > >   }
> > > +static int raw_lockf(BlockDriverState *bs, BdrvLockfCmd cmd)
> > > +{
> > > +
> > > +    BDRVRawState *s = bs->opaque;
> > > +    int ret;
> > > +    struct flock fl = (struct flock) {
> > > +        .l_whence  = SEEK_SET,
> > > +        /* Locking byte 1 avoids interfereing with virtlockd. */
> > > +        .l_start = 1,
> > > +        .l_len = 1,
> > > +    };
> > > +
> > > +    switch (cmd) {
> > > +    case BDRV_LOCKF_RWLOCK:
> > > +        fl.l_type = F_WRLCK;
> > > +        break;
> > > +    case BDRV_LOCKF_ROLOCK:
> > > +        fl.l_type = F_RDLCK;
> > > +        break;
> > > +    case BDRV_LOCKF_UNLOCK:
> > > +        fl.l_type = F_UNLCK;
> > > +        break;
> > > +    default:
> > > +        abort();
> > > +    }
> > > +    ret = fcntl(s->fd, F_SETLK, &fl);
> > > +    if (ret) {
> > > +        ret = -errno;
> > > +    }
> > > +    return ret;
> > > +}
> > > +
> > >   #ifdef CONFIG_LINUX_AIO
> > >   static int raw_set_aio(void **aio_ctx, int *use_aio, int bdrv_flags)
> > >   {
> > > @@ -1960,6 +1993,8 @@ BlockDriver bdrv_file = {
> > >       .bdrv_detach_aio_context = raw_detach_aio_context,
> > >       .bdrv_attach_aio_context = raw_attach_aio_context,
> > > +    .bdrv_lockf = raw_lockf,
> > > +
> > >       .create_opts = &raw_create_opts,
> > >   };
> > would it be better to use
> >
> >         int flock(int fd, int operation);
> >
> > DESCRIPTION
> >         Apply or remove an advisory lock on the open file specified by fd.
> > The
> >         argument operation is one of the following:
> >
> >             LOCK_SH  Place a shared lock.  More than one  process may  hold
> > a
> >                      shared lock for a given file at a given time.
> >
> >             LOCK_EX  Place  an  exclusive  lock.   Only one process may hold
> > an
> >                      exclusive lock for a given file at a given time.
> >
> >             LOCK_UN  Remove an existing lock held by this process.
> >
> > for this purpose?
> >
> > Sorry that missed this point in the initial review...
> > We will not intersect with libvirt for the case.
> As noted in the cover letter, flock() is nop on NFS mount points on Linux, so
> fcntl is safer.
>
> Fam
that seems STRANGE to me. For the time being flock() was
working for Linux NFS server. Here is the implementation.

int nfs_flock(struct file *filp, int cmd, struct file_lock *fl)
{
        struct inode *inode = filp->f_mapping->host;
        int is_local = 0;

        dprintk("NFS: flock(%pD2, t=%x, fl=%x)\n",
                        filp, fl->fl_type, fl->fl_flags);

        if (!(fl->fl_flags & FL_FLOCK))
                return -ENOLCK;

        /*
         * The NFSv4 protocol doesn't support LOCK_MAND, which is not 
part of
         * any standard. In principle we might be able to support LOCK_MAND
         * on NFSv2/3 since NLMv3/4 support DOS share modes, but for 
now the
         * NFS code is not set up for it.
         */
        if (fl->fl_type & LOCK_MAND)
                return -EINVAL;

        if (NFS_SERVER(inode)->flags & NFS_MOUNT_LOCAL_FLOCK)
                is_local = 1;

        /* We're simulating flock() locks using posix locks on the 
server */
        if (fl->fl_type == F_UNLCK)
                return do_unlk(filp, cmd, fl, is_local);
        return do_setlk(filp, cmd, fl, is_local);
}

Den