| Message ID | 20200721181656.16171-1-josef@toxicpanda.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] btrfs: don't show full path of bind mounts in subvol= | expand |
On 21/07/2020 20:17, Josef Bacik wrote: > Chris Murphy reported a problem where rpm ostree will bind mount a bunch > of things for whatever voodoo it's doing. But when it does this > /proc/mounts shows something like > > /dev/vda4 on /usr type btrfs (ro,relatime,seclabel,space_cache,subvolid=256,subvol=/root/ostree/deploy/fedora/deploy/610b0f9be3141c79f19a65800f89746c70183cc7f14f3cfba29d695d49128075.0/usr) > For the sake of documentation, could you add a line showing the /proc/mounts entry after your patch is applied? Thanks, Johannes
On Tue, Jul 21, 2020 at 02:16:56PM -0400, Josef Bacik wrote: > Chris Murphy reported a problem where rpm ostree will bind mount a bunch > of things for whatever voodoo it's doing. But when it does this > /proc/mounts shows something like > > /dev/vda4 on /usr type btrfs (ro,relatime,seclabel,space_cache,subvolid=256,subvol=/root/ostree/deploy/fedora/deploy/610b0f9be3141c79f19a65800f89746c70183cc7f14f3cfba29d695d49128075.0/usr) > > Despite subvolid=256 being subvol=/root. This is because we're just > spitting out the dentry of the mount point, which in the case of bind > mounts is the source path for the mountpoint. Instead we should spit > out the path to the actual subvol. Fix this by looking up the name for > the subvolid we have mounted. > > Fixes: c8d3fe028f64 ("Btrfs: show subvol= and subvolid= in /proc/mounts") > Reported-by: Chris Murphy <chris@colorremedies.com> > Signed-off-by: Josef Bacik <josef@toxicpanda.com> > --- > > I talked with Omar about this and his other suggestion is we simply don't spit > out subvol=<path> if we're not the actual subvolume dentry. I'm ok with that > option too, and it avoids a memory alloc for show_options, however it does mean > that we'll stop spitting out subvol= in some cases, which may cause problems? > If we prefer that option I can code that up instead. That sounds like a valid alternative, in case it's not a subvolume dentry I'd drop the subvolid= too. One of the points of the subvol= option was to allow copy&paste from /proc/mounts and get the same result. The bind mount is the underlying mechanism for subvolume mount but it can be also done by user and we have no way to distinguish that. So manual bind mount to a subvolume is effectively the same thing as mount -o subvol=/path. The result in /proc/mounts would be the same. I don't think this is a problem. Current output of non-subvolume bind mount is confusing and known, I once tried to fix it by traversing back the dentry chain until it reaches the parent subvolume but there are some other cases like nested subvolumes where it's not clear where to stop: /dir1/subvol1/dir2/subvol2/dir3/subvol3/dir4 when eg. dir3 is bind mounted, if subvol= should be subvol2 or subvol1. Resolving the subvolume from the subvolid we get from the dentry/inode should work, it's the actual mount point so something the user has specified at some point. So the question is: 1) resolve subvol from subvolid in all cases, it could be something else in case of bind mounts, but it's still close 2) non-subvolume bind mounts won't print subvolid/subvol at all, that way it could be determined it's bind mount in some random directory I'g go for option 1, ie. what you implemented. The extra allocation should not be a big concern. > fs/btrfs/super.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c > index 58f890f73650..0e1647c08610 100644 > --- a/fs/btrfs/super.c > +++ b/fs/btrfs/super.c > @@ -1367,6 +1367,7 @@ static int btrfs_show_options(struct seq_file *seq, struct dentry *dentry) > { > struct btrfs_fs_info *info = btrfs_sb(dentry->d_sb); > const char *compress_type; > + const char *subvol_name; > > if (btrfs_test_opt(info, DEGRADED)) > seq_puts(seq, ",degraded"); > @@ -1453,8 +1454,12 @@ static int btrfs_show_options(struct seq_file *seq, struct dentry *dentry) > seq_puts(seq, ",ref_verify"); > seq_printf(seq, ",subvolid=%llu", > BTRFS_I(d_inode(dentry))->root->root_key.objectid); > - seq_puts(seq, ",subvol="); > - seq_dentry(seq, dentry, " \t\n\\"); > + subvol_name = btrfs_get_subvol_name_from_objectid(info, > + BTRFS_I(d_inode(dentry))->root->root_key.objectid); > + if (subvol_name) { > + seq_printf(seq, ",subvol=%s", subvol_name); The path still needs to be escaped, seq_escape > + kfree(subvol_name); > + } > return 0; > } > > -- > 2.24.1
diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c index 58f890f73650..0e1647c08610 100644 --- a/fs/btrfs/super.c +++ b/fs/btrfs/super.c @@ -1367,6 +1367,7 @@ static int btrfs_show_options(struct seq_file *seq, struct dentry *dentry) { struct btrfs_fs_info *info = btrfs_sb(dentry->d_sb); const char *compress_type; + const char *subvol_name; if (btrfs_test_opt(info, DEGRADED)) seq_puts(seq, ",degraded"); @@ -1453,8 +1454,12 @@ static int btrfs_show_options(struct seq_file *seq, struct dentry *dentry) seq_puts(seq, ",ref_verify"); seq_printf(seq, ",subvolid=%llu", BTRFS_I(d_inode(dentry))->root->root_key.objectid); - seq_puts(seq, ",subvol="); - seq_dentry(seq, dentry, " \t\n\\"); + subvol_name = btrfs_get_subvol_name_from_objectid(info, + BTRFS_I(d_inode(dentry))->root->root_key.objectid); + if (subvol_name) { + seq_printf(seq, ",subvol=%s", subvol_name); + kfree(subvol_name); + } return 0; }
Chris Murphy reported a problem where rpm ostree will bind mount a bunch of things for whatever voodoo it's doing. But when it does this /proc/mounts shows something like /dev/vda4 on /usr type btrfs (ro,relatime,seclabel,space_cache,subvolid=256,subvol=/root/ostree/deploy/fedora/deploy/610b0f9be3141c79f19a65800f89746c70183cc7f14f3cfba29d695d49128075.0/usr) Despite subvolid=256 being subvol=/root. This is because we're just spitting out the dentry of the mount point, which in the case of bind mounts is the source path for the mountpoint. Instead we should spit out the path to the actual subvol. Fix this by looking up the name for the subvolid we have mounted. Fixes: c8d3fe028f64 ("Btrfs: show subvol= and subvolid= in /proc/mounts") Reported-by: Chris Murphy <chris@colorremedies.com> Signed-off-by: Josef Bacik <josef@toxicpanda.com> --- I talked with Omar about this and his other suggestion is we simply don't spit out subvol=<path> if we're not the actual subvolume dentry. I'm ok with that option too, and it avoids a memory alloc for show_options, however it does mean that we'll stop spitting out subvol= in some cases, which may cause problems? If we prefer that option I can code that up instead. fs/btrfs/super.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)