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 (address@hidden) wrote:
>>> Use F_GETLK for fcntl when F_OFD_GETLK not defined, such as kernel 3.10.
>>>
>>> Signed-off-by: Jun Piao <address@hidden>
>>
>>
>>> ---
>>> 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
>