| Message ID | 20201102161859.156603-1-mreitz@redhat.com |
|---|---|
| Headers | show |
| Series | virtiofsd: Announce submounts to the guest | expand |
On Mon, Nov 02, 2020 at 05:18:59PM +0100, Max Reitz wrote: > This test invokes several shell scripts to create a random directory > tree full of submounts, and then check in the VM whether every submount > has its own ID and the structure looks as expected. > > (Note that the test scripts must be non-executable, so Avocado will not > try to execute them as if they were tests on their own, too.) > > Because at this commit's date it is unlikely that the Linux kernel on > the image provided by boot_linux.py supports submounts in virtio-fs, the > test will be cancelled if no custom Linux binary is provided through the > vmlinuz parameter. (The on-image kernel can be used by providing an > empty string via vmlinuz=.) > > So, invoking the test can be done as follows: > $ avocado run \ > tests/acceptance/virtiofs_submounts.py \ > -p vmlinuz=/path/to/linux/build/arch/x86/boot/bzImage > > This test requires root privileges (through passwordless sudo -n), > because at this point, virtiofsd requires them. (If you have a > timestamp_timeout period for sudoers (e.g. the default of 5 min), you > can provide this by executing something like "sudo true" before invoking > Avocado.) > > Signed-off-by: Max Reitz <mreitz@redhat.com> Fixes the issue detected in v3. Tested-by: Eduardo Habkost <ehabkost@redhat.com> -- Eduardo
On Mon, Nov 2, 2020 at 5:19 PM Max Reitz <mreitz@redhat.com> wrote: > > Whenever we encounter a directory with an st_dev or mount ID that > differs from that of its parent, we set the FUSE_ATTR_SUBMOUNT flag so > the guest can create a submount for it. > > We only need to do so in lo_do_lookup(). The following functions return > a fuse_attr object: > - lo_create(), though fuse_reply_create(): Calls lo_do_lookup(). > - lo_lookup(), though fuse_reply_entry(): Calls lo_do_lookup(). > - lo_mknod_symlink(), through fuse_reply_entry(): Calls lo_do_lookup(). > - lo_link(), through fuse_reply_entry(): Creating a link cannot create a > submount, so there is no need to check for it. > - lo_getattr(), through fuse_reply_attr(): Announcing submounts when the > node is first detected (at lookup) is sufficient. We do not need to > return the submount attribute later. > - lo_do_readdir(), through fuse_add_direntry_plus(): Calls > lo_do_lookup(). > > Make announcing submounts optional, so submounts are only announced to > the guest with the announce_submounts option. Some users may prefer the > current behavior, so that the guest learns nothing about the host mount > structure. > > (announce_submounts is force-disabled when the guest does not present > the FUSE_SUBMOUNTS capability, or when there is no statx().) > > Signed-off-by: Max Reitz <mreitz@redhat.com> > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> > --- > tools/virtiofsd/helper.c | 1 + > tools/virtiofsd/passthrough_ll.c | 22 ++++++++++++++++++++++ > 2 files changed, 23 insertions(+) > > diff --git a/tools/virtiofsd/helper.c b/tools/virtiofsd/helper.c > index 2e181a49b5..4433724d3a 100644 > --- a/tools/virtiofsd/helper.c > +++ b/tools/virtiofsd/helper.c > @@ -190,6 +190,7 @@ void fuse_cmdline_help(void) > " retain/discard O_DIRECT flags passed down\n" > " to virtiofsd from guest applications.\n" > " default: no_allow_direct_io\n" > + " -o announce_submounts Announce sub-mount points to the guest\n" > ); > } > > diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c > index 34d107975f..ec1008bceb 100644 > --- a/tools/virtiofsd/passthrough_ll.c > +++ b/tools/virtiofsd/passthrough_ll.c > @@ -40,6 +40,7 @@ > #include "fuse_virtio.h" > #include "fuse_log.h" > #include "fuse_lowlevel.h" > +#include "standard-headers/linux/fuse.h" > #include <assert.h> > #include <cap-ng.h> > #include <dirent.h> > @@ -167,6 +168,7 @@ struct lo_data { > int readdirplus_set; > int readdirplus_clear; > int allow_direct_io; > + int announce_submounts; > bool use_statx; > struct lo_inode root; > GHashTable *inodes; /* protected by lo->mutex */ > @@ -207,6 +209,7 @@ static const struct fuse_opt lo_opts[] = { > { "no_readdirplus", offsetof(struct lo_data, readdirplus_clear), 1 }, > { "allow_direct_io", offsetof(struct lo_data, allow_direct_io), 1 }, > { "no_allow_direct_io", offsetof(struct lo_data, allow_direct_io), 0 }, > + { "announce_submounts", offsetof(struct lo_data, announce_submounts), 1 }, > FUSE_OPT_END > }; > static bool use_syslog = false; > @@ -601,6 +604,20 @@ static void lo_init(void *userdata, struct fuse_conn_info *conn) > fuse_log(FUSE_LOG_DEBUG, "lo_init: disabling readdirplus\n"); > conn->want &= ~FUSE_CAP_READDIRPLUS; > } > + > + if (!(conn->capable & FUSE_CAP_SUBMOUNTS) && lo->announce_submounts) { > + fuse_log(FUSE_LOG_WARNING, "lo_init: Cannot announce submounts, client " > + "does not support it\n"); > + lo->announce_submounts = false; > + } > + > +#ifndef CONFIG_STATX > + if (lo->announce_submounts) { > + fuse_log(FUSE_LOG_WARNING, "lo_init: Cannot announce submounts, there " > + "is no statx()\n"); > + lo->announce_submounts = false; Minor issue: this warns only when libc doesn't have statx, and not when kernel doesn't have it. > + } > +#endif > } > > static void lo_getattr(fuse_req_t req, fuse_ino_t ino, > @@ -877,6 +894,11 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name, > goto out_err; > } > > + 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; > + } ... and since announce_submounts isn't turned off in case the kernel doesn't have STATX_MNT_ID, this will trigger a submount when crossing into a different filesystem. Possible solutions: a) test and warn at startup in case kernel doesn't have statx b) do not test st_dev, only mnt_id (which will always be zero if not supported) c) merge announce_submounts and use_statx, which are basically doing the same thing > + > inode = lo_find(lo, &e->attr, mnt_id); > if (inode) { > close(newfd); > -- > 2.26.2 >
On Mon, Nov 2, 2020 at 5:19 PM Max Reitz <mreitz@redhat.com> wrote: > > RFC: https://www.redhat.com/archives/virtio-fs/2020-May/msg00024.html > v1: https://lists.nongnu.org/archive/html/qemu-devel/2020-09/msg03598.html > v2: https://lists.nongnu.org/archive/html/qemu-devel/2020-10/msg09117.html > > Branch: https://github.com/XanClic/qemu.git virtiofs-submounts-v4 > Branch: https://git.xanclic.moe/XanClic/qemu.git virtiofs-submounts-v4 > > > Hi, > > In an effort to cut this cover letter shorter, I’ll defer to the cover > letter of v2 linked above concerning the general description of this > series, and limit myself to describing the differences from v2: Other than the issues in 5/7: Reviewed-by: Miklos Szeredi <mszeredi@redhat.com> Thanks, Miklos
On 03.11.20 09:10, Miklos Szeredi wrote: > On Mon, Nov 2, 2020 at 5:19 PM Max Reitz <mreitz@redhat.com> wrote: >> >> Whenever we encounter a directory with an st_dev or mount ID that >> differs from that of its parent, we set the FUSE_ATTR_SUBMOUNT flag so >> the guest can create a submount for it. >> >> We only need to do so in lo_do_lookup(). The following functions return >> a fuse_attr object: >> - lo_create(), though fuse_reply_create(): Calls lo_do_lookup(). >> - lo_lookup(), though fuse_reply_entry(): Calls lo_do_lookup(). >> - lo_mknod_symlink(), through fuse_reply_entry(): Calls lo_do_lookup(). >> - lo_link(), through fuse_reply_entry(): Creating a link cannot create a >> submount, so there is no need to check for it. >> - lo_getattr(), through fuse_reply_attr(): Announcing submounts when the >> node is first detected (at lookup) is sufficient. We do not need to >> return the submount attribute later. >> - lo_do_readdir(), through fuse_add_direntry_plus(): Calls >> lo_do_lookup(). >> >> Make announcing submounts optional, so submounts are only announced to >> the guest with the announce_submounts option. Some users may prefer the >> current behavior, so that the guest learns nothing about the host mount >> structure. >> >> (announce_submounts is force-disabled when the guest does not present >> the FUSE_SUBMOUNTS capability, or when there is no statx().) >> >> Signed-off-by: Max Reitz <mreitz@redhat.com> >> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> >> --- >> tools/virtiofsd/helper.c | 1 + >> tools/virtiofsd/passthrough_ll.c | 22 ++++++++++++++++++++++ >> 2 files changed, 23 insertions(+) >> >> diff --git a/tools/virtiofsd/helper.c b/tools/virtiofsd/helper.c >> index 2e181a49b5..4433724d3a 100644 >> --- a/tools/virtiofsd/helper.c >> +++ b/tools/virtiofsd/helper.c >> @@ -190,6 +190,7 @@ void fuse_cmdline_help(void) >> " retain/discard O_DIRECT flags passed down\n" >> " to virtiofsd from guest applications.\n" >> " default: no_allow_direct_io\n" >> + " -o announce_submounts Announce sub-mount points to the guest\n" >> ); >> } >> >> diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c >> index 34d107975f..ec1008bceb 100644 >> --- a/tools/virtiofsd/passthrough_ll.c >> +++ b/tools/virtiofsd/passthrough_ll.c >> @@ -40,6 +40,7 @@ >> #include "fuse_virtio.h" >> #include "fuse_log.h" >> #include "fuse_lowlevel.h" >> +#include "standard-headers/linux/fuse.h" >> #include <assert.h> >> #include <cap-ng.h> >> #include <dirent.h> >> @@ -167,6 +168,7 @@ struct lo_data { >> int readdirplus_set; >> int readdirplus_clear; >> int allow_direct_io; >> + int announce_submounts; >> bool use_statx; >> struct lo_inode root; >> GHashTable *inodes; /* protected by lo->mutex */ >> @@ -207,6 +209,7 @@ static const struct fuse_opt lo_opts[] = { >> { "no_readdirplus", offsetof(struct lo_data, readdirplus_clear), 1 }, >> { "allow_direct_io", offsetof(struct lo_data, allow_direct_io), 1 }, >> { "no_allow_direct_io", offsetof(struct lo_data, allow_direct_io), 0 }, >> + { "announce_submounts", offsetof(struct lo_data, announce_submounts), 1 }, >> FUSE_OPT_END >> }; >> static bool use_syslog = false; >> @@ -601,6 +604,20 @@ static void lo_init(void *userdata, struct fuse_conn_info *conn) >> fuse_log(FUSE_LOG_DEBUG, "lo_init: disabling readdirplus\n"); >> conn->want &= ~FUSE_CAP_READDIRPLUS; >> } >> + >> + if (!(conn->capable & FUSE_CAP_SUBMOUNTS) && lo->announce_submounts) { >> + fuse_log(FUSE_LOG_WARNING, "lo_init: Cannot announce submounts, client " >> + "does not support it\n"); >> + lo->announce_submounts = false; >> + } >> + >> +#ifndef CONFIG_STATX >> + if (lo->announce_submounts) { >> + fuse_log(FUSE_LOG_WARNING, "lo_init: Cannot announce submounts, there " >> + "is no statx()\n"); >> + lo->announce_submounts = false; > > Minor issue: this warns only when libc doesn't have statx, and not > when kernel doesn't have it. > >> + } >> +#endif >> } >> >> static void lo_getattr(fuse_req_t req, fuse_ino_t ino, >> @@ -877,6 +894,11 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name, >> goto out_err; >> } >> >> + 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; >> + } > > ... and since announce_submounts isn't turned off in case the kernel > doesn't have STATX_MNT_ID, this will trigger a submount when crossing > into a different filesystem. Oh. Yes. I totally forgot that when writing the warning. > Possible solutions: > > a) test and warn at startup in case kernel doesn't have statx > > b) do not test st_dev, only mnt_id (which will always be zero if not supported) > > c) merge announce_submounts and use_statx, which are basically doing > the same thing Isn’t that a single thing? I.e., if we decide not to test st_dev, then we should do all of these, I think. OTOH, we could also just drop the warning (that statx()) isn’t available, because as the code is, we can still announce submounts. The only problem is that we’ll suffer from the bug fixed by patch 4 (different mounts with the same st_dev being treated as the same tree), but that’s unrelated to announcing submounts. (Well, to be fair, not having statx() does break one thing about submounts: I suppose you could mount a device inside of its own mount (“mount $mount_opts $dir; mount $mount_opts $dir/sub” – then $dir/sub wouldn’t be marked as a submount without statx()), but that probably yields a host of other problems besides not announcing the nested mount as a submount (virtiofsd would treat $dir/sub as the same node as $dir, I think), so again, I wouldn’t worry too much about not getting the FUSE_SUBMOUNT flag right.) So I think I’d rather just drop the warning and leave the rest as it is. Not least because STATX_MNT_ID is rather new. Max
On Tue, Nov 3, 2020 at 10:00 AM Max Reitz <mreitz@redhat.com> wrote: > > On 03.11.20 09:10, Miklos Szeredi wrote: > > On Mon, Nov 2, 2020 at 5:19 PM Max Reitz <mreitz@redhat.com> wrote: > >> > >> Whenever we encounter a directory with an st_dev or mount ID that > >> differs from that of its parent, we set the FUSE_ATTR_SUBMOUNT flag so > >> the guest can create a submount for it. > >> > >> We only need to do so in lo_do_lookup(). The following functions return > >> a fuse_attr object: > >> - lo_create(), though fuse_reply_create(): Calls lo_do_lookup(). > >> - lo_lookup(), though fuse_reply_entry(): Calls lo_do_lookup(). > >> - lo_mknod_symlink(), through fuse_reply_entry(): Calls lo_do_lookup(). > >> - lo_link(), through fuse_reply_entry(): Creating a link cannot create a > >> submount, so there is no need to check for it. > >> - lo_getattr(), through fuse_reply_attr(): Announcing submounts when the > >> node is first detected (at lookup) is sufficient. We do not need to > >> return the submount attribute later. > >> - lo_do_readdir(), through fuse_add_direntry_plus(): Calls > >> lo_do_lookup(). > >> > >> Make announcing submounts optional, so submounts are only announced to > >> the guest with the announce_submounts option. Some users may prefer the > >> current behavior, so that the guest learns nothing about the host mount > >> structure. > >> > >> (announce_submounts is force-disabled when the guest does not present > >> the FUSE_SUBMOUNTS capability, or when there is no statx().) > >> > >> Signed-off-by: Max Reitz <mreitz@redhat.com> > >> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> > >> --- > >> tools/virtiofsd/helper.c | 1 + > >> tools/virtiofsd/passthrough_ll.c | 22 ++++++++++++++++++++++ > >> 2 files changed, 23 insertions(+) > >> > >> diff --git a/tools/virtiofsd/helper.c b/tools/virtiofsd/helper.c > >> index 2e181a49b5..4433724d3a 100644 > >> --- a/tools/virtiofsd/helper.c > >> +++ b/tools/virtiofsd/helper.c > >> @@ -190,6 +190,7 @@ void fuse_cmdline_help(void) > >> " retain/discard O_DIRECT flags passed down\n" > >> " to virtiofsd from guest applications.\n" > >> " default: no_allow_direct_io\n" > >> + " -o announce_submounts Announce sub-mount points to the guest\n" > >> ); > >> } > >> > >> diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c > >> index 34d107975f..ec1008bceb 100644 > >> --- a/tools/virtiofsd/passthrough_ll.c > >> +++ b/tools/virtiofsd/passthrough_ll.c > >> @@ -40,6 +40,7 @@ > >> #include "fuse_virtio.h" > >> #include "fuse_log.h" > >> #include "fuse_lowlevel.h" > >> +#include "standard-headers/linux/fuse.h" > >> #include <assert.h> > >> #include <cap-ng.h> > >> #include <dirent.h> > >> @@ -167,6 +168,7 @@ struct lo_data { > >> int readdirplus_set; > >> int readdirplus_clear; > >> int allow_direct_io; > >> + int announce_submounts; > >> bool use_statx; > >> struct lo_inode root; > >> GHashTable *inodes; /* protected by lo->mutex */ > >> @@ -207,6 +209,7 @@ static const struct fuse_opt lo_opts[] = { > >> { "no_readdirplus", offsetof(struct lo_data, readdirplus_clear), 1 }, > >> { "allow_direct_io", offsetof(struct lo_data, allow_direct_io), 1 }, > >> { "no_allow_direct_io", offsetof(struct lo_data, allow_direct_io), 0 }, > >> + { "announce_submounts", offsetof(struct lo_data, announce_submounts), 1 }, > >> FUSE_OPT_END > >> }; > >> static bool use_syslog = false; > >> @@ -601,6 +604,20 @@ static void lo_init(void *userdata, struct fuse_conn_info *conn) > >> fuse_log(FUSE_LOG_DEBUG, "lo_init: disabling readdirplus\n"); > >> conn->want &= ~FUSE_CAP_READDIRPLUS; > >> } > >> + > >> + if (!(conn->capable & FUSE_CAP_SUBMOUNTS) && lo->announce_submounts) { > >> + fuse_log(FUSE_LOG_WARNING, "lo_init: Cannot announce submounts, client " > >> + "does not support it\n"); > >> + lo->announce_submounts = false; > >> + } > >> + > >> +#ifndef CONFIG_STATX > >> + if (lo->announce_submounts) { > >> + fuse_log(FUSE_LOG_WARNING, "lo_init: Cannot announce submounts, there " > >> + "is no statx()\n"); > >> + lo->announce_submounts = false; > > > > Minor issue: this warns only when libc doesn't have statx, and not > > when kernel doesn't have it. > > > >> + } > >> +#endif > >> } > >> > >> static void lo_getattr(fuse_req_t req, fuse_ino_t ino, > >> @@ -877,6 +894,11 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name, > >> goto out_err; > >> } > >> > >> + 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; > >> + } > > > > ... and since announce_submounts isn't turned off in case the kernel > > doesn't have STATX_MNT_ID, this will trigger a submount when crossing > > into a different filesystem. > > Oh. Yes. I totally forgot that when writing the warning. > > > Possible solutions: > > > > a) test and warn at startup in case kernel doesn't have statx > > > > b) do not test st_dev, only mnt_id (which will always be zero if not supported) > > > > c) merge announce_submounts and use_statx, which are basically doing > > the same thing > > Isn’t that a single thing? I.e., if we decide not to test st_dev, then > we should do all of these, I think. > > OTOH, we could also just drop the warning (that statx()) isn’t > available, because as the code is, we can still announce submounts. The > only problem is that we’ll suffer from the bug fixed by patch 4 > (different mounts with the same st_dev being treated as the same tree), > but that’s unrelated to announcing submounts. > > (Well, to be fair, not having statx() does break one thing about > submounts: I suppose you could mount a device inside of its own mount > (“mount $mount_opts $dir; mount $mount_opts $dir/sub” – then $dir/sub > wouldn’t be marked as a submount without statx()), but that probably > yields a host of other problems besides not announcing the nested mount > as a submount (virtiofsd would treat $dir/sub as the same node as $dir, > I think), so again, I wouldn’t worry too much about not getting the > FUSE_SUBMOUNT flag right.) > > So I think I’d rather just drop the warning and leave the rest as it is. > Not least because STATX_MNT_ID is rather new. Okay, makes sense. The nested bind mount case is likely not very likely to turn up in real life, and if it does, submounts can still be disabled explicitly. Thanks, Miklos
RFC: https://www.redhat.com/archives/virtio-fs/2020-May/msg00024.html v1: https://lists.nongnu.org/archive/html/qemu-devel/2020-09/msg03598.html v2: https://lists.nongnu.org/archive/html/qemu-devel/2020-10/msg09117.html Branch: https://github.com/XanClic/qemu.git virtiofs-submounts-v4 Branch: https://git.xanclic.moe/XanClic/qemu.git virtiofs-submounts-v4 Hi, In an effort to cut this cover letter shorter, I’ll defer to the cover letter of v2 linked above concerning the general description of this series, and limit myself to describing the differences from v2: v3: - Patch 7: - Replace specific has_passwordless_sudo() function by a generic has_cmd() function that can check for any shell command whether it works or not - Let this function catch exceptions, too, so if sudo is not present altogether, the test doesn’t fail but is skipped as intended [Eduardo] - (Add has_cmds() to run multiple instances of has_cmd() in a decorator) - Skip the test not only if “sudo -n true” doesn’t work, but also check for ssh-keygen, bash, losetup, mkfs.xfs, and mount, which are some other tools we need that might not be present on the host git-backport-diff against v2: Key: [----] : patches are identical [####] : number of functional differences between upstream/downstream patch [down] : patch is downstream-only The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively 001/7:[----] [--] 'virtiofsd: Check FUSE_SUBMOUNTS' 002/7:[----] [--] 'virtiofsd: Add attr_flags to fuse_entry_param' 003/7:[----] [--] 'meson.build: Check for statx()' 004/7:[----] [--] 'virtiofsd: Add mount ID to the lo_inode key' 005/7:[----] [--] 'virtiofsd: Announce sub-mount points' 006/7:[----] [--] 'tests/acceptance/boot_linux: Accept SSH pubkey' 007/7:[0042] [FC] 'tests/acceptance: Add virtiofs_submounts.py' Max Reitz (7): virtiofsd: Check FUSE_SUBMOUNTS virtiofsd: Add attr_flags to fuse_entry_param meson.build: Check for statx() virtiofsd: Add mount ID to the lo_inode key virtiofsd: Announce sub-mount points tests/acceptance/boot_linux: Accept SSH pubkey tests/acceptance: Add virtiofs_submounts.py meson.build | 16 + tools/virtiofsd/fuse_common.h | 7 + tools/virtiofsd/fuse_lowlevel.h | 5 + tools/virtiofsd/fuse_lowlevel.c | 5 + tools/virtiofsd/helper.c | 1 + tools/virtiofsd/passthrough_ll.c | 117 ++++++- tools/virtiofsd/passthrough_seccomp.c | 1 + tests/acceptance/boot_linux.py | 13 +- tests/acceptance/virtiofs_submounts.py | 321 ++++++++++++++++++ .../virtiofs_submounts.py.data/cleanup.sh | 46 +++ .../guest-cleanup.sh | 30 ++ .../virtiofs_submounts.py.data/guest.sh | 138 ++++++++ .../virtiofs_submounts.py.data/host.sh | 127 +++++++ 13 files changed, 811 insertions(+), 16 deletions(-) create mode 100644 tests/acceptance/virtiofs_submounts.py create mode 100644 tests/acceptance/virtiofs_submounts.py.data/cleanup.sh create mode 100644 tests/acceptance/virtiofs_submounts.py.data/guest-cleanup.sh create mode 100644 tests/acceptance/virtiofs_submounts.py.data/guest.sh create mode 100644 tests/acceptance/virtiofs_submounts.py.data/host.sh