Re: [PATCH v4 10/12] virtiofsd: Add inodes_by_handle hash table
On Wed, Oct 20, 2021 at 04:10:51PM +0200, Hanna Reitz wrote:
> On 20.10.21 14:29, Vivek Goyal wrote:
> > On Wed, Oct 20, 2021 at 12:02:32PM +0200, Hanna Reitz wrote:
> > > On 19.10.21 22:02, Vivek Goyal wrote:
> > > > On Thu, Sep 16, 2021 at 10:40:43AM +0200, Hanna Reitz wrote:
> > > > > Currently, lo_inode.fhandle is always NULL and so always keep an
> > > > > O_PATH
> > > > > FD in lo_inode.fd. Therefore, when the respective inode is unlinked,
> > > > > its inode ID will remain in use until we drop our lo_inode (and
> > > > > lo_inode_put() thus closes the FD). Therefore, lo_find() can safely
> > > > > use
> > > > > the inode ID as an lo_inode key, because any inode with an inode ID we
> > > > > find in lo_data.inodes (on the same filesystem) must be the exact same
> > > > > file.
> > > > >
> > > > > This will change when we start setting lo_inode.fhandle so we do not
> > > > > have to keep an O_PATH FD open. Then, unlinking such an inode will
> > > > > immediately remove it, so its ID can then be reused by newly created
> > > > > files, even while the lo_inode object is still there[1].
> > > > >
> > > > > So creating a new file can then reuse the old file's inode ID, and
> > > > > looking up the new file would lead to us finding the old file's
> > > > > lo_inode, which is not ideal.
> > > > >
> > > > > Luckily, just as file handles cause this problem, they also solve it:
> > > > > A
> > > > > file handle contains a generation ID, which changes when an inode ID
> > > > > is
> > > > > reused, so the new file can be distinguished from the old one. So all
> > > > > we need to do is to add a second map besides lo_data.inodes that maps
> > > > > file handles to lo_inodes, namely lo_data.inodes_by_handle. For
> > > > > clarity, lo_data.inodes is renamed to lo_data.inodes_by_ids.
> > > > >
> > > > > Unfortunately, we cannot rely on being able to generate file handles
> > > > > every time. Therefore, we still enter every lo_inode object into
> > > > > inodes_by_ids, but having an entry in inodes_by_handle is optional. A
> > > > > potential inodes_by_handle entry then has precedence, the
> > > > > inodes_by_ids
> > > > > entry is just a fallback.
> > > > >
> > > > > Note that we do not generate lo_fhandle objects yet, and so we also do
> > > > > not enter anything into the inodes_by_handle map yet. Also, all
> > > > > lookups
> > > > > skip that map. We might manually create file handles with some code
> > > > > that is immediately removed by the next patch again, but that would
> > > > > break the assumption in lo_find() that every lo_inode with a non-NULL
> > > > > .fhandle must have an entry in inodes_by_handle and vice versa. So we
> > > > > leave actually using the inodes_by_handle map for the next patch.
> > > > >
> > > > > [1] If some application in the guest still has the file open, there is
> > > > > going to be a corresponding FD mapping in lo_data.fd_map. In such a
> > > > > case, the inode will only go away once every application in the guest
> > > > > has closed it. The problem described only applies to cases where the
> > > > > guest does not have the file open, and it is just in the dentry cache,
> > > > > basically.
> > > > >
> > > > > Signed-off-by: Hanna Reitz
> > > > > ---
> > > > >tools/virtiofsd/passthrough_ll.c | 81
> > > > > +---
> > > > >1 file changed, 65 insertions(+), 16 deletions(-)
> > > > >
> > > > > diff --git a/tools/virtiofsd/passthrough_ll.c
> > > > > b/tools/virtiofsd/passthrough_ll.c
> > > > > index bd8fc922ea..b7d6aa7f9d 100644
> > > > > --- a/tools/virtiofsd/passthrough_ll.c
> > > > > +++ b/tools/virtiofsd/passthrough_ll.c
> > > > > @@ -186,7 +186,8 @@ struct lo_data {
> > > > >int announce_submounts;
> > > > >bool use_statx;
> > > > >struct lo_inode root;
> > > > > -GHashTable *inodes; /* protected by lo->mutex */
> > > > > +GHashTable *inodes_by_ids; /* protected by lo->mutex */
> > > > > +GHashTable *inodes_by_handle; /* protected by lo->mutex */
> > > > >struct lo_map ino_map; /* protected by lo->mutex */
> > > > >struct lo_map dirp_map; /* protected by lo->mutex */
> > > > >struct lo_map fd_map; /* protected by lo->mutex */
> > > > > @@ -275,8 +276,9 @@ static struct {
> > > > >/* That we loaded cap-ng in the current thread from the saved */
> > > > >static __thread bool cap_loaded = 0;
> > > > > -static struct lo_inode *lo_find(struct lo_data *lo, struct stat *st,
> > > > > -uint64_t mnt_id);
> > > > > +static struct lo_inode *lo_find(struct lo_data *lo,
> > > > > +const struct lo_fhandle *fhandle,
> > > > > +struct stat *st, uint64_t mnt_id);
> > > > >static int xattr_map_client(const struct lo_data *lo, const char
> > > > > *client_name,
> > > > >char **out_name);
> > > > > @@ -1143,18 +1145,40 @@
Re: [PATCH v4 10/12] virtiofsd: Add inodes_by_handle hash table
On 20.10.21 14:29, Vivek Goyal wrote:
On Wed, Oct 20, 2021 at 12:02:32PM +0200, Hanna Reitz wrote:
On 19.10.21 22:02, Vivek Goyal wrote:
On Thu, Sep 16, 2021 at 10:40:43AM +0200, Hanna Reitz wrote:
Currently, lo_inode.fhandle is always NULL and so always keep an O_PATH
FD in lo_inode.fd. Therefore, when the respective inode is unlinked,
its inode ID will remain in use until we drop our lo_inode (and
lo_inode_put() thus closes the FD). Therefore, lo_find() can safely use
the inode ID as an lo_inode key, because any inode with an inode ID we
find in lo_data.inodes (on the same filesystem) must be the exact same
file.
This will change when we start setting lo_inode.fhandle so we do not
have to keep an O_PATH FD open. Then, unlinking such an inode will
immediately remove it, so its ID can then be reused by newly created
files, even while the lo_inode object is still there[1].
So creating a new file can then reuse the old file's inode ID, and
looking up the new file would lead to us finding the old file's
lo_inode, which is not ideal.
Luckily, just as file handles cause this problem, they also solve it: A
file handle contains a generation ID, which changes when an inode ID is
reused, so the new file can be distinguished from the old one. So all
we need to do is to add a second map besides lo_data.inodes that maps
file handles to lo_inodes, namely lo_data.inodes_by_handle. For
clarity, lo_data.inodes is renamed to lo_data.inodes_by_ids.
Unfortunately, we cannot rely on being able to generate file handles
every time. Therefore, we still enter every lo_inode object into
inodes_by_ids, but having an entry in inodes_by_handle is optional. A
potential inodes_by_handle entry then has precedence, the inodes_by_ids
entry is just a fallback.
Note that we do not generate lo_fhandle objects yet, and so we also do
not enter anything into the inodes_by_handle map yet. Also, all lookups
skip that map. We might manually create file handles with some code
that is immediately removed by the next patch again, but that would
break the assumption in lo_find() that every lo_inode with a non-NULL
.fhandle must have an entry in inodes_by_handle and vice versa. So we
leave actually using the inodes_by_handle map for the next patch.
[1] If some application in the guest still has the file open, there is
going to be a corresponding FD mapping in lo_data.fd_map. In such a
case, the inode will only go away once every application in the guest
has closed it. The problem described only applies to cases where the
guest does not have the file open, and it is just in the dentry cache,
basically.
Signed-off-by: Hanna Reitz
---
tools/virtiofsd/passthrough_ll.c | 81 +---
1 file changed, 65 insertions(+), 16 deletions(-)
diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index bd8fc922ea..b7d6aa7f9d 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -186,7 +186,8 @@ struct lo_data {
int announce_submounts;
bool use_statx;
struct lo_inode root;
-GHashTable *inodes; /* protected by lo->mutex */
+GHashTable *inodes_by_ids; /* protected by lo->mutex */
+GHashTable *inodes_by_handle; /* protected by lo->mutex */
struct lo_map ino_map; /* protected by lo->mutex */
struct lo_map dirp_map; /* protected by lo->mutex */
struct lo_map fd_map; /* protected by lo->mutex */
@@ -275,8 +276,9 @@ static struct {
/* That we loaded cap-ng in the current thread from the saved */
static __thread bool cap_loaded = 0;
-static struct lo_inode *lo_find(struct lo_data *lo, struct stat *st,
-uint64_t mnt_id);
+static struct lo_inode *lo_find(struct lo_data *lo,
+const struct lo_fhandle *fhandle,
+struct stat *st, uint64_t mnt_id);
static int xattr_map_client(const struct lo_data *lo, const char
*client_name,
char **out_name);
@@ -1143,18 +1145,40 @@ out_err:
fuse_reply_err(req, saverr);
}
-static struct lo_inode *lo_find(struct lo_data *lo, struct stat *st,
-uint64_t mnt_id)
+static struct lo_inode *lo_find(struct lo_data *lo,
+const struct lo_fhandle *fhandle,
+struct stat *st, uint64_t mnt_id)
{
-struct lo_inode *p;
-struct lo_key key = {
+struct lo_inode *p = NULL;
+struct lo_key ids_key = {
.ino = st->st_ino,
.dev = st->st_dev,
.mnt_id = mnt_id,
};
pthread_mutex_lock(>mutex);
-p = g_hash_table_lookup(lo->inodes, );
+if (fhandle) {
+p = g_hash_table_lookup(lo->inodes_by_handle, fhandle);
+}
+if (!p) {
+p = g_hash_table_lookup(lo->inodes_by_ids, _key);
+/*
+ * When we had to fall back to looking up an inode by its
+ * inode ID, ensure
Re: [PATCH v4 10/12] virtiofsd: Add inodes_by_handle hash table
On Wed, Oct 20, 2021 at 12:02:32PM +0200, Hanna Reitz wrote:
> On 19.10.21 22:02, Vivek Goyal wrote:
> > On Thu, Sep 16, 2021 at 10:40:43AM +0200, Hanna Reitz wrote:
> > > Currently, lo_inode.fhandle is always NULL and so always keep an O_PATH
> > > FD in lo_inode.fd. Therefore, when the respective inode is unlinked,
> > > its inode ID will remain in use until we drop our lo_inode (and
> > > lo_inode_put() thus closes the FD). Therefore, lo_find() can safely use
> > > the inode ID as an lo_inode key, because any inode with an inode ID we
> > > find in lo_data.inodes (on the same filesystem) must be the exact same
> > > file.
> > >
> > > This will change when we start setting lo_inode.fhandle so we do not
> > > have to keep an O_PATH FD open. Then, unlinking such an inode will
> > > immediately remove it, so its ID can then be reused by newly created
> > > files, even while the lo_inode object is still there[1].
> > >
> > > So creating a new file can then reuse the old file's inode ID, and
> > > looking up the new file would lead to us finding the old file's
> > > lo_inode, which is not ideal.
> > >
> > > Luckily, just as file handles cause this problem, they also solve it: A
> > > file handle contains a generation ID, which changes when an inode ID is
> > > reused, so the new file can be distinguished from the old one. So all
> > > we need to do is to add a second map besides lo_data.inodes that maps
> > > file handles to lo_inodes, namely lo_data.inodes_by_handle. For
> > > clarity, lo_data.inodes is renamed to lo_data.inodes_by_ids.
> > >
> > > Unfortunately, we cannot rely on being able to generate file handles
> > > every time. Therefore, we still enter every lo_inode object into
> > > inodes_by_ids, but having an entry in inodes_by_handle is optional. A
> > > potential inodes_by_handle entry then has precedence, the inodes_by_ids
> > > entry is just a fallback.
> > >
> > > Note that we do not generate lo_fhandle objects yet, and so we also do
> > > not enter anything into the inodes_by_handle map yet. Also, all lookups
> > > skip that map. We might manually create file handles with some code
> > > that is immediately removed by the next patch again, but that would
> > > break the assumption in lo_find() that every lo_inode with a non-NULL
> > > .fhandle must have an entry in inodes_by_handle and vice versa. So we
> > > leave actually using the inodes_by_handle map for the next patch.
> > >
> > > [1] If some application in the guest still has the file open, there is
> > > going to be a corresponding FD mapping in lo_data.fd_map. In such a
> > > case, the inode will only go away once every application in the guest
> > > has closed it. The problem described only applies to cases where the
> > > guest does not have the file open, and it is just in the dentry cache,
> > > basically.
> > >
> > > Signed-off-by: Hanna Reitz
> > > ---
> > > tools/virtiofsd/passthrough_ll.c | 81 +---
> > > 1 file changed, 65 insertions(+), 16 deletions(-)
> > >
> > > diff --git a/tools/virtiofsd/passthrough_ll.c
> > > b/tools/virtiofsd/passthrough_ll.c
> > > index bd8fc922ea..b7d6aa7f9d 100644
> > > --- a/tools/virtiofsd/passthrough_ll.c
> > > +++ b/tools/virtiofsd/passthrough_ll.c
> > > @@ -186,7 +186,8 @@ struct lo_data {
> > > int announce_submounts;
> > > bool use_statx;
> > > struct lo_inode root;
> > > -GHashTable *inodes; /* protected by lo->mutex */
> > > +GHashTable *inodes_by_ids; /* protected by lo->mutex */
> > > +GHashTable *inodes_by_handle; /* protected by lo->mutex */
> > > struct lo_map ino_map; /* protected by lo->mutex */
> > > struct lo_map dirp_map; /* protected by lo->mutex */
> > > struct lo_map fd_map; /* protected by lo->mutex */
> > > @@ -275,8 +276,9 @@ static struct {
> > > /* That we loaded cap-ng in the current thread from the saved */
> > > static __thread bool cap_loaded = 0;
> > > -static struct lo_inode *lo_find(struct lo_data *lo, struct stat *st,
> > > -uint64_t mnt_id);
> > > +static struct lo_inode *lo_find(struct lo_data *lo,
> > > +const struct lo_fhandle *fhandle,
> > > +struct stat *st, uint64_t mnt_id);
> > > static int xattr_map_client(const struct lo_data *lo, const char
> > > *client_name,
> > > char **out_name);
> > > @@ -1143,18 +1145,40 @@ out_err:
> > > fuse_reply_err(req, saverr);
> > > }
> > > -static struct lo_inode *lo_find(struct lo_data *lo, struct stat *st,
> > > -uint64_t mnt_id)
> > > +static struct lo_inode *lo_find(struct lo_data *lo,
> > > +const struct lo_fhandle *fhandle,
> > > +struct stat *st, uint64_t mnt_id)
> > > {
> > > -struct lo_inode *p;
> > > -struct lo_key key = {
> > > +struct lo_inode *p = NULL;
> > > +
Re: [PATCH v4 10/12] virtiofsd: Add inodes_by_handle hash table
On Wed, Oct 20, 2021 at 12:02:32PM +0200, Hanna Reitz wrote:
> On 19.10.21 22:02, Vivek Goyal wrote:
> > On Thu, Sep 16, 2021 at 10:40:43AM +0200, Hanna Reitz wrote:
> > > Currently, lo_inode.fhandle is always NULL and so always keep an O_PATH
> > > FD in lo_inode.fd. Therefore, when the respective inode is unlinked,
> > > its inode ID will remain in use until we drop our lo_inode (and
> > > lo_inode_put() thus closes the FD). Therefore, lo_find() can safely use
> > > the inode ID as an lo_inode key, because any inode with an inode ID we
> > > find in lo_data.inodes (on the same filesystem) must be the exact same
> > > file.
> > >
> > > This will change when we start setting lo_inode.fhandle so we do not
> > > have to keep an O_PATH FD open. Then, unlinking such an inode will
> > > immediately remove it, so its ID can then be reused by newly created
> > > files, even while the lo_inode object is still there[1].
> > >
> > > So creating a new file can then reuse the old file's inode ID, and
> > > looking up the new file would lead to us finding the old file's
> > > lo_inode, which is not ideal.
> > >
> > > Luckily, just as file handles cause this problem, they also solve it: A
> > > file handle contains a generation ID, which changes when an inode ID is
> > > reused, so the new file can be distinguished from the old one. So all
> > > we need to do is to add a second map besides lo_data.inodes that maps
> > > file handles to lo_inodes, namely lo_data.inodes_by_handle. For
> > > clarity, lo_data.inodes is renamed to lo_data.inodes_by_ids.
> > >
> > > Unfortunately, we cannot rely on being able to generate file handles
> > > every time. Therefore, we still enter every lo_inode object into
> > > inodes_by_ids, but having an entry in inodes_by_handle is optional. A
> > > potential inodes_by_handle entry then has precedence, the inodes_by_ids
> > > entry is just a fallback.
> > >
> > > Note that we do not generate lo_fhandle objects yet, and so we also do
> > > not enter anything into the inodes_by_handle map yet. Also, all lookups
> > > skip that map. We might manually create file handles with some code
> > > that is immediately removed by the next patch again, but that would
> > > break the assumption in lo_find() that every lo_inode with a non-NULL
> > > .fhandle must have an entry in inodes_by_handle and vice versa. So we
> > > leave actually using the inodes_by_handle map for the next patch.
> > >
> > > [1] If some application in the guest still has the file open, there is
> > > going to be a corresponding FD mapping in lo_data.fd_map. In such a
> > > case, the inode will only go away once every application in the guest
> > > has closed it. The problem described only applies to cases where the
> > > guest does not have the file open, and it is just in the dentry cache,
> > > basically.
> > >
> > > Signed-off-by: Hanna Reitz
> > > ---
> > > tools/virtiofsd/passthrough_ll.c | 81 +---
> > > 1 file changed, 65 insertions(+), 16 deletions(-)
> > >
> > > diff --git a/tools/virtiofsd/passthrough_ll.c
> > > b/tools/virtiofsd/passthrough_ll.c
> > > index bd8fc922ea..b7d6aa7f9d 100644
> > > --- a/tools/virtiofsd/passthrough_ll.c
> > > +++ b/tools/virtiofsd/passthrough_ll.c
> > > @@ -186,7 +186,8 @@ struct lo_data {
> > > int announce_submounts;
> > > bool use_statx;
> > > struct lo_inode root;
> > > -GHashTable *inodes; /* protected by lo->mutex */
> > > +GHashTable *inodes_by_ids; /* protected by lo->mutex */
> > > +GHashTable *inodes_by_handle; /* protected by lo->mutex */
> > > struct lo_map ino_map; /* protected by lo->mutex */
> > > struct lo_map dirp_map; /* protected by lo->mutex */
> > > struct lo_map fd_map; /* protected by lo->mutex */
> > > @@ -275,8 +276,9 @@ static struct {
> > > /* That we loaded cap-ng in the current thread from the saved */
> > > static __thread bool cap_loaded = 0;
> > > -static struct lo_inode *lo_find(struct lo_data *lo, struct stat *st,
> > > -uint64_t mnt_id);
> > > +static struct lo_inode *lo_find(struct lo_data *lo,
> > > +const struct lo_fhandle *fhandle,
> > > +struct stat *st, uint64_t mnt_id);
> > > static int xattr_map_client(const struct lo_data *lo, const char
> > > *client_name,
> > > char **out_name);
> > > @@ -1143,18 +1145,40 @@ out_err:
> > > fuse_reply_err(req, saverr);
> > > }
> > > -static struct lo_inode *lo_find(struct lo_data *lo, struct stat *st,
> > > -uint64_t mnt_id)
> > > +static struct lo_inode *lo_find(struct lo_data *lo,
> > > +const struct lo_fhandle *fhandle,
> > > +struct stat *st, uint64_t mnt_id)
> > > {
> > > -struct lo_inode *p;
> > > -struct lo_key key = {
> > > +struct lo_inode *p = NULL;
> > > +
Re: [PATCH v4 10/12] virtiofsd: Add inodes_by_handle hash table
On 19.10.21 22:02, Vivek Goyal wrote:
On Thu, Sep 16, 2021 at 10:40:43AM +0200, Hanna Reitz wrote:
Currently, lo_inode.fhandle is always NULL and so always keep an O_PATH
FD in lo_inode.fd. Therefore, when the respective inode is unlinked,
its inode ID will remain in use until we drop our lo_inode (and
lo_inode_put() thus closes the FD). Therefore, lo_find() can safely use
the inode ID as an lo_inode key, because any inode with an inode ID we
find in lo_data.inodes (on the same filesystem) must be the exact same
file.
This will change when we start setting lo_inode.fhandle so we do not
have to keep an O_PATH FD open. Then, unlinking such an inode will
immediately remove it, so its ID can then be reused by newly created
files, even while the lo_inode object is still there[1].
So creating a new file can then reuse the old file's inode ID, and
looking up the new file would lead to us finding the old file's
lo_inode, which is not ideal.
Luckily, just as file handles cause this problem, they also solve it: A
file handle contains a generation ID, which changes when an inode ID is
reused, so the new file can be distinguished from the old one. So all
we need to do is to add a second map besides lo_data.inodes that maps
file handles to lo_inodes, namely lo_data.inodes_by_handle. For
clarity, lo_data.inodes is renamed to lo_data.inodes_by_ids.
Unfortunately, we cannot rely on being able to generate file handles
every time. Therefore, we still enter every lo_inode object into
inodes_by_ids, but having an entry in inodes_by_handle is optional. A
potential inodes_by_handle entry then has precedence, the inodes_by_ids
entry is just a fallback.
Note that we do not generate lo_fhandle objects yet, and so we also do
not enter anything into the inodes_by_handle map yet. Also, all lookups
skip that map. We might manually create file handles with some code
that is immediately removed by the next patch again, but that would
break the assumption in lo_find() that every lo_inode with a non-NULL
.fhandle must have an entry in inodes_by_handle and vice versa. So we
leave actually using the inodes_by_handle map for the next patch.
[1] If some application in the guest still has the file open, there is
going to be a corresponding FD mapping in lo_data.fd_map. In such a
case, the inode will only go away once every application in the guest
has closed it. The problem described only applies to cases where the
guest does not have the file open, and it is just in the dentry cache,
basically.
Signed-off-by: Hanna Reitz
---
tools/virtiofsd/passthrough_ll.c | 81 +---
1 file changed, 65 insertions(+), 16 deletions(-)
diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index bd8fc922ea..b7d6aa7f9d 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -186,7 +186,8 @@ struct lo_data {
int announce_submounts;
bool use_statx;
struct lo_inode root;
-GHashTable *inodes; /* protected by lo->mutex */
+GHashTable *inodes_by_ids; /* protected by lo->mutex */
+GHashTable *inodes_by_handle; /* protected by lo->mutex */
struct lo_map ino_map; /* protected by lo->mutex */
struct lo_map dirp_map; /* protected by lo->mutex */
struct lo_map fd_map; /* protected by lo->mutex */
@@ -275,8 +276,9 @@ static struct {
/* That we loaded cap-ng in the current thread from the saved */
static __thread bool cap_loaded = 0;
-static struct lo_inode *lo_find(struct lo_data *lo, struct stat *st,
-uint64_t mnt_id);
+static struct lo_inode *lo_find(struct lo_data *lo,
+const struct lo_fhandle *fhandle,
+struct stat *st, uint64_t mnt_id);
static int xattr_map_client(const struct lo_data *lo, const char *client_name,
char **out_name);
@@ -1143,18 +1145,40 @@ out_err:
fuse_reply_err(req, saverr);
}
-static struct lo_inode *lo_find(struct lo_data *lo, struct stat *st,
-uint64_t mnt_id)
+static struct lo_inode *lo_find(struct lo_data *lo,
+const struct lo_fhandle *fhandle,
+struct stat *st, uint64_t mnt_id)
{
-struct lo_inode *p;
-struct lo_key key = {
+struct lo_inode *p = NULL;
+struct lo_key ids_key = {
.ino = st->st_ino,
.dev = st->st_dev,
.mnt_id = mnt_id,
};
pthread_mutex_lock(>mutex);
-p = g_hash_table_lookup(lo->inodes, );
+if (fhandle) {
+p = g_hash_table_lookup(lo->inodes_by_handle, fhandle);
+}
+if (!p) {
+p = g_hash_table_lookup(lo->inodes_by_ids, _key);
+/*
+ * When we had to fall back to looking up an inode by its
+ * inode ID, ensure that we hit an entry that has a valid file
+ * descriptor. Having an FD open means that the inode
Re: [PATCH v4 10/12] virtiofsd: Add inodes_by_handle hash table
On Thu, Sep 16, 2021 at 10:40:43AM +0200, Hanna Reitz wrote:
> Currently, lo_inode.fhandle is always NULL and so always keep an O_PATH
> FD in lo_inode.fd. Therefore, when the respective inode is unlinked,
> its inode ID will remain in use until we drop our lo_inode (and
> lo_inode_put() thus closes the FD). Therefore, lo_find() can safely use
> the inode ID as an lo_inode key, because any inode with an inode ID we
> find in lo_data.inodes (on the same filesystem) must be the exact same
> file.
>
> This will change when we start setting lo_inode.fhandle so we do not
> have to keep an O_PATH FD open. Then, unlinking such an inode will
> immediately remove it, so its ID can then be reused by newly created
> files, even while the lo_inode object is still there[1].
>
> So creating a new file can then reuse the old file's inode ID, and
> looking up the new file would lead to us finding the old file's
> lo_inode, which is not ideal.
>
> Luckily, just as file handles cause this problem, they also solve it: A
> file handle contains a generation ID, which changes when an inode ID is
> reused, so the new file can be distinguished from the old one. So all
> we need to do is to add a second map besides lo_data.inodes that maps
> file handles to lo_inodes, namely lo_data.inodes_by_handle. For
> clarity, lo_data.inodes is renamed to lo_data.inodes_by_ids.
>
> Unfortunately, we cannot rely on being able to generate file handles
> every time. Therefore, we still enter every lo_inode object into
> inodes_by_ids, but having an entry in inodes_by_handle is optional. A
> potential inodes_by_handle entry then has precedence, the inodes_by_ids
> entry is just a fallback.
>
> Note that we do not generate lo_fhandle objects yet, and so we also do
> not enter anything into the inodes_by_handle map yet. Also, all lookups
> skip that map. We might manually create file handles with some code
> that is immediately removed by the next patch again, but that would
> break the assumption in lo_find() that every lo_inode with a non-NULL
> .fhandle must have an entry in inodes_by_handle and vice versa. So we
> leave actually using the inodes_by_handle map for the next patch.
>
> [1] If some application in the guest still has the file open, there is
> going to be a corresponding FD mapping in lo_data.fd_map. In such a
> case, the inode will only go away once every application in the guest
> has closed it. The problem described only applies to cases where the
> guest does not have the file open, and it is just in the dentry cache,
> basically.
>
> Signed-off-by: Hanna Reitz
> ---
> tools/virtiofsd/passthrough_ll.c | 81 +---
> 1 file changed, 65 insertions(+), 16 deletions(-)
>
> diff --git a/tools/virtiofsd/passthrough_ll.c
> b/tools/virtiofsd/passthrough_ll.c
> index bd8fc922ea..b7d6aa7f9d 100644
> --- a/tools/virtiofsd/passthrough_ll.c
> +++ b/tools/virtiofsd/passthrough_ll.c
> @@ -186,7 +186,8 @@ struct lo_data {
> int announce_submounts;
> bool use_statx;
> struct lo_inode root;
> -GHashTable *inodes; /* protected by lo->mutex */
> +GHashTable *inodes_by_ids; /* protected by lo->mutex */
> +GHashTable *inodes_by_handle; /* protected by lo->mutex */
> struct lo_map ino_map; /* protected by lo->mutex */
> struct lo_map dirp_map; /* protected by lo->mutex */
> struct lo_map fd_map; /* protected by lo->mutex */
> @@ -275,8 +276,9 @@ static struct {
> /* That we loaded cap-ng in the current thread from the saved */
> static __thread bool cap_loaded = 0;
>
> -static struct lo_inode *lo_find(struct lo_data *lo, struct stat *st,
> -uint64_t mnt_id);
> +static struct lo_inode *lo_find(struct lo_data *lo,
> +const struct lo_fhandle *fhandle,
> +struct stat *st, uint64_t mnt_id);
> static int xattr_map_client(const struct lo_data *lo, const char
> *client_name,
> char **out_name);
>
> @@ -1143,18 +1145,40 @@ out_err:
> fuse_reply_err(req, saverr);
> }
>
> -static struct lo_inode *lo_find(struct lo_data *lo, struct stat *st,
> -uint64_t mnt_id)
> +static struct lo_inode *lo_find(struct lo_data *lo,
> +const struct lo_fhandle *fhandle,
> +struct stat *st, uint64_t mnt_id)
> {
> -struct lo_inode *p;
> -struct lo_key key = {
> +struct lo_inode *p = NULL;
> +struct lo_key ids_key = {
> .ino = st->st_ino,
> .dev = st->st_dev,
> .mnt_id = mnt_id,
> };
>
> pthread_mutex_lock(>mutex);
> -p = g_hash_table_lookup(lo->inodes, );
> +if (fhandle) {
> +p = g_hash_table_lookup(lo->inodes_by_handle, fhandle);
> +}
> +if (!p) {
> +p = g_hash_table_lookup(lo->inodes_by_ids, _key);
> +/*
> + * When we had to fall back to looking up an
[PATCH v4 10/12] virtiofsd: Add inodes_by_handle hash table
Currently, lo_inode.fhandle is always NULL and so always keep an O_PATH
FD in lo_inode.fd. Therefore, when the respective inode is unlinked,
its inode ID will remain in use until we drop our lo_inode (and
lo_inode_put() thus closes the FD). Therefore, lo_find() can safely use
the inode ID as an lo_inode key, because any inode with an inode ID we
find in lo_data.inodes (on the same filesystem) must be the exact same
file.
This will change when we start setting lo_inode.fhandle so we do not
have to keep an O_PATH FD open. Then, unlinking such an inode will
immediately remove it, so its ID can then be reused by newly created
files, even while the lo_inode object is still there[1].
So creating a new file can then reuse the old file's inode ID, and
looking up the new file would lead to us finding the old file's
lo_inode, which is not ideal.
Luckily, just as file handles cause this problem, they also solve it: A
file handle contains a generation ID, which changes when an inode ID is
reused, so the new file can be distinguished from the old one. So all
we need to do is to add a second map besides lo_data.inodes that maps
file handles to lo_inodes, namely lo_data.inodes_by_handle. For
clarity, lo_data.inodes is renamed to lo_data.inodes_by_ids.
Unfortunately, we cannot rely on being able to generate file handles
every time. Therefore, we still enter every lo_inode object into
inodes_by_ids, but having an entry in inodes_by_handle is optional. A
potential inodes_by_handle entry then has precedence, the inodes_by_ids
entry is just a fallback.
Note that we do not generate lo_fhandle objects yet, and so we also do
not enter anything into the inodes_by_handle map yet. Also, all lookups
skip that map. We might manually create file handles with some code
that is immediately removed by the next patch again, but that would
break the assumption in lo_find() that every lo_inode with a non-NULL
.fhandle must have an entry in inodes_by_handle and vice versa. So we
leave actually using the inodes_by_handle map for the next patch.
[1] If some application in the guest still has the file open, there is
going to be a corresponding FD mapping in lo_data.fd_map. In such a
case, the inode will only go away once every application in the guest
has closed it. The problem described only applies to cases where the
guest does not have the file open, and it is just in the dentry cache,
basically.
Signed-off-by: Hanna Reitz
---
tools/virtiofsd/passthrough_ll.c | 81 +---
1 file changed, 65 insertions(+), 16 deletions(-)
diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index bd8fc922ea..b7d6aa7f9d 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -186,7 +186,8 @@ struct lo_data {
int announce_submounts;
bool use_statx;
struct lo_inode root;
-GHashTable *inodes; /* protected by lo->mutex */
+GHashTable *inodes_by_ids; /* protected by lo->mutex */
+GHashTable *inodes_by_handle; /* protected by lo->mutex */
struct lo_map ino_map; /* protected by lo->mutex */
struct lo_map dirp_map; /* protected by lo->mutex */
struct lo_map fd_map; /* protected by lo->mutex */
@@ -275,8 +276,9 @@ static struct {
/* That we loaded cap-ng in the current thread from the saved */
static __thread bool cap_loaded = 0;
-static struct lo_inode *lo_find(struct lo_data *lo, struct stat *st,
-uint64_t mnt_id);
+static struct lo_inode *lo_find(struct lo_data *lo,
+const struct lo_fhandle *fhandle,
+struct stat *st, uint64_t mnt_id);
static int xattr_map_client(const struct lo_data *lo, const char *client_name,
char **out_name);
@@ -1143,18 +1145,40 @@ out_err:
fuse_reply_err(req, saverr);
}
-static struct lo_inode *lo_find(struct lo_data *lo, struct stat *st,
-uint64_t mnt_id)
+static struct lo_inode *lo_find(struct lo_data *lo,
+const struct lo_fhandle *fhandle,
+struct stat *st, uint64_t mnt_id)
{
-struct lo_inode *p;
-struct lo_key key = {
+struct lo_inode *p = NULL;
+struct lo_key ids_key = {
.ino = st->st_ino,
.dev = st->st_dev,
.mnt_id = mnt_id,
};
pthread_mutex_lock(>mutex);
-p = g_hash_table_lookup(lo->inodes, );
+if (fhandle) {
+p = g_hash_table_lookup(lo->inodes_by_handle, fhandle);
+}
+if (!p) {
+p = g_hash_table_lookup(lo->inodes_by_ids, _key);
+/*
+ * When we had to fall back to looking up an inode by its
+ * inode ID, ensure that we hit an entry that has a valid file
+ * descriptor. Having an FD open means that the inode cannot
+ * really be deleted until the FD is closed, so that the inode
+ * ID remains valid until we evict our
