* Re: [PATCH][RFC] btrfs: don't show full path of bind mounts in subvol=
2020-07-21 18:16 [PATCH][RFC] btrfs: don't show full path of bind mounts in subvol= Josef Bacik
@ 2020-07-22 14:46 ` Johannes Thumshirn
2020-07-22 15:36 ` David Sterba
1 sibling, 0 replies; 3+ messages in thread
From: Johannes Thumshirn @ 2020-07-22 14:46 UTC (permalink / raw)
To: Josef Bacik, linux-btrfs@vger.kernel.org, kernel-team@fb.com; +Cc: Chris Murphy
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH][RFC] btrfs: don't show full path of bind mounts in subvol=
2020-07-21 18:16 [PATCH][RFC] btrfs: don't show full path of bind mounts in subvol= Josef Bacik
2020-07-22 14:46 ` Johannes Thumshirn
@ 2020-07-22 15:36 ` David Sterba
1 sibling, 0 replies; 3+ messages in thread
From: David Sterba @ 2020-07-22 15:36 UTC (permalink / raw)
To: Josef Bacik; +Cc: linux-btrfs, kernel-team, Chris Murphy
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
^ permalink raw reply [flat|nested] 3+ messages in thread