From: Vivek Goyal <vgoyal@redhat.com>
To: Max Reitz <mreitz@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 10/10] virtiofsd: Add lazy lo_do_find()
Date: Mon, 9 Aug 2021 15:08:51 -0400 [thread overview]
Message-ID: <YRF9Q+MSjrbeybd2@redhat.com> (raw)
In-Reply-To: <20210730150134.216126-11-mreitz@redhat.com>
On Fri, Jul 30, 2021 at 05:01:34PM +0200, Max Reitz wrote:
> lo_find() right now takes two lookup keys for two maps, namely the file
> handle for inodes_by_handle and the statx information for inodes_by_ids.
> However, we only need the statx information if looking up the inode by
> the file handle failed.
>
> There are two callers of lo_find(): The first one, lo_do_lookup(), has
> both keys anyway, so passing them does not incur any additional cost.
> The second one, lookup_name(), though, needs to explicitly invoke
> name_to_handle_at() (through get_file_handle()) and statx() (through
> do_statx()). We need to try to get a file handle as the primary key, so
> we cannot get rid of get_file_handle(), but we only need the statx
> information if looking up an inode by handle failed; so we can defer
> that until the lookup has indeed failed.
So IIUC, this patch seems to be all about avoiding do_statx()
call in lookup_name() if file handle could be successfully
generated.
So can't we just not modify lookup_name() to not call statx()
if file handle could be generated. And also modfiy lo_find()
to use st/mnt_id only if fhandle==NULL.
That probably is much simpler change as compared to passing function
pointers around.
Vivek
>
> To this end, replace lo_find()'s st/mnt_id parameters by a get_ids()
> closure that is invoked to fill the lo_key struct if necessary.
>
> Also, lo_find() is renamed to lo_do_find(), so we can add a new
> lo_find() wrapper whose closure just initializes the lo_key from the
> st/mnt_id parameters, just like the old lo_find() did.
>
> lookup_name() directly calls lo_do_find() now and passes its own
> closure, which performs the do_statx() call.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> tools/virtiofsd/passthrough_ll.c | 93 ++++++++++++++++++++++++++------
> 1 file changed, 76 insertions(+), 17 deletions(-)
>
> diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> index ac95961d12..41e9f53878 100644
> --- a/tools/virtiofsd/passthrough_ll.c
> +++ b/tools/virtiofsd/passthrough_ll.c
> @@ -1200,22 +1200,23 @@ out_err:
> fuse_reply_err(req, saverr);
> }
>
> -static struct lo_inode *lo_find(struct lo_data *lo,
> - const struct lo_fhandle *fhandle,
> - struct stat *st, uint64_t mnt_id)
> +/*
> + * get_ids() will be called to get the key for lo->inodes_by_ids if
> + * the lookup by file handle has failed.
> + */
> +static struct lo_inode *lo_do_find(struct lo_data *lo,
> + const struct lo_fhandle *fhandle,
> + int (*get_ids)(struct lo_key *, const void *),
> + const void *get_ids_opaque)
> {
> struct lo_inode *p = NULL;
> - struct lo_key ids_key = {
> - .ino = st->st_ino,
> - .dev = st->st_dev,
> - .mnt_id = mnt_id,
> - };
> + struct lo_key ids_key;
>
> pthread_mutex_lock(&lo->mutex);
> if (fhandle) {
> p = g_hash_table_lookup(lo->inodes_by_handle, fhandle);
> }
> - if (!p) {
> + if (!p && get_ids(&ids_key, get_ids_opaque) == 0) {
> p = g_hash_table_lookup(lo->inodes_by_ids, &ids_key);
> /*
> * When we had to fall back to looking up an inode by its
> @@ -1244,6 +1245,36 @@ static struct lo_inode *lo_find(struct lo_data *lo,
> return p;
> }
>
> +struct lo_find_get_ids_key_opaque {
> + const struct stat *st;
> + uint64_t mnt_id;
> +};
> +
> +static int lo_find_get_ids_key(struct lo_key *ids_key, const void *opaque)
> +{
> + const struct lo_find_get_ids_key_opaque *stat_info = opaque;
> +
> + *ids_key = (struct lo_key){
> + .ino = stat_info->st->st_ino,
> + .dev = stat_info->st->st_dev,
> + .mnt_id = stat_info->mnt_id,
> + };
> +
> + return 0;
> +}
> +
> +static struct lo_inode *lo_find(struct lo_data *lo,
> + const struct lo_fhandle *fhandle,
> + struct stat *st, uint64_t mnt_id)
> +{
> + const struct lo_find_get_ids_key_opaque stat_info = {
> + .st = st,
> + .mnt_id = mnt_id,
> + };
> +
> + return lo_do_find(lo, fhandle, lo_find_get_ids_key, &stat_info);
> +}
> +
> /* value_destroy_func for posix_locks GHashTable */
> static void posix_locks_value_destroy(gpointer data)
> {
> @@ -1769,14 +1800,41 @@ out_err:
> fuse_reply_err(req, saverr);
> }
>
> +struct lookup_name_get_ids_key_opaque {
> + struct lo_data *lo;
> + int parent_fd;
> + const char *name;
> +};
> +
> +static int lookup_name_get_ids_key(struct lo_key *ids_key, const void *opaque)
> +{
> + const struct lookup_name_get_ids_key_opaque *stat_params = opaque;
> + uint64_t mnt_id;
> + struct stat attr;
> + int res;
> +
> + res = do_statx(stat_params->lo, stat_params->parent_fd, stat_params->name,
> + &attr, AT_SYMLINK_NOFOLLOW, &mnt_id);
> + if (res < 0) {
> + return -errno;
> + }
> +
> + *ids_key = (struct lo_key){
> + .ino = attr.st_ino,
> + .dev = attr.st_dev,
> + .mnt_id = mnt_id,
> + };
> +
> + return 0;
> +}
> +
> /* Increments nlookup and caller must release refcount using lo_inode_put() */
> static struct lo_inode *lookup_name(fuse_req_t req, fuse_ino_t parent,
> const char *name)
> {
> g_auto(TempFd) dir_fd = TEMP_FD_INIT;
> int res;
> - uint64_t mnt_id;
> - struct stat attr;
> + struct lookup_name_get_ids_key_opaque stat_params;
> struct lo_fhandle *fh;
> struct lo_data *lo = lo_data(req);
> struct lo_inode *dir = lo_inode(req, parent);
> @@ -1794,12 +1852,13 @@ static struct lo_inode *lookup_name(fuse_req_t req, fuse_ino_t parent,
> fh = get_file_handle(lo, dir_fd.fd, name);
> /* Ignore errors, this is just an optional key for the lookup */
>
> - res = do_statx(lo, dir_fd.fd, name, &attr, AT_SYMLINK_NOFOLLOW, &mnt_id);
> - if (res == -1) {
> - goto out;
> - }
> -
> - inode = lo_find(lo, fh, &attr, mnt_id);
> + stat_params = (struct lookup_name_get_ids_key_opaque){
> + .lo = lo,
> + .parent_fd = dir_fd.fd,
> + .name = name,
> + };
> + inode = lo_do_find(lo, fh, lookup_name_get_ids_key, &stat_params);
> + lo_inode_put(lo, &dir);
> g_free(fh);
>
> out:
> --
> 2.31.1
>
next prev parent reply other threads:[~2021-08-09 19:10 UTC|newest]
Thread overview: 44+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-07-30 15:01 [PATCH v3 00/10] virtiofsd: Allow using file handles instead of O_PATH FDs Max Reitz
2021-07-30 15:01 ` [PATCH v3 01/10] virtiofsd: Limit setxattr()'s creds-dropped region Max Reitz
2021-08-06 14:16 ` Vivek Goyal
2021-08-09 10:30 ` Max Reitz
2021-07-30 15:01 ` [PATCH v3 02/10] virtiofsd: Add TempFd structure Max Reitz
2021-08-06 14:41 ` Vivek Goyal
2021-08-09 10:44 ` Max Reitz
2021-07-30 15:01 ` [PATCH v3 03/10] virtiofsd: Use lo_inode_open() instead of openat() Max Reitz
2021-08-06 15:42 ` Vivek Goyal
2021-07-30 15:01 ` [PATCH v3 04/10] virtiofsd: Add lo_inode_fd() helper Max Reitz
2021-08-06 18:25 ` Vivek Goyal
2021-08-09 10:48 ` Max Reitz
2021-07-30 15:01 ` [PATCH v3 05/10] virtiofsd: Let lo_fd() return a TempFd Max Reitz
2021-07-30 15:01 ` [PATCH v3 06/10] virtiofsd: Let lo_inode_open() " Max Reitz
2021-08-06 19:55 ` Vivek Goyal
2021-08-09 13:40 ` Max Reitz
2021-07-30 15:01 ` [PATCH v3 07/10] virtiofsd: Add lo_inode.fhandle Max Reitz
2021-08-09 15:21 ` Vivek Goyal
2021-08-09 16:41 ` Hanna Reitz
2021-07-30 15:01 ` [PATCH v3 08/10] virtiofsd: Add inodes_by_handle hash table Max Reitz
2021-08-09 16:10 ` Vivek Goyal
2021-08-09 16:47 ` Hanna Reitz
2021-08-10 14:07 ` Vivek Goyal
2021-08-10 14:13 ` Hanna Reitz
2021-08-10 17:51 ` Vivek Goyal
2021-07-30 15:01 ` [PATCH v3 09/10] virtiofsd: Optionally fill lo_inode.fhandle Max Reitz
2021-08-09 18:41 ` Vivek Goyal
2021-08-10 8:32 ` Hanna Reitz
2021-08-10 15:23 ` Vivek Goyal
2021-08-10 15:26 ` Hanna Reitz
2021-08-10 15:57 ` Vivek Goyal
2021-08-11 6:41 ` Hanna Reitz
2021-08-16 19:44 ` Vivek Goyal
2021-08-17 8:27 ` Hanna Reitz
2021-08-17 19:45 ` Vivek Goyal
2021-08-18 0:14 ` Vivek Goyal
2021-08-18 13:32 ` Vivek Goyal
2021-08-18 13:48 ` Hanna Reitz
2021-08-19 16:38 ` Dr. David Alan Gilbert
2021-07-30 15:01 ` [PATCH v3 10/10] virtiofsd: Add lazy lo_do_find() Max Reitz
2021-08-09 19:08 ` Vivek Goyal [this message]
2021-08-10 8:38 ` Hanna Reitz
2021-08-10 14:12 ` Vivek Goyal
2021-08-10 14:17 ` Hanna Reitz
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YRF9Q+MSjrbeybd2@redhat.com \
--to=vgoyal@redhat.com \
--cc=dgilbert@redhat.com \
--cc=mreitz@redhat.com \
--cc=qemu-devel@nongnu.org \
--cc=stefanha@redhat.com \
--cc=virtio-fs@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).