Bug 2057882

Well, the only thing that libvirt can do is improve the error message potentially. Otherwise libvirt can't ignore user provided configuration.

(In reply to Peter Krempa from comment #1)
> Well, the only thing that libvirt can do is improve the error message
> potentially. Otherwise libvirt can't ignore user provided configuration.

Hi Peter, Is libvirt launch the virtiofsd process? If that's the truth, can libvirt drop the '-o flock', then the process can start successfully? then the VM can start successfully as before?

Sorry, I finally realized that "<lock posix='on' flock='on'/>" is set by myself in the xml. If we use "<lock posix='off' flock='off'/>" or ignore this setting, vm can start successfully.

 <filesystem type='mount' accessmode='passthrough'>
      <driver type='virtiofs'/>
      <binary path='/usr/libexec/virtiofsd' xattr='on'>
        <cache mode='none'/>
        <lock posix='off' flock='off'/>
      </binary>
...
# virsh start avocado-vt-vm1
Domain 'avocado-vt-vm1' started

# ps aux | grep virtiofsd
root     1759570  0.0  0.0   5676  1436 ?        S    04:18   0:00 /usr/libexec/virtiofsd --fd=24 -o source=/path1,cache=none,xattr,no_flock,no_posix_lock
root     1759572  0.0  0.0 140876  2492 ?        Sl   04:18   0:00 /usr/libexec/virtiofsd --fd=24 -o source=/path1,cache=none,xattr,no_flock,no_posix_lock

when ignore with:
<binary path='/usr/libexec/virtiofsd' xattr='on'>
        <cache mode='none'/>
      </binary>
guest start successfully with:
# ps aux  | grep virtiofsd | grep -v grep
root     1759789  0.0  0.0   5676  1436 ?        S    04:20   0:00 /usr/libexec/virtiofsd --fd=24 -o source=/path1,cache=none,xattr
root     1759792  0.0  0.0 140876  2392 ?        Sl   04:20   0:00 /usr/libexec/virtiofsd --fd=24 -o source=/path1,cache=none,xattr

I think we might need a fix in virtiofsd.

error: Invalid compat argument '-o flock'

Looks like this compatibility argument might not be implemented in rust-virtiofsd. I think that's the first problem we need to fix. Daemon should not die.

We do not support blocking flock/posix_locks. And that's why these are disabled by default.

If user does choose to enable "-flock" and then calls a blocking operation, then daemon returns -EOPNOTSUPP.

So definitely daemon should not die.

Thinking more about it. We probably should not support remote "flock" or "posix_lock" at all and fail early. Just supporting half the functionality is not very useful. Applications will start failing later. So may be we should fail early saying posix_lock/flock are not supported.

I just tested with upstream rust virtiofsd and it accepts "-o flock" and continues to run and does not crash. If we completely block it upstream, then people will complain about regression. So may be don't do anything as it is off by default.

(In reply to Vivek Goyal from comment #5)
> I just tested with upstream rust virtiofsd and it accepts "-o flock" and
> continues to run and does not crash. If we completely block it upstream,
> then people will complain about regression. So may be don't do anything as
> it is off by default.

That is strange, I don't see the same behavior, the upstream Rust virtiofsd only accepts '-o no_flock' and '-o no_posix_lock'.

What do you see when you specify "-o flock". For me it did not complain.

Starting program: /root/vgoyal/git/rhvgoyal-qemu/build/tools/virtiofsd/virtiofsd --socket-path=/tmp/vhostqemu -o source=/mnt/intel-ssd-p1/virtiofs-source/ -o cache=auto -o xattr -o no_allow_direct_io -d -o sandbox=chroot -o no_killpriv_v2 -o xattrmap=:prefix:all:security.selinux:trusted.virtiofsd.::bad:client:trusted.virtiofsd.:::bad:server::security.selinux::ok:all::: -o modcaps=+sys_admin -o security_label -o flock --rlimit-nofile=1048576

My bad. In my scripts I was using C virtiofsd. I will switch to rust virtiofsd and test again.

Ok, finally I have rust version running and I see.

"error: Invalid compat argument '-o flock'"

And if I pass "--flock". I see.

error: Found argument 'flock' which wasn't expected, or isn't valid in this context

USAGE:
    virtiofsd -d -o <compat-options>... --fd <fd> --socket <socket> --socket-path <socket-path>

So current behavior is along the expected lines that we don't support flock yet.

There is a chance that some people might complain that it is not a complete drop in replacement of C version. I guess we can push back saying without blocking lock support, its not very useful anyway. Lets see how does it go.

(In reply to Vivek Goyal from comment #9)

> 
> There is a chance that some people might complain that it is not a complete
> drop in replacement of C version. I guess we can push back saying without
> blocking lock support, its not very useful anyway. Lets see how does it go.

Vivek, given the above, should we WILLNOTFIX this bug? Or at lease lower the priority significantly?

Let us make sure libvirt is not enabling posix_lock and flock on by default.

<lock posix='on' flock='on'/>

If libvirt enables it by default, I think that should be fixed. Rust version does not support it at all. And C version only support non-blocking posix locks.

If this is not a default in libvirt, we can close this bug saying WILLNOTFIX.

(In reply to Vivek Goyal from comment #11)
> Let us make sure libvirt is not enabling posix_lock and flock on by default.
> 
> <lock posix='on' flock='on'/>
> 
> If libvirt enables it by default, I think that should be fixed. Rust version
> does not support it at all. And C version only support non-blocking posix
> locks.
> 
> If this is not a default in libvirt, we can close this bug saying WILLNOTFIX.

Bouncing this needingo back to Jan...

Libvirt does not add any posix_lock or flock options by default.
Only when the user specifies it in the XML.

Good to hear that libvirt does not enable (or recommend) to enable remote posix/flock by default.

I am looking at the original bug description again.

Expected results:
virtiofsd process should drop '-o flock' or reject enabling it, and VM should start successfully.

I think this is not a fair expectation given how code is structured. If user passes an option to virtiofsd which is not supported, dameon fails. It does not fallback to not enabling it automatically.

I think only exception at this point of time is option --inode-file-handles which allows one to specify never/prefer/mandatory. And in case of prefer we will fallback to not enabling it.

We want to add these "prefer" kind of options very carefully and add it if it really makes lot of sense. In case of enabling remote posix/flock, I can't see how it makes sense.

Bottom line, I think we will not fix this. If user passes "-o flock" or "-o posix_lock", virtiofsd will exit with an error (rust version) as it does not support these options.

(In reply to Vivek Goyal from comment #14)
> Good to hear that libvirt does not enable (or recommend) to enable remote
> posix/flock by default.
> 
> I am looking at the original bug description again.
> 
> Expected results:
> virtiofsd process should drop '-o flock' or reject enabling it, and VM
> should start successfully.
> 
> I think this is not a fair expectation given how code is structured. If user
> passes an option to virtiofsd which is not supported, dameon fails. It does
> not fallback to not enabling it automatically.
> 
> I think only exception at this point of time is option --inode-file-handles
> which allows one to specify never/prefer/mandatory. And in case of prefer we
> will fallback to not enabling it.
> 
> We want to add these "prefer" kind of options very carefully and add it if
> it really makes lot of sense. In case of enabling remote posix/flock, I
> can't see how it makes sense.
> 
> Bottom line, I think we will not fix this. If user passes "-o flock" or "-o
> posix_lock", virtiofsd will exit with an error (rust version) as it does not
> support these options.

I'll go even further and say this is not a bug, as we at the XML level we probably shouldn't be trying to prevent the user from having complete control (and eventually shooting themselves in the foot).