[PATCH v2 0/9] virtiofsd: Allow using file handles instead of O_PATH FDs

Max Reitz posted 9 patches 2 years, 9 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/next-importer-push tags/patchew/20210609155551.44437-1-mreitz@redhat.com
Maintainers: "Dr. David Alan Gilbert" <dgilbert@redhat.com>, Stefan Hajnoczi <stefanha@redhat.com>
There is a newer version of this series
tools/virtiofsd/helper.c              |   3 +
tools/virtiofsd/passthrough_ll.c      | 836 +++++++++++++++++++++-----
tools/virtiofsd/passthrough_seccomp.c |   2 +
3 files changed, 694 insertions(+), 147 deletions(-)
[PATCH v2 0/9] virtiofsd: Allow using file handles instead of O_PATH FDs
Posted by Max Reitz 2 years, 9 months ago
Hi,

v1 cover letter for an overview:
https://listman.redhat.com/archives/virtio-fs/2021-June/msg00033.html

In v2, I (tried to) fix the bug Dave found, which is that
get_file_handle() indiscriminately opened the given dirfd/name
combination to get an O_RDONLY fd without checking whether we’re
actually allowed to open dirfd/name; namely, we don’t allow ourselves to
open files that aren’t regular files or directories.

So that openat(.., O_RDONLY) is changed to an openat(..., O_PATH), and
then check the file type with the statx() we’re doing anyway.  If the
file is OK to open, we reopen it O_RDONLY with the help of
/proc/self/fd, like we always do.

(This only affects patch 8.)


git-backport-diff against v1:

Key:
[----] : patches are identical
[####] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively

001/9:[----] [--] 'virtiofsd: Add TempFd structure'
002/9:[----] [--] 'virtiofsd: Use lo_inode_open() instead of openat()'
003/9:[----] [--] 'virtiofsd: Add lo_inode_fd() helper'
004/9:[----] [--] 'virtiofsd: Let lo_fd() return a TempFd'
005/9:[----] [--] 'virtiofsd: Let lo_inode_open() return a TempFd'
006/9:[----] [--] 'virtiofsd: Add lo_inode.fhandle'
007/9:[----] [--] 'virtiofsd: Add inodes_by_handle hash table'
008/9:[0045] [FC] 'virtiofsd: Optionally fill lo_inode.fhandle'
009/9:[----] [--] 'virtiofsd: Add lazy lo_do_find()'


Max Reitz (9):
  virtiofsd: Add TempFd structure
  virtiofsd: Use lo_inode_open() instead of openat()
  virtiofsd: Add lo_inode_fd() helper
  virtiofsd: Let lo_fd() return a TempFd
  virtiofsd: Let lo_inode_open() return a TempFd
  virtiofsd: Add lo_inode.fhandle
  virtiofsd: Add inodes_by_handle hash table
  virtiofsd: Optionally fill lo_inode.fhandle
  virtiofsd: Add lazy lo_do_find()

 tools/virtiofsd/helper.c              |   3 +
 tools/virtiofsd/passthrough_ll.c      | 836 +++++++++++++++++++++-----
 tools/virtiofsd/passthrough_seccomp.c |   2 +
 3 files changed, 694 insertions(+), 147 deletions(-)

-- 
2.31.1


Re: [PATCH v2 0/9] virtiofsd: Allow using file handles instead of O_PATH FDs
Posted by Vivek Goyal 2 years, 9 months ago
On Wed, Jun 09, 2021 at 05:55:42PM +0200, Max Reitz wrote:
> Hi,
> 
> v1 cover letter for an overview:
> https://listman.redhat.com/archives/virtio-fs/2021-June/msg00033.html

Hi Max,

What's the impact of these patches on performance? Just trying to 
get some idea what to expect. Performance remains more or less
same or we expect a hit.

Thanks
Vivek

> 
> In v2, I (tried to) fix the bug Dave found, which is that
> get_file_handle() indiscriminately opened the given dirfd/name
> combination to get an O_RDONLY fd without checking whether we’re
> actually allowed to open dirfd/name; namely, we don’t allow ourselves to
> open files that aren’t regular files or directories.
> 
> So that openat(.., O_RDONLY) is changed to an openat(..., O_PATH), and
> then check the file type with the statx() we’re doing anyway.  If the
> file is OK to open, we reopen it O_RDONLY with the help of
> /proc/self/fd, like we always do.
> 
> (This only affects patch 8.)
> 
> 
> git-backport-diff against v1:
> 
> Key:
> [----] : patches are identical
> [####] : number of functional differences between upstream/downstream patch
> [down] : patch is downstream-only
> The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively
> 
> 001/9:[----] [--] 'virtiofsd: Add TempFd structure'
> 002/9:[----] [--] 'virtiofsd: Use lo_inode_open() instead of openat()'
> 003/9:[----] [--] 'virtiofsd: Add lo_inode_fd() helper'
> 004/9:[----] [--] 'virtiofsd: Let lo_fd() return a TempFd'
> 005/9:[----] [--] 'virtiofsd: Let lo_inode_open() return a TempFd'
> 006/9:[----] [--] 'virtiofsd: Add lo_inode.fhandle'
> 007/9:[----] [--] 'virtiofsd: Add inodes_by_handle hash table'
> 008/9:[0045] [FC] 'virtiofsd: Optionally fill lo_inode.fhandle'
> 009/9:[----] [--] 'virtiofsd: Add lazy lo_do_find()'
> 
> 
> Max Reitz (9):
>   virtiofsd: Add TempFd structure
>   virtiofsd: Use lo_inode_open() instead of openat()
>   virtiofsd: Add lo_inode_fd() helper
>   virtiofsd: Let lo_fd() return a TempFd
>   virtiofsd: Let lo_inode_open() return a TempFd
>   virtiofsd: Add lo_inode.fhandle
>   virtiofsd: Add inodes_by_handle hash table
>   virtiofsd: Optionally fill lo_inode.fhandle
>   virtiofsd: Add lazy lo_do_find()
> 
>  tools/virtiofsd/helper.c              |   3 +
>  tools/virtiofsd/passthrough_ll.c      | 836 +++++++++++++++++++++-----
>  tools/virtiofsd/passthrough_seccomp.c |   2 +
>  3 files changed, 694 insertions(+), 147 deletions(-)
> 
> -- 
> 2.31.1
> 
> 


Re: [PATCH v2 0/9] virtiofsd: Allow using file handles instead of O_PATH FDs
Posted by Max Reitz 2 years, 9 months ago
On 11.06.21 21:19, Vivek Goyal wrote:
> On Wed, Jun 09, 2021 at 05:55:42PM +0200, Max Reitz wrote:
>> Hi,
>>
>> v1 cover letter for an overview:
>> https://listman.redhat.com/archives/virtio-fs/2021-June/msg00033.html
> Hi Max,
>
> What's the impact of these patches on performance? Just trying to
> get some idea what to expect. Performance remains more or less
> same or we expect a hit.
I definitely expect a hit if you just benchmark the right way. Having to 
open FDs all the time (for metadata operations) will have an impact 
(when you do lookups all the time, or open files and close them 
immediately). How much of an impact, that’s completely up to the 
filesystem’s open_by_handle_at() implementation, I presume.

I don’t expect it to be significant for real-world use cases, though. 
When really doing I/O, there should be absolutely no difference, because 
then we’re operating with these FUSE handles that have actual (non-path) 
FDs attached to them.

Max