| Message ID | 20210512125544.9536-2-mreitz@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | virtiofsd: Find original inode ID of mount points | expand |
On 5/12/21 7:55 AM, Max Reitz wrote: > Mount point directories represent two inodes: On one hand, they are a > normal directory on their parent filesystem. On the other, they are > the > root node of the filesystem mounted there. Thus, they have two inode > IDs. > > Right now, we only report the latter inode ID (i.e. the inode ID of > the > mounted filesystem's root node). This is fine once the guest has > auto-mounted a submount there (so this inode ID goes with a device ID > that is distinct from the parent filesystem), but before the > auto-mount, > they have the device ID of the parent and the inode ID for the > submount. > This is problematic because this is likely exactly the same > st_dev/st_ino combination as the parent filesystem's root node. This > leads to problems for example with `find`, which will thus complain > about a filesystem loop if it has visited the parent filesystem's root > node before, and then refuse to descend into the submount. > > There is a way to find the mount directory's original inode ID, and > that > is to readdir(3) the parent directory, look for the mount directory, > and > read the dirent.d_ino field. Using this, we can let lookup and > readdirplus return that original inode ID, which the guest will thus > show until the submount is auto-mounted. (Then, it will invoke > getattr > and that stat(2) call will return the inode ID for the submount.) > > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- This is a clever way of uncovering the inode ID. Reviewed-by: Connor Kuehl <ckuehl@redhat.com>
On Wed, May 12, 2021 at 02:55:42PM +0200, Max Reitz wrote: > Mount point directories represent two inodes: On one hand, they are a > normal directory on their parent filesystem. On the other, they are the > root node of the filesystem mounted there. Thus, they have two inode > IDs. > > Right now, we only report the latter inode ID (i.e. the inode ID of the > mounted filesystem's root node). This is fine once the guest has > auto-mounted a submount there (so this inode ID goes with a device ID > that is distinct from the parent filesystem), but before the auto-mount, > they have the device ID of the parent and the inode ID for the submount. > This is problematic because this is likely exactly the same > st_dev/st_ino combination as the parent filesystem's root node. This > leads to problems for example with `find`, which will thus complain > about a filesystem loop if it has visited the parent filesystem's root > node before, and then refuse to descend into the submount. > > There is a way to find the mount directory's original inode ID, and that > is to readdir(3) the parent directory, look for the mount directory, and > read the dirent.d_ino field. Using this, we can let lookup and > readdirplus return that original inode ID, which the guest will thus > show until the submount is auto-mounted. (Then, it will invoke getattr > and that stat(2) call will return the inode ID for the submount.) > > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > tools/virtiofsd/passthrough_ll.c | 104 +++++++++++++++++++++++++++++-- > 1 file changed, 99 insertions(+), 5 deletions(-) > > diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c > index 1553d2ef45..110b6e7e5b 100644 > --- a/tools/virtiofsd/passthrough_ll.c > +++ b/tools/virtiofsd/passthrough_ll.c > @@ -968,14 +968,87 @@ static int do_statx(struct lo_data *lo, int dirfd, const char *pathname, > return 0; > } > > +/* > + * Use readdir() to find mp_name's inode ID on the parent's filesystem. > + * (For mount points, stat() will only return the inode ID on the > + * filesystem mounted there, i.e. the root directory's inode ID. The > + * mount point originally was a directory on the parent filesystem, > + * though, and so has a different inode ID there. When passing > + * submount information to the guest, we need to pass this other ID, > + * so the guest can use it as the inode ID until the submount is > + * auto-mounted. (At which point the guest will invoke getattr and > + * find the inode ID on the submount.)) > + * > + * Return 0 on success, and -errno otherwise. *pino is set only in > + * case of success. > + */ > +static int get_mp_ino_on_parent(const struct lo_inode *dir, const char *mp_name, > + ino_t *pino) > +{ > + int dirfd = -1; > + int ret; > + DIR *dp = NULL; > + > + dirfd = openat(dir->fd, ".", O_RDONLY); > + if (dirfd < 0) { > + ret = -errno; > + goto out; > + } > + > + dp = fdopendir(dirfd); > + if (!dp) { > + ret = -errno; > + goto out; > + } > + /* Owned by dp now */ > + dirfd = -1; > + > + while (true) { > + struct dirent *de; > + > + errno = 0; > + de = readdir(dp); > + if (!de) { > + ret = errno ? -errno : -ENOENT; > + goto out; > + } > + > + if (!strcmp(de->d_name, mp_name)) { > + *pino = de->d_ino; > + ret = 0; > + goto out; > + } > + } > + > +out: > + if (dp) { > + closedir(dp); > + } > + if (dirfd >= 0) { > + close(dirfd); > + } > + return ret; > +} > + > /* > * Increments nlookup on the inode on success. unref_inode_lolocked() must be > * called eventually to decrement nlookup again. If inodep is non-NULL, the > * inode pointer is stored and the caller must call lo_inode_put(). > + * > + * If parent_fs_st_ino is true, the entry is a mount point, and submounts are > + * announced to the guest, set e->attr.st_ino to the entry's inode ID on its > + * parent filesystem instead of its inode ID on the filesystem mounted on it. > + * (For mount points, the entry encompasses two inodes: One on the parent FS, > + * and one on the mounted FS (where it is the root node), so it has two inode > + * IDs. When looking up entries, we should show the guest the parent FS's inode > + * ID, because as long as the guest has not auto-mounted the submount, it should > + * see that original ID. Once it does perform the auto-mount, it will invoke > + * getattr and see the root node's inode ID.) > */ > static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name, > struct fuse_entry_param *e, > - struct lo_inode **inodep) > + struct lo_inode **inodep, > + bool parent_fs_st_ino) > { > int newfd; > int res; > @@ -984,6 +1057,7 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name, > struct lo_data *lo = lo_data(req); > struct lo_inode *inode = NULL; > struct lo_inode *dir = lo_inode(req, parent); > + ino_t ino_id_for_guest; > > if (inodep) { > *inodep = NULL; /* in case there is an error */ > @@ -1018,9 +1092,22 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name, > goto out_err; > } > > + ino_id_for_guest = e->attr.st_ino; > + > if (S_ISDIR(e->attr.st_mode) && lo->announce_submounts && > (e->attr.st_dev != dir->key.dev || mnt_id != dir->key.mnt_id)) { > e->attr_flags |= FUSE_ATTR_SUBMOUNT; > + > + if (parent_fs_st_ino) { > + /* > + * Best effort, so ignore errors. > + * Also note that using readdir() means there may be races: > + * The directory entry we find (if any) may be different > + * from newfd. Again, this is a best effort. Reporting > + * the wrong inode ID to the guest is not catastrophic. > + */ > + get_mp_ino_on_parent(dir, name, &ino_id_for_guest); Hi Max, [CC virtio-fs list ] In general patch looks good to me. A minor nit. get_mp_ino_on_parent() is retruning error. It might be better to capture error and print a message and continue. I have couple of general questions about submounts. - What happens in case of single file mounted on top of another file. mount --bind foo.txt bar.txt Do submounts work when mount point is not a directory. - Say a directory is not a mount point yet and lookup instantiates an inode. Later user mounts something on that directory. When does client/server notice this change. I am assuming this is probably part of revalidation path. Thanks Vivek > + } > } > > inode = lo_find(lo, &e->attr, mnt_id); > @@ -1043,6 +1130,10 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name, > > inode->nlookup = 1; > inode->fd = newfd; > + /* > + * For the inode key, use the dev/ino/mnt ID as reported by stat() > + * (i.e. not ino_id_for_guest) > + */ > inode->key.ino = e->attr.st_ino; > inode->key.dev = e->attr.st_dev; > inode->key.mnt_id = mnt_id; > @@ -1058,6 +1149,9 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name, > } > e->ino = inode->fuse_ino; > > + /* Report ino_id_for_guest to the guest */ > + e->attr.st_ino = ino_id_for_guest; > + > /* Transfer ownership of inode pointer to caller or drop it */ > if (inodep) { > *inodep = inode; > @@ -1104,7 +1198,7 @@ static void lo_lookup(fuse_req_t req, fuse_ino_t parent, const char *name) > return; > } > > - err = lo_do_lookup(req, parent, name, &e, NULL); > + err = lo_do_lookup(req, parent, name, &e, NULL, true); > if (err) { > fuse_reply_err(req, err); > } else { > @@ -1217,7 +1311,7 @@ static void lo_mknod_symlink(fuse_req_t req, fuse_ino_t parent, > goto out; > } > > - saverr = lo_do_lookup(req, parent, name, &e, NULL); > + saverr = lo_do_lookup(req, parent, name, &e, NULL, false); > if (saverr) { > goto out; > } > @@ -1714,7 +1808,7 @@ static void lo_do_readdir(fuse_req_t req, fuse_ino_t ino, size_t size, > > if (plus) { > if (!is_dot_or_dotdot(name)) { > - err = lo_do_lookup(req, ino, name, &e, NULL); > + err = lo_do_lookup(req, ino, name, &e, NULL, true); > if (err) { > goto error; > } > @@ -1936,7 +2030,7 @@ static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name, > goto out; > } > > - err = lo_do_lookup(req, parent, name, &e, &inode); > + err = lo_do_lookup(req, parent, name, &e, &inode, false); > if (err) { > goto out; > } > -- > 2.31.1 > >
On 17.05.21 16:57, Vivek Goyal wrote: > On Wed, May 12, 2021 at 02:55:42PM +0200, Max Reitz wrote: >> Mount point directories represent two inodes: On one hand, they are a >> normal directory on their parent filesystem. On the other, they are the >> root node of the filesystem mounted there. Thus, they have two inode >> IDs. >> >> Right now, we only report the latter inode ID (i.e. the inode ID of the >> mounted filesystem's root node). This is fine once the guest has >> auto-mounted a submount there (so this inode ID goes with a device ID >> that is distinct from the parent filesystem), but before the auto-mount, >> they have the device ID of the parent and the inode ID for the submount. >> This is problematic because this is likely exactly the same >> st_dev/st_ino combination as the parent filesystem's root node. This >> leads to problems for example with `find`, which will thus complain >> about a filesystem loop if it has visited the parent filesystem's root >> node before, and then refuse to descend into the submount. >> >> There is a way to find the mount directory's original inode ID, and that >> is to readdir(3) the parent directory, look for the mount directory, and >> read the dirent.d_ino field. Using this, we can let lookup and >> readdirplus return that original inode ID, which the guest will thus >> show until the submount is auto-mounted. (Then, it will invoke getattr >> and that stat(2) call will return the inode ID for the submount.) >> >> Signed-off-by: Max Reitz <mreitz@redhat.com> >> --- >> tools/virtiofsd/passthrough_ll.c | 104 +++++++++++++++++++++++++++++-- >> 1 file changed, 99 insertions(+), 5 deletions(-) >> >> diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c >> index 1553d2ef45..110b6e7e5b 100644 >> --- a/tools/virtiofsd/passthrough_ll.c >> +++ b/tools/virtiofsd/passthrough_ll.c >> @@ -968,14 +968,87 @@ static int do_statx(struct lo_data *lo, int dirfd, const char *pathname, >> return 0; >> } >> >> +/* >> + * Use readdir() to find mp_name's inode ID on the parent's filesystem. >> + * (For mount points, stat() will only return the inode ID on the >> + * filesystem mounted there, i.e. the root directory's inode ID. The >> + * mount point originally was a directory on the parent filesystem, >> + * though, and so has a different inode ID there. When passing >> + * submount information to the guest, we need to pass this other ID, >> + * so the guest can use it as the inode ID until the submount is >> + * auto-mounted. (At which point the guest will invoke getattr and >> + * find the inode ID on the submount.)) >> + * >> + * Return 0 on success, and -errno otherwise. *pino is set only in >> + * case of success. >> + */ >> +static int get_mp_ino_on_parent(const struct lo_inode *dir, const char *mp_name, >> + ino_t *pino) >> +{ >> + int dirfd = -1; >> + int ret; >> + DIR *dp = NULL; >> + >> + dirfd = openat(dir->fd, ".", O_RDONLY); >> + if (dirfd < 0) { >> + ret = -errno; >> + goto out; >> + } >> + >> + dp = fdopendir(dirfd); >> + if (!dp) { >> + ret = -errno; >> + goto out; >> + } >> + /* Owned by dp now */ >> + dirfd = -1; >> + >> + while (true) { >> + struct dirent *de; >> + >> + errno = 0; >> + de = readdir(dp); >> + if (!de) { >> + ret = errno ? -errno : -ENOENT; >> + goto out; >> + } >> + >> + if (!strcmp(de->d_name, mp_name)) { >> + *pino = de->d_ino; >> + ret = 0; >> + goto out; >> + } >> + } >> + >> +out: >> + if (dp) { >> + closedir(dp); >> + } >> + if (dirfd >= 0) { >> + close(dirfd); >> + } >> + return ret; >> +} >> + >> /* >> * Increments nlookup on the inode on success. unref_inode_lolocked() must be >> * called eventually to decrement nlookup again. If inodep is non-NULL, the >> * inode pointer is stored and the caller must call lo_inode_put(). >> + * >> + * If parent_fs_st_ino is true, the entry is a mount point, and submounts are >> + * announced to the guest, set e->attr.st_ino to the entry's inode ID on its >> + * parent filesystem instead of its inode ID on the filesystem mounted on it. >> + * (For mount points, the entry encompasses two inodes: One on the parent FS, >> + * and one on the mounted FS (where it is the root node), so it has two inode >> + * IDs. When looking up entries, we should show the guest the parent FS's inode >> + * ID, because as long as the guest has not auto-mounted the submount, it should >> + * see that original ID. Once it does perform the auto-mount, it will invoke >> + * getattr and see the root node's inode ID.) >> */ >> static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name, >> struct fuse_entry_param *e, >> - struct lo_inode **inodep) >> + struct lo_inode **inodep, >> + bool parent_fs_st_ino) >> { >> int newfd; >> int res; >> @@ -984,6 +1057,7 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name, >> struct lo_data *lo = lo_data(req); >> struct lo_inode *inode = NULL; >> struct lo_inode *dir = lo_inode(req, parent); >> + ino_t ino_id_for_guest; >> >> if (inodep) { >> *inodep = NULL; /* in case there is an error */ >> @@ -1018,9 +1092,22 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name, >> goto out_err; >> } >> >> + ino_id_for_guest = e->attr.st_ino; >> + >> if (S_ISDIR(e->attr.st_mode) && lo->announce_submounts && >> (e->attr.st_dev != dir->key.dev || mnt_id != dir->key.mnt_id)) { >> e->attr_flags |= FUSE_ATTR_SUBMOUNT; >> + >> + if (parent_fs_st_ino) { >> + /* >> + * Best effort, so ignore errors. >> + * Also note that using readdir() means there may be races: >> + * The directory entry we find (if any) may be different >> + * from newfd. Again, this is a best effort. Reporting >> + * the wrong inode ID to the guest is not catastrophic. >> + */ >> + get_mp_ino_on_parent(dir, name, &ino_id_for_guest); > > Hi Max, > > [CC virtio-fs list ] > > In general patch looks good to me. A minor nit. get_mp_ino_on_parent() > is retruning error. It might be better to capture error and print a > message and continue. Sure, why not. > I have couple of general questions about submounts. > > - What happens in case of single file mounted on top of another file. > > mount --bind foo.txt bar.txt > > Do submounts work when mount point is not a directory. No, as you can see in the condition quoted above, we only set the FUSE_ATTR_SUBMOUNT flag for directories. That seemed the most common case for me, and I didn’t want to have to worry about weirdness that might ensue for file mounts. > - Say a directory is not a mount point yet and lookup instantiates an > inode. Later user mounts something on that directory. When does > client/server notice this change. I am assuming this is probably > part of revalidation path. I guess at least before this patch this is no different from any other filesystem change. Because st_dev+st_ino changed, it should basically look like the old directory was removed and a different one was put in its place. Now, with this patch, we will return the old st_ino to the guest, but internally virtiofsd will still use the submount’s st_dev/st_ino, so a new lo_inode should be created, and so fuse_dentry_revalidate()’s lookup should return a different node ID, resulting it to consider the entry expired. Besides, fuse_dentry_revalidate() has a condition on IS_AUTOMOUNT(inode) != flags & FUSE_ATTR_SUBMOUNT. Considering the previous paragraph, this doesn’t seem necessary to me, but it can’t hurt. Max
* Max Reitz (mreitz@redhat.com) wrote: > On 17.05.21 16:57, Vivek Goyal wrote: > > On Wed, May 12, 2021 at 02:55:42PM +0200, Max Reitz wrote: > > > Mount point directories represent two inodes: On one hand, they are a > > > normal directory on their parent filesystem. On the other, they are the > > > root node of the filesystem mounted there. Thus, they have two inode > > > IDs. > > > > > > Right now, we only report the latter inode ID (i.e. the inode ID of the > > > mounted filesystem's root node). This is fine once the guest has > > > auto-mounted a submount there (so this inode ID goes with a device ID > > > that is distinct from the parent filesystem), but before the auto-mount, > > > they have the device ID of the parent and the inode ID for the submount. > > > This is problematic because this is likely exactly the same > > > st_dev/st_ino combination as the parent filesystem's root node. This > > > leads to problems for example with `find`, which will thus complain > > > about a filesystem loop if it has visited the parent filesystem's root > > > node before, and then refuse to descend into the submount. > > > > > > There is a way to find the mount directory's original inode ID, and that > > > is to readdir(3) the parent directory, look for the mount directory, and > > > read the dirent.d_ino field. Using this, we can let lookup and > > > readdirplus return that original inode ID, which the guest will thus > > > show until the submount is auto-mounted. (Then, it will invoke getattr > > > and that stat(2) call will return the inode ID for the submount.) > > > > > > Signed-off-by: Max Reitz <mreitz@redhat.com> > > > --- > > > tools/virtiofsd/passthrough_ll.c | 104 +++++++++++++++++++++++++++++-- > > > 1 file changed, 99 insertions(+), 5 deletions(-) > > > > > > diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c > > > index 1553d2ef45..110b6e7e5b 100644 > > > --- a/tools/virtiofsd/passthrough_ll.c > > > +++ b/tools/virtiofsd/passthrough_ll.c > > > @@ -968,14 +968,87 @@ static int do_statx(struct lo_data *lo, int dirfd, const char *pathname, > > > return 0; > > > } > > > +/* > > > + * Use readdir() to find mp_name's inode ID on the parent's filesystem. > > > + * (For mount points, stat() will only return the inode ID on the > > > + * filesystem mounted there, i.e. the root directory's inode ID. The > > > + * mount point originally was a directory on the parent filesystem, > > > + * though, and so has a different inode ID there. When passing > > > + * submount information to the guest, we need to pass this other ID, > > > + * so the guest can use it as the inode ID until the submount is > > > + * auto-mounted. (At which point the guest will invoke getattr and > > > + * find the inode ID on the submount.)) > > > + * > > > + * Return 0 on success, and -errno otherwise. *pino is set only in > > > + * case of success. > > > + */ > > > +static int get_mp_ino_on_parent(const struct lo_inode *dir, const char *mp_name, > > > + ino_t *pino) > > > +{ > > > + int dirfd = -1; > > > + int ret; > > > + DIR *dp = NULL; > > > + > > > + dirfd = openat(dir->fd, ".", O_RDONLY); > > > + if (dirfd < 0) { > > > + ret = -errno; > > > + goto out; > > > + } > > > + > > > + dp = fdopendir(dirfd); > > > + if (!dp) { > > > + ret = -errno; > > > + goto out; > > > + } > > > + /* Owned by dp now */ > > > + dirfd = -1; > > > + > > > + while (true) { > > > + struct dirent *de; > > > + > > > + errno = 0; > > > + de = readdir(dp); > > > + if (!de) { > > > + ret = errno ? -errno : -ENOENT; > > > + goto out; > > > + } > > > + > > > + if (!strcmp(de->d_name, mp_name)) { > > > + *pino = de->d_ino; > > > + ret = 0; > > > + goto out; > > > + } > > > + } > > > + > > > +out: > > > + if (dp) { > > > + closedir(dp); > > > + } > > > + if (dirfd >= 0) { > > > + close(dirfd); > > > + } > > > + return ret; > > > +} > > > + > > > /* > > > * Increments nlookup on the inode on success. unref_inode_lolocked() must be > > > * called eventually to decrement nlookup again. If inodep is non-NULL, the > > > * inode pointer is stored and the caller must call lo_inode_put(). > > > + * > > > + * If parent_fs_st_ino is true, the entry is a mount point, and submounts are > > > + * announced to the guest, set e->attr.st_ino to the entry's inode ID on its > > > + * parent filesystem instead of its inode ID on the filesystem mounted on it. > > > + * (For mount points, the entry encompasses two inodes: One on the parent FS, > > > + * and one on the mounted FS (where it is the root node), so it has two inode > > > + * IDs. When looking up entries, we should show the guest the parent FS's inode > > > + * ID, because as long as the guest has not auto-mounted the submount, it should > > > + * see that original ID. Once it does perform the auto-mount, it will invoke > > > + * getattr and see the root node's inode ID.) > > > */ > > > static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name, > > > struct fuse_entry_param *e, > > > - struct lo_inode **inodep) > > > + struct lo_inode **inodep, > > > + bool parent_fs_st_ino) > > > { > > > int newfd; > > > int res; > > > @@ -984,6 +1057,7 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name, > > > struct lo_data *lo = lo_data(req); > > > struct lo_inode *inode = NULL; > > > struct lo_inode *dir = lo_inode(req, parent); > > > + ino_t ino_id_for_guest; > > > if (inodep) { > > > *inodep = NULL; /* in case there is an error */ > > > @@ -1018,9 +1092,22 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name, > > > goto out_err; > > > } > > > + ino_id_for_guest = e->attr.st_ino; > > > + > > > if (S_ISDIR(e->attr.st_mode) && lo->announce_submounts && > > > (e->attr.st_dev != dir->key.dev || mnt_id != dir->key.mnt_id)) { > > > e->attr_flags |= FUSE_ATTR_SUBMOUNT; > > > + > > > + if (parent_fs_st_ino) { > > > + /* > > > + * Best effort, so ignore errors. > > > + * Also note that using readdir() means there may be races: > > > + * The directory entry we find (if any) may be different > > > + * from newfd. Again, this is a best effort. Reporting > > > + * the wrong inode ID to the guest is not catastrophic. > > > + */ > > > + get_mp_ino_on_parent(dir, name, &ino_id_for_guest); > > > > Hi Max, > > > > [CC virtio-fs list ] > > > > In general patch looks good to me. A minor nit. get_mp_ino_on_parent() > > is retruning error. It might be better to capture error and print a > > message and continue. > > Sure, why not. > > > I have couple of general questions about submounts. > > > > - What happens in case of single file mounted on top of another file. > > > > mount --bind foo.txt bar.txt > > > > Do submounts work when mount point is not a directory. > > No, as you can see in the condition quoted above, we only set the > FUSE_ATTR_SUBMOUNT flag for directories. That seemed the most common case > for me, and I didn’t want to have to worry about weirdness that might ensue > for file mounts. It might be worth checking file mounts don't break too badly; I'm pretty sure I've seen the container systems use them a lot in /etc Dave > > - Say a directory is not a mount point yet and lookup instantiates an > > inode. Later user mounts something on that directory. When does > > client/server notice this change. I am assuming this is probably > > part of revalidation path. > > I guess at least before this patch this is no different from any other > filesystem change. Because st_dev+st_ino changed, it should basically look > like the old directory was removed and a different one was put in its place. > > Now, with this patch, we will return the old st_ino to the guest, but > internally virtiofsd will still use the submount’s st_dev/st_ino, so a new > lo_inode should be created, and so fuse_dentry_revalidate()’s lookup should > return a different node ID, resulting it to consider the entry expired. > > Besides, fuse_dentry_revalidate() has a condition on IS_AUTOMOUNT(inode) != > flags & FUSE_ATTR_SUBMOUNT. Considering the previous paragraph, this > doesn’t seem necessary to me, but it can’t hurt. > > Max
On Wed, May 12, 2021 at 02:55:42PM +0200, Max Reitz wrote: > Mount point directories represent two inodes: On one hand, they are a > normal directory on their parent filesystem. On the other, they are the > root node of the filesystem mounted there. Thus, they have two inode > IDs. > > Right now, we only report the latter inode ID (i.e. the inode ID of the > mounted filesystem's root node). This is fine once the guest has > auto-mounted a submount there (so this inode ID goes with a device ID > that is distinct from the parent filesystem), but before the auto-mount, > they have the device ID of the parent and the inode ID for the submount. > This is problematic because this is likely exactly the same > st_dev/st_ino combination as the parent filesystem's root node. This > leads to problems for example with `find`, which will thus complain > about a filesystem loop if it has visited the parent filesystem's root > node before, and then refuse to descend into the submount. > > There is a way to find the mount directory's original inode ID, and that > is to readdir(3) the parent directory, look for the mount directory, and > read the dirent.d_ino field. Using this, we can let lookup and > readdirplus return that original inode ID, which the guest will thus > show until the submount is auto-mounted. > (Then, it will invoke getattr > and that stat(2) call will return the inode ID for the submount.) Hi Max, How are we sure that GETATTR() will always be called and that will allow us to return inode number in mounted filesystem (instead of parent filesystem). I thought GETATTR will be called only if cached attrs have expired. (1 second default for cache=auto). Otherwise stat() will fill inode->i_no from cache and return. And I am afraid that in that case we will return inode number from parent fs, instead of mounted fs. Say following sequence of events happens pretty fast one after the other. Say /mnt/virtiofs/foo is a mount point in server but client has not created submount yet. A. stat(/mnt/virtiofs/foo, AT_NO_AUTOMOUNT) -> This should get inode number in parent filesystem on host and store in guest inode->i_no and return to user space. Say inode in guest is called a_ino. B. stat(/mnt/virtiofs/foo) -> This should create submount and create new inode (say b_ino), using properties from a_ino. IOW, this should copy a_ino->i_no to b_ino->b_ino given current code, IIUC. -> Assume timeout has not happened and cached attrs have not expired. -> And now b_ino->i_no (or ->orig_ino) will be returned to user space. Am I missing something. Do we need to always expire inode attrs when we create submount so that client is forced to issue GETATTR. Thanks Vivek > > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > tools/virtiofsd/passthrough_ll.c | 104 +++++++++++++++++++++++++++++-- > 1 file changed, 99 insertions(+), 5 deletions(-) > > diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c > index 1553d2ef45..110b6e7e5b 100644 > --- a/tools/virtiofsd/passthrough_ll.c > +++ b/tools/virtiofsd/passthrough_ll.c > @@ -968,14 +968,87 @@ static int do_statx(struct lo_data *lo, int dirfd, const char *pathname, > return 0; > } > > +/* > + * Use readdir() to find mp_name's inode ID on the parent's filesystem. > + * (For mount points, stat() will only return the inode ID on the > + * filesystem mounted there, i.e. the root directory's inode ID. The > + * mount point originally was a directory on the parent filesystem, > + * though, and so has a different inode ID there. When passing > + * submount information to the guest, we need to pass this other ID, > + * so the guest can use it as the inode ID until the submount is > + * auto-mounted. (At which point the guest will invoke getattr and > + * find the inode ID on the submount.)) > + * > + * Return 0 on success, and -errno otherwise. *pino is set only in > + * case of success. > + */ > +static int get_mp_ino_on_parent(const struct lo_inode *dir, const char *mp_name, > + ino_t *pino) > +{ > + int dirfd = -1; > + int ret; > + DIR *dp = NULL; > + > + dirfd = openat(dir->fd, ".", O_RDONLY); > + if (dirfd < 0) { > + ret = -errno; > + goto out; > + } > + > + dp = fdopendir(dirfd); > + if (!dp) { > + ret = -errno; > + goto out; > + } > + /* Owned by dp now */ > + dirfd = -1; > + > + while (true) { > + struct dirent *de; > + > + errno = 0; > + de = readdir(dp); > + if (!de) { > + ret = errno ? -errno : -ENOENT; > + goto out; > + } > + > + if (!strcmp(de->d_name, mp_name)) { > + *pino = de->d_ino; > + ret = 0; > + goto out; > + } > + } > + > +out: > + if (dp) { > + closedir(dp); > + } > + if (dirfd >= 0) { > + close(dirfd); > + } > + return ret; > +} > + > /* > * Increments nlookup on the inode on success. unref_inode_lolocked() must be > * called eventually to decrement nlookup again. If inodep is non-NULL, the > * inode pointer is stored and the caller must call lo_inode_put(). > + * > + * If parent_fs_st_ino is true, the entry is a mount point, and submounts are > + * announced to the guest, set e->attr.st_ino to the entry's inode ID on its > + * parent filesystem instead of its inode ID on the filesystem mounted on it. > + * (For mount points, the entry encompasses two inodes: One on the parent FS, > + * and one on the mounted FS (where it is the root node), so it has two inode > + * IDs. When looking up entries, we should show the guest the parent FS's inode > + * ID, because as long as the guest has not auto-mounted the submount, it should > + * see that original ID. Once it does perform the auto-mount, it will invoke > + * getattr and see the root node's inode ID.) > */ > static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name, > struct fuse_entry_param *e, > - struct lo_inode **inodep) > + struct lo_inode **inodep, > + bool parent_fs_st_ino) > { > int newfd; > int res; > @@ -984,6 +1057,7 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name, > struct lo_data *lo = lo_data(req); > struct lo_inode *inode = NULL; > struct lo_inode *dir = lo_inode(req, parent); > + ino_t ino_id_for_guest; > > if (inodep) { > *inodep = NULL; /* in case there is an error */ > @@ -1018,9 +1092,22 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name, > goto out_err; > } > > + ino_id_for_guest = e->attr.st_ino; > + > if (S_ISDIR(e->attr.st_mode) && lo->announce_submounts && > (e->attr.st_dev != dir->key.dev || mnt_id != dir->key.mnt_id)) { > e->attr_flags |= FUSE_ATTR_SUBMOUNT; > + > + if (parent_fs_st_ino) { > + /* > + * Best effort, so ignore errors. > + * Also note that using readdir() means there may be races: > + * The directory entry we find (if any) may be different > + * from newfd. Again, this is a best effort. Reporting > + * the wrong inode ID to the guest is not catastrophic. > + */ > + get_mp_ino_on_parent(dir, name, &ino_id_for_guest); > + } > } > > inode = lo_find(lo, &e->attr, mnt_id); > @@ -1043,6 +1130,10 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name, > > inode->nlookup = 1; > inode->fd = newfd; > + /* > + * For the inode key, use the dev/ino/mnt ID as reported by stat() > + * (i.e. not ino_id_for_guest) > + */ > inode->key.ino = e->attr.st_ino; > inode->key.dev = e->attr.st_dev; > inode->key.mnt_id = mnt_id; > @@ -1058,6 +1149,9 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name, > } > e->ino = inode->fuse_ino; > > + /* Report ino_id_for_guest to the guest */ > + e->attr.st_ino = ino_id_for_guest; > + > /* Transfer ownership of inode pointer to caller or drop it */ > if (inodep) { > *inodep = inode; > @@ -1104,7 +1198,7 @@ static void lo_lookup(fuse_req_t req, fuse_ino_t parent, const char *name) > return; > } > > - err = lo_do_lookup(req, parent, name, &e, NULL); > + err = lo_do_lookup(req, parent, name, &e, NULL, true); > if (err) { > fuse_reply_err(req, err); > } else { > @@ -1217,7 +1311,7 @@ static void lo_mknod_symlink(fuse_req_t req, fuse_ino_t parent, > goto out; > } > > - saverr = lo_do_lookup(req, parent, name, &e, NULL); > + saverr = lo_do_lookup(req, parent, name, &e, NULL, false); > if (saverr) { > goto out; > } > @@ -1714,7 +1808,7 @@ static void lo_do_readdir(fuse_req_t req, fuse_ino_t ino, size_t size, > > if (plus) { > if (!is_dot_or_dotdot(name)) { > - err = lo_do_lookup(req, ino, name, &e, NULL); > + err = lo_do_lookup(req, ino, name, &e, NULL, true); > if (err) { > goto error; > } > @@ -1936,7 +2030,7 @@ static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name, > goto out; > } > > - err = lo_do_lookup(req, parent, name, &e, &inode); > + err = lo_do_lookup(req, parent, name, &e, &inode, false); > if (err) { > goto out; > } > -- > 2.31.1 > >
On Wed, May 26, 2021 at 02:13:24PM -0400, Vivek Goyal wrote: > On Wed, May 12, 2021 at 02:55:42PM +0200, Max Reitz wrote: > > Mount point directories represent two inodes: On one hand, they are a > > normal directory on their parent filesystem. On the other, they are the > > root node of the filesystem mounted there. Thus, they have two inode > > IDs. > > > > Right now, we only report the latter inode ID (i.e. the inode ID of the > > mounted filesystem's root node). This is fine once the guest has > > auto-mounted a submount there (so this inode ID goes with a device ID > > that is distinct from the parent filesystem), but before the auto-mount, > > they have the device ID of the parent and the inode ID for the submount. > > This is problematic because this is likely exactly the same > > st_dev/st_ino combination as the parent filesystem's root node. This > > leads to problems for example with `find`, which will thus complain > > about a filesystem loop if it has visited the parent filesystem's root > > node before, and then refuse to descend into the submount. > > > > There is a way to find the mount directory's original inode ID, and that > > is to readdir(3) the parent directory, look for the mount directory, and > > read the dirent.d_ino field. Using this, we can let lookup and > > readdirplus return that original inode ID, which the guest will thus > > show until the submount is auto-mounted. > > > (Then, it will invoke getattr > > and that stat(2) call will return the inode ID for the submount.) > > Hi Max, > > How are we sure that GETATTR() will always be called and that will > allow us to return inode number in mounted filesystem (instead of > parent filesystem). I thought GETATTR will be called only if cached > attrs have expired. (1 second default for cache=auto). Otherwise > stat() will fill inode->i_no from cache and return. And I am afraid > that in that case we will return inode number from parent fs, > instead of mounted fs. > > Say following sequence of events happens pretty fast one after the > other. Say /mnt/virtiofs/foo is a mount point in server but client > has not created submount yet. > > A. stat(/mnt/virtiofs/foo, AT_NO_AUTOMOUNT) > -> This should get inode number in parent filesystem on host and > store in guest inode->i_no and return to user space. Say inode > in guest is called a_ino. > B. stat(/mnt/virtiofs/foo) > -> This should create submount and create new inode (say b_ino), using > properties from a_ino. IOW, this should copy a_ino->i_no to > b_ino->b_ino given current code, IIUC. > > -> Assume timeout has not happened and cached attrs have not expired. > > -> And now b_ino->i_no (or ->orig_ino) will be returned to user space. > > Am I missing something. Do we need to always expire inode attrs when > we create submount so that client is forced to issue GETATTR. Looks like while initialzing b_ino, we are setting attr_valid=0, which should set fi->i_time=0 and force issuing GETATTR later. fuse_fill_super_submount() root = fuse_iget(sb, parent_fi->nodeid, 0, &root_attr, 0, 0); ^ fuse_iget(attr_valid=0) fuse_change_attributes(attr_valid=0) fuse_change_attributes_common(attr_valid=0) fi->i_time = attr_valid; So may be this will force GETATTR and fetch new inode id when second stat() is called. Thanks Vivek > > Signed-off-by: Max Reitz <mreitz@redhat.com> > > --- > > tools/virtiofsd/passthrough_ll.c | 104 +++++++++++++++++++++++++++++-- > > 1 file changed, 99 insertions(+), 5 deletions(-) > > > > diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c > > index 1553d2ef45..110b6e7e5b 100644 > > --- a/tools/virtiofsd/passthrough_ll.c > > +++ b/tools/virtiofsd/passthrough_ll.c > > @@ -968,14 +968,87 @@ static int do_statx(struct lo_data *lo, int dirfd, const char *pathname, > > return 0; > > } > > > > +/* > > + * Use readdir() to find mp_name's inode ID on the parent's filesystem. > > + * (For mount points, stat() will only return the inode ID on the > > + * filesystem mounted there, i.e. the root directory's inode ID. The > > + * mount point originally was a directory on the parent filesystem, > > + * though, and so has a different inode ID there. When passing > > + * submount information to the guest, we need to pass this other ID, > > + * so the guest can use it as the inode ID until the submount is > > + * auto-mounted. (At which point the guest will invoke getattr and > > + * find the inode ID on the submount.)) > > + * > > + * Return 0 on success, and -errno otherwise. *pino is set only in > > + * case of success. > > + */ > > +static int get_mp_ino_on_parent(const struct lo_inode *dir, const char *mp_name, > > + ino_t *pino) > > +{ > > + int dirfd = -1; > > + int ret; > > + DIR *dp = NULL; > > + > > + dirfd = openat(dir->fd, ".", O_RDONLY); > > + if (dirfd < 0) { > > + ret = -errno; > > + goto out; > > + } > > + > > + dp = fdopendir(dirfd); > > + if (!dp) { > > + ret = -errno; > > + goto out; > > + } > > + /* Owned by dp now */ > > + dirfd = -1; > > + > > + while (true) { > > + struct dirent *de; > > + > > + errno = 0; > > + de = readdir(dp); > > + if (!de) { > > + ret = errno ? -errno : -ENOENT; > > + goto out; > > + } > > + > > + if (!strcmp(de->d_name, mp_name)) { > > + *pino = de->d_ino; > > + ret = 0; > > + goto out; > > + } > > + } > > + > > +out: > > + if (dp) { > > + closedir(dp); > > + } > > + if (dirfd >= 0) { > > + close(dirfd); > > + } > > + return ret; > > +} > > + > > /* > > * Increments nlookup on the inode on success. unref_inode_lolocked() must be > > * called eventually to decrement nlookup again. If inodep is non-NULL, the > > * inode pointer is stored and the caller must call lo_inode_put(). > > + * > > + * If parent_fs_st_ino is true, the entry is a mount point, and submounts are > > + * announced to the guest, set e->attr.st_ino to the entry's inode ID on its > > + * parent filesystem instead of its inode ID on the filesystem mounted on it. > > + * (For mount points, the entry encompasses two inodes: One on the parent FS, > > + * and one on the mounted FS (where it is the root node), so it has two inode > > + * IDs. When looking up entries, we should show the guest the parent FS's inode > > + * ID, because as long as the guest has not auto-mounted the submount, it should > > + * see that original ID. Once it does perform the auto-mount, it will invoke > > + * getattr and see the root node's inode ID.) > > */ > > static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name, > > struct fuse_entry_param *e, > > - struct lo_inode **inodep) > > + struct lo_inode **inodep, > > + bool parent_fs_st_ino) > > { > > int newfd; > > int res; > > @@ -984,6 +1057,7 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name, > > struct lo_data *lo = lo_data(req); > > struct lo_inode *inode = NULL; > > struct lo_inode *dir = lo_inode(req, parent); > > + ino_t ino_id_for_guest; > > > > if (inodep) { > > *inodep = NULL; /* in case there is an error */ > > @@ -1018,9 +1092,22 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name, > > goto out_err; > > } > > > > + ino_id_for_guest = e->attr.st_ino; > > + > > if (S_ISDIR(e->attr.st_mode) && lo->announce_submounts && > > (e->attr.st_dev != dir->key.dev || mnt_id != dir->key.mnt_id)) { > > e->attr_flags |= FUSE_ATTR_SUBMOUNT; > > + > > + if (parent_fs_st_ino) { > > + /* > > + * Best effort, so ignore errors. > > + * Also note that using readdir() means there may be races: > > + * The directory entry we find (if any) may be different > > + * from newfd. Again, this is a best effort. Reporting > > + * the wrong inode ID to the guest is not catastrophic. > > + */ > > + get_mp_ino_on_parent(dir, name, &ino_id_for_guest); > > + } > > } > > > > inode = lo_find(lo, &e->attr, mnt_id); > > @@ -1043,6 +1130,10 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name, > > > > inode->nlookup = 1; > > inode->fd = newfd; > > + /* > > + * For the inode key, use the dev/ino/mnt ID as reported by stat() > > + * (i.e. not ino_id_for_guest) > > + */ > > inode->key.ino = e->attr.st_ino; > > inode->key.dev = e->attr.st_dev; > > inode->key.mnt_id = mnt_id; > > @@ -1058,6 +1149,9 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name, > > } > > e->ino = inode->fuse_ino; > > > > + /* Report ino_id_for_guest to the guest */ > > + e->attr.st_ino = ino_id_for_guest; > > + > > /* Transfer ownership of inode pointer to caller or drop it */ > > if (inodep) { > > *inodep = inode; > > @@ -1104,7 +1198,7 @@ static void lo_lookup(fuse_req_t req, fuse_ino_t parent, const char *name) > > return; > > } > > > > - err = lo_do_lookup(req, parent, name, &e, NULL); > > + err = lo_do_lookup(req, parent, name, &e, NULL, true); > > if (err) { > > fuse_reply_err(req, err); > > } else { > > @@ -1217,7 +1311,7 @@ static void lo_mknod_symlink(fuse_req_t req, fuse_ino_t parent, > > goto out; > > } > > > > - saverr = lo_do_lookup(req, parent, name, &e, NULL); > > + saverr = lo_do_lookup(req, parent, name, &e, NULL, false); > > if (saverr) { > > goto out; > > } > > @@ -1714,7 +1808,7 @@ static void lo_do_readdir(fuse_req_t req, fuse_ino_t ino, size_t size, > > > > if (plus) { > > if (!is_dot_or_dotdot(name)) { > > - err = lo_do_lookup(req, ino, name, &e, NULL); > > + err = lo_do_lookup(req, ino, name, &e, NULL, true); > > if (err) { > > goto error; > > } > > @@ -1936,7 +2030,7 @@ static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name, > > goto out; > > } > > > > - err = lo_do_lookup(req, parent, name, &e, &inode); > > + err = lo_do_lookup(req, parent, name, &e, &inode, false); > > if (err) { > > goto out; > > } > > -- > > 2.31.1 > > > > > > _______________________________________________ > Virtio-fs mailing list > Virtio-fs@redhat.com > https://listman.redhat.com/mailman/listinfo/virtio-fs >
On 26.05.21 20:50, Vivek Goyal wrote: > On Wed, May 26, 2021 at 02:13:24PM -0400, Vivek Goyal wrote: >> On Wed, May 12, 2021 at 02:55:42PM +0200, Max Reitz wrote: >>> Mount point directories represent two inodes: On one hand, they are a >>> normal directory on their parent filesystem. On the other, they are the >>> root node of the filesystem mounted there. Thus, they have two inode >>> IDs. >>> >>> Right now, we only report the latter inode ID (i.e. the inode ID of the >>> mounted filesystem's root node). This is fine once the guest has >>> auto-mounted a submount there (so this inode ID goes with a device ID >>> that is distinct from the parent filesystem), but before the auto-mount, >>> they have the device ID of the parent and the inode ID for the submount. >>> This is problematic because this is likely exactly the same >>> st_dev/st_ino combination as the parent filesystem's root node. This >>> leads to problems for example with `find`, which will thus complain >>> about a filesystem loop if it has visited the parent filesystem's root >>> node before, and then refuse to descend into the submount. >>> >>> There is a way to find the mount directory's original inode ID, and that >>> is to readdir(3) the parent directory, look for the mount directory, and >>> read the dirent.d_ino field. Using this, we can let lookup and >>> readdirplus return that original inode ID, which the guest will thus >>> show until the submount is auto-mounted. >> >>> (Then, it will invoke getattr >>> and that stat(2) call will return the inode ID for the submount.) >> >> Hi Max, >> >> How are we sure that GETATTR() will always be called and that will >> allow us to return inode number in mounted filesystem (instead of >> parent filesystem). I thought GETATTR will be called only if cached >> attrs have expired. (1 second default for cache=auto). Otherwise >> stat() will fill inode->i_no from cache and return. And I am afraid >> that in that case we will return inode number from parent fs, >> instead of mounted fs. >> >> Say following sequence of events happens pretty fast one after the >> other. Say /mnt/virtiofs/foo is a mount point in server but client >> has not created submount yet. >> >> A. stat(/mnt/virtiofs/foo, AT_NO_AUTOMOUNT) >> -> This should get inode number in parent filesystem on host and >> store in guest inode->i_no and return to user space. Say inode >> in guest is called a_ino. >> B. stat(/mnt/virtiofs/foo) >> -> This should create submount and create new inode (say b_ino), using >> properties from a_ino. IOW, this should copy a_ino->i_no to >> b_ino->b_ino given current code, IIUC. >> >> -> Assume timeout has not happened and cached attrs have not expired. >> >> -> And now b_ino->i_no (or ->orig_ino) will be returned to user space. Well, I mean, this sounds easy enough to test. For example, this passes for me: count=1000 root_st_ino=128 tag=host mountpoint=/mnt submount_path=submount for i in $(seq $count) do mount -t virtiofs $tag $mountpoint || break if [ $(stat -c '%i' $mountpoint/$submount_path) -eq $root_st_ino ] then echo 'fail 0' break fi ls $mountpoint/$submount_path > /dev/null if [ $(stat -c '%i' $mountpoint/$submount_path) -ne $root_st_ino ] then echo 'fail 1' break fi umount $mountpoint || break done if [ $i -ne $count ] then echo 'Something failed' else echo 'OK' fi >> Am I missing something. Do we need to always expire inode attrs when >> we create submount so that client is forced to issue GETATTR. > > Looks like while initialzing b_ino, we are setting attr_valid=0, which > should set fi->i_time=0 and force issuing GETATTR later. > > fuse_fill_super_submount() > root = fuse_iget(sb, parent_fi->nodeid, 0, &root_attr, 0, 0); > ^ > fuse_iget(attr_valid=0) > fuse_change_attributes(attr_valid=0) > fuse_change_attributes_common(attr_valid=0) > fi->i_time = attr_valid; > > So may be this will force GETATTR and fetch new inode id when second > stat() is called. i_time is at least what fuse_update_get_attr() uses to decide whether to invoke GETATTR or not – although AT_STATX_DONT_SYNC can override that, but I don’t think that’s a problem. If i_time is 0, that function will always invoke GETATTR (unless AT_STATX_DONT_SYNC). So I think it works in practice. When the inode ID is looked up through some stat-y function, this should go through fuse_update_get_attr(), which will fetch the st_ino on the submount. There is still i_ino, though... To be absolutely certain, we could invoke fuse_update_attributes() in fuse_fill_super_submount(), but then again, fuse_fill_super_common() doesn’t do that either for its root node. It just initializes i_ino to FUSE_ROOT_ID, i.e. 1. Max
On Wed, May 12, 2021 at 02:55:42PM +0200, Max Reitz wrote: > Mount point directories represent two inodes: On one hand, they are a > normal directory on their parent filesystem. On the other, they are the > root node of the filesystem mounted there. Thus, they have two inode > IDs. > > Right now, we only report the latter inode ID (i.e. the inode ID of the > mounted filesystem's root node). This is fine once the guest has > auto-mounted a submount there (so this inode ID goes with a device ID > that is distinct from the parent filesystem), but before the auto-mount, > they have the device ID of the parent and the inode ID for the submount. > This is problematic because this is likely exactly the same > st_dev/st_ino combination as the parent filesystem's root node. This > leads to problems for example with `find`, which will thus complain > about a filesystem loop if it has visited the parent filesystem's root > node before, and then refuse to descend into the submount. > > There is a way to find the mount directory's original inode ID, and that > is to readdir(3) the parent directory, look for the mount directory, and > read the dirent.d_ino field. Using this, we can let lookup and > readdirplus return that original inode ID, which the guest will thus > show until the submount is auto-mounted. (Then, it will invoke getattr > and that stat(2) call will return the inode ID for the submount.) [ CC miklos ] Hi Max, Though we discussed this in chat room, I am still responding to this email with the concern I have, so that there is a record of it. So with this patch for FUSE_LOOKUP we always return submount's parentinode id and with GETATTR request we return actual inode id of submount. That kind of bothers me a bit as we are assuming that there is always going to be a GETATTR request after FUSE_LOOKUP. FUSE_LOOKUP itself has attrs returned with it and it might happen that after FUSE_LOOKUP, FUSE_GETATTR might not be called at all because FUSE_LOOKUP itself got the latest updated attrs with certain timeout. For example, if I call stat on a normal file (not submount), I see that after FUSE_LOOKUP, no FUSE_GETATTR request is going and fuse_update_get_attr() is using attrs from locally cached inode attrs. But same thing does not seem to be happening in case of submount. Once submount is created in guest, I see that we never seem to be calling ->revalidate() on newly created dentry of submount root. I am not sure why. And that means we don't do FUSE_LOOKUP and that means FUSE_GETATTR always gets called. I am not sure if this is a bug or expected behavior. I am not confident that we can rely on this behavior of submount dentries. Ccing Miklos. He might have thoughts on this. As we discussed, may be it is a better idea to create two inodes in virtiofsd (like we do in guest). One for submount parent and one for actual submount. Thanks Vivek > > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > tools/virtiofsd/passthrough_ll.c | 104 +++++++++++++++++++++++++++++-- > 1 file changed, 99 insertions(+), 5 deletions(-) > > diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c > index 1553d2ef45..110b6e7e5b 100644 > --- a/tools/virtiofsd/passthrough_ll.c > +++ b/tools/virtiofsd/passthrough_ll.c > @@ -968,14 +968,87 @@ static int do_statx(struct lo_data *lo, int dirfd, const char *pathname, > return 0; > } > > +/* > + * Use readdir() to find mp_name's inode ID on the parent's filesystem. > + * (For mount points, stat() will only return the inode ID on the > + * filesystem mounted there, i.e. the root directory's inode ID. The > + * mount point originally was a directory on the parent filesystem, > + * though, and so has a different inode ID there. When passing > + * submount information to the guest, we need to pass this other ID, > + * so the guest can use it as the inode ID until the submount is > + * auto-mounted. (At which point the guest will invoke getattr and > + * find the inode ID on the submount.)) > + * > + * Return 0 on success, and -errno otherwise. *pino is set only in > + * case of success. > + */ > +static int get_mp_ino_on_parent(const struct lo_inode *dir, const char *mp_name, > + ino_t *pino) > +{ > + int dirfd = -1; > + int ret; > + DIR *dp = NULL; > + > + dirfd = openat(dir->fd, ".", O_RDONLY); > + if (dirfd < 0) { > + ret = -errno; > + goto out; > + } > + > + dp = fdopendir(dirfd); > + if (!dp) { > + ret = -errno; > + goto out; > + } > + /* Owned by dp now */ > + dirfd = -1; > + > + while (true) { > + struct dirent *de; > + > + errno = 0; > + de = readdir(dp); > + if (!de) { > + ret = errno ? -errno : -ENOENT; > + goto out; > + } > + > + if (!strcmp(de->d_name, mp_name)) { > + *pino = de->d_ino; > + ret = 0; > + goto out; > + } > + } > + > +out: > + if (dp) { > + closedir(dp); > + } > + if (dirfd >= 0) { > + close(dirfd); > + } > + return ret; > +} > + > /* > * Increments nlookup on the inode on success. unref_inode_lolocked() must be > * called eventually to decrement nlookup again. If inodep is non-NULL, the > * inode pointer is stored and the caller must call lo_inode_put(). > + * > + * If parent_fs_st_ino is true, the entry is a mount point, and submounts are > + * announced to the guest, set e->attr.st_ino to the entry's inode ID on its > + * parent filesystem instead of its inode ID on the filesystem mounted on it. > + * (For mount points, the entry encompasses two inodes: One on the parent FS, > + * and one on the mounted FS (where it is the root node), so it has two inode > + * IDs. When looking up entries, we should show the guest the parent FS's inode > + * ID, because as long as the guest has not auto-mounted the submount, it should > + * see that original ID. Once it does perform the auto-mount, it will invoke > + * getattr and see the root node's inode ID.) > */ > static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name, > struct fuse_entry_param *e, > - struct lo_inode **inodep) > + struct lo_inode **inodep, > + bool parent_fs_st_ino) > { > int newfd; > int res; > @@ -984,6 +1057,7 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name, > struct lo_data *lo = lo_data(req); > struct lo_inode *inode = NULL; > struct lo_inode *dir = lo_inode(req, parent); > + ino_t ino_id_for_guest; > > if (inodep) { > *inodep = NULL; /* in case there is an error */ > @@ -1018,9 +1092,22 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name, > goto out_err; > } > > + ino_id_for_guest = e->attr.st_ino; > + > if (S_ISDIR(e->attr.st_mode) && lo->announce_submounts && > (e->attr.st_dev != dir->key.dev || mnt_id != dir->key.mnt_id)) { > e->attr_flags |= FUSE_ATTR_SUBMOUNT; > + > + if (parent_fs_st_ino) { > + /* > + * Best effort, so ignore errors. > + * Also note that using readdir() means there may be races: > + * The directory entry we find (if any) may be different > + * from newfd. Again, this is a best effort. Reporting > + * the wrong inode ID to the guest is not catastrophic. > + */ > + get_mp_ino_on_parent(dir, name, &ino_id_for_guest); > + } > } > > inode = lo_find(lo, &e->attr, mnt_id); > @@ -1043,6 +1130,10 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name, > > inode->nlookup = 1; > inode->fd = newfd; > + /* > + * For the inode key, use the dev/ino/mnt ID as reported by stat() > + * (i.e. not ino_id_for_guest) > + */ > inode->key.ino = e->attr.st_ino; > inode->key.dev = e->attr.st_dev; > inode->key.mnt_id = mnt_id; > @@ -1058,6 +1149,9 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name, > } > e->ino = inode->fuse_ino; > > + /* Report ino_id_for_guest to the guest */ > + e->attr.st_ino = ino_id_for_guest; > + > /* Transfer ownership of inode pointer to caller or drop it */ > if (inodep) { > *inodep = inode; > @@ -1104,7 +1198,7 @@ static void lo_lookup(fuse_req_t req, fuse_ino_t parent, const char *name) > return; > } > > - err = lo_do_lookup(req, parent, name, &e, NULL); > + err = lo_do_lookup(req, parent, name, &e, NULL, true); > if (err) { > fuse_reply_err(req, err); > } else { > @@ -1217,7 +1311,7 @@ static void lo_mknod_symlink(fuse_req_t req, fuse_ino_t parent, > goto out; > } > > - saverr = lo_do_lookup(req, parent, name, &e, NULL); > + saverr = lo_do_lookup(req, parent, name, &e, NULL, false); > if (saverr) { > goto out; > } > @@ -1714,7 +1808,7 @@ static void lo_do_readdir(fuse_req_t req, fuse_ino_t ino, size_t size, > > if (plus) { > if (!is_dot_or_dotdot(name)) { > - err = lo_do_lookup(req, ino, name, &e, NULL); > + err = lo_do_lookup(req, ino, name, &e, NULL, true); > if (err) { > goto error; > } > @@ -1936,7 +2030,7 @@ static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name, > goto out; > } > > - err = lo_do_lookup(req, parent, name, &e, &inode); > + err = lo_do_lookup(req, parent, name, &e, &inode, false); > if (err) { > goto out; > } > -- > 2.31.1 > >
On Wed, 2 Jun 2021 at 20:20, Vivek Goyal <vgoyal@redhat.com> wrote: > > On Wed, May 12, 2021 at 02:55:42PM +0200, Max Reitz wrote: > > Mount point directories represent two inodes: On one hand, they are a > > normal directory on their parent filesystem. On the other, they are the > > root node of the filesystem mounted there. Thus, they have two inode > > IDs. > > > > Right now, we only report the latter inode ID (i.e. the inode ID of the > > mounted filesystem's root node). This is fine once the guest has > > auto-mounted a submount there (so this inode ID goes with a device ID > > that is distinct from the parent filesystem), but before the auto-mount, > > they have the device ID of the parent and the inode ID for the submount. > > This is problematic because this is likely exactly the same > > st_dev/st_ino combination as the parent filesystem's root node. This > > leads to problems for example with `find`, which will thus complain > > about a filesystem loop if it has visited the parent filesystem's root > > node before, and then refuse to descend into the submount. > > > > There is a way to find the mount directory's original inode ID, and that > > is to readdir(3) the parent directory, look for the mount directory, and > > read the dirent.d_ino field. Using this, we can let lookup and > > readdirplus return that original inode ID, which the guest will thus > > show until the submount is auto-mounted. (Then, it will invoke getattr > > and that stat(2) call will return the inode ID for the submount.) > > [ CC miklos ] > > Hi Max, > > Though we discussed this in chat room, I am still responding to this > email with the concern I have, so that there is a record of it. > > So with this patch for FUSE_LOOKUP we always return submount's parentinode > id and with GETATTR request we return actual inode id of submount. That > kind of bothers me a bit as we are assuming that there is always going > to be a GETATTR request after FUSE_LOOKUP. FUSE_LOOKUP itself has attrs > returned with it and it might happen that after FUSE_LOOKUP, FUSE_GETATTR > might not be called at all because FUSE_LOOKUP itself got the latest > updated attrs with certain timeout. > > For example, if I call stat on a normal file (not submount), I see that > after FUSE_LOOKUP, no FUSE_GETATTR request is going and > fuse_update_get_attr() is using attrs from locally cached inode attrs. > > But same thing does not seem to be happening in case of submount. Once > submount is created in guest, I see that we never seem to be calling > ->revalidate() on newly created dentry of submount root. I am not sure > why. And that means we don't do FUSE_LOOKUP and that means FUSE_GETATTR > always gets called. Yes, this sounds normal. The lookup sequence for "/mnt/virtiofs/dir1/submount/file" will be: LOOKUP(1, "dir") create inode I1 in sb1 create dentry "dir" with inode I1 in sb1 LOOKUP(I1, "submount") create inode I2 in sb1, set S_AUTOMOUNT create dentry "submount" with inode I2 in sb1 automount triggered: create sb2 create inode I2 in sb2 create dentry "/" with inode I2 in sb2 GETATTR(I2) fill inode I2 LOOKUP(I2, "file") create inode I3 create dentry "file" with inode I3 in sb2 Notice how there's two inodes numbered I2, but in separate sb's. However the server has only the notion of a single I2 inode which is the root of the submount, since the mountpoint is not visible (except for d_ino in readdir()). Now AFAICS what this patch does is set the inode number in the attributes returned by LOOKUP(I1, "submount") to the covered inode, so that AT_NO_AUTOMOUNT stat will return the correct value. While this seems to work, it's not a fundamental fix to the problem, since the attributes on sb1/I2 will time out and the next stat(..., AT_NO_AUTOMOUNT) will trigger a GETATTR request, which will return the inode number of the submount root. Solving this properly would mean that the server would have to generate separate nodeid's for the mountpoint and the submount root and the protocol would have to be extended so that this information could be transferred to the client. Not sure whether this is worth it or not. Thanks, Miklos
On 02.06.21 20:59, Miklos Szeredi wrote: > On Wed, 2 Jun 2021 at 20:20, Vivek Goyal <vgoyal@redhat.com> wrote: >> On Wed, May 12, 2021 at 02:55:42PM +0200, Max Reitz wrote: >>> Mount point directories represent two inodes: On one hand, they are a >>> normal directory on their parent filesystem. On the other, they are the >>> root node of the filesystem mounted there. Thus, they have two inode >>> IDs. >>> >>> Right now, we only report the latter inode ID (i.e. the inode ID of the >>> mounted filesystem's root node). This is fine once the guest has >>> auto-mounted a submount there (so this inode ID goes with a device ID >>> that is distinct from the parent filesystem), but before the auto-mount, >>> they have the device ID of the parent and the inode ID for the submount. >>> This is problematic because this is likely exactly the same >>> st_dev/st_ino combination as the parent filesystem's root node. This >>> leads to problems for example with `find`, which will thus complain >>> about a filesystem loop if it has visited the parent filesystem's root >>> node before, and then refuse to descend into the submount. >>> >>> There is a way to find the mount directory's original inode ID, and that >>> is to readdir(3) the parent directory, look for the mount directory, and >>> read the dirent.d_ino field. Using this, we can let lookup and >>> readdirplus return that original inode ID, which the guest will thus >>> show until the submount is auto-mounted. (Then, it will invoke getattr >>> and that stat(2) call will return the inode ID for the submount.) >> [ CC miklos ] >> >> Hi Max, >> >> Though we discussed this in chat room, I am still responding to this >> email with the concern I have, so that there is a record of it. That sounds scary :) >> So with this patch for FUSE_LOOKUP we always return submount's parentinode >> id and with GETATTR request we return actual inode id of submount. That >> kind of bothers me a bit as we are assuming that there is always going >> to be a GETATTR request after FUSE_LOOKUP. FUSE_LOOKUP itself has attrs >> returned with it and it might happen that after FUSE_LOOKUP, FUSE_GETATTR >> might not be called at all because FUSE_LOOKUP itself got the latest >> updated attrs with certain timeout. >> >> For example, if I call stat on a normal file (not submount), I see that >> after FUSE_LOOKUP, no FUSE_GETATTR request is going and >> fuse_update_get_attr() is using attrs from locally cached inode attrs. >> >> But same thing does not seem to be happening in case of submount. Once >> submount is created in guest, I see that we never seem to be calling >> ->revalidate() on newly created dentry of submount root. I am not sure >> why. And that means we don't do FUSE_LOOKUP and that means FUSE_GETATTR >> always gets called. Even if it worked reliably, I have to admit it’s kind of relying on at most implementation-defined behavior. Having two inodes would definitely be the right solution. > Yes, this sounds normal. > > The lookup sequence for "/mnt/virtiofs/dir1/submount/file" will be: > > LOOKUP(1, "dir") > create inode I1 in sb1 > create dentry "dir" with inode I1 in sb1 > LOOKUP(I1, "submount") > create inode I2 in sb1, set S_AUTOMOUNT > create dentry "submount" with inode I2 in sb1 > automount triggered: > create sb2 > create inode I2 in sb2 > create dentry "/" with inode I2 in sb2 > GETATTR(I2) > fill inode I2 > LOOKUP(I2, "file") > create inode I3 > create dentry "file" with inode I3 in sb2 > > Notice how there's two inodes numbered I2, but in separate sb's. > However the server has only the notion of a single I2 inode which is > the root of the submount, since the mountpoint is not visible (except > for d_ino in readdir()). > > Now AFAICS what this patch does is set the inode number in the > attributes returned by LOOKUP(I1, "submount") to the covered inode, so > that AT_NO_AUTOMOUNT stat will return the correct value. While this > seems to work, it's not a fundamental fix to the problem, since the > attributes on sb1/I2 will time out and the next stat(..., > AT_NO_AUTOMOUNT) will trigger a GETATTR request, which will return the > inode number of the submount root. Ah, yeah. > Solving this properly would mean that the server would have to > generate separate nodeid's for the mountpoint and the submount root > and the protocol would have to be extended so that this information > could be transferred to the client. Yes, we’d somehow need to do a separate lookup for the root inode of the submount... > Not sure whether this is worth it or not. I’m wondering the same. This series was mostly the result of an incidental finding and seeing that getting it to work and do what I want seemed to be pretty easy. Turns out doing something right can never really be easy... But we have already decided that we don’t deem the inode ID problem too important (not least considering NFS has the same issue), so fixing it is not really top priority. Maybe I’ll get back to it, but I think for now I consider it on the backlog. Max
diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c index 1553d2ef45..110b6e7e5b 100644 --- a/tools/virtiofsd/passthrough_ll.c +++ b/tools/virtiofsd/passthrough_ll.c @@ -968,14 +968,87 @@ static int do_statx(struct lo_data *lo, int dirfd, const char *pathname, return 0; } +/* + * Use readdir() to find mp_name's inode ID on the parent's filesystem. + * (For mount points, stat() will only return the inode ID on the + * filesystem mounted there, i.e. the root directory's inode ID. The + * mount point originally was a directory on the parent filesystem, + * though, and so has a different inode ID there. When passing + * submount information to the guest, we need to pass this other ID, + * so the guest can use it as the inode ID until the submount is + * auto-mounted. (At which point the guest will invoke getattr and + * find the inode ID on the submount.)) + * + * Return 0 on success, and -errno otherwise. *pino is set only in + * case of success. + */ +static int get_mp_ino_on_parent(const struct lo_inode *dir, const char *mp_name, + ino_t *pino) +{ + int dirfd = -1; + int ret; + DIR *dp = NULL; + + dirfd = openat(dir->fd, ".", O_RDONLY); + if (dirfd < 0) { + ret = -errno; + goto out; + } + + dp = fdopendir(dirfd); + if (!dp) { + ret = -errno; + goto out; + } + /* Owned by dp now */ + dirfd = -1; + + while (true) { + struct dirent *de; + + errno = 0; + de = readdir(dp); + if (!de) { + ret = errno ? -errno : -ENOENT; + goto out; + } + + if (!strcmp(de->d_name, mp_name)) { + *pino = de->d_ino; + ret = 0; + goto out; + } + } + +out: + if (dp) { + closedir(dp); + } + if (dirfd >= 0) { + close(dirfd); + } + return ret; +} + /* * Increments nlookup on the inode on success. unref_inode_lolocked() must be * called eventually to decrement nlookup again. If inodep is non-NULL, the * inode pointer is stored and the caller must call lo_inode_put(). + * + * If parent_fs_st_ino is true, the entry is a mount point, and submounts are + * announced to the guest, set e->attr.st_ino to the entry's inode ID on its + * parent filesystem instead of its inode ID on the filesystem mounted on it. + * (For mount points, the entry encompasses two inodes: One on the parent FS, + * and one on the mounted FS (where it is the root node), so it has two inode + * IDs. When looking up entries, we should show the guest the parent FS's inode + * ID, because as long as the guest has not auto-mounted the submount, it should + * see that original ID. Once it does perform the auto-mount, it will invoke + * getattr and see the root node's inode ID.) */ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name, struct fuse_entry_param *e, - struct lo_inode **inodep) + struct lo_inode **inodep, + bool parent_fs_st_ino) { int newfd; int res; @@ -984,6 +1057,7 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name, struct lo_data *lo = lo_data(req); struct lo_inode *inode = NULL; struct lo_inode *dir = lo_inode(req, parent); + ino_t ino_id_for_guest; if (inodep) { *inodep = NULL; /* in case there is an error */ @@ -1018,9 +1092,22 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name, goto out_err; } + ino_id_for_guest = e->attr.st_ino; + if (S_ISDIR(e->attr.st_mode) && lo->announce_submounts && (e->attr.st_dev != dir->key.dev || mnt_id != dir->key.mnt_id)) { e->attr_flags |= FUSE_ATTR_SUBMOUNT; + + if (parent_fs_st_ino) { + /* + * Best effort, so ignore errors. + * Also note that using readdir() means there may be races: + * The directory entry we find (if any) may be different + * from newfd. Again, this is a best effort. Reporting + * the wrong inode ID to the guest is not catastrophic. + */ + get_mp_ino_on_parent(dir, name, &ino_id_for_guest); + } } inode = lo_find(lo, &e->attr, mnt_id); @@ -1043,6 +1130,10 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name, inode->nlookup = 1; inode->fd = newfd; + /* + * For the inode key, use the dev/ino/mnt ID as reported by stat() + * (i.e. not ino_id_for_guest) + */ inode->key.ino = e->attr.st_ino; inode->key.dev = e->attr.st_dev; inode->key.mnt_id = mnt_id; @@ -1058,6 +1149,9 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name, } e->ino = inode->fuse_ino; + /* Report ino_id_for_guest to the guest */ + e->attr.st_ino = ino_id_for_guest; + /* Transfer ownership of inode pointer to caller or drop it */ if (inodep) { *inodep = inode; @@ -1104,7 +1198,7 @@ static void lo_lookup(fuse_req_t req, fuse_ino_t parent, const char *name) return; } - err = lo_do_lookup(req, parent, name, &e, NULL); + err = lo_do_lookup(req, parent, name, &e, NULL, true); if (err) { fuse_reply_err(req, err); } else { @@ -1217,7 +1311,7 @@ static void lo_mknod_symlink(fuse_req_t req, fuse_ino_t parent, goto out; } - saverr = lo_do_lookup(req, parent, name, &e, NULL); + saverr = lo_do_lookup(req, parent, name, &e, NULL, false); if (saverr) { goto out; } @@ -1714,7 +1808,7 @@ static void lo_do_readdir(fuse_req_t req, fuse_ino_t ino, size_t size, if (plus) { if (!is_dot_or_dotdot(name)) { - err = lo_do_lookup(req, ino, name, &e, NULL); + err = lo_do_lookup(req, ino, name, &e, NULL, true); if (err) { goto error; } @@ -1936,7 +2030,7 @@ static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name, goto out; } - err = lo_do_lookup(req, parent, name, &e, &inode); + err = lo_do_lookup(req, parent, name, &e, &inode, false); if (err) { goto out; }
Mount point directories represent two inodes: On one hand, they are a normal directory on their parent filesystem. On the other, they are the root node of the filesystem mounted there. Thus, they have two inode IDs. Right now, we only report the latter inode ID (i.e. the inode ID of the mounted filesystem's root node). This is fine once the guest has auto-mounted a submount there (so this inode ID goes with a device ID that is distinct from the parent filesystem), but before the auto-mount, they have the device ID of the parent and the inode ID for the submount. This is problematic because this is likely exactly the same st_dev/st_ino combination as the parent filesystem's root node. This leads to problems for example with `find`, which will thus complain about a filesystem loop if it has visited the parent filesystem's root node before, and then refuse to descend into the submount. There is a way to find the mount directory's original inode ID, and that is to readdir(3) the parent directory, look for the mount directory, and read the dirent.d_ino field. Using this, we can let lookup and readdirplus return that original inode ID, which the guest will thus show until the submount is auto-mounted. (Then, it will invoke getattr and that stat(2) call will return the inode ID for the submount.) Signed-off-by: Max Reitz <mreitz@redhat.com> --- tools/virtiofsd/passthrough_ll.c | 104 +++++++++++++++++++++++++++++-- 1 file changed, 99 insertions(+), 5 deletions(-)