Re: [PATCH v3 07/10] virtiofsd: Add lo_inode.fhandle

From: Hanna Reitz <hreitz@redhat.com>
To: Vivek Goyal <vgoyal@redhat.com>
Cc: virtio-fs@redhat.com, qemu-devel@nongnu.org,
	Stefan Hajnoczi <stefanha@redhat.com>,
	"Dr . David Alan Gilbert" <dgilbert@redhat.com>
Subject: Re: [PATCH v3 07/10] virtiofsd: Add lo_inode.fhandle
Date: Mon, 9 Aug 2021 18:41:59 +0200	[thread overview]
Message-ID: <cbf910d8-47cd-f9b1-c9ac-6e0e2036c04a@redhat.com> (raw)
In-Reply-To: <YRFH+vE30fwgGmu8@redhat.com>

On 09.08.21 17:21, Vivek Goyal wrote:
> On Fri, Jul 30, 2021 at 05:01:31PM +0200, Max Reitz wrote:
>> This new field is an alternative to lo_inode.fd: Either of the two must
>> be set.  In case an O_PATH FD is needed for some lo_inode, it is either
>> taken from lo_inode.fd, if valid, or a temporary FD is opened with
>> open_by_handle_at().
>>
>> Using a file handle instead of an FD has the advantage of keeping the
>> number of open file descriptors low.
>>
>> Because open_by_handle_at() requires a mount FD (i.e. a non-O_PATH FD
>> opened on the filesystem to which the file handle refers), but every
>> lo_fhandle only has a mount ID (as returned by name_to_handle_at()), we
>> keep a hash map of such FDs in mount_fds (mapping ID to FD).
>> get_file_handle(), which is added by a later patch, will ensure that
>> every mount ID for which we have generated a handle has a corresponding
>> entry in mount_fds.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> Reviewed-by: Connor Kuehl <ckuehl@redhat.com>
>> ---
>>   tools/virtiofsd/passthrough_ll.c      | 116 ++++++++++++++++++++++----
>>   tools/virtiofsd/passthrough_seccomp.c |   1 +
>>   2 files changed, 102 insertions(+), 15 deletions(-)
>>
>> diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
>> index 292b7f7e27..487448d666 100644
>> --- a/tools/virtiofsd/passthrough_ll.c
>> +++ b/tools/virtiofsd/passthrough_ll.c
>> @@ -88,8 +88,25 @@ struct lo_key {
>>       uint64_t mnt_id;
>>   };
>>   
>> +struct lo_fhandle {
>> +    union {
>> +        struct file_handle handle;
>> +        char padding[sizeof(struct file_handle) + MAX_HANDLE_SZ];
>> +    };
>> +    int mount_id;
>> +};
>> +
>> +/* Maps mount IDs to an FD that we can pass to open_by_handle_at() */
>> +static GHashTable *mount_fds;
>> +pthread_rwlock_t mount_fds_lock = PTHREAD_RWLOCK_INITIALIZER;
>> +
> How about if we move this hash table inside "struct lo_data". That seems to
> be one global data structure keeping all the info. Also it can be
> cleaned up during lo_destroy().

Yes, sounds good and right, will do.

Hanna