[Qemu-devel] [PATCH v2] virtiofsd: fix compile error if 'F_OFD_GETLK' not defined

[Qemu-devel] [PATCH v2] virtiofsd: fix compile error if 'F_OFD_GETLK' not defined

[piaojun]

Use F_GETLK for fcntl when F_OFD_GETLK not defined, such as kernel 3.10.

Signed-off-by: Jun Piao <piaojun@huawei.com>
---
v2:
- Use F_OFD_SETLK to replace F_OFD_GETLK in #ifdef.

---
 contrib/virtiofsd/passthrough_ll.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/contrib/virtiofsd/passthrough_ll.c b/contrib/virtiofsd/passthrough_ll.c
index a81c01d..c69f2f3 100644
--- a/contrib/virtiofsd/passthrough_ll.c
+++ b/contrib/virtiofsd/passthrough_ll.c
@@ -1780,7 +1780,11 @@ static void lo_getlk(fuse_req_t req, fuse_ino_t ino,
 		goto out;
 	}

+#ifdef F_OFD_GETLK
 	ret = fcntl(plock->fd, F_OFD_GETLK, lock);
+#else
+	ret = fcntl(plock->fd, F_GETLK, lock);
+#endif
 	if (ret == -1)
 		saverr = errno;

@@ -1831,7 +1835,11 @@ static void lo_setlk(fuse_req_t req, fuse_ino_t ino,

 	/* TODO: Is it alright to modify flock? */
 	lock->l_pid = 0;
+#ifdef F_OFD_SETLK
 	ret = fcntl(plock->fd, F_OFD_SETLK, lock);
+#else
+	ret = fcntl(plock->fd, F_GETLK, lock);
+#endif
 	if (ret == -1) {
 		saverr = errno;
 	}
-- 

Re: [Qemu-devel] [PATCH v2] virtiofsd: fix compile error if 'F_OFD_GETLK' not defined

[Dr. David Alan Gilbert]

* piaojun (piaojun@huawei.com) wrote:
> Use F_GETLK for fcntl when F_OFD_GETLK not defined, such as kernel 3.10.
> 
> Signed-off-by: Jun Piao <piaojun@huawei.com>


> ---
> v2:
> - Use F_OFD_SETLK to replace F_OFD_GETLK in #ifdef.
> 
> ---
>  contrib/virtiofsd/passthrough_ll.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/contrib/virtiofsd/passthrough_ll.c b/contrib/virtiofsd/passthrough_ll.c
> index a81c01d..c69f2f3 100644
> --- a/contrib/virtiofsd/passthrough_ll.c
> +++ b/contrib/virtiofsd/passthrough_ll.c
> @@ -1780,7 +1780,11 @@ static void lo_getlk(fuse_req_t req, fuse_ino_t ino,
>  		goto out;
>  	}
> 
> +#ifdef F_OFD_GETLK
>  	ret = fcntl(plock->fd, F_OFD_GETLK, lock);
> +#else
> +	ret = fcntl(plock->fd, F_GETLK, lock);
> +#endif
>  	if (ret == -1)
>  		saverr = errno;
> 
> @@ -1831,7 +1835,11 @@ static void lo_setlk(fuse_req_t req, fuse_ino_t ino,
> 
>  	/* TODO: Is it alright to modify flock? */
>  	lock->l_pid = 0;
> +#ifdef F_OFD_SETLK
>  	ret = fcntl(plock->fd, F_OFD_SETLK, lock);
> +#else
> +	ret = fcntl(plock->fd, F_GETLK, lock);
                               ^^^^^^^

Typo! You've got GETLK rather than SETLK.

But, a bigger question - does this actually work!
The manpage says:
   'If a process closes any file descriptor referring to a file, then
   all of the process's locks on that file are released, regardless of the
   file descriptor(s) on which the locks were obtained.'

the fd we're using here came from lookup_create_plock_ctx which did
a new openat to get this fd; so we've already got multiple fd's
referring to this file; and thus I worry we're going to close
one of them and lose all our locks on it.

Dave


> +#endif

>  	if (ret == -1) {
>  		saverr = errno;
>  	}
> -- 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v2] virtiofsd: fix compile error if 'F_OFD_GETLK' not defined

[Daniel P. Berrangé]

On Fri, Aug 02, 2019 at 11:53:52AM +0100, Dr. David Alan Gilbert wrote:
> * piaojun (piaojun@huawei.com) wrote:
> > Use F_GETLK for fcntl when F_OFD_GETLK not defined, such as kernel 3.10.
> > 
> > Signed-off-by: Jun Piao <piaojun@huawei.com>
> 
> 
> > ---
> > v2:
> > - Use F_OFD_SETLK to replace F_OFD_GETLK in #ifdef.
> > 
> > ---
> >  contrib/virtiofsd/passthrough_ll.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/contrib/virtiofsd/passthrough_ll.c b/contrib/virtiofsd/passthrough_ll.c
> > index a81c01d..c69f2f3 100644
> > --- a/contrib/virtiofsd/passthrough_ll.c
> > +++ b/contrib/virtiofsd/passthrough_ll.c
> > @@ -1780,7 +1780,11 @@ static void lo_getlk(fuse_req_t req, fuse_ino_t ino,
> >  		goto out;
> >  	}
> > 
> > +#ifdef F_OFD_GETLK
> >  	ret = fcntl(plock->fd, F_OFD_GETLK, lock);
> > +#else
> > +	ret = fcntl(plock->fd, F_GETLK, lock);
> > +#endif
> >  	if (ret == -1)
> >  		saverr = errno;
> > 
> > @@ -1831,7 +1835,11 @@ static void lo_setlk(fuse_req_t req, fuse_ino_t ino,
> > 
> >  	/* TODO: Is it alright to modify flock? */
> >  	lock->l_pid = 0;
> > +#ifdef F_OFD_SETLK
> >  	ret = fcntl(plock->fd, F_OFD_SETLK, lock);
> > +#else
> > +	ret = fcntl(plock->fd, F_GETLK, lock);
>                                ^^^^^^^
> 
> Typo! You've got GETLK rather than SETLK.
> 
> But, a bigger question - does this actually work!
> The manpage says:
>    'If a process closes any file descriptor referring to a file, then
>    all of the process's locks on that file are released, regardless of the
>    file descriptor(s) on which the locks were obtained.'
> 
> the fd we're using here came from lookup_create_plock_ctx which did
> a new openat to get this fd; so we've already got multiple fd's
> referring to this file; and thus I worry we're going to close
> one of them and lose all our locks on it.

Yeah, this is what makes F_GETLK/F_SETLK such an awful thing to
use. It is just about managable if an app is single threaded
and the developer can examine all code paths to make sure there
are no other open FDs referring to the same underling file.
If code has multiple FDs open, and/or is a multithreaded app,
F_SETLK is really fragile / error prone.

In QEMU proper, we used the fallback to F_GETLK/F_SETLK because
we were adding locking to existing features. If we didn't have
the fallback then we would either be breaking existing usage by
mandating OFD locks, or leaving those users with no locking at
all by disabling locking entirely.

For a program like virtiofsd that is brand new functionality
we don't have to worry about breaking existing users. Thus I
would strongly recommend we just mandate OFD locks, and entirely
disable the build of virtiofsd if OFD is missing on the host.

RHEL-7's 3.10 kernel *does* have OFD locking as it was backported
and QEMU in RHEL-7 already uses this. Users just need to make
sure they have updates applied to their RHEL-7 hosts to get this.

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 :|

Re: [Qemu-devel] [PATCH v2] virtiofsd: fix compile error if 'F_OFD_GETLK' not defined

[piaojun]

Hi Daniel and Dave,

On 2019/8/2 19:10, Daniel P. Berrangé wrote:
> On Fri, Aug 02, 2019 at 11:53:52AM +0100, Dr. David Alan Gilbert wrote:
>> * piaojun (piaojun@huawei.com) wrote:
>>> Use F_GETLK for fcntl when F_OFD_GETLK not defined, such as kernel 3.10.
>>>
>>> Signed-off-by: Jun Piao <piaojun@huawei.com>
>>
>>
>>> ---
>>> v2:
>>> - Use F_OFD_SETLK to replace F_OFD_GETLK in #ifdef.
>>>
>>> ---
>>>  contrib/virtiofsd/passthrough_ll.c | 8 ++++++++
>>>  1 file changed, 8 insertions(+)
>>>
>>> diff --git a/contrib/virtiofsd/passthrough_ll.c b/contrib/virtiofsd/passthrough_ll.c
>>> index a81c01d..c69f2f3 100644
>>> --- a/contrib/virtiofsd/passthrough_ll.c
>>> +++ b/contrib/virtiofsd/passthrough_ll.c
>>> @@ -1780,7 +1780,11 @@ static void lo_getlk(fuse_req_t req, fuse_ino_t ino,
>>>  		goto out;
>>>  	}
>>>
>>> +#ifdef F_OFD_GETLK
>>>  	ret = fcntl(plock->fd, F_OFD_GETLK, lock);
>>> +#else
>>> +	ret = fcntl(plock->fd, F_GETLK, lock);
>>> +#endif
>>>  	if (ret == -1)
>>>  		saverr = errno;
>>>
>>> @@ -1831,7 +1835,11 @@ static void lo_setlk(fuse_req_t req, fuse_ino_t ino,
>>>
>>>  	/* TODO: Is it alright to modify flock? */
>>>  	lock->l_pid = 0;
>>> +#ifdef F_OFD_SETLK
>>>  	ret = fcntl(plock->fd, F_OFD_SETLK, lock);
>>> +#else
>>> +	ret = fcntl(plock->fd, F_GETLK, lock);
>>                                ^^^^^^^
>>
>> Typo! You've got GETLK rather than SETLK.

Yes, it's a shame for the mistake.

>>
>> But, a bigger question - does this actually work!
>> The manpage says:
>>    'If a process closes any file descriptor referring to a file, then
>>    all of the process's locks on that file are released, regardless of the
>>    file descriptor(s) on which the locks were obtained.'
>>
>> the fd we're using here came from lookup_create_plock_ctx which did
>> a new openat to get this fd; so we've already got multiple fd's
>> referring to this file; and thus I worry we're going to close
>> one of them and lose all our locks on it.
> 
> Yeah, this is what makes F_GETLK/F_SETLK such an awful thing to
> use. It is just about managable if an app is single threaded
> and the developer can examine all code paths to make sure there
> are no other open FDs referring to the same underling file.
> If code has multiple FDs open, and/or is a multithreaded app,
> F_SETLK is really fragile / error prone.
> 
> In QEMU proper, we used the fallback to F_GETLK/F_SETLK because
> we were adding locking to existing features. If we didn't have
> the fallback then we would either be breaking existing usage by
> mandating OFD locks, or leaving those users with no locking at
> all by disabling locking entirely.
> 
> For a program like virtiofsd that is brand new functionality
> we don't have to worry about breaking existing users. Thus I
> would strongly recommend we just mandate OFD locks, and entirely
> disable the build of virtiofsd if OFD is missing on the host.
> 
> RHEL-7's 3.10 kernel *does* have OFD locking as it was backported
> and QEMU in RHEL-7 already uses this. Users just need to make
> sure they have updates applied to their RHEL-7 hosts to get this.

I checked the linux kernel commit history and found that F_OFD_SETLK was
introduced at v3.15 as below:

https://github.com/torvalds/linux/commit/0d3f7a2dd2f5cf9642982515e020c1aee2cf7af6

So maybe I need update my kernel to fit this new macro, and as you said,
most users will compile virtiofsd based on new kernel, that seems not a
big deal. At last, thanks for your detailed explanation.

Thanks,
Jun

> 
> Regards,
> Daniel
> 

Re: [Qemu-devel] [Virtio-fs] [PATCH v2] virtiofsd: fix compile error if 'F_OFD_GETLK' not defined

[Vivek Goyal]

On Fri, Aug 02, 2019 at 11:53:52AM +0100, Dr. David Alan Gilbert wrote:
> * piaojun (piaojun@huawei.com) wrote:
> > Use F_GETLK for fcntl when F_OFD_GETLK not defined, such as kernel 3.10.
> > 
> > Signed-off-by: Jun Piao <piaojun@huawei.com>
> 
> 
> > ---
> > v2:
> > - Use F_OFD_SETLK to replace F_OFD_GETLK in #ifdef.
> > 
> > ---
> >  contrib/virtiofsd/passthrough_ll.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/contrib/virtiofsd/passthrough_ll.c b/contrib/virtiofsd/passthrough_ll.c
> > index a81c01d..c69f2f3 100644
> > --- a/contrib/virtiofsd/passthrough_ll.c
> > +++ b/contrib/virtiofsd/passthrough_ll.c
> > @@ -1780,7 +1780,11 @@ static void lo_getlk(fuse_req_t req, fuse_ino_t ino,
> >  		goto out;
> >  	}
> > 
> > +#ifdef F_OFD_GETLK
> >  	ret = fcntl(plock->fd, F_OFD_GETLK, lock);
> > +#else
> > +	ret = fcntl(plock->fd, F_GETLK, lock);
> > +#endif
> >  	if (ret == -1)
> >  		saverr = errno;
> > 
> > @@ -1831,7 +1835,11 @@ static void lo_setlk(fuse_req_t req, fuse_ino_t ino,
> > 
> >  	/* TODO: Is it alright to modify flock? */
> >  	lock->l_pid = 0;
> > +#ifdef F_OFD_SETLK
> >  	ret = fcntl(plock->fd, F_OFD_SETLK, lock);
> > +#else
> > +	ret = fcntl(plock->fd, F_GETLK, lock);
>                                ^^^^^^^
> 
> Typo! You've got GETLK rather than SETLK.
> 
> But, a bigger question - does this actually work!
> The manpage says:
>    'If a process closes any file descriptor referring to a file, then
>    all of the process's locks on that file are released, regardless of the
>    file descriptor(s) on which the locks were obtained.'
> 
> the fd we're using here came from lookup_create_plock_ctx which did
> a new openat to get this fd; so we've already got multiple fd's
> referring to this file; and thus I worry we're going to close
> one of them and lose all our locks on it.

Right, we can't use F_GETLK/F_SETLK here. We are emulating posix locks
using open file description locks (OFD locks).

So having OFD locks will probably be one of the requirements on host.

Also we need to look into fuse support of OFD locks in general. It
might require little work to enable it.

Thanks
Vivek